Re: 3.1 rt_imx_uart.c regression

[Jan Kiszka <jan.kiszka@siemens.com>]

On 10.02.20 02:28, steve freyder via Xenomai wrote:
> Hello Xenomai group.
> 
> 
> We appear to have a regression in the rt_imx_uart driver in Xenomai 
> 3.1.  The problem didn't exist in 3.0.7, and I believe it did exist in 
> 3.0.10.  Between those two I did not try to narrow it down.  I 
> discovered this when connecting a UART on a 3.1 system to a UART on a 
> 3.0.7 system, both being IMX6 Dual-Lite SOM's, and the cross-connected 
> UARTs would not intercommunicate (100% framing errors).  A simple TX/RX 
> loopback test on either system was successful, but the cross-connect 
> (like cross-link.c might use) was unsuccessful with a mixed 3.0.7 <--> 
> 3.1 environment.
> 
> 
> I traced this to the setting (or not setting) of the UCR2_SRST bit in 
> the rt_imx_uart_set_config() routine.  The IMX6 TRM defines this bit as 
> SRST_B (B as in 'bar' as in inverted) and so setting this bit means *no 
> software reset", so not setting it results in a software reset, which 
> results in leaving the UART clock divisors at default and the resulting 
> rate is 80mhz.  The user-selected (via ioctl()) baud rate and other 
> parameters therefore have no effect.
> 
> 
> The patch below fixes this problem.  I hope I have the proper format.
> 
> 
> There may be another "benign" issue with this driver, but I would defer 
> that to someone more familiar with it.  Specifically, it's with the 
> routine rt_imx_uart_setup_ufcr(), whose function appears to be 
> duplicated in rt_imx_uart_set_config().  In the driver init, 
> rt_imx_uart_setup_ufcr() is called after the call to 
> rt_imx_uart_set_config(), but in another place it is not called after 
> the set_config() call.  I think rt_imx_uart_setup_ufcr() should be 
> removed along with the call to it during initialization but the patch 
> below does not include that mod.
> 
> 
> Finally, the fact that this made it through testing brings up the test 
> methodology for UART driver validation as concerns "character timing". 
> With this kind of bug, simple loopback tests probably pass.  In a short 
> test program that I built, I had code to read the nanosecond clock 
> before initiating transmission, and then again after reception 
> completed, and displaying the delta time.  Using that, the problem 
> showed itself immediately (the delta ns for transmission of a single 
> character was between 40K and 100K, much too fast given my 
> ioctl(SET_CONFIG) had requested 9600 baud).  I wonder if it's worth 
> adding a similar check to the testing logic.
> 
> 
> Best regards,
> 
> Steve
> 
> 
> 
> ====================================================================================== 
> 
> 
> diff --git a/kernel/drivers/serial/rt_imx_uart.c 
> b/kernel/drivers/serial/rt_imx_uart.c
> index 2b27b7d..6894c1c 100644
> --- a/kernel/drivers/serial/rt_imx_uart.c
> +++ b/kernel/drivers/serial/rt_imx_uart.c
> @@ -703,9 +703,9 @@ static int rt_imx_uart_set_config(struct 
> rt_imx_uart_ctx *ctx,
>                  uint64_t tdiv64;
> 
>                  if (ctx->config.data_bits == RTSER_8_BITS)
> -                       ucr2 = UCR2_WS | UCR2_IRTS;
> +                       ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
>                  else
> -                       ucr2 = UCR2_IRTS;
> +                       ucr2 = UCR2_SRST | UCR2_IRTS;
> 

Yep, that got broken in dcba8ed40183.


Could you write a patch in the standard format?

Subject: brief description

Commit message explaining the reason (can be simple here, just pointed 
out the inverse logic of UCR2).

Signed-off-by: ...
---
<Patch>


git format-patch does the formatting if you feed in the right content. 
Thanks!

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux