[PATCH v2 0/2] serial_mxc: fixing reception
while writing to a imx-serial device is probably thoroughly tested - and obviosly works for the debug-serial - using a serial driver to read data received over the serial interface does not work reliably.
the patches in this series address issues found during the implementation of a custom uboot-command to query a coprocessor, connected to an imx8mm over uart4, for mainboard-identification strings
(no changes since v1)
Johannes Schneider (2): serial: mxc: enable the RX pipeline serial: mxc: have putc use the TXFIFO
drivers/serial/serial_mxc.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
on imx8(mm) the RXDMUXSEL needs to be set for data going over the wire (as observable on a connected 'scope) to actually make it into the RXFIFO
the reference manual is not overly clear about this, and only mentiones that "UCR3_RXDMUXSEL should always be set." - and since the CR3 register reverts to its reset values after setting the baudrate, setting this bit is done during '_mxc_serial_setbgr'
Signed-off-by: Johannes Schneider johannes.schneider@leica-geosystems.com
---
(no changes since v1)
drivers/serial/serial_mxc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index 70a0e5e919..5f283cc635 100644 --- a/drivers/serial/serial_mxc.c +++ b/drivers/serial/serial_mxc.c @@ -61,6 +61,11 @@ #define UCR3_AWAKEN (1<<4) /* Async wake interrupt enable */ #define UCR3_REF25 (1<<3) /* Ref freq 25 MHz */ #define UCR3_REF30 (1<<2) /* Ref Freq 30 MHz */ + +//imx8 names these bitsfields instead: +#define UCR3_DTRDEN BIT(3) /* bit not used in this chip */ +#define UCR3_RXDMUXSEL BIT(2) /* RXD muxed input selected; 'should always be set' */ + #define UCR3_INVT (1<<1) /* Inverted Infrared transmission */ #define UCR3_BPEN (1<<0) /* Preset registers enable */ #define UCR4_CTSTL_32 (32<<10) /* CTS trigger level (32 chars) */ @@ -176,6 +181,12 @@ static void _mxc_serial_setbrg(struct mxc_uart *base, unsigned long clk,
writel(UCR2_WS | UCR2_IRTS | UCR2_RXEN | UCR2_TXEN | UCR2_SRST, &base->cr2); + + // setting the baudrate triggers a reset, returning cr3 to its + // reset value but UCR3_RXDMUXSEL "should always be set." + // according to the imx8 reference-manual + writel(readl(&base->cr3) | UCR3_RXDMUXSEL, &base->cr3); + writel(UCR1_UARTEN, &base->cr1); }
On 9/5/2022 5:58 PM, Johannes Schneider wrote:
on imx8(mm) the RXDMUXSEL needs to be set for data going over the wire (as observable on a connected 'scope) to actually make it into the RXFIFO
the reference manual is not overly clear about this, and only mentiones that "UCR3_RXDMUXSEL should always be set." - and since the CR3 register reverts to its reset values after setting the baudrate, setting this bit is done during '_mxc_serial_setbgr'
Signed-off-by: Johannes Schneider johannes.schneider@leica-geosystems.com
(no changes since v1)
drivers/serial/serial_mxc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index 70a0e5e919..5f283cc635 100644 --- a/drivers/serial/serial_mxc.c +++ b/drivers/serial/serial_mxc.c @@ -61,6 +61,11 @@ #define UCR3_AWAKEN (1<<4) /* Async wake interrupt enable */ #define UCR3_REF25 (1<<3) /* Ref freq 25 MHz */ #define UCR3_REF30 (1<<2) /* Ref Freq 30 MHz */
+//imx8 names these bitsfields instead: +#define UCR3_DTRDEN BIT(3) /* bit not used in this chip */ +#define UCR3_RXDMUXSEL BIT(2) /* RXD muxed input selected; 'should always be set' */
- #define UCR3_INVT (1<<1) /* Inverted Infrared transmission */ #define UCR3_BPEN (1<<0) /* Preset registers enable */ #define UCR4_CTSTL_32 (32<<10) /* CTS trigger level (32 chars) */
@@ -176,6 +181,12 @@ static void _mxc_serial_setbrg(struct mxc_uart *base, unsigned long clk,
writel(UCR2_WS | UCR2_IRTS | UCR2_RXEN | UCR2_TXEN | UCR2_SRST, &base->cr2);
- // setting the baudrate triggers a reset, returning cr3 to its
- // reset value but UCR3_RXDMUXSEL "should always be set."
- // according to the imx8 reference-manual
- writel(readl(&base->cr3) | UCR3_RXDMUXSEL, &base->cr3);
Please fix comment format.
I just give a look at i.MX6QDL, i.MX7D, i.MX8M*, this bit should be set for them all.
Thanks, Peng.
- writel(UCR1_UARTEN, &base->cr1); }
On Tue, Sep 06, 2022 at 09:04:05AM +0800, Peng Fan wrote:
On 9/5/2022 5:58 PM, Johannes Schneider wrote:
on imx8(mm) the RXDMUXSEL needs to be set for data going over the wire (as observable on a connected 'scope) to actually make it into the RXFIFO
the reference manual is not overly clear about this, and only mentiones that "UCR3_RXDMUXSEL should always be set." - and since the CR3 register reverts to its reset values after setting the baudrate, setting this bit is done during '_mxc_serial_setbgr'
Signed-off-by: Johannes Schneider johannes.schneider@leica-geosystems.com
(no changes since v1)
drivers/serial/serial_mxc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index 70a0e5e919..5f283cc635 100644 --- a/drivers/serial/serial_mxc.c +++ b/drivers/serial/serial_mxc.c @@ -61,6 +61,11 @@ #define UCR3_AWAKEN (1<<4) /* Async wake interrupt enable */ #define UCR3_REF25 (1<<3) /* Ref freq 25 MHz */ #define UCR3_REF30 (1<<2) /* Ref Freq 30 MHz */
+//imx8 names these bitsfields instead: +#define UCR3_DTRDEN BIT(3) /* bit not used in this chip */ +#define UCR3_RXDMUXSEL BIT(2) /* RXD muxed input selected; 'should always be set' */
- #define UCR3_INVT (1<<1) /* Inverted Infrared transmission */ #define UCR3_BPEN (1<<0) /* Preset registers enable */ #define UCR4_CTSTL_32 (32<<10) /* CTS trigger level (32 chars) */
@@ -176,6 +181,12 @@ static void _mxc_serial_setbrg(struct mxc_uart *base, unsigned long clk, writel(UCR2_WS | UCR2_IRTS | UCR2_RXEN | UCR2_TXEN | UCR2_SRST, &base->cr2);
- // setting the baudrate triggers a reset, returning cr3 to its
- // reset value but UCR3_RXDMUXSEL "should always be set."
- // according to the imx8 reference-manual
- writel(readl(&base->cr3) | UCR3_RXDMUXSEL, &base->cr3);
Please fix comment format.
We allow '//' style comments, just like the linux kernel does these days. There should be a space before the "imx8" in the first hunk here.
On Mon, Sep 5, 2022 at 10:43 PM Tom Rini trini@konsulko.com wrote:
We allow '//' style comments, just like the linux kernel does these days. There should be a space before the "imx8" in the first hunk here.
Yes, // style comments are allowed, but for consistency with the other comments in this file, it would be better to keep using the /* style.
Mixing the two styles in the same file makes it confusing.
only waiting for TXEMPTY leads to corrupted messages going over the wire - which is fixed by making use of the FIFO
Signed-off-by: Johannes Schneider johannes.schneider@leica-geosystems.com ---
(no changes since v1)
drivers/serial/serial_mxc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index 5f283cc635..1e0add7281 100644 --- a/drivers/serial/serial_mxc.c +++ b/drivers/serial/serial_mxc.c @@ -309,7 +309,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch) struct mxc_serial_plat *plat = dev_get_plat(dev); struct mxc_uart *const uart = plat->reg;
- if (!(readl(&uart->ts) & UTS_TXEMPTY)) + if (readl(&uart->ts) & UTS_TXFULL) return -EAGAIN;
writel(ch, &uart->txd);
On 9/5/2022 5:58 PM, Johannes Schneider wrote:
only waiting for TXEMPTY leads to corrupted messages going over the wire - which is fixed by making use of the FIFO
Could you please explain a bit more, why waiting TXEMPTY could lead to corrupted message?
Thanks, Peng.
Signed-off-by: Johannes Schneider johannes.schneider@leica-geosystems.com
(no changes since v1)
drivers/serial/serial_mxc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index 5f283cc635..1e0add7281 100644 --- a/drivers/serial/serial_mxc.c +++ b/drivers/serial/serial_mxc.c @@ -309,7 +309,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch) struct mxc_serial_plat *plat = dev_get_plat(dev); struct mxc_uart *const uart = plat->reg;
- if (!(readl(&uart->ts) & UTS_TXEMPTY))
if (readl(&uart->ts) & UTS_TXFULL) return -EAGAIN;
writel(ch, &uart->txd);
Hi Peng Fan,
... well i'm actually not sure why the messages get corrupted; it's just what i observed on a connected oscilloscope :-s the code that does uses the transmit functions is making use of the returned -EAGAIN and retries - and even strewing in some delays to wait for the buffer or checking the status register didn't resolve the issue -- switching the serical driver from locking onto the single send-register to actually checking the fifo fixed it though.
i can put these "vague reasons" in the commit message... but i dont/didn't think theyd help much (?)
regards ________________________________ From: Peng Fan peng.fan@oss.nxp.com Sent: Tuesday, September 6, 2022 02:54 To: SCHNEIDER Johannes johannes.schneider@leica-geosystems.com; u-boot@lists.denx.de u-boot@lists.denx.de Cc: trini trini@konsulko.com; GEO-CHHER-bsp-development bsp-development.geo@leica-geosystems.com; festevam@denx.de festevam@denx.de; sbabic@denx.de sbabic@denx.de; Pali Rohár pali@kernel.org Subject: Re: [PATCH v2 2/2] serial: mxc: have putc use the TXFIFO
This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
On 9/5/2022 5:58 PM, Johannes Schneider wrote:
only waiting for TXEMPTY leads to corrupted messages going over the wire - which is fixed by making use of the FIFO
Could you please explain a bit more, why waiting TXEMPTY could lead to corrupted message?
Thanks, Peng.
Signed-off-by: Johannes Schneider johannes.schneider@leica-geosystems.com
(no changes since v1)
drivers/serial/serial_mxc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index 5f283cc635..1e0add7281 100644 --- a/drivers/serial/serial_mxc.c +++ b/drivers/serial/serial_mxc.c @@ -309,7 +309,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch) struct mxc_serial_plat *plat = dev_get_plat(dev); struct mxc_uart *const uart = plat->reg;
if (!(readl(&uart->ts) & UTS_TXEMPTY))
if (readl(&uart->ts) & UTS_TXFULL) return -EAGAIN; writel(ch, &uart->txd);
On 9/6/2022 11:38 AM, SCHNEIDER Johannes wrote:
Hi Peng Fan,
... well i'm actually not sure why the messages get corrupted; it's just what i observed on a connected oscilloscope :-s the code that does uses the transmit functions is making use of the returned -EAGAIN and retries - and even strewing in some delays to wait for the buffer or checking the status register didn't resolve the issue -- switching the serical driver from locking onto the single send-register to actually checking the fifo fixed it though.
i can put these "vague reasons" in the commit message... but i dont/didn't think theyd help much (?)
That's fine, I just wanna know more details. Maybe add one line "followling linux kernel i.MX8M* uart driver, check UTS_TXFULL, not UTS_EMPTY."
Regards, Peng.
regards ________________________________ From: Peng Fan peng.fan@oss.nxp.com Sent: Tuesday, September 6, 2022 02:54 To: SCHNEIDER Johannes johannes.schneider@leica-geosystems.com; u-boot@lists.denx.de u-boot@lists.denx.de Cc: trini trini@konsulko.com; GEO-CHHER-bsp-development bsp-development.geo@leica-geosystems.com; festevam@denx.de festevam@denx.de; sbabic@denx.de sbabic@denx.de; Pali Rohár pali@kernel.org Subject: Re: [PATCH v2 2/2] serial: mxc: have putc use the TXFIFO
This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
On 9/5/2022 5:58 PM, Johannes Schneider wrote:
only waiting for TXEMPTY leads to corrupted messages going over the wire - which is fixed by making use of the FIFO
Could you please explain a bit more, why waiting TXEMPTY could lead to corrupted message?
Thanks, Peng.
Signed-off-by: Johannes Schneider johannes.schneider@leica-geosystems.com
(no changes since v1)
drivers/serial/serial_mxc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index 5f283cc635..1e0add7281 100644 --- a/drivers/serial/serial_mxc.c +++ b/drivers/serial/serial_mxc.c @@ -309,7 +309,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch) struct mxc_serial_plat *plat = dev_get_plat(dev); struct mxc_uart *const uart = plat->reg;
if (!(readl(&uart->ts) & UTS_TXEMPTY))
if (readl(&uart->ts) & UTS_TXFULL) return -EAGAIN; writel(ch, &uart->txd);
Hi,
and thanks, that reference in the kernel uart driver is/might be actually better - my "reference" was the barebox sourcecode - which also checks the TXFULL instead
i'll update the message and send out a v3
regards
________________________________ From: Peng Fan peng.fan@oss.nxp.com Sent: Tuesday, September 6, 2022 07:15 To: SCHNEIDER Johannes johannes.schneider@leica-geosystems.com; u-boot@lists.denx.de u-boot@lists.denx.de Cc: trini trini@konsulko.com; GEO-CHHER-bsp-development bsp-development.geo@leica-geosystems.com; festevam@denx.de festevam@denx.de; sbabic@denx.de sbabic@denx.de; Pali Rohár pali@kernel.org Subject: Re: [PATCH v2 2/2] serial: mxc: have putc use the TXFIFO
[You don't often get email from peng.fan@oss.nxp.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
On 9/6/2022 11:38 AM, SCHNEIDER Johannes wrote:
Hi Peng Fan,
... well i'm actually not sure why the messages get corrupted; it's just what i observed on a connected oscilloscope :-s the code that does uses the transmit functions is making use of the returned -EAGAIN and retries - and even strewing in some delays to wait for the buffer or checking the status register didn't resolve the issue -- switching the serical driver from locking onto the single send-register to actually checking the fifo fixed it though.
i can put these "vague reasons" in the commit message... but i dont/didn't think theyd help much (?)
That's fine, I just wanna know more details. Maybe add one line "followling linux kernel i.MX8M* uart driver, check UTS_TXFULL, not UTS_EMPTY."
Regards, Peng.
regards ________________________________ From: Peng Fan peng.fan@oss.nxp.com Sent: Tuesday, September 6, 2022 02:54 To: SCHNEIDER Johannes johannes.schneider@leica-geosystems.com; u-boot@lists.denx.de u-boot@lists.denx.de Cc: trini trini@konsulko.com; GEO-CHHER-bsp-development bsp-development.geo@leica-geosystems.com; festevam@denx.de festevam@denx.de; sbabic@denx.de sbabic@denx.de; Pali Rohár pali@kernel.org Subject: Re: [PATCH v2 2/2] serial: mxc: have putc use the TXFIFO
This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
On 9/5/2022 5:58 PM, Johannes Schneider wrote:
only waiting for TXEMPTY leads to corrupted messages going over the wire - which is fixed by making use of the FIFO
Could you please explain a bit more, why waiting TXEMPTY could lead to corrupted message?
Thanks, Peng.
Signed-off-by: Johannes Schneider johannes.schneider@leica-geosystems.com
(no changes since v1)
drivers/serial/serial_mxc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index 5f283cc635..1e0add7281 100644 --- a/drivers/serial/serial_mxc.c +++ b/drivers/serial/serial_mxc.c @@ -309,7 +309,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch) struct mxc_serial_plat *plat = dev_get_plat(dev); struct mxc_uart *const uart = plat->reg;
if (!(readl(&uart->ts) & UTS_TXEMPTY))
if (readl(&uart->ts) & UTS_TXFULL) return -EAGAIN; writel(ch, &uart->txd);
participants (5)
-
Fabio Estevam -
Johannes Schneider -
Peng Fan -
SCHNEIDER Johannes
-
Tom Rini