* [PATCH] serial: pl011: use DMA RX polling by default @ 2013-11-28 9:41 Linus Walleij 2013-12-09 11:53 ` [PATCH] serial: pl011: also enable DMA Rx polling with no platform data Guennadi Liakhovetski 0 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2013-11-28 9:41 UTC (permalink / raw) To: Greg Kroah-Hartman, linux-serial Cc: linux-arm-kernel, Linus Walleij, Jongsung Kim, Chanho Min, Russell King Making DMA RX polling optional when DMA is on was just over-cautious: there is one single system in the kernel tree using this facility, Ux500 and after some testing I turned this on also for Ux500, which means it should simply be on by default if DMA is enabled. Cc: Jongsung Kim <neidhard.kim@lge.com> Cc: Chanho Min <chanho.min@lge.com> Cc: Russell King <rmk+kernel@arm.linux.org.uk> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/tty/serial/amba-pl011.c | 2 +- include/linux/amba/serial.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 7203864992a5..501d96453466 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -327,7 +327,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * dmaengine_slave_config(chan, &rx_conf); uap->dmarx.chan = chan; - if (plat && plat->dma_rx_poll_enable) { + if (plat) { /* Set poll rate if specified. */ if (plat->dma_rx_poll_rate) { uap->dmarx.auto_poll_rate = false; diff --git a/include/linux/amba/serial.h b/include/linux/amba/serial.h index 0ddb5c02ad8b..0891ea0458f6 100644 --- a/include/linux/amba/serial.h +++ b/include/linux/amba/serial.h @@ -203,7 +203,6 @@ struct amba_pl011_data { bool (*dma_filter)(struct dma_chan *chan, void *filter_param); void *dma_rx_param; void *dma_tx_param; - bool dma_rx_poll_enable; unsigned int dma_rx_poll_rate; unsigned int dma_rx_poll_timeout; void (*init) (void); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] serial: pl011: also enable DMA Rx polling with no platform data 2013-11-28 9:41 [PATCH] serial: pl011: use DMA RX polling by default Linus Walleij @ 2013-12-09 11:53 ` Guennadi Liakhovetski 2013-12-09 14:33 ` Linus Walleij 2013-12-12 9:18 ` Guennadi Liakhovetski 0 siblings, 2 replies; 6+ messages in thread From: Guennadi Liakhovetski @ 2013-12-09 11:53 UTC (permalink / raw) To: Linus Walleij Cc: Greg Kroah-Hartman, linux-serial, Jongsung Kim, Chanho Min, linux-arm-kernel, Russell King An earlier patch "serial: pl011: use DMA RX polling by default" enabled DMA Rx polling on PL011 only in configurations, using platform data. A simple extension of that patch also enables DMA Rx polling when no platform data is used, e.g. in Device Tree configurations. In such cases a default poll timeout and an automatically calculated poll rate will be used. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- drivers/tty/serial/amba-pl011.c | 39 ++++++++++++++++++--------------------- 1 files changed, 18 insertions(+), 21 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index e593f8d..baecdfa 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -327,28 +327,25 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * dmaengine_slave_config(chan, &rx_conf); uap->dmarx.chan = chan; - if (plat) { - /* Set poll rate if specified. */ - if (plat->dma_rx_poll_rate) { - uap->dmarx.auto_poll_rate = false; - uap->dmarx.poll_rate = plat->dma_rx_poll_rate; - } else { - /* - * 100 ms defaults to poll rate if not - * specified. This will be adjusted with - * the baud rate at set_termios. - */ - uap->dmarx.auto_poll_rate = true; - uap->dmarx.poll_rate = 100; - } - /* 3 secs defaults poll_timeout if not specified. */ - if (plat->dma_rx_poll_timeout) - uap->dmarx.poll_timeout = - plat->dma_rx_poll_timeout; - else - uap->dmarx.poll_timeout = 3000; - } else + /* Set poll rate if specified. */ + if (plat && plat->dma_rx_poll_rate) { uap->dmarx.auto_poll_rate = false; + uap->dmarx.poll_rate = plat->dma_rx_poll_rate; + } else { + /* + * 100 ms defaults to poll rate if not + * specified. This will be adjusted with + * the baud rate at set_termios. + */ + uap->dmarx.auto_poll_rate = true; + uap->dmarx.poll_rate = 100; + } + /* 3 secs defaults poll_timeout if not specified. */ + if (plat && plat->dma_rx_poll_timeout) + uap->dmarx.poll_timeout = + plat->dma_rx_poll_timeout; + else + uap->dmarx.poll_timeout = 3000; dev_info(uap->port.dev, "DMA channel RX %s\n", dma_chan_name(uap->dmarx.chan)); -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: pl011: also enable DMA Rx polling with no platform data 2013-12-09 11:53 ` [PATCH] serial: pl011: also enable DMA Rx polling with no platform data Guennadi Liakhovetski @ 2013-12-09 14:33 ` Linus Walleij 2013-12-12 9:18 ` Guennadi Liakhovetski 1 sibling, 0 replies; 6+ messages in thread From: Linus Walleij @ 2013-12-09 14:33 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org, Jongsung Kim, Chanho Min, linux-arm-kernel@lists.infradead.org, Russell King On Mon, Dec 9, 2013 at 12:53 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > An earlier patch "serial: pl011: use DMA RX polling by default" enabled > DMA Rx polling on PL011 only in configurations, using platform data. A > simple extension of that patch also enables DMA Rx polling when no > platform data is used, e.g. in Device Tree configurations. In such cases > a default poll timeout and an automatically calculated poll rate will be > used. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Makes perfect sense. Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: pl011: also enable DMA Rx polling with no platform data 2013-12-09 11:53 ` [PATCH] serial: pl011: also enable DMA Rx polling with no platform data Guennadi Liakhovetski 2013-12-09 14:33 ` Linus Walleij @ 2013-12-12 9:18 ` Guennadi Liakhovetski 2013-12-12 13:24 ` Linus Walleij 1 sibling, 1 reply; 6+ messages in thread From: Guennadi Liakhovetski @ 2013-12-12 9:18 UTC (permalink / raw) To: Linus Walleij Cc: Greg Kroah-Hartman, Chanho Min, Phil Edworthy, linux-serial, Jongsung Kim, Russell King, linux-arm-kernel An addendum to this patch, that might well lead to dropping it and, possibly, doing the opposite - removing the DMA Rx polling completely. This patch was a logical continuation of the referenced below patch from Linus W. It doesn't make matters worse on its own, it just unifies the two cases - with platform data and with DT. However, after fixing other issues with my set up, it has been identified, that this polling is actually causing problems, rather than fixing any. In our set up we don't use the "single character DMA transfer request line," but the burst DMA request line, which means, a DMA request is only activated, when a watermark is reached. And as soon as DMA picks up data from the FIFO to drop below the threshold, the request line is deactivated again. This means, if less than a complete DMA buffer of data is received, "threshold - 1" (15 in our case) bytes will stay in UART FIFO, causing an Rx timeout interrupt, which will pick those 15 bytes up and deliver the data to the tty layer. All works beautifully. Whereas with polling we get data loss and corruption. This isn't my top priority at the moment to investigate and fix this. Please, feel free to decide whether to take this patch to unify driver's behaviour with and without DT, or to drop it to at least give some configurations a chance to work correctly, or maybe someone has an idea what exactly can be going wrong and how to fix it. To help a bit with looking for a fix, in one of the tests the first 4144 bytes have been received correctly, which = 4096 (DMA buffer size) + 32 (UART FIFO size) + 16 (half UART FIFO size). Then 45 bytes went lost (3 * 15?), then reception resumed correctly. Thanks Guennadi On Mon, 9 Dec 2013, Guennadi Liakhovetski wrote: > An earlier patch "serial: pl011: use DMA RX polling by default" enabled > DMA Rx polling on PL011 only in configurations, using platform data. A > simple extension of that patch also enables DMA Rx polling when no > platform data is used, e.g. in Device Tree configurations. In such cases > a default poll timeout and an automatically calculated poll rate will be > used. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > --- > drivers/tty/serial/amba-pl011.c | 39 ++++++++++++++++++--------------------- > 1 files changed, 18 insertions(+), 21 deletions(-) > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index e593f8d..baecdfa 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -327,28 +327,25 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * > dmaengine_slave_config(chan, &rx_conf); > uap->dmarx.chan = chan; > > - if (plat) { > - /* Set poll rate if specified. */ > - if (plat->dma_rx_poll_rate) { > - uap->dmarx.auto_poll_rate = false; > - uap->dmarx.poll_rate = plat->dma_rx_poll_rate; > - } else { > - /* > - * 100 ms defaults to poll rate if not > - * specified. This will be adjusted with > - * the baud rate at set_termios. > - */ > - uap->dmarx.auto_poll_rate = true; > - uap->dmarx.poll_rate = 100; > - } > - /* 3 secs defaults poll_timeout if not specified. */ > - if (plat->dma_rx_poll_timeout) > - uap->dmarx.poll_timeout = > - plat->dma_rx_poll_timeout; > - else > - uap->dmarx.poll_timeout = 3000; > - } else > + /* Set poll rate if specified. */ > + if (plat && plat->dma_rx_poll_rate) { > uap->dmarx.auto_poll_rate = false; > + uap->dmarx.poll_rate = plat->dma_rx_poll_rate; > + } else { > + /* > + * 100 ms defaults to poll rate if not > + * specified. This will be adjusted with > + * the baud rate at set_termios. > + */ > + uap->dmarx.auto_poll_rate = true; > + uap->dmarx.poll_rate = 100; > + } > + /* 3 secs defaults poll_timeout if not specified. */ > + if (plat && plat->dma_rx_poll_timeout) > + uap->dmarx.poll_timeout = > + plat->dma_rx_poll_timeout; > + else > + uap->dmarx.poll_timeout = 3000; > > dev_info(uap->port.dev, "DMA channel RX %s\n", > dma_chan_name(uap->dmarx.chan)); > -- > 1.7.2.5 > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: pl011: also enable DMA Rx polling with no platform data 2013-12-12 9:18 ` Guennadi Liakhovetski @ 2013-12-12 13:24 ` Linus Walleij 2013-12-17 17:34 ` Greg Kroah-Hartman 0 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2013-12-12 13:24 UTC (permalink / raw) To: Guennadi Liakhovetski, Greg Kroah-Hartman Cc: linux-serial@vger.kernel.org, Jongsung Kim, Chanho Min, linux-arm-kernel@lists.infradead.org, Russell King, Phil Edworthy On Thu, Dec 12, 2013 at 10:18 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > An addendum to this patch, that might well lead to dropping it and, > possibly, doing the opposite - removing the DMA Rx polling completely. I agree with your reasoning, but not with dropping the Rx code altogether. However we should also drop my patch so we restore the opt-in-only characteristic until we can rootcause this. Greg can you please drop or revert commit 17438217a6f5e33d920ed3821a4b857311cc2872 "serial: pl011: use DMA RX polling by default" from the TTY tree until this has been sorted out? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: pl011: also enable DMA Rx polling with no platform data 2013-12-12 13:24 ` Linus Walleij @ 2013-12-17 17:34 ` Greg Kroah-Hartman 0 siblings, 0 replies; 6+ messages in thread From: Greg Kroah-Hartman @ 2013-12-17 17:34 UTC (permalink / raw) To: Linus Walleij Cc: Guennadi Liakhovetski, linux-serial@vger.kernel.org, Jongsung Kim, Chanho Min, linux-arm-kernel@lists.infradead.org, Russell King, Phil Edworthy On Thu, Dec 12, 2013 at 02:24:37PM +0100, Linus Walleij wrote: > On Thu, Dec 12, 2013 at 10:18 AM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > > An addendum to this patch, that might well lead to dropping it and, > > possibly, doing the opposite - removing the DMA Rx polling completely. > > I agree with your reasoning, but not with dropping the Rx code > altogether. However we should also drop my patch so we restore > the opt-in-only characteristic until we can rootcause this. > > Greg can you please drop or revert > commit 17438217a6f5e33d920ed3821a4b857311cc2872 > "serial: pl011: use DMA RX polling by default" > from the TTY tree until this has been sorted out? Now reverted. thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-17 17:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-28 9:41 [PATCH] serial: pl011: use DMA RX polling by default Linus Walleij 2013-12-09 11:53 ` [PATCH] serial: pl011: also enable DMA Rx polling with no platform data Guennadi Liakhovetski 2013-12-09 14:33 ` Linus Walleij 2013-12-12 9:18 ` Guennadi Liakhovetski 2013-12-12 13:24 ` Linus Walleij 2013-12-17 17:34 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).