[PATCH] serial/core: Initialize the console pm state

[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 <***@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;

[Greg KH]

On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan

Post by 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Geert Uytterhoeven]

On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan

Post by 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.

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 -- ***@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

[Sudhir Sreedharan]

Hi Geert,

Post by Geert Uytterhoeven

On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan

Post by 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Sudhir Sreedharan]

Hi Kevin,

On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan

Post by 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Sudhir Sreedharan]

Hi,

On Mon, Oct 6, 2014 at 2:57 PM, Sudhir Sreedharan

Post by Sudhir Sreedharan

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.

Post by Sudhir Sreedharan
Thanks,
Sudhir

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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 <***@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);

[Peter Hurley]

Hi Sudhir,

Post by 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.
---
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

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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 <***@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:

[Kevin Hilman]

Post by 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.

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

Thanks,

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Sudhir Sreedharan]

Hi Kevin,

Post by Kevin Hilman

Post by 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.

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

Post by Kevin Hilman
Thanks,
Kevin

[Peter Hurley]

Post by 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.
---
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

Post by Sudhir Sreedharan
}
}

[Sudhir Sreedharan]

Hi Peter,

Post by Peter Hurley

Post by 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.
---
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

Post by Peter Hurley
Regards,
Peter Hurley

Post by Sudhir Sreedharan
}
}

[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 <***@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:

[Peter Hurley]

Post by 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.

Thanks.

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

[Kevin Hilman]

Post by 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.

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

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

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html