[PATCH 1/2] serial: omap: enable PM runtime only when its fully configured

[PATCH 1/2] serial: omap: enable PM runtime only when its fully configured

[Grygorii Strashko]

If earlyprintk is enabled and current UART is console port the platform
code can mark it as RPM_ACTIVE to sync real IP state with PM Runtime and
avoid resuming of already active device, but now, driver initialization
will be performed in the wrong way:

	pm_runtime_enable(&pdev->dev);
    <-- PM runtime alowed (device state RPM_ACTIVE)
	if (omap_up_info->autosuspend_timeout == 0)
		omap_up_info->autosuspend_timeout = -1;
	device_init_wakeup(up->dev, true);
	pm_runtime_use_autosuspend(&pdev->dev);
	<-- update_autosuspend() will be called and it will disable device
        (device state RPM_SUSPENDED)
	pm_runtime_set_autosuspend_delay(&pdev->dev,
			omap_up_info->autosuspend_timeout);
	<-- update_autosuspend() will be called which will re-enable device
        (device state RPM_ACTIVE), because autosuspend_timeout < 0

	pm_runtime_irq_safe(&pdev->dev);
	pm_runtime_get_sync(&pdev->dev);
	<-- will do nothing

Such behavior isn't expected by OMAP serial drivers and causes
unpredictable calls of serial_omap_runtime_suspend() and
serial_omap_runtime_resume().

Hence, fix it by allowing PM runtime only after all its parameters are
configured.

CC: Tony Lindgren <tony@atomide.com>
CC: Rajendra Nayak <rnayak@ti.com>
CC: Felipe Balbi <balbi@ti.com>
CC: Kevin Hilman <khilman@linaro.org>

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
tested on OMAP4 SDP with and without earlyprintk enabled.
 drivers/tty/serial/omap-serial.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index b6d1728..f39bf0c 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1501,7 +1501,6 @@ static int serial_omap_probe(struct platform_device *pdev)
 	INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
 
 	platform_set_drvdata(pdev, up);
-	pm_runtime_enable(&pdev->dev);
 	if (omap_up_info->autosuspend_timeout == 0)
 		omap_up_info->autosuspend_timeout = -1;
 	device_init_wakeup(up->dev, true);
@@ -1510,6 +1509,8 @@ static int serial_omap_probe(struct platform_device *pdev)
 			omap_up_info->autosuspend_timeout);
 
 	pm_runtime_irq_safe(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	pm_runtime_get_sync(&pdev->dev);
 
 	omap_serial_fill_features_erratas(up);
-- 
1.7.9.5

[PATCH 1/2] serial: omap: enable PM runtime only when its fully configured

[Grygorii Strashko]

If earlyprintk is enabled and current UART is console port the platform
code can mark it as RPM_ACTIVE to sync real IP state with PM Runtime and
avoid resuming of already active device, but now, driver initialization
will be performed in the wrong way:

	pm_runtime_enable(&pdev->dev);
    <-- PM runtime alowed (device state RPM_ACTIVE)
	if (omap_up_info->autosuspend_timeout == 0)
		omap_up_info->autosuspend_timeout = -1;
	device_init_wakeup(up->dev, true);
	pm_runtime_use_autosuspend(&pdev->dev);
	<-- update_autosuspend() will be called and it will disable device
        (device state RPM_SUSPENDED)
	pm_runtime_set_autosuspend_delay(&pdev->dev,
			omap_up_info->autosuspend_timeout);
	<-- update_autosuspend() will be called which will re-enable device
        (device state RPM_ACTIVE), because autosuspend_timeout < 0

	pm_runtime_irq_safe(&pdev->dev);
	pm_runtime_get_sync(&pdev->dev);
	<-- will do nothing

Such behavior isn't expected by OMAP serial drivers and causes
unpredictable calls of serial_omap_runtime_suspend() and
serial_omap_runtime_resume().

Hence, fix it by allowing PM runtime only after all its parameters are
configured.

CC: Tony Lindgren <tony@atomide.com>
CC: Rajendra Nayak <rnayak@ti.com>
CC: Felipe Balbi <balbi@ti.com>
CC: Kevin Hilman <khilman@linaro.org>

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
tested on OMAP4 SDP with and without earlyprintk enabled.
 drivers/tty/serial/omap-serial.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index b6d1728..f39bf0c 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1501,7 +1501,6 @@ static int serial_omap_probe(struct platform_device *pdev)
 	INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
 
 	platform_set_drvdata(pdev, up);
-	pm_runtime_enable(&pdev->dev);
 	if (omap_up_info->autosuspend_timeout == 0)
 		omap_up_info->autosuspend_timeout = -1;
 	device_init_wakeup(up->dev, true);
@@ -1510,6 +1509,8 @@ static int serial_omap_probe(struct platform_device *pdev)
 			omap_up_info->autosuspend_timeout);
 
 	pm_runtime_irq_safe(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	pm_runtime_get_sync(&pdev->dev);
 
 	omap_serial_fill_features_erratas(up);
-- 
1.7.9.5

[PATCH 2/2] serial: omap: fix wrong context restoration on init

[Grygorii Strashko]

Since commit a630fbf "serial: omap: Fix device tree based PM runtime"
the OMAP serial driver will always try to restore its context in
serial_omap_runtime_resume(). But the problem is that during driver
initialization the UART context is not ready yet and, as result, first
call to pm_runtime_get*() will cause UART register overwriting by all
zeros. This causes Kernel boot hang in case if "earlyprintk" feature is
enabled at least [1].

Unfortunately, there is no exact place in driver now where we can
determine that UART context is ready - most of registers configured in
serial_omap_set_termios(), but some of them in other places.
More over, even if PM runtime will be disabled (blocked) during OMAP
serial driver probe() execution [2],[3] it will fix only console UART,
but context of other UARTs will be overwriting by all zeros during first
access to the corresponding UART.

To fix this issue:
- introduce additional "initialized" flag and update PM runtime callback
to do nothing if its not set. Set "initialized" at the end of probe().
- read current UART registers configuration in probe and use it by
default.

[1] http://www.spinics.net/lists/arm-kernel/msg256828.html
[2] http://www.spinics.net/lists/arm-kernel/msg258062.html
[3] http://www.spinics.net/lists/arm-kernel/msg258040.html

CC: Tony Lindgren <tony@atomide.com>
CC: Rajendra Nayak <rnayak@ti.com>
CC: Felipe Balbi <balbi@ti.com>
CC: Kevin Hilman <khilman@linaro.org>

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
tested on OMAP4 SDP with and without earlyprintk enabled.
 drivers/tty/serial/omap-serial.c |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index f39bf0c..e1e9667 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -162,6 +162,7 @@ struct uart_omap_port {
 	struct work_struct	qos_work;
 	struct pinctrl		*pins;
 	bool			is_suspending;
+	bool			initialized;
 };
 
 #define to_uart_omap_port(p)	((container_of((p), struct uart_omap_port, port)))
@@ -1395,6 +1396,19 @@ static struct omap_uart_port_info *of_get_uart_port_info(struct device *dev)
 	return omap_up_info;
 }
 
+static void serial_omap_save_context_def(struct uart_omap_port *up)
+{
+	up->dll = serial_in(up, UART_DLL);
+	up->dlh = serial_in(up, UART_DLM);
+	up->ier = serial_in(up, UART_IER);
+	up->fcr = serial_in(up, UART_FCR);
+	up->mcr = serial_in(up, UART_MCR);
+	up->scr = serial_in(up, UART_OMAP_SCR);
+	up->efr = serial_in(up, UART_EFR);
+	up->lcr = serial_in(up, UART_LCR);
+	up->mdr1 = serial_in(up, UART_OMAP_MDR1);
+}
+
 static int serial_omap_probe(struct platform_device *pdev)
 {
 	struct uart_omap_port	*up;
@@ -1518,10 +1532,14 @@ static int serial_omap_probe(struct platform_device *pdev)
 	ui[up->port.line] = up;
 	serial_omap_add_console_port(up);
 
+	serial_omap_save_context_def(up);
+
 	ret = uart_add_one_port(&serial_omap_reg, &up->port);
 	if (ret != 0)
 		goto err_add_port;
 
+	up->initialized = true;
+
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
 	return 0;
@@ -1619,6 +1637,9 @@ static int serial_omap_runtime_suspend(struct device *dev)
 	if (!up)
 		return -EINVAL;
 
+	if (!up->initialized)
+		return 0;
+
 	/*
 	* When using 'no_console_suspend', the console UART must not be
 	* suspended. Since driver suspend is managed by runtime suspend,
@@ -1652,8 +1673,12 @@ static int serial_omap_runtime_suspend(struct device *dev)
 static int serial_omap_runtime_resume(struct device *dev)
 {
 	struct uart_omap_port *up = dev_get_drvdata(dev);
+	int loss_cnt;
+
+	if (!up->initialized)
+		return 0;
 
-	int loss_cnt = serial_omap_get_context_loss_count(up);
+	loss_cnt = serial_omap_get_context_loss_count(up);
 
 	if (loss_cnt < 0) {
 		dev_dbg(dev, "serial_omap_get_context_loss_count failed : %d\n",
-- 
1.7.9.5

[PATCH 2/2] serial: omap: fix wrong context restoration on init

[Grygorii Strashko]

Since commit a630fbf "serial: omap: Fix device tree based PM runtime"
the OMAP serial driver will always try to restore its context in
serial_omap_runtime_resume(). But the problem is that during driver
initialization the UART context is not ready yet and, as result, first
call to pm_runtime_get*() will cause UART register overwriting by all
zeros. This causes Kernel boot hang in case if "earlyprintk" feature is
enabled at least [1].

Unfortunately, there is no exact place in driver now where we can
determine that UART context is ready - most of registers configured in
serial_omap_set_termios(), but some of them in other places.
More over, even if PM runtime will be disabled (blocked) during OMAP
serial driver probe() execution [2],[3] it will fix only console UART,
but context of other UARTs will be overwriting by all zeros during first
access to the corresponding UART.

To fix this issue:
- introduce additional "initialized" flag and update PM runtime callback
to do nothing if its not set. Set "initialized" at the end of probe().
- read current UART registers configuration in probe and use it by
default.

[1] http://www.spinics.net/lists/arm-kernel/msg256828.html
[2] http://www.spinics.net/lists/arm-kernel/msg258062.html
[3] http://www.spinics.net/lists/arm-kernel/msg258040.html

CC: Tony Lindgren <tony@atomide.com>
CC: Rajendra Nayak <rnayak@ti.com>
CC: Felipe Balbi <balbi@ti.com>
CC: Kevin Hilman <khilman@linaro.org>

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
tested on OMAP4 SDP with and without earlyprintk enabled.
 drivers/tty/serial/omap-serial.c |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index f39bf0c..e1e9667 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -162,6 +162,7 @@ struct uart_omap_port {
 	struct work_struct	qos_work;
 	struct pinctrl		*pins;
 	bool			is_suspending;
+	bool			initialized;
 };
 
 #define to_uart_omap_port(p)	((container_of((p), struct uart_omap_port, port)))
@@ -1395,6 +1396,19 @@ static struct omap_uart_port_info *of_get_uart_port_info(struct device *dev)
 	return omap_up_info;
 }
 
+static void serial_omap_save_context_def(struct uart_omap_port *up)
+{
+	up->dll = serial_in(up, UART_DLL);
+	up->dlh = serial_in(up, UART_DLM);
+	up->ier = serial_in(up, UART_IER);
+	up->fcr = serial_in(up, UART_FCR);
+	up->mcr = serial_in(up, UART_MCR);
+	up->scr = serial_in(up, UART_OMAP_SCR);
+	up->efr = serial_in(up, UART_EFR);
+	up->lcr = serial_in(up, UART_LCR);
+	up->mdr1 = serial_in(up, UART_OMAP_MDR1);
+}
+
 static int serial_omap_probe(struct platform_device *pdev)
 {
 	struct uart_omap_port	*up;
@@ -1518,10 +1532,14 @@ static int serial_omap_probe(struct platform_device *pdev)
 	ui[up->port.line] = up;
 	serial_omap_add_console_port(up);
 
+	serial_omap_save_context_def(up);
+
 	ret = uart_add_one_port(&serial_omap_reg, &up->port);
 	if (ret != 0)
 		goto err_add_port;
 
+	up->initialized = true;
+
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
 	return 0;
@@ -1619,6 +1637,9 @@ static int serial_omap_runtime_suspend(struct device *dev)
 	if (!up)
 		return -EINVAL;
 
+	if (!up->initialized)
+		return 0;
+
 	/*
 	* When using 'no_console_suspend', the console UART must not be
 	* suspended. Since driver suspend is managed by runtime suspend,
@@ -1652,8 +1673,12 @@ static int serial_omap_runtime_suspend(struct device *dev)
 static int serial_omap_runtime_resume(struct device *dev)
 {
 	struct uart_omap_port *up = dev_get_drvdata(dev);
+	int loss_cnt;
+
+	if (!up->initialized)
+		return 0;
 
-	int loss_cnt = serial_omap_get_context_loss_count(up);
+	loss_cnt = serial_omap_get_context_loss_count(up);
 
 	if (loss_cnt < 0) {
 		dev_dbg(dev, "serial_omap_get_context_loss_count failed : %d\n",
-- 
1.7.9.5

Re: [PATCH 2/2] serial: omap: fix wrong context restoration on init

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2418 bytes --]

hi,

On Fri, Jul 12, 2013 at 02:55:42PM +0300, Grygorii Strashko wrote:
> Since commit a630fbf "serial: omap: Fix device tree based PM runtime"
> the OMAP serial driver will always try to restore its context in
> serial_omap_runtime_resume(). But the problem is that during driver
> initialization the UART context is not ready yet and, as result, first
> call to pm_runtime_get*() will cause UART register overwriting by all
> zeros. This causes Kernel boot hang in case if "earlyprintk" feature is
> enabled at least [1].
> 
> Unfortunately, there is no exact place in driver now where we can
> determine that UART context is ready - most of registers configured in
> serial_omap_set_termios(), but some of them in other places.
> More over, even if PM runtime will be disabled (blocked) during OMAP
> serial driver probe() execution [2],[3] it will fix only console UART,
> but context of other UARTs will be overwriting by all zeros during first
> access to the corresponding UART.
> 
> To fix this issue:
> - introduce additional "initialized" flag and update PM runtime callback
> to do nothing if its not set. Set "initialized" at the end of probe().
> - read current UART registers configuration in probe and use it by
> default.
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg256828.html
> [2] http://www.spinics.net/lists/arm-kernel/msg258062.html
> [3] http://www.spinics.net/lists/arm-kernel/msg258040.html
> 
> CC: Tony Lindgren <tony@atomide.com>
> CC: Rajendra Nayak <rnayak@ti.com>
> CC: Felipe Balbi <balbi@ti.com>
> CC: Kevin Hilman <khilman@linaro.org>
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> tested on OMAP4 SDP with and without earlyprintk enabled.
>  drivers/tty/serial/omap-serial.c |   27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index f39bf0c..e1e9667 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -162,6 +162,7 @@ struct uart_omap_port {
>  	struct work_struct	qos_work;
>  	struct pinctrl		*pins;
>  	bool			is_suspending;
> +	bool			initialized;

you really think adding this sort of bool flag is the best thing we can
do ? Something which will, quite likely, spread through every single
driver ?

oh well...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] serial: omap: fix wrong context restoration on init

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2418 bytes --]

hi,

On Fri, Jul 12, 2013 at 02:55:42PM +0300, Grygorii Strashko wrote:
> Since commit a630fbf "serial: omap: Fix device tree based PM runtime"
> the OMAP serial driver will always try to restore its context in
> serial_omap_runtime_resume(). But the problem is that during driver
> initialization the UART context is not ready yet and, as result, first
> call to pm_runtime_get*() will cause UART register overwriting by all
> zeros. This causes Kernel boot hang in case if "earlyprintk" feature is
> enabled at least [1].
> 
> Unfortunately, there is no exact place in driver now where we can
> determine that UART context is ready - most of registers configured in
> serial_omap_set_termios(), but some of them in other places.
> More over, even if PM runtime will be disabled (blocked) during OMAP
> serial driver probe() execution [2],[3] it will fix only console UART,
> but context of other UARTs will be overwriting by all zeros during first
> access to the corresponding UART.
> 
> To fix this issue:
> - introduce additional "initialized" flag and update PM runtime callback
> to do nothing if its not set. Set "initialized" at the end of probe().
> - read current UART registers configuration in probe and use it by
> default.
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg256828.html
> [2] http://www.spinics.net/lists/arm-kernel/msg258062.html
> [3] http://www.spinics.net/lists/arm-kernel/msg258040.html
> 
> CC: Tony Lindgren <tony@atomide.com>
> CC: Rajendra Nayak <rnayak@ti.com>
> CC: Felipe Balbi <balbi@ti.com>
> CC: Kevin Hilman <khilman@linaro.org>
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> tested on OMAP4 SDP with and without earlyprintk enabled.
>  drivers/tty/serial/omap-serial.c |   27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index f39bf0c..e1e9667 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -162,6 +162,7 @@ struct uart_omap_port {
>  	struct work_struct	qos_work;
>  	struct pinctrl		*pins;
>  	bool			is_suspending;
> +	bool			initialized;

you really think adding this sort of bool flag is the best thing we can
do ? Something which will, quite likely, spread through every single
driver ?

oh well...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] serial: omap: fix wrong context restoration on init

[Greg Kroah-Hartman]

On Fri, Jul 12, 2013 at 03:11:46PM +0300, Felipe Balbi wrote:
> hi,
> 
> On Fri, Jul 12, 2013 at 02:55:42PM +0300, Grygorii Strashko wrote:
> > Since commit a630fbf "serial: omap: Fix device tree based PM runtime"
> > the OMAP serial driver will always try to restore its context in
> > serial_omap_runtime_resume(). But the problem is that during driver
> > initialization the UART context is not ready yet and, as result, first
> > call to pm_runtime_get*() will cause UART register overwriting by all
> > zeros. This causes Kernel boot hang in case if "earlyprintk" feature is
> > enabled at least [1].
> > 
> > Unfortunately, there is no exact place in driver now where we can
> > determine that UART context is ready - most of registers configured in
> > serial_omap_set_termios(), but some of them in other places.
> > More over, even if PM runtime will be disabled (blocked) during OMAP
> > serial driver probe() execution [2],[3] it will fix only console UART,
> > but context of other UARTs will be overwriting by all zeros during first
> > access to the corresponding UART.
> > 
> > To fix this issue:
> > - introduce additional "initialized" flag and update PM runtime callback
> > to do nothing if its not set. Set "initialized" at the end of probe().
> > - read current UART registers configuration in probe and use it by
> > default.
> > 
> > [1] http://www.spinics.net/lists/arm-kernel/msg256828.html
> > [2] http://www.spinics.net/lists/arm-kernel/msg258062.html
> > [3] http://www.spinics.net/lists/arm-kernel/msg258040.html
> > 
> > CC: Tony Lindgren <tony@atomide.com>
> > CC: Rajendra Nayak <rnayak@ti.com>
> > CC: Felipe Balbi <balbi@ti.com>
> > CC: Kevin Hilman <khilman@linaro.org>
> > 
> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> > ---
> > tested on OMAP4 SDP with and without earlyprintk enabled.
> >  drivers/tty/serial/omap-serial.c |   27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> > index f39bf0c..e1e9667 100644
> > --- a/drivers/tty/serial/omap-serial.c
> > +++ b/drivers/tty/serial/omap-serial.c
> > @@ -162,6 +162,7 @@ struct uart_omap_port {
> >  	struct work_struct	qos_work;
> >  	struct pinctrl		*pins;
> >  	bool			is_suspending;
> > +	bool			initialized;
> 
> you really think adding this sort of bool flag is the best thing we can
> do ? Something which will, quite likely, spread through every single
> driver ?

I agree, that's not ok, please fix this up "properly" somehow.

thanks,

greg k-h