#4903 new defect

TMS570 console driver, SCI frame error (baudrate calculation error)

Reported by: Usha Owned by:
Priority: normal Milestone: Indefinite
Component: arch/arm Version: 6
Severity: normal Keywords:
Cc: Blocked By:
Blocking:

Description

bsps/arm/tms570/console/tms570-sci.c: 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;

replacing with 2 fixed frame error

ctx->regs->BRS = bauddiv? bauddiv - 2: 0;

there is issue in baudrate calculation

Change History (4)

comment:1 Changed on 06/22/23 at 20:58:14 by Joel Sherrill

Can you please submit this as a patch? See https://docs.rtems.org/branches/master/user/support/contrib.html for details.

Thanks.

comment:3 Changed on 07/13/23 at 06:41:03 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);

comment:4 Changed on 07/27/23 at 01:21:34 by z.ling111

Fri, 14 Jul, 21:37 (12 days ago)
hi, so do you want me make this change?
Gedare Bloom via gwmail.gwu.edu

Mon, 17 Jul, 14:17 (9 days ago)


to me, joel, Pavel, Přemysl, devel
No, because we don't have a reproducible test case.

Do you want to close this ticket as we do not have a test case?

comment:5 Changed on 07/27/23 at 18:31:31 by Gedare Bloom

Milestone: 6.1Indefinite

Leaving open for now, but not setting a milestone until the bug can be confirmed.

Note: See TracTickets for help on using tickets.