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

[Gedare Bloom]

No, because we don't have a reproducible test case.

On Fri, Jul 14, 2023 at 7:38 PM zack leung <zakthertemsdev at gmail.com> wrote:
>
> 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