[PATCH] serial: pl011: use DMA RX polling by default

[PATCH] serial: pl011: use DMA RX polling by default

[Linus Walleij]

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

[PATCH] serial: pl011: use DMA RX polling by default

[Linus Walleij]

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

[PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Guennadi Liakhovetski]

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

[PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Guennadi Liakhovetski]

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

Re: [PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Linus Walleij]

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

[PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Linus Walleij]

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

Re: [PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Guennadi Liakhovetski]

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/

[PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Guennadi Liakhovetski]

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/

Re: [PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Linus Walleij]

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

[PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Linus Walleij]

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

Re: [PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Greg Kroah-Hartman]

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

[PATCH] serial: pl011: also enable DMA Rx polling with no platform data

[Greg Kroah-Hartman]

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