* 3.1 rt_imx_uart.c regression
@ 2020-02-10 1:28 steve freyder
2020-02-10 6:53 ` Jan Kiszka
0 siblings, 1 reply; 2+ messages in thread
From: steve freyder @ 2020-02-10 1:28 UTC (permalink / raw)
To: xenomai
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;
if (ctx->config.handshake == RTSER_RTSCTS_HAND) {
if (port->have_rtscts) {
======================================================================================
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: 3.1 rt_imx_uart.c regression
2020-02-10 1:28 3.1 rt_imx_uart.c regression steve freyder
@ 2020-02-10 6:53 ` Jan Kiszka
0 siblings, 0 replies; 2+ messages in thread
From: Jan Kiszka @ 2020-02-10 6:53 UTC (permalink / raw)
To: steve freyder, xenomai
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-02-10 6:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10 1:28 3.1 rt_imx_uart.c regression steve freyder
2020-02-10 6:53 ` Jan Kiszka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.