[PATCH] TMS570 console driver, SCI frame error (baudrate calculation error)

[zack leung]

hi, so do you want me make this change?

On Wed, 12 Jul 2023 at 10:54, Gedare Bloom <gedare at rtems.org> wrote:

> On Wed, Jul 12, 2023 at 8:06 AM Joel Sherrill <joel at rtems.org> wrote:
> >
> >
> > Pavel this was filed as https://devel.rtems.org/ticket/4903. The ticket
> submitter
> > didn't give a patch, just a code change snippet and explanation. Zack
> turned it into
> > a patch. You will need to post that on the ticket to get the submitter's
> feedback.
> >
> > I hope you don't mind copying that to the ticket and seeing what you
> think is
> > really up.
> >
> I posted it over there. Thanks Pavel and Joel.
>
> > Thanks.
> >
> > --joel
> >
> > On Wed, Jul 12, 2023 at 8:49 AM Pavel Pisa <ppisa4lists at pikron.com>
> wrote:
> >>
> >> Hello Zack and Gedare,
> >>
> >> On Tuesday 11 of July 2023 19:52:27 Gedare Bloom wrote:
> >> > Thanks for the patch. Someone should probably test it, or identify in
> >> > the documentation why this calculation was off-by-1. Pavel, any clues?
> >> > On Sun, Jul 9, 2023 at 10:09 PM zack <zakthertemsdev at gmail.com>
> wrote:
> >> > > Fixes #4903
> >> > > diff --git a/bsps/arm/tms570/console/tms570-sci.c
> >> > > b/bsps/arm/tms570/console/tms570-sci.c index 768770a4c8..59a0b7e6f1
> >> > > 100644
> >> > > --- a/bsps/arm/tms570/console/tms570-sci.c
> >> > > +++ b/bsps/arm/tms570/console/tms570-sci.c
> >> > > @@ -311,7 +311,7 @@ bool tms570_sci_set_attributes(
> >> > >    /* Apply baudrate to the hardware */
> >> > >    baudrate *= 2 * 16;
> >> > >    bauddiv = (BSP_PLL_OUT_CLOCK + baudrate / 2) / baudrate;
> >> > > -  ctx->regs->BRS = bauddiv? bauddiv - 1: 0;
> >> > > +  ctx->regs->BRS = bauddiv? bauddiv - 2: 0;
> >>
> >> I think that change is not correct. The actual used values
> >> for BSP_PLL_OUT_CLOCK and baudrate should be provided to analyze
> >> the case. The code can result in some rounding error and can
> >> be enhanced if fractional divider is used or even super finegrained
> >> fractional divider. But these options are available only for
> >> for SCI/LIN peripheral case.
> >>
> >> According to
> >>
> >> TMS570LS31x/21x 16/32-Bit RISC Flash Microcontroller
> >> Technical Reference Manual
> >> Literature Number: SPNU499B
> >>
> >> 26.2.3 SCI Baud Rate
> >>
> >>   SCICLK Frequency = VCLK Frequency / (P + 1 + M / 16)
> >>
> >>   Asynchronous baud value = SCICLK Frequency / 16
> >>
> >> So the substraction of one corresponds to the manual.
> >>
> >> Actual code does not use M part. It would be problem if it is
> >> leftover from some boot/monitor but it is part of BRS 32-bit
> >> register which is overwritten in the whole, so such problem
> >> should not appear either.
> >>
> >> So I vote against the proposed change for now and suggest
> >> to do analysis what happens in the computation and what
> >> are input values and output. Change would/could affect
> >> negatively large number of combinations of the baudrate
> >> and clocks.
> >>
> >> I would consider to discuss if the rounding formula
> >> could/should be updated, but I think that it is the best
> >> which cane be achieved for rations which do not result
> >> in exact ratio.
> >>
> >>   (BSP_PLL_OUT_CLOCK + baudrate / 2) / baudrate;
> >>
> >> If there is interrest then code can be enhanced by fraction
> >> dividers for SCI/LIN peripheral case. The field with variant
> >> should be added into tms570_sci_context and in this case
> >> the alternative formula can be used
> >>
> >>   long long bauddiv;
> >>   bauddiv = (BSP_PLL_OUT_CLOCK * 16ll + baudrate / 2) / baudrate;
> >>   ctx->regs->BRS = ((bauddiv >> 4) & 0xffffff) | ((bauddiv & 0xf) <<
> 24);
> >>
> >> which should be rewritten after header for SCI/LIN update to
> >>
> >>   ctx->regs->BRS = TMS570_LIN_BRS_P(bauddiv >> 4) |
> TMS570_LIN_BRS_M(bauddiv & 0xf);
> >
> >
> >
> >>
> >>
> >> Best wishes,
> >>
> >>
> >>                 Pavel
> >> --
> >>                 Pavel Pisa
> >>     phone:      +420 603531357
> >>     e-mail:     pisa at cmp.felk.cvut.cz
> >>     Department of Control Engineering FEE CVUT
> >>     Karlovo namesti 13, 121 35, Prague 2
> >>     university: http://control.fel.cvut.cz/
> >>     personal:   http://cmp.felk.cvut.cz/~pisa
> >>     projects:   https://www.openhub.net/accounts/ppisa
> >>     CAN related:http://canbus.pages.fel.cvut.cz/
> >>     RISC-V education: https://comparch.edu.cvut.cz/
> >>     Open Technologies Research Education and Exchange Services
> >>     https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home
> >>
> >> _______________________________________________
> >> devel mailing list
> >> devel at rtems.org
> >> http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20230714/69c69323/attachment.htm>