[PATCH] serial: imx: Improve PIO prevention if TX DMA has been started

[PATCH] serial: imx: Improve PIO prevention if TX DMA has been started

[Clemens Gruber]

The imx_transmit_buffer function should return if TX DMA has already
been started and not just skip over the buffer PIO write loop. (Which
did fix the initial problem, but could have unintentional side-effects)

Tested on an i.MX6Q board with half-duplex RS-485 and with RS-232.

Cc: Ian Jamison <ian.dev@arkver.com>
Cc: Uwe-Kleine König <u.kleine-koenig@pengutronix.de>
Fixes: 514ab34dbad6 ("serial: imx: Prevent TX buffer PIO write when a
DMA has been started")
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/tty/serial/imx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 80934e7bd67f..fce538eb8c77 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -452,13 +452,14 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
 		if (sport->dma_is_txing) {
 			temp |= UCR1_TDMAEN;
 			writel(temp, sport->port.membase + UCR1);
+			return;
 		} else {
 			writel(temp, sport->port.membase + UCR1);
 			imx_dma_tx(sport);
 		}
 	}
 
-	while (!uart_circ_empty(xmit) && !sport->dma_is_txing &&
+	while (!uart_circ_empty(xmit) &&
 	       !(readl(sport->port.membase + uts_reg(sport)) & UTS_TXFULL)) {
 		/* send xmit->buf[xmit->tail]
 		 * out the port here */
-- 
2.14.1

Re: [PATCH] serial: imx: Improve PIO prevention if TX DMA has been started

[Uwe Kleine-König]

On Sat, Aug 12, 2017 at 05:12:10PM +0200, Clemens Gruber wrote:
> The imx_transmit_buffer function should return if TX DMA has already
> been started and not just skip over the buffer PIO write loop. (Which
> did fix the initial problem, but could have unintentional side-effects)
> 
> Tested on an i.MX6Q board with half-duplex RS-485 and with RS-232.
> 
> Cc: Ian Jamison <ian.dev@arkver.com>
> Cc: Uwe-Kleine König <u.kleine-koenig@pengutronix.de>
> Fixes: 514ab34dbad6 ("serial: imx: Prevent TX buffer PIO write when a
> DMA has been started")

AFAIK no newline in the Fixes: line.

> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
>  drivers/tty/serial/imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 80934e7bd67f..fce538eb8c77 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -452,13 +452,14 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
>  		if (sport->dma_is_txing) {
>  			temp |= UCR1_TDMAEN;
>  			writel(temp, sport->port.membase + UCR1);
> +			return;
>  		} else {
>  			writel(temp, sport->port.membase + UCR1);
>  			imx_dma_tx(sport);
>  		}

Shouldn't the return go here?

Did you understand the problem? Can you say why this only hurts in RS485
half-duplex but not (as it seems) in regular rs232 mode?

>  	}
>  
> -	while (!uart_circ_empty(xmit) && !sport->dma_is_txing &&
> +	while (!uart_circ_empty(xmit) &&
>  	       !(readl(sport->port.membase + uts_reg(sport)) & UTS_TXFULL)) {
>  		/* send xmit->buf[xmit->tail]
>  		 * out the port here */

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] serial: imx: Improve PIO prevention if TX DMA has been started

[Clemens Gruber]

On Sat, Aug 12, 2017 at 09:54:51PM +0200, Uwe Kleine-König wrote:
> On Sat, Aug 12, 2017 at 05:12:10PM +0200, Clemens Gruber wrote:
> > The imx_transmit_buffer function should return if TX DMA has already
> > been started and not just skip over the buffer PIO write loop. (Which
> > did fix the initial problem, but could have unintentional side-effects)
> > 
> > Tested on an i.MX6Q board with half-duplex RS-485 and with RS-232.
> > 
> > Cc: Ian Jamison <ian.dev@arkver.com>
> > Cc: Uwe-Kleine König <u.kleine-koenig@pengutronix.de>
> > Fixes: 514ab34dbad6 ("serial: imx: Prevent TX buffer PIO write when a
> > DMA has been started")
> 
> AFAIK no newline in the Fixes: line.

Thanks. A checkpatch warning for this would be great.

> 
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> >  drivers/tty/serial/imx.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 80934e7bd67f..fce538eb8c77 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -452,13 +452,14 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
> >  		if (sport->dma_is_txing) {
> >  			temp |= UCR1_TDMAEN;
> >  			writel(temp, sport->port.membase + UCR1);
> > +			return;
> >  		} else {
> >  			writel(temp, sport->port.membase + UCR1);
> >  			imx_dma_tx(sport);
> >  		}
> 
> Shouldn't the return go here?

Yes, it can also go here (and probably should). The problem of
xmit->tail jumping over xmit->head occurs only if we are already DMA
txing and then go into the PIO loop, but not the first time after
calling imx_dma_tx. That's why the v1 passed the tests too.
I'll have to conduct a few more tests and if they succeed I'll send a
v2 where we return in both cases (already txing and starting to).

> Did you understand the problem? Can you say why this only hurts in RS485
> half-duplex but not (as it seems) in regular rs232 mode?

I am not sure anyone understands (yet) why it a) only hurts RS-485 and
b) only occurs on SMP systems.
If you have more insight, please share it. :)

Cheers,
Clemens

Re: [PATCH] serial: imx: Improve PIO prevention if TX DMA has been started

[Uwe Kleine-König]

Hello Clemens,

On Sun, Aug 13, 2017 at 12:07:56AM +0200, Clemens Gruber wrote:
> On Sat, Aug 12, 2017 at 09:54:51PM +0200, Uwe Kleine-König wrote:
> > On Sat, Aug 12, 2017 at 05:12:10PM +0200, Clemens Gruber wrote:
> > > The imx_transmit_buffer function should return if TX DMA has already
> > > been started and not just skip over the buffer PIO write loop. (Which
> > > did fix the initial problem, but could have unintentional side-effects)
> > > 
> > > Tested on an i.MX6Q board with half-duplex RS-485 and with RS-232.
> > > 
> > > Cc: Ian Jamison <ian.dev@arkver.com>
> > > Cc: Uwe-Kleine König <u.kleine-koenig@pengutronix.de>
> > > Fixes: 514ab34dbad6 ("serial: imx: Prevent TX buffer PIO write when a
> > > DMA has been started")
> > 
> > AFAIK no newline in the Fixes: line.
> 
> Thanks. A checkpatch warning for this would be great.

I assume that is a note to yourself to look into that? :-)

> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 80934e7bd67f..fce538eb8c77 100644
> > > --- a/drivers/tty/serial/imx.c
> > > +++ b/drivers/tty/serial/imx.c
> > > @@ -452,13 +452,14 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
> > >  		if (sport->dma_is_txing) {
> > >  			temp |= UCR1_TDMAEN;
> > >  			writel(temp, sport->port.membase + UCR1);
> > > +			return;
> > >  		} else {
> > >  			writel(temp, sport->port.membase + UCR1);
> > >  			imx_dma_tx(sport);
> > >  		}
> > 
> > Shouldn't the return go here?
> 
> Yes, it can also go here (and probably should). The problem of
> xmit->tail jumping over xmit->head occurs only if we are already DMA
> txing and then go into the PIO loop, but not the first time after
> calling imx_dma_tx. That's why the v1 passed the tests too.
> I'll have to conduct a few more tests and if they succeed I'll send a
> v2 where we return in both cases (already txing and starting to).
> 
> > Did you understand the problem? Can you say why this only hurts in RS485
> > half-duplex but not (as it seems) in regular rs232 mode?
> 
> I am not sure anyone understands (yet) why it a) only hurts RS-485 and
> b) only occurs on SMP systems.
> If you have more insight, please share it. :)

I asked because I thought you might have understood it before patching
it ...

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] serial: imx: Improve PIO prevention if TX DMA has been started

[Clemens Gruber]

Hello Uwe,

On Mon, Aug 14, 2017 at 08:51:49AM +0200, Uwe Kleine-König wrote:
> Hello Clemens,
> 
> On Sun, Aug 13, 2017 at 12:07:56AM +0200, Clemens Gruber wrote:
> > On Sat, Aug 12, 2017 at 09:54:51PM +0200, Uwe Kleine-König wrote:
> > > On Sat, Aug 12, 2017 at 05:12:10PM +0200, Clemens Gruber wrote:
> > > > The imx_transmit_buffer function should return if TX DMA has already
> > > > been started and not just skip over the buffer PIO write loop. (Which
> > > > did fix the initial problem, but could have unintentional side-effects)
> > > > 
> > > > Tested on an i.MX6Q board with half-duplex RS-485 and with RS-232.
> > > > 
> > > > Cc: Ian Jamison <ian.dev@arkver.com>
> > > > Cc: Uwe-Kleine König <u.kleine-koenig@pengutronix.de>
> > > > Fixes: 514ab34dbad6 ("serial: imx: Prevent TX buffer PIO write when a
> > > > DMA has been started")
> > > 
> > > AFAIK no newline in the Fixes: line.
> > 
> > Thanks. A checkpatch warning for this would be great.
> 
> I assume that is a note to yourself to look into that? :-)

It's on my TODO list ;)

> 
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index 80934e7bd67f..fce538eb8c77 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -452,13 +452,14 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
> > > >  		if (sport->dma_is_txing) {
> > > >  			temp |= UCR1_TDMAEN;
> > > >  			writel(temp, sport->port.membase + UCR1);
> > > > +			return;
> > > >  		} else {
> > > >  			writel(temp, sport->port.membase + UCR1);
> > > >  			imx_dma_tx(sport);
> > > >  		}
> > > 
> > > Shouldn't the return go here?
> > 
> > Yes, it can also go here (and probably should). The problem of
> > xmit->tail jumping over xmit->head occurs only if we are already DMA
> > txing and then go into the PIO loop, but not the first time after
> > calling imx_dma_tx. That's why the v1 passed the tests too.
> > I'll have to conduct a few more tests and if they succeed I'll send a
> > v2 where we return in both cases (already txing and starting to).
> > 
> > > Did you understand the problem? Can you say why this only hurts in RS485
> > > half-duplex but not (as it seems) in regular rs232 mode?
> > 
> > I am not sure anyone understands (yet) why it a) only hurts RS-485 and
> > b) only occurs on SMP systems.
> > If you have more insight, please share it. :)
> 
> I asked because I thought you might have understood it before patching
> it ...

Yeah, this patch went out way too early, sorry for that! :/

@gregkh: Please ignore this patch!

About the underlying problem (b) why it only occurs on SMP systems:
I think Ian's theory is correct:
DMA is started, then the PIO is done until the xmit buffer is empty and
immediately after that, DMA is stopped.
On SMP systems, where the DMA TX thread can run on another core, it is
already too late.

Regarding problem (a) why it only hurts RS-485: One possibility could be
the timing difference / additional delay due to for example toggling the
transmit-enable GPIO via mctrl_gpio_set.
Meaning that with RS-232 on SMP systems DMA is also stopped just early
enough to not bork the circular xmit buffer.

If this is true then the imx driver did not really use TX DMA in
practice before.

Thoughts?

I'll try to trace this next week to verify these hypotheses.

Best regards,
Clemens

Re: [PATCH] serial: imx: Improve PIO prevention if TX DMA has been started

[Uwe Kleine-König]

On Mon, Aug 14, 2017 at 08:38:04PM +0200, Clemens Gruber wrote:
> Hello Uwe,
> 
> On Mon, Aug 14, 2017 at 08:51:49AM +0200, Uwe Kleine-König wrote:
> > Hello Clemens,
> > 
> > On Sun, Aug 13, 2017 at 12:07:56AM +0200, Clemens Gruber wrote:
> > > On Sat, Aug 12, 2017 at 09:54:51PM +0200, Uwe Kleine-König wrote:
> > > > On Sat, Aug 12, 2017 at 05:12:10PM +0200, Clemens Gruber wrote:
> > > > > The imx_transmit_buffer function should return if TX DMA has already
> > > > > been started and not just skip over the buffer PIO write loop. (Which
> > > > > did fix the initial problem, but could have unintentional side-effects)
> > > > > 
> > > > > Tested on an i.MX6Q board with half-duplex RS-485 and with RS-232.
> > > > > 
> > > > > Cc: Ian Jamison <ian.dev@arkver.com>
> > > > > Cc: Uwe-Kleine König <u.kleine-koenig@pengutronix.de>
> > > > > Fixes: 514ab34dbad6 ("serial: imx: Prevent TX buffer PIO write when a
> > > > > DMA has been started")
> > > > 
> > > > AFAIK no newline in the Fixes: line.
> > > 
> > > Thanks. A checkpatch warning for this would be great.
> > 
> > I assume that is a note to yourself to look into that? :-)
> 
> It's on my TODO list ;)
> 
> > 
> > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > > index 80934e7bd67f..fce538eb8c77 100644
> > > > > --- a/drivers/tty/serial/imx.c
> > > > > +++ b/drivers/tty/serial/imx.c
> > > > > @@ -452,13 +452,14 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
> > > > >  		if (sport->dma_is_txing) {
> > > > >  			temp |= UCR1_TDMAEN;
> > > > >  			writel(temp, sport->port.membase + UCR1);
> > > > > +			return;
> > > > >  		} else {
> > > > >  			writel(temp, sport->port.membase + UCR1);
> > > > >  			imx_dma_tx(sport);
> > > > >  		}
> > > > 
> > > > Shouldn't the return go here?
> > > 
> > > Yes, it can also go here (and probably should). The problem of
> > > xmit->tail jumping over xmit->head occurs only if we are already DMA
> > > txing and then go into the PIO loop, but not the first time after
> > > calling imx_dma_tx. That's why the v1 passed the tests too.
> > > I'll have to conduct a few more tests and if they succeed I'll send a
> > > v2 where we return in both cases (already txing and starting to).
> > > 
> > > > Did you understand the problem? Can you say why this only hurts in RS485
> > > > half-duplex but not (as it seems) in regular rs232 mode?
> > > 
> > > I am not sure anyone understands (yet) why it a) only hurts RS-485 and
> > > b) only occurs on SMP systems.
> > > If you have more insight, please share it. :)
> > 
> > I asked because I thought you might have understood it before patching
> > it ...
> 
> Yeah, this patch went out way too early, sorry for that! :/
> 
> @gregkh: Please ignore this patch!
> 
> About the underlying problem (b) why it only occurs on SMP systems:
> I think Ian's theory is correct:
> DMA is started, then the PIO is done until the xmit buffer is empty and
> immediately after that, DMA is stopped.
> On SMP systems, where the DMA TX thread can run on another core, it is
> already too late.
> 
> Regarding problem (a) why it only hurts RS-485: One possibility could be
> the timing difference / additional delay due to for example toggling the
> transmit-enable GPIO via mctrl_gpio_set.
> Meaning that with RS-232 on SMP systems DMA is also stopped just early
> enough to not bork the circular xmit buffer.
> 
> If this is true then the imx driver did not really use TX DMA in
> practice before.
> 
> Thoughts?

This matches my theory about this driver and its (not-)usage of DMA.
 
> I'll try to trace this next week to verify these hypotheses.

Great. That's earlier than I would find time for that.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] serial: imx: Improve PIO prevention if TX DMA has been started

[Ian Jamison]

Hi,

On 14/08/17 19:38, Clemens Gruber wrote:
> Hello Uwe,
>
> On Mon, Aug 14, 2017 at 08:51:49AM +0200, Uwe Kleine-König wrote:
>> Hello Clemens,
>>
>> On Sun, Aug 13, 2017 at 12:07:56AM +0200, Clemens Gruber wrote:
>>> On Sat, Aug 12, 2017 at 09:54:51PM +0200, Uwe Kleine-König wrote:
>>>> On Sat, Aug 12, 2017 at 05:12:10PM +0200, Clemens Gruber wrote:
>>>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>>>> index 80934e7bd67f..fce538eb8c77 100644
>>>>> --- a/drivers/tty/serial/imx.c
>>>>> +++ b/drivers/tty/serial/imx.c
>>>>> @@ -452,13 +452,14 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
>>>>>   		if (sport->dma_is_txing) {
>>>>>   			temp |= UCR1_TDMAEN;
>>>>>   			writel(temp, sport->port.membase + UCR1);
>>>>> +			return;
>>>>>   		} else {
>>>>>   			writel(temp, sport->port.membase + UCR1);
>>>>>   			imx_dma_tx(sport);
>>>>>   		}
>>>> Shouldn't the return go here?
>>> Yes, it can also go here (and probably should). The problem of
>>> xmit->tail jumping over xmit->head occurs only if we are already DMA
>>> txing and then go into the PIO loop, but not the first time after
>>> calling imx_dma_tx. That's why the v1 passed the tests too.
>>> I'll have to conduct a few more tests and if they succeed I'll send a
>>> v2 where we return in both cases (already txing and starting to).
>>>
>>>> Did you understand the problem? Can you say why this only hurts in RS485
>>>> half-duplex but not (as it seems) in regular rs232 mode?
>>> I am not sure anyone understands (yet) why it a) only hurts RS-485 and
>>> b) only occurs on SMP systems.
>>> If you have more insight, please share it. :)
>> I asked because I thought you might have understood it before patching
>> it ...
> Yeah, this patch went out way too early, sorry for that! :/
>
> @gregkh: Please ignore this patch!
>
> About the underlying problem (b) why it only occurs on SMP systems:
> I think Ian's theory is correct:
> DMA is started, then the PIO is done until the xmit buffer is empty and
> immediately after that, DMA is stopped.
> On SMP systems, where the DMA TX thread can run on another core, it is
> already too late.
>
> Regarding problem (a) why it only hurts RS-485: One possibility could be
> the timing difference / additional delay due to for example toggling the
> transmit-enable GPIO via mctrl_gpio_set.
> Meaning that with RS-232 on SMP systems DMA is also stopped just early
> enough to not bork the circular xmit buffer.
>
> If this is true then the imx driver did not really use TX DMA in
> practice before.
>
> Thoughts?
>
> I'll try to trace this next week to verify these hypotheses.
>
This sounds plausible to me. I guess you could try adding a GPIO wiggle
in non-RS485 mode (or even a usleep) to see if it makes the problem
more noticeable.

I had broken my build for a while there, but am now testing 4.13-rc5
with RS485 mode and DMA enabled and it seems OK since my v1 patch
is included now. I also tried removing my change to the while loop and
adding a return before it (if dma_is_txing), as Uwe suggested, and that
also seems to work fine. My tests are not very exhaustive though. I
can post that as a patch if you like, or I can test your v2 if you prefer.

Regards,
Ian Jamison,
Arkver.