[PATCH] amba-pl011: do not disable RTS during shutdown

[PATCH] amba-pl011: do not disable RTS during shutdown

[Shreshtha Kumar SAHU]

From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>

In present driver, shutdown clears RTS in CR register. But the
documentation "Documentation/serial/driver" suggests not to
disable RTS in shutdown(). Also RTS is preserved between shutdown
and startup calls, i.e. it is restored in startup if it was enabled
during shutdown. So that if autorts is set and RTS is set using
pl011_set_mctrl then it should continue even after shutdown->startup
sequence. And hence during set_termios it will enable RTS only if RTS
bit is set in UARTx_CR register. For throttling/unthrottling user
should call pl011_set_mctrl.

Change-Id: I743f33fb10e7e655657cd5dae1ec585e914a65bc
Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/tty/serial/amba-pl011.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6958594..46a4690 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -160,6 +160,7 @@ struct uart_amba_port {
 	unsigned int		lcrh_tx;	/* vendor-specific */
 	unsigned int		lcrh_rx;	/* vendor-specific */
 	bool			autorts;
+	bool			rts_state;	/* state during shutdown */
 	char			type[12];
 	bool			interrupt_may_hang; /* vendor-specific */
 #ifdef CONFIG_DMA_ENGINE
@@ -1412,6 +1413,8 @@ static int pl011_startup(struct uart_port *port)
 		barrier();
 
 	cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
+	if (uap->rts_state)
+		cr |= UART011_CR_RTS;
 	writew(cr, uap->port.membase + UART011_CR);
 
 	/* Clear pending error interrupts */
@@ -1469,6 +1472,7 @@ static void pl011_shutdown_channel(struct uart_amba_port *uap,
 static void pl011_shutdown(struct uart_port *port)
 {
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;
+	unsigned int cr;
 
 	/*
 	 * disable all interrupts
@@ -1488,9 +1492,19 @@ static void pl011_shutdown(struct uart_port *port)
 
 	/*
 	 * disable the port
+	 * disable the port. It should not disable RTS.
+	 * Also RTS state should be preserved to restore
+	 * it during startup().
 	 */
 	uap->autorts = false;
-	writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
+	cr = readw(uap->port.membase + UART011_CR);
+	if (cr & UART011_CR_RTS) {
+		uap->rts_state = true;
+		cr = UART011_CR_RTS;
+	} else
+		uap->rts_state = false;
+	cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
+	writew(cr, uap->port.membase + UART011_CR);
 
 	/*
 	 * disable break condition and fifos
@@ -1905,6 +1919,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	uap->vendor = vendor;
 	uap->lcrh_rx = vendor->lcrh_rx;
 	uap->lcrh_tx = vendor->lcrh_tx;
+	uap->rts_state = false;
 	uap->fifosize = vendor->fifosize;
 	uap->interrupt_may_hang = vendor->interrupt_may_hang;
 	uap->port.dev = &dev->dev;
-- 
1.7.4.3

Re: [PATCH] amba-pl011: do not disable RTS during shutdown

[Greg KH]

On Tue, Jan 17, 2012 at 03:59:49PM +0530, Shreshtha Kumar SAHU wrote:
> From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
> 
> In present driver, shutdown clears RTS in CR register. But the
> documentation "Documentation/serial/driver" suggests not to
> disable RTS in shutdown(). Also RTS is preserved between shutdown
> and startup calls, i.e. it is restored in startup if it was enabled
> during shutdown. So that if autorts is set and RTS is set using
> pl011_set_mctrl then it should continue even after shutdown->startup
> sequence. And hence during set_termios it will enable RTS only if RTS
> bit is set in UARTx_CR register. For throttling/unthrottling user
> should call pl011_set_mctrl.
> 
> Change-Id: I743f33fb10e7e655657cd5dae1ec585e914a65bc

What is this field, and why would we want it in the kernel.org
changelogs?

Please remove it.  Same goes for your other patch.

greg k-h

Re: [PATCH] amba-pl011: do not disable RTS during shutdown

[Russell King]

On Tue, Jan 17, 2012 at 03:59:49PM +0530, Shreshtha Kumar SAHU wrote:
> From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
> 
> In present driver, shutdown clears RTS in CR register. But the
> documentation "Documentation/serial/driver" suggests not to
> disable RTS in shutdown(). Also RTS is preserved between shutdown
> and startup calls, i.e. it is restored in startup if it was enabled
> during shutdown. So that if autorts is set and RTS is set using
> pl011_set_mctrl then it should continue even after shutdown->startup
> sequence. And hence during set_termios it will enable RTS only if RTS
> bit is set in UARTx_CR register. For throttling/unthrottling user
> should call pl011_set_mctrl.
> 
> Change-Id: I743f33fb10e7e655657cd5dae1ec585e914a65bc

Please get rid of this line.

> Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/tty/serial/amba-pl011.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 6958594..46a4690 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -160,6 +160,7 @@ struct uart_amba_port {
>  	unsigned int		lcrh_tx;	/* vendor-specific */
>  	unsigned int		lcrh_rx;	/* vendor-specific */
>  	bool			autorts;
> +	bool			rts_state;	/* state during shutdown */

Do we really need a variable to track this?

>  	char			type[12];
>  	bool			interrupt_may_hang; /* vendor-specific */
>  #ifdef CONFIG_DMA_ENGINE
> @@ -1412,6 +1413,8 @@ static int pl011_startup(struct uart_port *port)
>  		barrier();
>  
>  	cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
> +	if (uap->rts_state)
> +		cr |= UART011_CR_RTS;
>  	writew(cr, uap->port.membase + UART011_CR);

Note that this should preserve DTR as well.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH] amba-pl011: do not disable RTS during shutdown

[Shreshtha Kumar SAHU]

On Tue, Jan 17, 2012 at 19:40:48 +0100, Russell King wrote:
> On Tue, Jan 17, 2012 at 03:59:49PM +0530, Shreshtha Kumar SAHU wrote:
> > From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
> > 
> > In present driver, shutdown clears RTS in CR register. But the
> > documentation "Documentation/serial/driver" suggests not to
> > disable RTS in shutdown(). Also RTS is preserved between shutdown
> > and startup calls, i.e. it is restored in startup if it was enabled
> > during shutdown. So that if autorts is set and RTS is set using
> > pl011_set_mctrl then it should continue even after shutdown->startup
> > sequence. And hence during set_termios it will enable RTS only if RTS
> > bit is set in UARTx_CR register. For throttling/unthrottling user
> > should call pl011_set_mctrl.
> > 
> > Change-Id: I743f33fb10e7e655657cd5dae1ec585e914a65bc
> 
> Please get rid of this line.
> 
done.
> > Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/tty/serial/amba-pl011.c |   17 ++++++++++++++++-
> >  1 files changed, 16 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 6958594..46a4690 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -160,6 +160,7 @@ struct uart_amba_port {
> >  	unsigned int		lcrh_tx;	/* vendor-specific */
> >  	unsigned int		lcrh_rx;	/* vendor-specific */
> >  	bool			autorts;
> > +	bool			rts_state;	/* state during shutdown */
> 
> Do we really need a variable to track this?
>
As register content will be reset during low power state after shutdown.
Hence we have to preserve it during shutdown.

> >  	char			type[12];
> >  	bool			interrupt_may_hang; /* vendor-specific */
> >  #ifdef CONFIG_DMA_ENGINE
> > @@ -1412,6 +1413,8 @@ static int pl011_startup(struct uart_port *port)
> >  		barrier();
> >  
> >  	cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
> > +	if (uap->rts_state)
> > +		cr |= UART011_CR_RTS;
> >  	writew(cr, uap->port.membase + UART011_CR);
> 
> Note that this should preserve DTR as well.
>
In the attached patch, DTR is preserved and restored.

> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:

Best Regards,
Shreshtha

>From 2a9062f4c3fd198a64a96779d6e74b324ddb81e4 Mon Sep 17 00:00:00 2001
From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Date: Tue, 17 Jan 2012 11:00:26 +0530
Subject: [PATCHv2] amba-pl011: do not disable RTS during shutdown

In present driver, shutdown clears RTS and DTR in CR register. But the
documentation "Documentation/serial/driver" suggests not to disable
RTS and DTR in shutdown(). Also RTS and DTR is preserved between shutdown
and startup calls, i.e. these are restored in startup if they were enabled
while doing shutdown. So that if RTS and DTR are set using pl011_set_mctrl
then it should continue even after shutdown->startup sequence.
For throttling/unthrottling user should call pl011_set_mctrl.

Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/tty/serial/amba-pl011.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6958594..673be75 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -160,6 +160,7 @@ struct uart_amba_port {
 	unsigned int		lcrh_tx;	/* vendor-specific */
 	unsigned int		lcrh_rx;	/* vendor-specific */
 	bool			autorts;
+	bool			old_cr;		/* state during shutdown */
 	char			type[12];
 	bool			interrupt_may_hang; /* vendor-specific */
 #ifdef CONFIG_DMA_ENGINE
@@ -1411,7 +1412,9 @@ static int pl011_startup(struct uart_port *port)
 	while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
 		barrier();
 
-	cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
+	/* restore RTS and DTR */
+	cr = (uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR));
+	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);
 
 	/* Clear pending error interrupts */
@@ -1469,6 +1472,7 @@ static void pl011_shutdown_channel(struct uart_amba_port *uap,
 static void pl011_shutdown(struct uart_port *port)
 {
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;
+	unsigned int cr;
 
 	/*
 	 * disable all interrupts
@@ -1488,9 +1492,16 @@ static void pl011_shutdown(struct uart_port *port)
 
 	/*
 	 * disable the port
+	 * disable the port. It should not disable RTS and DTR.
+	 * Also RTS and DTR state should be preserved to restore
+	 * it during startup().
 	 */
 	uap->autorts = false;
-	writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
+	cr = readw(uap->port.membase + UART011_CR);
+	uap->old_cr = cr;
+	cr &= (UART011_CR_RTS | UART011_CR_DTR);
+	cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
+	writew(cr, uap->port.membase + UART011_CR);
 
 	/*
 	 * disable break condition and fifos
@@ -1905,6 +1916,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	uap->vendor = vendor;
 	uap->lcrh_rx = vendor->lcrh_rx;
 	uap->lcrh_tx = vendor->lcrh_tx;
+	uap->old_cr = 0;
 	uap->fifosize = vendor->fifosize;
 	uap->interrupt_may_hang = vendor->interrupt_may_hang;
 	uap->port.dev = &dev->dev;
-- 
1.7.4.3

Re: [PATCH] amba-pl011: do not disable RTS during shutdown

[Shreshtha Kumar SAHU]

data type of old_cr was bool in patchv2, corrected in this patchv3

>From 5500b7d25b5382026bc9392a9b44c3c69bf172b0 Mon Sep 17 00:00:00 2001
From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Date: Tue, 17 Jan 2012 11:00:26 +0530
Subject: [PATCHv3] amba-pl011: do not disable RTS during shutdown

In present driver, shutdown clears RTS and DTR in CR register. But the
documentation "Documentation/serial/driver" suggests not to disable
RTS and DTR in shutdown(). Also RTS and DTR is preserved between shutdown
and startup calls, i.e. these are restored in startup if they were enabled
while doing shutdown. So that if RTS and DTR are set using pl011_set_mctrl
then it should continue even after shutdown->startup sequence.
For throttling/unthrottling user should call pl011_set_mctrl.

Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/tty/serial/amba-pl011.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6958594..62ff7da 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -159,6 +159,7 @@ struct uart_amba_port {
 	unsigned int		fifosize;	/* vendor-specific */
 	unsigned int		lcrh_tx;	/* vendor-specific */
 	unsigned int		lcrh_rx;	/* vendor-specific */
+	unsigned int		old_cr;		/* state during shutdown */
 	bool			autorts;
 	char			type[12];
 	bool			interrupt_may_hang; /* vendor-specific */
@@ -1411,7 +1412,9 @@ static int pl011_startup(struct uart_port *port)
 	while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
 		barrier();
 
-	cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
+	/* restore RTS and DTR */
+	cr = (uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR));
+	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);
 
 	/* Clear pending error interrupts */
@@ -1469,6 +1472,7 @@ static void pl011_shutdown_channel(struct uart_amba_port *uap,
 static void pl011_shutdown(struct uart_port *port)
 {
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;
+	unsigned int cr;
 
 	/*
 	 * disable all interrupts
@@ -1488,9 +1492,16 @@ static void pl011_shutdown(struct uart_port *port)
 
 	/*
 	 * disable the port
+	 * disable the port. It should not disable RTS and DTR.
+	 * Also RTS and DTR state should be preserved to restore
+	 * it during startup().
 	 */
 	uap->autorts = false;
-	writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
+	cr = readw(uap->port.membase + UART011_CR);
+	uap->old_cr = cr;
+	cr &= (UART011_CR_RTS | UART011_CR_DTR);
+	cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
+	writew(cr, uap->port.membase + UART011_CR);
 
 	/*
 	 * disable break condition and fifos
@@ -1905,6 +1916,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	uap->vendor = vendor;
 	uap->lcrh_rx = vendor->lcrh_rx;
 	uap->lcrh_tx = vendor->lcrh_tx;
+	uap->old_cr = 0;
 	uap->fifosize = vendor->fifosize;
 	uap->interrupt_may_hang = vendor->interrupt_may_hang;
 	uap->port.dev = &dev->dev;
-- 
1.7.4.3

Re: [PATCH] amba-pl011: do not disable RTS during shutdown

[Russell King]

On Wed, Jan 18, 2012 at 03:15:06PM +0530, Shreshtha Kumar SAHU wrote:
> data type of old_cr was bool in patchv2, corrected in this patchv3

This is much better, just a small little niggle with it.

> @@ -1411,7 +1412,9 @@ static int pl011_startup(struct uart_port *port)
>  	while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
>  		barrier();
>  
> -	cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
> +	/* restore RTS and DTR */
> +	cr = (uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR));

You don't need the outer set of parens around this (additional
unnecessary parens can cause confusion when trying to read code.)

> @@ -1488,9 +1492,16 @@ static void pl011_shutdown(struct uart_port *port)
>  
>  	/*
>  	 * disable the port
> +	 * disable the port. It should not disable RTS and DTR.
> +	 * Also RTS and DTR state should be preserved to restore
> +	 * it during startup().
>  	 */
>  	uap->autorts = false;
> -	writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
> +	cr = readw(uap->port.membase + UART011_CR);
> +	uap->old_cr = cr;
> +	cr &= (UART011_CR_RTS | UART011_CR_DTR);

Ditto.

Once those are fixed:

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH] amba-pl011: do not disable RTS during shutdown

[Shreshtha Kumar SAHU]

>From 1b9b0de2d7fd099d775e03065ad96414e7c4ed60 Mon Sep 17 00:00:00 2001
From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Date: Tue, 17 Jan 2012 11:00:26 +0530
Subject: [PATCH v4] amba-pl011: do not disable RTS during shutdown

In present driver, shutdown clears RTS and DTR in CR register. But the
documentation "Documentation/serial/driver" suggests not to disable
RTS and DTR in shutdown(). Also RTS and DTR is preserved between shutdown
and startup calls, i.e. these are restored in startup if they were enabled
while doing shutdown. So that if RTS and DTR are set using pl011_set_mctrl
then it should continue even after shutdown->startup sequence.
For throttling/unthrottling user should call pl011_set_mctrl.

Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/tty/serial/amba-pl011.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6958594..d155e45 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -159,6 +159,7 @@ struct uart_amba_port {
 	unsigned int		fifosize;	/* vendor-specific */
 	unsigned int		lcrh_tx;	/* vendor-specific */
 	unsigned int		lcrh_rx;	/* vendor-specific */
+	unsigned int		old_cr;		/* state during shutdown */
 	bool			autorts;
 	char			type[12];
 	bool			interrupt_may_hang; /* vendor-specific */
@@ -1411,7 +1412,9 @@ static int pl011_startup(struct uart_port *port)
 	while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
 		barrier();
 
-	cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
+	/* restore RTS and DTR */
+	cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR);
+	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);
 
 	/* Clear pending error interrupts */
@@ -1469,6 +1472,7 @@ static void pl011_shutdown_channel(struct uart_amba_port *uap,
 static void pl011_shutdown(struct uart_port *port)
 {
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;
+	unsigned int cr;
 
 	/*
 	 * disable all interrupts
@@ -1488,9 +1492,16 @@ static void pl011_shutdown(struct uart_port *port)
 
 	/*
 	 * disable the port
+	 * disable the port. It should not disable RTS and DTR.
+	 * Also RTS and DTR state should be preserved to restore
+	 * it during startup().
 	 */
 	uap->autorts = false;
-	writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
+	cr = readw(uap->port.membase + UART011_CR);
+	uap->old_cr = cr;
+	cr &= UART011_CR_RTS | UART011_CR_DTR;
+	cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
+	writew(cr, uap->port.membase + UART011_CR);
 
 	/*
 	 * disable break condition and fifos
@@ -1905,6 +1916,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	uap->vendor = vendor;
 	uap->lcrh_rx = vendor->lcrh_rx;
 	uap->lcrh_tx = vendor->lcrh_tx;
+	uap->old_cr = 0;
 	uap->fifosize = vendor->fifosize;
 	uap->interrupt_may_hang = vendor->interrupt_may_hang;
 	uap->port.dev = &dev->dev;
-- 
1.7.4.3