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

[Pavel Pisa]

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