On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
<[email protected]> 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 <[email protected]>
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
On Wed, Oct 01, 2014 at 10:27:20AM -0700, Kevin Hilman wrote:
> On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
> <[email protected]> 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 <[email protected]>
>
> 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
On Wed, Oct 1, 2014 at 7:27 PM, Kevin Hilman <[email protected]> wrote:
> On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
> <[email protected]> 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 <[email protected]>
>
> 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 -- [email protected]
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
Hi Kevin,
On Wed, Oct 1, 2014 at 10:57 PM, Kevin Hilman <[email protected]> wrote:
> On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
> <[email protected]> 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 <[email protected]>
>
> 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
Hi Geert,
On Fri, Oct 3, 2014 at 5:52 PM, Geert Uytterhoeven <[email protected]> wrote:
> On Wed, Oct 1, 2014 at 7:27 PM, Kevin Hilman <[email protected]> wrote:
>> On Sun, Sep 21, 2014 at 11:30 PM, Sudhir Sreedharan
>> <[email protected]> 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 <[email protected]>
>>
>> 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
Hi,
On Mon, Oct 6, 2014 at 2:57 PM, Sudhir Sreedharan
<[email protected]> 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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
Sudhir Sreedharan <[email protected]> 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 <[email protected]>
Since this caused regressions in -next last time, could you describe how
this was tested, and on what platforms?
Thanks,
Kevin
Hi Kevin,
On Thu, Oct 16, 2014 at 12:51 PM, Kevin Hilman <[email protected]> wrote:
> Sudhir Sreedharan <[email protected]> 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 <[email protected]>
>
> 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
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 <[email protected]>
> ---
> 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:
>
Hi Peter,
On Thu, Oct 16, 2014 at 6:05 PM, Peter Hurley <[email protected]> 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 <[email protected]>
>> ---
>> 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:
>>
>
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 <[email protected]>
---
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
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 <[email protected]>
Sudhir Sreedharan <[email protected]> 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 <[email protected]>
Tested-by: Kevin Hilman <[email protected]>
I boot tested this on a bunch of ARM boards and don't see any
more failures/regressions.
Kevin
On Mon, Oct 20, 2014 at 8:31 PM, Kevin Hilman <[email protected]> wrote:
> Sudhir Sreedharan <[email protected]> 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 <[email protected]>
>
> Tested-by: Kevin Hilman <[email protected]>
>
> I boot tested this on a bunch of ARM boards and don't see any
> more failures/regressions.
Thanks Kevin