[PATCH] tty: serial: fsl_lpuart: mark last busy before uart_add_one_port

[PATCH] tty: serial: fsl_lpuart: mark last busy before uart_add_one_port

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

With "earlycon initcall_debug=1 loglevel=8" in bootargs, kernel
sometimes boot hang. It is because normal console still is not ready,
but runtime suspend is called, so early console putchar will hang
in waiting TRDE set in UARTSTAT.

The lpuart driver has auto suspend delay set to 3000ms, but during
uart_add_one_port, a child device serial ctrl will added and probed with
its pm runtime enabled(see serial_ctrl.c).
The runtime suspend call path is:
device_add
     |-> bus_probe_device
           |->device_initial_probe
	           |->__device_attach
                         |-> pm_runtime_get_sync(dev->parent);
			 |-> pm_request_idle(dev);
			 |-> pm_runtime_put(dev->parent);

So in the end, before normal console ready, the lpuart get runtime
suspended. And earlycon putchar will hang.

To address the issue, mark last busy just after pm_runtime_enable,
three seconds is long enough to switch from bootconsole to normal
console.

Fixes: 43543e6f539b ("tty: serial: fsl_lpuart: Add runtime pm support")
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 615291ea9b5e..77efa7ee6eda 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2923,6 +2923,7 @@ static int lpuart_probe(struct platform_device *pdev)
 	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
+	pm_runtime_mark_last_busy(&pdev->dev);
 
 	ret = lpuart_global_reset(sport);
 	if (ret)
-- 
2.37.1

Re: [PATCH] tty: serial: fsl_lpuart: mark last busy before uart_add_one_port

[Alexander Stein]

Am Donnerstag, 8. August 2024, 16:03:25 CEST schrieb Peng Fan (OSS):
> From: Peng Fan <peng.fan@nxp.com>
> 
> With "earlycon initcall_debug=1 loglevel=8" in bootargs, kernel
> sometimes boot hang. It is because normal console still is not ready,
> but runtime suspend is called, so early console putchar will hang
> in waiting TRDE set in UARTSTAT.
> 
> The lpuart driver has auto suspend delay set to 3000ms, but during
> uart_add_one_port, a child device serial ctrl will added and probed with
> its pm runtime enabled(see serial_ctrl.c).
> The runtime suspend call path is:
> device_add
>      |-> bus_probe_device
>            |->device_initial_probe
> 	           |->__device_attach
>                          |-> pm_runtime_get_sync(dev->parent);
> 			 |-> pm_request_idle(dev);
> 			 |-> pm_runtime_put(dev->parent);
> 
> So in the end, before normal console ready, the lpuart get runtime
> suspended. And earlycon putchar will hang.
> 
> To address the issue, mark last busy just after pm_runtime_enable,
> three seconds is long enough to switch from bootconsole to normal
> console.
> 
> Fixes: 43543e6f539b ("tty: serial: fsl_lpuart: Add runtime pm support")
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/tty/serial/fsl_lpuart.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index 615291ea9b5e..77efa7ee6eda 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2923,6 +2923,7 @@ static int lpuart_probe(struct platform_device *pdev)
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_mark_last_busy(&pdev->dev);

This change looks sensible to me. Is maybe [1] addressing the same issue at a
different level?

Best regards,
Alexander

[1] https://lore.kernel.org/all/20240808-gs101-non-essential-clocks-2-v6-0-e91c537acedc@linaro.org/

>  
>  	ret = lpuart_global_reset(sport);
>  	if (ret)
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

RE: [PATCH] tty: serial: fsl_lpuart: mark last busy before uart_add_one_port

[Peng Fan]

> Subject: Re: [PATCH] tty: serial: fsl_lpuart: mark last busy before
> uart_add_one_port
> 
> Am Donnerstag, 8. August 2024, 16:03:25 CEST schrieb Peng Fan (OSS):
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > With "earlycon initcall_debug=1 loglevel=8" in bootargs, kernel
> > sometimes boot hang. It is because normal console still is not ready,
> > but runtime suspend is called, so early console putchar will hang in
> > waiting TRDE set in UARTSTAT.
> >
> > The lpuart driver has auto suspend delay set to 3000ms, but during
> > uart_add_one_port, a child device serial ctrl will added and probed
> > with its pm runtime enabled(see serial_ctrl.c).
> > The runtime suspend call path is:
> > device_add
> >      |-> bus_probe_device
> >            |->device_initial_probe
> > 	           |->__device_attach
> >                          |-> pm_runtime_get_sync(dev->parent);
> > 			 |-> pm_request_idle(dev);
> > 			 |-> pm_runtime_put(dev->parent);
> >
> > So in the end, before normal console ready, the lpuart get runtime
> > suspended. And earlycon putchar will hang.
> >
> > To address the issue, mark last busy just after pm_runtime_enable,
> > three seconds is long enough to switch from bootconsole to normal
> > console.
> >
> > Fixes: 43543e6f539b ("tty: serial: fsl_lpuart: Add runtime pm
> > support")
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/tty/serial/fsl_lpuart.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c index 615291ea9b5e..77efa7ee6eda
> > 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -2923,6 +2923,7 @@ static int lpuart_probe(struct
> platform_device *pdev)
> >  	pm_runtime_set_autosuspend_delay(&pdev->dev,
> UART_AUTOSUSPEND_TIMEOUT);
> >  	pm_runtime_set_active(&pdev->dev);
> >  	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_mark_last_busy(&pdev->dev);
> 
> This change looks sensible to me. Is maybe [1] addressing the same
> issue at a different level?

If the lpuart driver is built as module, [1] could not resolve the issue.

And if lpuart driver is built as module, [1] might break earlycon.
of_clk_drop_stdout_clocks disables clocks, but lpuart driver not
insmod at that time.

The current patch is simple and easy for backporting to stable
tree. [1] is not suitable for backporting to stable tree.

Regards,
Peng.

> 
> Best regards,
> Alexander
> 
> [1] https://lore.kernel.org/all/20240808-gs101-non-essential-clocks-2-v6-0-e91c537acedc@linaro.org/
> 
> >
> >  	ret = lpuart_global_reset(sport);
> >  	if (ret)
> >
> 
> 
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld,
> Germany Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.tq-
> group.com%2F&data=05%7C02%7Cpeng.fan%40nxp.com%7C40e104d
> ceb2146494b3608dcb7bea82c%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C638587276699315042%7CUnknown%7CTWFpbGZ
> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C0%7C%7C%7C&sdata=F%2FHNvj%2Bh5VdTVfRaYkXF
> VwYfm7NTUcy2o4UaewMHvT4%3D&reserved=0
>