[RTEMS Project] #4903: TMS570 console driver, SCI frame error (baudrate calculation error)

[RTEMS trac]

#4903: TMS570 console driver, SCI frame error (baudrate calculation error)
----------------------+---------------------
 Reporter:  Usha      |       Owner:  (none)
     Type:  defect    |      Status:  new
 Priority:  normal    |   Milestone:  6.1
Component:  arch/arm  |     Version:  6
 Severity:  normal    |  Resolution:
 Keywords:            |  Blocked By:
 Blocking:            |
----------------------+---------------------

Comment (by Pavel Pisa):

 Comment to the next patch proposal
 {{{
 --- 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 fine-grained 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 subtraction 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);
 }}}

--
Ticket URL: <http://devel.rtems.org/ticket/4903#comment:3>
RTEMS Project <http://www.rtems.org/>
RTEMS Project