linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] serial/core: Initialize the console pm state
@ 2014-09-22  6:30 Sudhir Sreedharan
  2014-10-01 17:27 ` Kevin Hilman
  0 siblings, 1 reply; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-09-22  6:30 UTC (permalink / raw)
  To: linux-serial; +Cc: gregkh, jslaby, linux-kernel, Sudhir Sreedharan

For console devices having UART_CAP_SLEEP capability, the uart_pm_state has
to be initialized to UART_PM_STATE_ON. Otherwise the LCR regiser values
are reinitialized when uart_change_pm is called from uart_configure_port.

Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
---
 drivers/tty/serial/serial_core.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 29a7be4..91bfd52 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2615,6 +2615,9 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	if (uport->cons && uport->dev)
 		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
 
+	if (uart_console(uport))
+		state->pm_state = UART_PM_STATE_ON;
+
 	uart_configure_port(drv, state, uport);
 
 	num_groups = 2;
-- 
1.7.0.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] serial/core: Initialize the console pm state
  2014-09-22  6:30 [PATCH] serial/core: Initialize the console pm state Sudhir Sreedharan
@ 2014-10-01 17:27 ` Kevin Hilman
  2014-10-03  4:35   ` Greg KH
                     ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Kevin Hilman @ 2014-10-01 17:27 UTC (permalink / raw)
  To: Sudhir Sreedharan
  Cc: linux-serial, gregkh, jslaby, lkml, olof, linux-arm-kernel

On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
<ssreedharan@mvista.com> wrote:
> For console devices having UART_CAP_SLEEP capability, the uart_pm_state has
> to be initialized to UART_PM_STATE_ON. Otherwise the LCR regiser values
> are reinitialized when uart_change_pm is called from uart_configure_port.
>
> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>

Multiple boot failures on ARM[1] were bisected down to this patch.

How was this patch tested, and on which platforms?

Also, the changelog states that this should be done only for
UART_CAP_SLEEP, but the patch does it for every UART.

Greg, I suggest this patch be dropped from tty-next until it has been
better described and tested.

Kevin

[1] http://lists.linaro.org/pipermail/kernel-build-reports/2014-October/005550.html

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] serial/core: Initialize the console pm state
  2014-10-01 17:27 ` Kevin Hilman
@ 2014-10-03  4:35   ` Greg KH
  2014-10-03 12:22   ` Geert Uytterhoeven
  2014-10-06  9:27   ` Sudhir Sreedharan
  2 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2014-10-03  4:35 UTC (permalink / raw)
  To: Kevin Hilman
  Cc: Sudhir Sreedharan, linux-serial, jslaby, lkml, olof, linux-arm-kernel

On Wed, Oct 01, 2014 at 10:27:20AM -0700, Kevin Hilman wrote:
> On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
> <ssreedharan@mvista.com> wrote:
> > For console devices having UART_CAP_SLEEP capability, the uart_pm_state has
> > to be initialized to UART_PM_STATE_ON. Otherwise the LCR regiser values
> > are reinitialized when uart_change_pm is called from uart_configure_port.
> >
> > Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
> 
> Multiple boot failures on ARM[1] were bisected down to this patch.
> 
> How was this patch tested, and on which platforms?
> 
> Also, the changelog states that this should be done only for
> UART_CAP_SLEEP, but the patch does it for every UART.
> 
> Greg, I suggest this patch be dropped from tty-next until it has been
> better described and tested.

Now reverted, thanks for letting me know.

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] serial/core: Initialize the console pm state
  2014-10-01 17:27 ` Kevin Hilman
  2014-10-03  4:35   ` Greg KH
@ 2014-10-03 12:22   ` Geert Uytterhoeven
  2014-10-06  9:48     ` Sudhir Sreedharan
  2014-10-06  9:27   ` Sudhir Sreedharan
  2 siblings, 1 reply; 18+ messages in thread
From: Geert Uytterhoeven @ 2014-10-03 12:22 UTC (permalink / raw)
  To: Kevin Hilman
  Cc: Sudhir Sreedharan, linux-serial, Greg KH, Jiri Slaby, lkml,
	Olof Johansson, linux-arm-kernel

On Wed, Oct 1, 2014 at 7:27 PM, Kevin Hilman <khilman@kernel.org> wrote:
> On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
> <ssreedharan@mvista.com> wrote:
>> For console devices having UART_CAP_SLEEP capability, the uart_pm_state has
>> to be initialized to UART_PM_STATE_ON. Otherwise the LCR regiser values
>> are reinitialized when uart_change_pm is called from uart_configure_port.
>>
>> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
>
> Multiple boot failures on ARM[1] were bisected down to this patch.
>
> How was this patch tested, and on which platforms?
>
> Also, the changelog states that this should be done only for
> UART_CAP_SLEEP, but the patch does it for every UART.
>
> Greg, I suggest this patch be dropped from tty-next until it has been
> better described and tested.
>
> Kevin
>
> [1] http://lists.linaro.org/pipermail/kernel-build-reports/2014-October/005550.html

Perhaps it should call "uart_change_pm(state, UART_PM_STATE_ON)"
instead, so the driver's .pm() method is called?

UART_CAP_SLEEP seems to be an 8250-only flag.

BTW, this was the first commit I reverted when I had an issue with a serial
console yesterday, but it didn't seem to have any influence (for me).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] serial/core: Initialize the console pm state
  2014-10-01 17:27 ` Kevin Hilman
  2014-10-03  4:35   ` Greg KH
  2014-10-03 12:22   ` Geert Uytterhoeven
@ 2014-10-06  9:27   ` Sudhir Sreedharan
  2014-10-09 13:42     ` Sudhir Sreedharan
  2 siblings, 1 reply; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-10-06  9:27 UTC (permalink / raw)
  To: Kevin Hilman; +Cc: linux-serial, gregkh, jslaby, lkml, olof, linux-arm-kernel

Hi Kevin,

On Wed, Oct 1, 2014 at 10:57 PM, Kevin Hilman <khilman@kernel.org> wrote:
> On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
> <ssreedharan@mvista.com> wrote:
>> For console devices having UART_CAP_SLEEP capability, the uart_pm_state has
>> to be initialized to UART_PM_STATE_ON. Otherwise the LCR regiser values
>> are reinitialized when uart_change_pm is called from uart_configure_port.
>>
>> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
>
> Multiple boot failures on ARM[1] were bisected down to this patch.
>
> How was this patch tested, and on which platforms?



This patch was tested on x86-64(haswell) board, which uses ST16650V2
uart(which has UART_CAP_SLEEP).
While serial driver gets initialized, console port LCR register is
getting reinitalized to 0.
Then boot logs will be seen as garbage characters.

I will re-check why this failed on the boards/archs you mentioned.

Thanks,
Sudhir

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] serial/core: Initialize the console pm state
  2014-10-03 12:22   ` Geert Uytterhoeven
@ 2014-10-06  9:48     ` Sudhir Sreedharan
  0 siblings, 0 replies; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-10-06  9:48 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Kevin Hilman, linux-serial, Greg KH, Jiri Slaby, lkml,
	Olof Johansson, linux-arm-kernel

Hi Geert,

On Fri, Oct 3, 2014 at 5:52 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Oct 1, 2014 at 7:27 PM, Kevin Hilman <khilman@kernel.org> wrote:
>> On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
>> <ssreedharan@mvista.com> wrote:
>>> For console devices having UART_CAP_SLEEP capability, the uart_pm_state has
>>> to be initialized to UART_PM_STATE_ON. Otherwise the LCR regiser values
>>> are reinitialized when uart_change_pm is called from uart_configure_port.
>>>
>>> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
>>
>> Multiple boot failures on ARM[1] were bisected down to this patch.
>>
>> How was this patch tested, and on which platforms?
>>
>> Also, the changelog states that this should be done only for
>> UART_CAP_SLEEP, but the patch does it for every UART.
>>
>> Greg, I suggest this patch be dropped from tty-next until it has been
>> better described and tested.
>>
>> Kevin
>>
>> [1] http://lists.linaro.org/pipermail/kernel-build-reports/2014-October/005550.html
>
> Perhaps it should call "uart_change_pm(state, UART_PM_STATE_ON)"
> instead, so the driver's .pm() method is called?
>

If  "uart_change_pm(state, UART_PM_STATE_ON);" is called, it will
reinitialize the LCR register, thus changing the configuration of
console port. This will throw garbage characters from the point where
the serial driver initializes till startup is called from userland.

Thanks,
Sudhir

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] serial/core: Initialize the console pm state
  2014-10-06  9:27   ` Sudhir Sreedharan
@ 2014-10-09 13:42     ` Sudhir Sreedharan
  2014-10-09 13:45       ` [PATCH] tty: serial: 8250_core: Do not call set_sleep for console port Sudhir Sreedharan
  0 siblings, 1 reply; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-10-09 13:42 UTC (permalink / raw)
  To: Kevin Hilman, Greg KH
  Cc: linux-serial, Jiri Slaby, lkml, Olof Johansson, linux-arm-kernel

Hi,

On Mon, Oct 6, 2014 at 2:57 PM, Sudhir Sreedharan
<ssreedharan@mvista.com> wrote:
>>
>> Multiple boot failures on ARM[1] were bisected down to this patch.
>>
>> How was this patch tested, and on which platforms?
>
>
>
> This patch was tested on x86-64(haswell) board, which uses ST16650V2
> uart(which has UART_CAP_SLEEP).
> While serial driver gets initialized, console port LCR register is
> getting reinitalized to 0.
> Then boot logs will be seen as garbage characters.
>
> I will re-check why this failed on the boards/archs you mentioned.


The issue is, in the boot logs, once the serial driver gets
initialized, it throws garbage in the console. The serial device being
used is ST16550V2 which is having SLEEP functionality. So when
uart_configure_port is called, it calls the serial8250_set_sleep and
set the LCR register to 0.

The previous patch got failed because those are not based on 8250 and
the do_pm functionality is different. Eg. for arndale board, in
s3c24xx_serial_pm it uses the clock enable and disable functionality.

I have created a new patch which will be confined only to 8250 based
serial devices. I have tested it on x86-64 based haswell board, ARM64
based, P5040 powerpc which all uses 8250 based serial device.

>
> Thanks,
> Sudhir

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] tty: serial: 8250_core: Do not call set_sleep for console port
  2014-10-09 13:42     ` Sudhir Sreedharan
@ 2014-10-09 13:45       ` Sudhir Sreedharan
  2014-10-09 14:45         ` Peter Hurley
  0 siblings, 1 reply; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-10-09 13:45 UTC (permalink / raw)
  To: gregkh, khilman
  Cc: linux-serial, jslaby, linux-kernel, olof, linux-arm-kernel,
	geert, Sudhir Sreedharan

In ST16650V2 based serial uarts, while initalizing the PM state,
LCR registers are being initialized to 0 in serial8250_set_sleep().
If console port is already initialized and being used, this will
throws garbage in the console.

Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
---
 drivers/tty/serial/8250/8250_core.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index ca5cfdc..994aa25 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2618,6 +2618,9 @@ void serial8250_do_pm(struct uart_port *port, unsigned int state,
 {
 	struct uart_8250_port *p = up_to_u8250p(port);
 
+	if (port->cons && (port->cons->flags & CON_ENABLED) && (state == 0))
+		return;
+
 	serial8250_set_sleep(p, state != 0);
 }
 EXPORT_SYMBOL(serial8250_do_pm);
-- 
1.7.0.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] tty: serial: 8250_core: Do not call set_sleep for console port
  2014-10-09 13:45       ` [PATCH] tty: serial: 8250_core: Do not call set_sleep for console port Sudhir Sreedharan
@ 2014-10-09 14:45         ` Peter Hurley
  2014-10-15  6:43           ` [PATCH] tty: serial: 8250_core: restore the LCR register in set_sleep Sudhir Sreedharan
  0 siblings, 1 reply; 18+ messages in thread
From: Peter Hurley @ 2014-10-09 14:45 UTC (permalink / raw)
  To: Sudhir Sreedharan, gregkh, khilman
  Cc: linux-serial, jslaby, linux-kernel, olof, linux-arm-kernel, geert

Hi Sudhir,

On 10/09/2014 09:45 AM, Sudhir Sreedharan wrote:
> In ST16650V2 based serial uarts, while initalizing the PM state,
> LCR registers are being initialized to 0 in serial8250_set_sleep().
> If console port is already initialized and being used, this will
> throws garbage in the console.
> 
> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
> ---
>  drivers/tty/serial/8250/8250_core.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index ca5cfdc..994aa25 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -2618,6 +2618,9 @@ void serial8250_do_pm(struct uart_port *port, unsigned int state,
>  {
>  	struct uart_8250_port *p = up_to_u8250p(port);
>  
> +	if (port->cons && (port->cons->flags & CON_ENABLED) && (state == 0))
> +		return;
> +
>  	serial8250_set_sleep(p, state != 0);
>  }
>  EXPORT_SYMBOL(serial8250_do_pm);

Please just fix serial8250_set_sleep() register programming to save and restore the
LCR.

You could also fix:
1. preserving the other EFR bits
2. acquiring the port lock around the register programming. Not entirely sure it's
absolutely necessary because the serial core serializes most heavy duty register
programming with port mutex, but it's probably wise anyway.

Regards,
Peter Hurley


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] tty: serial: 8250_core: restore the LCR register in set_sleep
  2014-10-09 14:45         ` Peter Hurley
@ 2014-10-15  6:43           ` Sudhir Sreedharan
  2014-10-16  7:21             ` Kevin Hilman
  2014-10-16 12:35             ` Peter Hurley
  0 siblings, 2 replies; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-10-15  6:43 UTC (permalink / raw)
  To: peter, gregkh, khilman
  Cc: linux-serial, jslaby, linux-kernel, olof, linux-arm-kernel,
	geert, Sudhir Sreedharan

In ST16650V2 based serial uarts, while initalizing the PM state,
LCR registers are being initialized to 0 in serial8250_set_sleep().
If console port is already initialized and being used, this will
throws garbage in the console.

Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
---
 drivers/tty/serial/8250/8250_core.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index ca5cfdc..e054482 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -595,6 +595,7 @@ static void serial8250_rpm_put_tx(struct uart_8250_port *p)
  */
 static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 {
+	unsigned char lcr, efr;
 	/*
 	 * Exar UARTs have a SLEEP register that enables or disables
 	 * each UART to enter sleep mode separately.  On the XR17V35x the
@@ -611,6 +612,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 
 	if (p->capabilities & UART_CAP_SLEEP) {
 		if (p->capabilities & UART_CAP_EFR) {
+			lcr = serial_in(p, UART_LCR);
+			efr = serial_in(p, UART_EFR);
 			serial_out(p, UART_LCR, UART_LCR_CONF_MODE_B);
 			serial_out(p, UART_EFR, UART_EFR_ECB);
 			serial_out(p, UART_LCR, 0);
@@ -618,8 +621,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 		serial_out(p, UART_IER, sleep ? UART_IERX_SLEEP : 0);
 		if (p->capabilities & UART_CAP_EFR) {
 			serial_out(p, UART_LCR, UART_LCR_CONF_MODE_B);
-			serial_out(p, UART_EFR, 0);
-			serial_out(p, UART_LCR, 0);
+			serial_out(p, UART_EFR, sleep ? 0 : efr);
+			serial_out(p, UART_LCR, sleep ? 0 : lcr);
 		}
 	}
 out:
-- 
1.7.0.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] tty: serial: 8250_core: restore the LCR register in set_sleep
  2014-10-15  6:43           ` [PATCH] tty: serial: 8250_core: restore the LCR register in set_sleep Sudhir Sreedharan
@ 2014-10-16  7:21             ` Kevin Hilman
  2014-10-16 10:15               ` Sudhir Sreedharan
  2014-10-16 12:35             ` Peter Hurley
  1 sibling, 1 reply; 18+ messages in thread
From: Kevin Hilman @ 2014-10-16  7:21 UTC (permalink / raw)
  To: Sudhir Sreedharan
  Cc: peter, gregkh, linux-serial, jslaby, linux-kernel, olof,
	linux-arm-kernel, geert

Sudhir Sreedharan <ssreedharan@mvista.com> writes:

> In ST16650V2 based serial uarts, while initalizing the PM state,
> LCR registers are being initialized to 0 in serial8250_set_sleep().
> If console port is already initialized and being used, this will
> throws garbage in the console.
>
> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>

Since this caused regressions in -next last time, could you describe how
this was tested, and on what platforms?

Thanks,

Kevin

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tty: serial: 8250_core: restore the LCR register in set_sleep
  2014-10-16  7:21             ` Kevin Hilman
@ 2014-10-16 10:15               ` Sudhir Sreedharan
  0 siblings, 0 replies; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-10-16 10:15 UTC (permalink / raw)
  To: Kevin Hilman
  Cc: peter, Greg KH, linux-serial, Jiri Slaby, lkml, Olof Johansson,
	linux-arm-kernel, Geert Uytterhoeven

Hi Kevin,

On Thu, Oct 16, 2014 at 12:51 PM, Kevin Hilman <khilman@kernel.org> wrote:
> Sudhir Sreedharan <ssreedharan@mvista.com> writes:
>
>> In ST16650V2 based serial uarts, while initalizing the PM state,
>> LCR registers are being initialized to 0 in serial8250_set_sleep().
>> If console port is already initialized and being used, this will
>> throws garbage in the console.
>>
>> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
>
> Since this caused regressions in -next last time, could you describe how
> this was tested, and on what platforms?

I have tested this on arm64 board((U6_16550A)),PowerPC P5040
board(16550A)), x86-64 (haswell(ST16650V2),  Rangeley(16550A)) board.
This patch will modify only for the 8250 based serial devices.

Test Case : Booting Multiple times, suspend-resume.

Thanks,
Sudhir

>
> Thanks,
>
> Kevin

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tty: serial: 8250_core: restore the LCR register in set_sleep
  2014-10-15  6:43           ` [PATCH] tty: serial: 8250_core: restore the LCR register in set_sleep Sudhir Sreedharan
  2014-10-16  7:21             ` Kevin Hilman
@ 2014-10-16 12:35             ` Peter Hurley
  2014-10-17 10:25               ` Sudhir Sreedharan
  1 sibling, 1 reply; 18+ messages in thread
From: Peter Hurley @ 2014-10-16 12:35 UTC (permalink / raw)
  To: Sudhir Sreedharan, gregkh, khilman
  Cc: linux-serial, jslaby, linux-kernel, olof, linux-arm-kernel, geert

On 10/15/2014 02:43 AM, Sudhir Sreedharan wrote:
> In ST16650V2 based serial uarts, while initalizing the PM state,
> LCR registers are being initialized to 0 in serial8250_set_sleep().
> If console port is already initialized and being used, this will
> throws garbage in the console.
> 
> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
> ---
>  drivers/tty/serial/8250/8250_core.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index ca5cfdc..e054482 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -595,6 +595,7 @@ static void serial8250_rpm_put_tx(struct uart_8250_port *p)
>   */
>  static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
>  {
> +	unsigned char lcr, efr;
>  	/*
>  	 * Exar UARTs have a SLEEP register that enables or disables
>  	 * each UART to enter sleep mode separately.  On the XR17V35x the
> @@ -611,6 +612,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
>  
>  	if (p->capabilities & UART_CAP_SLEEP) {
>  		if (p->capabilities & UART_CAP_EFR) {
> +			lcr = serial_in(p, UART_LCR);
> +			efr = serial_in(p, UART_EFR);
>  			serial_out(p, UART_LCR, UART_LCR_CONF_MODE_B);
>  			serial_out(p, UART_EFR, UART_EFR_ECB);
>  			serial_out(p, UART_LCR, 0);
> @@ -618,8 +621,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
>  		serial_out(p, UART_IER, sleep ? UART_IERX_SLEEP : 0);
>  		if (p->capabilities & UART_CAP_EFR) {
>  			serial_out(p, UART_LCR, UART_LCR_CONF_MODE_B);
> -			serial_out(p, UART_EFR, 0);
> -			serial_out(p, UART_LCR, 0);
> +			serial_out(p, UART_EFR, sleep ? 0 : efr);
> +			serial_out(p, UART_LCR, sleep ? 0 : lcr);

Why is it necessary to clear EFR and LCR here? Does the UART not
power down?

UARTs with CAP_SLEEP but not CAP_EFR don't clear LCR before sleep.

However, if there is some kind of intentional side-effect here,
then a comment should note that.

Regards,
Peter Hurley

>  		}
>  	}
>  out:
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tty: serial: 8250_core: restore the LCR register in set_sleep
  2014-10-16 12:35             ` Peter Hurley
@ 2014-10-17 10:25               ` Sudhir Sreedharan
  2014-10-17 12:39                 ` [PATCH v1] " Sudhir Sreedharan
  0 siblings, 1 reply; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-10-17 10:25 UTC (permalink / raw)
  To: Peter Hurley
  Cc: Greg KH, Kevin Hilman, linux-serial, Jiri Slaby, lkml,
	Olof Johansson, linux-arm-kernel, Geert Uytterhoeven

Hi Peter,

On Thu, Oct 16, 2014 at 6:05 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 10/15/2014 02:43 AM, Sudhir Sreedharan wrote:
>> In ST16650V2 based serial uarts, while initalizing the PM state,
>> LCR registers are being initialized to 0 in serial8250_set_sleep().
>> If console port is already initialized and being used, this will
>> throws garbage in the console.
>>
>> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
>> ---
>>  drivers/tty/serial/8250/8250_core.c |    7 +++++--
>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index ca5cfdc..e054482 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -595,6 +595,7 @@ static void serial8250_rpm_put_tx(struct uart_8250_port *p)
>>   */
>>  static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
>>  {
>> +     unsigned char lcr, efr;
>>       /*
>>        * Exar UARTs have a SLEEP register that enables or disables
>>        * each UART to enter sleep mode separately.  On the XR17V35x the
>> @@ -611,6 +612,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
>>
>>       if (p->capabilities & UART_CAP_SLEEP) {
>>               if (p->capabilities & UART_CAP_EFR) {
>> +                     lcr = serial_in(p, UART_LCR);
>> +                     efr = serial_in(p, UART_EFR);
>>                       serial_out(p, UART_LCR, UART_LCR_CONF_MODE_B);
>>                       serial_out(p, UART_EFR, UART_EFR_ECB);
>>                       serial_out(p, UART_LCR, 0);
>> @@ -618,8 +621,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
>>               serial_out(p, UART_IER, sleep ? UART_IERX_SLEEP : 0);
>>               if (p->capabilities & UART_CAP_EFR) {
>>                       serial_out(p, UART_LCR, UART_LCR_CONF_MODE_B);
>> -                     serial_out(p, UART_EFR, 0);
>> -                     serial_out(p, UART_LCR, 0);
>> +                     serial_out(p, UART_EFR, sleep ? 0 : efr);
>> +                     serial_out(p, UART_LCR, sleep ? 0 : lcr);
>
> Why is it necessary to clear EFR and LCR here? Does the UART not
> power down?
>
> UARTs with CAP_SLEEP but not CAP_EFR don't clear LCR before sleep.
>
> However, if there is some kind of intentional side-effect here,
> then a comment should note that.

Yes, we can restore the LCR and EFR with the saved values.
                     serial_out(p, UART_EFR, efr);
                     serial_out(p, UART_LCR, lcr);

I will send a new patch with the changes.

Thanks,
Sudhir

>
> Regards,
> Peter Hurley
>
>>               }
>>       }
>>  out:
>>
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v1] tty: serial: 8250_core: restore the LCR register in set_sleep
  2014-10-17 10:25               ` Sudhir Sreedharan
@ 2014-10-17 12:39                 ` Sudhir Sreedharan
  2014-10-17 12:46                   ` Peter Hurley
  2014-10-20 15:01                   ` Kevin Hilman
  0 siblings, 2 replies; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-10-17 12:39 UTC (permalink / raw)
  To: peter, gregkh, khilman
  Cc: linux-serial, jslaby, linux-kernel, olof, linux-arm-kernel,
	geert, Sudhir Sreedharan

In ST16650V2 based serial uarts, while initalizing the PM state,
LCR registers are being initialized to 0 in serial8250_set_sleep().
If console port is already initialized and being used, this will
throws garbage in the console.

Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
---
Changes in v1:
	Removed the condition of sleep flag for restoring the LCR and EFR.
	Initialized the lcr and efr variables.

 drivers/tty/serial/8250/8250_core.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index ca5cfdc..b170487 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -595,6 +595,7 @@ static void serial8250_rpm_put_tx(struct uart_8250_port *p)
  */
 static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 {
+	unsigned char lcr = 0, efr = 0;
 	/*
 	 * Exar UARTs have a SLEEP register that enables or disables
 	 * each UART to enter sleep mode separately.  On the XR17V35x the
@@ -611,6 +612,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 
 	if (p->capabilities & UART_CAP_SLEEP) {
 		if (p->capabilities & UART_CAP_EFR) {
+			lcr = serial_in(p, UART_LCR);
+			efr = serial_in(p, UART_EFR);
 			serial_out(p, UART_LCR, UART_LCR_CONF_MODE_B);
 			serial_out(p, UART_EFR, UART_EFR_ECB);
 			serial_out(p, UART_LCR, 0);
@@ -618,8 +621,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 		serial_out(p, UART_IER, sleep ? UART_IERX_SLEEP : 0);
 		if (p->capabilities & UART_CAP_EFR) {
 			serial_out(p, UART_LCR, UART_LCR_CONF_MODE_B);
-			serial_out(p, UART_EFR, 0);
-			serial_out(p, UART_LCR, 0);
+			serial_out(p, UART_EFR, efr);
+			serial_out(p, UART_LCR, lcr);
 		}
 	}
 out:
-- 
1.7.0.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v1] tty: serial: 8250_core: restore the LCR register in set_sleep
  2014-10-17 12:39                 ` [PATCH v1] " Sudhir Sreedharan
@ 2014-10-17 12:46                   ` Peter Hurley
  2014-10-20 15:01                   ` Kevin Hilman
  1 sibling, 0 replies; 18+ messages in thread
From: Peter Hurley @ 2014-10-17 12:46 UTC (permalink / raw)
  To: Sudhir Sreedharan, gregkh
  Cc: khilman, linux-serial, jslaby, linux-kernel, olof,
	linux-arm-kernel, geert

On 10/17/2014 08:39 AM, Sudhir Sreedharan wrote:
> In ST16650V2 based serial uarts, while initalizing the PM state,
> LCR registers are being initialized to 0 in serial8250_set_sleep().
> If console port is already initialized and being used, this will
> throws garbage in the console.

Thanks.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1] tty: serial: 8250_core: restore the LCR register in set_sleep
  2014-10-17 12:39                 ` [PATCH v1] " Sudhir Sreedharan
  2014-10-17 12:46                   ` Peter Hurley
@ 2014-10-20 15:01                   ` Kevin Hilman
  2014-10-21  6:45                     ` Sudhir Sreedharan
  1 sibling, 1 reply; 18+ messages in thread
From: Kevin Hilman @ 2014-10-20 15:01 UTC (permalink / raw)
  To: Sudhir Sreedharan
  Cc: peter, gregkh, linux-serial, jslaby, linux-kernel, olof,
	linux-arm-kernel, geert

Sudhir Sreedharan <ssreedharan@mvista.com> writes:

> In ST16650V2 based serial uarts, while initalizing the PM state,
> LCR registers are being initialized to 0 in serial8250_set_sleep().
> If console port is already initialized and being used, this will
> throws garbage in the console.
>
> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>

Tested-by: Kevin Hilman <khilman@linaro.org>

I boot tested this on a bunch of ARM boards and don't see any
more failures/regressions.

Kevin

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1] tty: serial: 8250_core: restore the LCR register in set_sleep
  2014-10-20 15:01                   ` Kevin Hilman
@ 2014-10-21  6:45                     ` Sudhir Sreedharan
  0 siblings, 0 replies; 18+ messages in thread
From: Sudhir Sreedharan @ 2014-10-21  6:45 UTC (permalink / raw)
  To: Kevin Hilman
  Cc: Peter Hurley, Greg KH, linux-serial, Jiri Slaby, lkml,
	Olof Johansson, linux-arm-kernel, Geert Uytterhoeven

On Mon, Oct 20, 2014 at 8:31 PM, Kevin Hilman <khilman@kernel.org> wrote:
> Sudhir Sreedharan <ssreedharan@mvista.com> writes:
>
>> In ST16650V2 based serial uarts, while initalizing the PM state,
>> LCR registers are being initialized to 0 in serial8250_set_sleep().
>> If console port is already initialized and being used, this will
>> throws garbage in the console.
>>
>> Signed-off-by: Sudhir Sreedharan <ssreedharan@mvista.com>
>
> Tested-by: Kevin Hilman <khilman@linaro.org>
>
> I boot tested this on a bunch of ARM boards and don't see any
> more failures/regressions.

Thanks Kevin

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2014-10-21  6:45 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-22  6:30 [PATCH] serial/core: Initialize the console pm state Sudhir Sreedharan
2014-10-01 17:27 ` Kevin Hilman
2014-10-03  4:35   ` Greg KH
2014-10-03 12:22   ` Geert Uytterhoeven
2014-10-06  9:48     ` Sudhir Sreedharan
2014-10-06  9:27   ` Sudhir Sreedharan
2014-10-09 13:42     ` Sudhir Sreedharan
2014-10-09 13:45       ` [PATCH] tty: serial: 8250_core: Do not call set_sleep for console port Sudhir Sreedharan
2014-10-09 14:45         ` Peter Hurley
2014-10-15  6:43           ` [PATCH] tty: serial: 8250_core: restore the LCR register in set_sleep Sudhir Sreedharan
2014-10-16  7:21             ` Kevin Hilman
2014-10-16 10:15               ` Sudhir Sreedharan
2014-10-16 12:35             ` Peter Hurley
2014-10-17 10:25               ` Sudhir Sreedharan
2014-10-17 12:39                 ` [PATCH v1] " Sudhir Sreedharan
2014-10-17 12:46                   ` Peter Hurley
2014-10-20 15:01                   ` Kevin Hilman
2014-10-21  6:45                     ` Sudhir Sreedharan

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).