[PATCH 0/2] Fix no_console_suspend in the 8250_omap driver

[PATCH 0/2] Fix no_console_suspend in the 8250_omap driver

[Thomas Richard]

This series fixes the no_console_suspend in the 8250_omap driver.
A first implementation was introducted [1].
But it was recently reverted [2] because it causes a crash during suspend on
the platforms that don't use PM domains.
This series implements the proposal sent by Théo Lebrun and discussed with
Kevin Hilman and Tony Lindgren in the thread [3].

This series was tested on a J7200 EVM board.

Regards,

Thomas

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68e6939ea9ec3d6579eadeab16060339cdeaf940
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0863bffda1131fd2fa9c05b653ad9ee3d8db127e
[3] https://lore.kernel.org/all/20231017130540.1149721-1-thomas.richard@bootlin.com/

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
Thomas Richard (2):
      pmdomain: ti-sci: set the GENPD_FLAG_ACTIVE_WAKEUP flag for all PM domains
      serial: 8250_omap: mark the serial as on the wakeup path if no_console_suspend

 drivers/pmdomain/ti/ti_sci_pm_domains.c | 1 +
 drivers/tty/serial/8250/8250_omap.c     | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)
---
base-commit: 6bf6745da1d37bc132690d96ef1455a62b0177cf
change-id: 20240819-8250-omap-no-console-suspend-1073308d3714

Best regards,
-- 
Thomas Richard <thomas.richard@bootlin.com>

[PATCH 1/2] pmdomain: ti-sci: set the GENPD_FLAG_ACTIVE_WAKEUP flag for all PM domains

[Thomas Richard]

With this flag, if a device is marked on the wakeup path, the corresponding
PM domain is kept powered on.

Suggested-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pmdomain/ti/ti_sci_pm_domains.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
index 1510d5ddae3d..38448b4a035a 100644
--- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
+++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
@@ -186,6 +186,7 @@ static int ti_sci_pm_domain_probe(struct platform_device *pdev)
 
 				pd->pd.power_off = ti_sci_pd_power_off;
 				pd->pd.power_on = ti_sci_pd_power_on;
+				pd->pd.flags |= GENPD_FLAG_ACTIVE_WAKEUP;
 				pd->idx = args.args[0];
 				pd->parent = pd_provider;
 

-- 
2.39.5

Re: [PATCH 1/2] pmdomain: ti-sci: set the GENPD_FLAG_ACTIVE_WAKEUP flag for all PM domains

[Ulf Hansson]

On Tue, 8 Oct 2024 at 11:34, Thomas Richard <thomas.richard@bootlin.com> wrote:
>
> With this flag, if a device is marked on the wakeup path, the corresponding
> PM domain is kept powered on.

Perhaps extend this with some more information that it fixes a problem
for the uart console too?

>
> Suggested-by: Théo Lebrun <theo.lebrun@bootlin.com>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Should we add a stable/fixes tag?

Kind regards
Uffe

> ---
>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> index 1510d5ddae3d..38448b4a035a 100644
> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> @@ -186,6 +186,7 @@ static int ti_sci_pm_domain_probe(struct platform_device *pdev)
>
>                                 pd->pd.power_off = ti_sci_pd_power_off;
>                                 pd->pd.power_on = ti_sci_pd_power_on;
> +                               pd->pd.flags |= GENPD_FLAG_ACTIVE_WAKEUP;
>                                 pd->idx = args.args[0];
>                                 pd->parent = pd_provider;
>
>
> --
> 2.39.5
>

Re: [PATCH 1/2] pmdomain: ti-sci: set the GENPD_FLAG_ACTIVE_WAKEUP flag for all PM domains

[Thomas Richard]

On 10/9/24 15:17, Ulf Hansson wrote:
> On Tue, 8 Oct 2024 at 11:34, Thomas Richard <thomas.richard@bootlin.com> wrote:
>>
>> With this flag, if a device is marked on the wakeup path, the corresponding
>> PM domain is kept powered on.
> 
> Perhaps extend this with some more information that it fixes a problem
> for the uart console too?

Yes, I'll send a v2 (only the patch 1/2) with a more verbose commit message.

> 
>>
>> Suggested-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> 
> Should we add a stable/fixes tag?

I guess no.
The first implementation, which was reverted, didn't have a stable/fixes
tag.

Regards,

Thomas

[PATCH 2/2] serial: 8250_omap: mark the serial as on the wakeup path if no_console_suspend

[Thomas Richard]

If the console suspend is disabled, mark the serial as on the wakeup path
to keep its PM domain powered on.

Suggested-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/tty/serial/8250/8250_omap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 88b58f44e4e9..6764fe2f9cad 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1660,8 +1660,12 @@ static int omap8250_suspend(struct device *dev)
 	if (!device_may_wakeup(dev))
 		priv->wer = 0;
 	serial_out(up, UART_OMAP_WER, priv->wer);
-	if (uart_console(&up->port) && console_suspend_enabled)
-		err = pm_runtime_force_suspend(dev);
+	if (uart_console(&up->port)) {
+		if (console_suspend_enabled)
+			err = pm_runtime_force_suspend(dev);
+		else
+			device_set_wakeup_path(dev);
+	}
 	flush_work(&priv->qos_work);
 
 	return err;

-- 
2.39.5

Re: [PATCH 2/2] serial: 8250_omap: mark the serial as on the wakeup path if no_console_suspend

[Ulf Hansson]

On Tue, 8 Oct 2024 at 11:34, Thomas Richard <thomas.richard@bootlin.com> wrote:
>
> If the console suspend is disabled, mark the serial as on the wakeup path
> to keep its PM domain powered on.
>
> Suggested-by: Théo Lebrun <theo.lebrun@bootlin.com>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 88b58f44e4e9..6764fe2f9cad 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1660,8 +1660,12 @@ static int omap8250_suspend(struct device *dev)
>         if (!device_may_wakeup(dev))
>                 priv->wer = 0;
>         serial_out(up, UART_OMAP_WER, priv->wer);
> -       if (uart_console(&up->port) && console_suspend_enabled)
> -               err = pm_runtime_force_suspend(dev);
> +       if (uart_console(&up->port)) {
> +               if (console_suspend_enabled)
> +                       err = pm_runtime_force_suspend(dev);
> +               else
> +                       device_set_wakeup_path(dev);
> +       }

Isn't this already managed by the serial core? See commit a47cf07f60dc
("serial: core: Call device_set_awake_path() for console port")

[...]

Kind regards
Uffe

Re: [PATCH 2/2] serial: 8250_omap: mark the serial as on the wakeup path if no_console_suspend

[Thomas Richard]

On 10/9/24 12:50, Ulf Hansson wrote:
> On Tue, 8 Oct 2024 at 11:34, Thomas Richard <thomas.richard@bootlin.com> wrote:
>>
>> If the console suspend is disabled, mark the serial as on the wakeup path
>> to keep its PM domain powered on.
>>
>> Suggested-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
>>  drivers/tty/serial/8250/8250_omap.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>> index 88b58f44e4e9..6764fe2f9cad 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -1660,8 +1660,12 @@ static int omap8250_suspend(struct device *dev)
>>         if (!device_may_wakeup(dev))
>>                 priv->wer = 0;
>>         serial_out(up, UART_OMAP_WER, priv->wer);
>> -       if (uart_console(&up->port) && console_suspend_enabled)
>> -               err = pm_runtime_force_suspend(dev);
>> +       if (uart_console(&up->port)) {
>> +               if (console_suspend_enabled)
>> +                       err = pm_runtime_force_suspend(dev);
>> +               else
>> +                       device_set_wakeup_path(dev);
>> +       }
> 
> Isn't this already managed by the serial core? See commit a47cf07f60dc
> ("serial: core: Call device_set_awake_path() for console port")

Yes you're right.
So this patch is useless.

Regards,

Thomas

Re: [PATCH 2/2] serial: 8250_omap: mark the serial as on the wakeup path if no_console_suspend

[Ulf Hansson]

On Wed, 9 Oct 2024 at 14:17, Thomas Richard <thomas.richard@bootlin.com> wrote:
>
> On 10/9/24 12:50, Ulf Hansson wrote:
> > On Tue, 8 Oct 2024 at 11:34, Thomas Richard <thomas.richard@bootlin.com> wrote:
> >>
> >> If the console suspend is disabled, mark the serial as on the wakeup path
> >> to keep its PM domain powered on.
> >>
> >> Suggested-by: Théo Lebrun <theo.lebrun@bootlin.com>
> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> >> ---
> >>  drivers/tty/serial/8250/8250_omap.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> >> index 88b58f44e4e9..6764fe2f9cad 100644
> >> --- a/drivers/tty/serial/8250/8250_omap.c
> >> +++ b/drivers/tty/serial/8250/8250_omap.c
> >> @@ -1660,8 +1660,12 @@ static int omap8250_suspend(struct device *dev)
> >>         if (!device_may_wakeup(dev))
> >>                 priv->wer = 0;
> >>         serial_out(up, UART_OMAP_WER, priv->wer);
> >> -       if (uart_console(&up->port) && console_suspend_enabled)
> >> -               err = pm_runtime_force_suspend(dev);
> >> +       if (uart_console(&up->port)) {
> >> +               if (console_suspend_enabled)
> >> +                       err = pm_runtime_force_suspend(dev);
> >> +               else
> >> +                       device_set_wakeup_path(dev);
> >> +       }
> >
> > Isn't this already managed by the serial core? See commit a47cf07f60dc
> > ("serial: core: Call device_set_awake_path() for console port")
>
> Yes you're right.
> So this patch is useless.

So patch 1/2 is all you need to get things working?

Kind regards
Uffe

Re: [PATCH 2/2] serial: 8250_omap: mark the serial as on the wakeup path if no_console_suspend

[Thomas Richard]

On 10/9/24 14:39, Ulf Hansson wrote:
> On Wed, 9 Oct 2024 at 14:17, Thomas Richard <thomas.richard@bootlin.com> wrote:
>>
>> On 10/9/24 12:50, Ulf Hansson wrote:
>>> On Tue, 8 Oct 2024 at 11:34, Thomas Richard <thomas.richard@bootlin.com> wrote:
>>>>
>>>> If the console suspend is disabled, mark the serial as on the wakeup path
>>>> to keep its PM domain powered on.
>>>>
>>>> Suggested-by: Théo Lebrun <theo.lebrun@bootlin.com>
>>>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>>>> ---
>>>>  drivers/tty/serial/8250/8250_omap.c | 8 ++++++--
>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>>>> index 88b58f44e4e9..6764fe2f9cad 100644
>>>> --- a/drivers/tty/serial/8250/8250_omap.c
>>>> +++ b/drivers/tty/serial/8250/8250_omap.c
>>>> @@ -1660,8 +1660,12 @@ static int omap8250_suspend(struct device *dev)
>>>>         if (!device_may_wakeup(dev))
>>>>                 priv->wer = 0;
>>>>         serial_out(up, UART_OMAP_WER, priv->wer);
>>>> -       if (uart_console(&up->port) && console_suspend_enabled)
>>>> -               err = pm_runtime_force_suspend(dev);
>>>> +       if (uart_console(&up->port)) {
>>>> +               if (console_suspend_enabled)
>>>> +                       err = pm_runtime_force_suspend(dev);
>>>> +               else
>>>> +                       device_set_wakeup_path(dev);
>>>> +       }
>>>
>>> Isn't this already managed by the serial core? See commit a47cf07f60dc
>>> ("serial: core: Call device_set_awake_path() for console port")
>>
>> Yes you're right.
>> So this patch is useless.
> 
> So patch 1/2 is all you need to get things working?

Yes only patch 1/2 is needed.

Regards,

Thomas