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@xxxxxxxxxx>
> > > > Cc: Uwe-Kleine König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > > 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