Re: [PATCH] serial: 8250_omap: Fix idling for unloaded serdev drivers

[Adam Ford]

On Tue, Jul 23, 2019 at 5:21 PM Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>
> For many years omap variants have been setting the runtime PM
> autosuspend delay to -1 to prevent unsafe policy with lossy first
> character on wake-up. The user must specifically enable the timeout
> for UARTs if desired.
>
> We must not enable the workaround for serdev devices though. It leads
> into UARTs not idling if no serdev devices are loaded and there is no
> sysfs entry to configure the UART in that case. And this means that
> my PM may not work unless the serdev modules are loaded.
>
> We can detect a serdev device being configured based on a dts child
> node, and we can simply skip the workround in that case. And the
> serdev driver can idle the port during runtime when suitable if an
> out-of-band wake-up GPIO line exists for example.
>
> Let's also add some comments to the workaround while at it.

This seems to help some of the stability issues I am seeing on the
DM3730 UART2 running Bluetooth at 3000000 baud.
Does it make sense to backport this to the stable kernels?

adam

>
> Cc: Johan Hovold <johan@xxxxxxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1234,7 +1234,16 @@ static int omap8250_probe(struct platform_device *pdev)
>
>         device_init_wakeup(&pdev->dev, true);
>         pm_runtime_use_autosuspend(&pdev->dev);
> -       pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
> +
> +       /*
> +        * Disable runtime PM until autosuspend delay unless specifically
> +        * enabled by the user via sysfs. This is the historic way to
> +        * prevent an unsafe default policy with lossy characters on wake-up.
> +        * For serdev devices this is not needed, the policy can be managed by
> +        * the serdev driver.
> +        */
> +       if (!of_get_available_child_count(pdev->dev.of_node))
> +               pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
>
>         pm_runtime_irq_safe(&pdev->dev);
>         pm_runtime_enable(&pdev->dev);
> --
> 2.21.0