diff mbox series

serial: sh-sci: Call device_set_wakeup_path() for serial console

Message ID 20240422111123.1622967-1-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series serial: sh-sci: Call device_set_wakeup_path() for serial console | expand

Commit Message

Claudiu Beznea April 22, 2024, 11:11 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

In case the SCI is used as a UART console, no_console_suspend is
available in bootargs and SCI is part of a software-controlled power
domain we need to call device_set_wakeup_path(). This lets the power
domain core code knows that this domain should not be powered off
durring system suspend. Otherwise, the SCI power domain is turned off,
nothing is printed while suspending and the suspend/resume process is
blocked. This was detected on the RZ/G3S SoC while adding support
for power domains.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/tty/serial/sh-sci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Geert Uytterhoeven April 23, 2024, 7:27 a.m. UTC | #1 
Hi Claudiu,

CC Peng

Thanks for your patch!

On Mon, Apr 22, 2024 at 1:11 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> In case the SCI is used as a UART console, no_console_suspend is
> available in bootargs and SCI is part of a software-controlled power
> domain we need to call device_set_wakeup_path(). This lets the power
> domain core code knows that this domain should not be powered off

know

> durring system suspend. Otherwise, the SCI power domain is turned off,

during

> nothing is printed while suspending and the suspend/resume process is
> blocked. This was detected on the RZ/G3S SoC while adding support
> for power domains.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/tty/serial/sh-sci.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 97031db26ae4..57a7f18e16e4 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3441,8 +3441,12 @@ static __maybe_unused int sci_suspend(struct device *dev)
>  {
>         struct sci_port *sport = dev_get_drvdata(dev);
>
> -       if (sport)
> +       if (sport) {
> +               if (uart_console(&sport->port) && !console_suspend_enabled)
> +                       device_set_wakeup_path(dev);

device_set_awake_path(), as of commit 10bb4e4ab7dd3898 ("PM: sleep:
Add helpers to allow a device to remain powered-on") in v6.6
(although I'm still a bit puzzled about the difference).

> +
>                 uart_suspend_port(&sci_uart_driver, &sport->port);

I think it would be better to make this more general, and move the call
to the existing console_suspend_enabled handling in uart_suspend_port().

> +       }
>
>         return 0;
>  }

If this works, we can remove the console_suspend_enabled handling
from drivers/pmdomain/renesas/rmobile-sysc.c, and revert commit
309864dcf92b76fc ("genpd: imx: scu-pd: do not power off console if
no_console_suspend").

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
Peng Fan April 23, 2024, 7:35 a.m. UTC | #2 
Hi Geert,

> Subject: Re: [PATCH] serial: sh-sci: Call device_set_wakeup_path() for serial
> console
> 
> Hi Claudiu,
> 
> CC Peng

Thanks for Ccing me, but I am not familiar with sh-sci driver (:

Thanks,
Peng.
> 
> Thanks for your patch!
> 
> On Mon, Apr 22, 2024 at 1:11 PM Claudiu <claudiu.beznea@tuxon.dev>
> wrote:
> > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > In case the SCI is used as a UART console, no_console_suspend is
> > available in bootargs and SCI is part of a software-controlled power
> > domain we need to call device_set_wakeup_path(). This lets the power
> > domain core code knows that this domain should not be powered off
> 
> know
> 
> > durring system suspend. Otherwise, the SCI power domain is turned off,
> 
> during
> 
> > nothing is printed while suspending and the suspend/resume process is
> > blocked. This was detected on the RZ/G3S SoC while adding support for
> > power domains.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > ---
> >  drivers/tty/serial/sh-sci.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index 97031db26ae4..57a7f18e16e4 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -3441,8 +3441,12 @@ static __maybe_unused int sci_suspend(struct
> > device *dev)  {
> >         struct sci_port *sport = dev_get_drvdata(dev);
> >
> > -       if (sport)
> > +       if (sport) {
> > +               if (uart_console(&sport->port) && !console_suspend_enabled)
> > +                       device_set_wakeup_path(dev);
> 
> device_set_awake_path(), as of commit 10bb4e4ab7dd3898 ("PM: sleep:
> Add helpers to allow a device to remain powered-on") in v6.6 (although I'm
> still a bit puzzled about the difference).
> 
> > +
> >                 uart_suspend_port(&sci_uart_driver, &sport->port);
> 
> I think it would be better to make this more general, and move the call to the
> existing console_suspend_enabled handling in uart_suspend_port().
> 
> > +       }
> >
> >         return 0;
> >  }
> 
> If this works, we can remove the console_suspend_enabled handling from
> drivers/pmdomain/renesas/rmobile-sysc.c, and revert commit
> 309864dcf92b76fc ("genpd: imx: scu-pd: do not power off console if
> no_console_suspend").
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> 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
Geert Uytterhoeven April 23, 2024, 7:45 a.m. UTC | #3 
Hi Peng,

On Tue, Apr 23, 2024 at 9:35 AM Peng Fan <peng.fan@nxp.com> wrote:
> > Subject: Re: [PATCH] serial: sh-sci: Call device_set_wakeup_path() for serial
> > console
> > CC Peng
>
> Thanks for Ccing me, but I am not familiar with sh-sci driver (:

But you are with the IMX pmdomain driver, so please read on ;-)

> > On Mon, Apr 22, 2024 at 1:11 PM Claudiu <claudiu.beznea@tuxon.dev>
> > wrote:
> > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >
> > > In case the SCI is used as a UART console, no_console_suspend is
> > > available in bootargs and SCI is part of a software-controlled power
> > > domain we need to call device_set_wakeup_path(). This lets the power
> > > domain core code knows that this domain should not be powered off
> > > durring system suspend. Otherwise, the SCI power domain is turned off,
> > > nothing is printed while suspending and the suspend/resume process is
> > > blocked. This was detected on the RZ/G3S SoC while adding support for
> > > power domains.
> > >
> > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> > > --- a/drivers/tty/serial/sh-sci.c
> > > +++ b/drivers/tty/serial/sh-sci.c
> > > @@ -3441,8 +3441,12 @@ static __maybe_unused int sci_suspend(struct
> > > device *dev)  {
> > >         struct sci_port *sport = dev_get_drvdata(dev);
> > >
> > > -       if (sport)
> > > +       if (sport) {
> > > +               if (uart_console(&sport->port) && !console_suspend_enabled)
> > > +                       device_set_wakeup_path(dev);
> >
> > device_set_awake_path(), as of commit 10bb4e4ab7dd3898 ("PM: sleep:
> > Add helpers to allow a device to remain powered-on") in v6.6 (although I'm
> > still a bit puzzled about the difference).
> >
> > > +
> > >                 uart_suspend_port(&sci_uart_driver, &sport->port);
> >
> > I think it would be better to make this more general, and move the call to the
> > existing console_suspend_enabled handling in uart_suspend_port().
> >
> > > +       }
> > >
> > >         return 0;
> > >  }
> >
> > If this works, we can remove the console_suspend_enabled handling from
> > drivers/pmdomain/renesas/rmobile-sysc.c, and revert commit
> > 309864dcf92b76fc ("genpd: imx: scu-pd: do not power off console if
> > no_console_suspend").

Gr{oetje,eeting}s,

                        Geert
Claudiu Beznea April 23, 2024, 12:10 p.m. UTC | #4 
Hi, Geert,

On 23.04.2024 10:27, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> CC Peng
> 
> Thanks for your patch!
> 
> On Mon, Apr 22, 2024 at 1:11 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> In case the SCI is used as a UART console, no_console_suspend is
>> available in bootargs and SCI is part of a software-controlled power
>> domain we need to call device_set_wakeup_path(). This lets the power
>> domain core code knows that this domain should not be powered off
> 
> know
> 
>> durring system suspend. Otherwise, the SCI power domain is turned off,
> 
> during
> 
>> nothing is printed while suspending and the suspend/resume process is
>> blocked. This was detected on the RZ/G3S SoC while adding support
>> for power domains.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/tty/serial/sh-sci.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
>> index 97031db26ae4..57a7f18e16e4 100644
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -3441,8 +3441,12 @@ static __maybe_unused int sci_suspend(struct device *dev)
>>  {
>>         struct sci_port *sport = dev_get_drvdata(dev);
>>
>> -       if (sport)
>> +       if (sport) {
>> +               if (uart_console(&sport->port) && !console_suspend_enabled)
>> +                       device_set_wakeup_path(dev);
> 
> device_set_awake_path(), as of commit 10bb4e4ab7dd3898 ("PM: sleep:
> Add helpers to allow a device to remain powered-on") in v6.6
> (although I'm still a bit puzzled about the difference).

Ok, I wasn't aware of it. I'll switch to this one.

> 
>> +
>>                 uart_suspend_port(&sci_uart_driver, &sport->port);
> 
> I think it would be better to make this more general, and move the call
> to the existing console_suspend_enabled handling in uart_suspend_port().

Ok, I'll try this way.

> 
>> +       }
>>
>>         return 0;
>>  }
> 
> If this works, we can remove the console_suspend_enabled handling
> from drivers/pmdomain/renesas/rmobile-sysc.c, and revert commit
> 309864dcf92b76fc ("genpd: imx: scu-pd: do not power off console if
> no_console_suspend").

OK, first I'll go with this patch and after things settles down with it
I'll propose changes for rmobile-sysc and imx. Is this ok for you?

Thank you,
Claudiu Beznea

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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
Geert Uytterhoeven April 23, 2024, 3:05 p.m. UTC | #5 
Hi Claudiu,

On Tue, Apr 23, 2024 at 2:47 PM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
> On 23.04.2024 10:27, Geert Uytterhoeven wrote:
> > On Mon, Apr 22, 2024 at 1:11 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> In case the SCI is used as a UART console, no_console_suspend is
> >> available in bootargs and SCI is part of a software-controlled power
> >> domain we need to call device_set_wakeup_path(). This lets the power
> >> domain core code knows that this domain should not be powered off
> >
> > know
> >
> >> durring system suspend. Otherwise, the SCI power domain is turned off,
> >
> > during
> >
> >> nothing is printed while suspending and the suspend/resume process is
> >> blocked. This was detected on the RZ/G3S SoC while adding support
> >> for power domains.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> > If this works, we can remove the console_suspend_enabled handling
> > from drivers/pmdomain/renesas/rmobile-sysc.c, and revert commit
> > 309864dcf92b76fc ("genpd: imx: scu-pd: do not power off console if
> > no_console_suspend").
>
> OK, first I'll go with this patch and after things settles down with it
> I'll propose changes for rmobile-sysc and imx. Is this ok for you?

I have already made these changes to rmobile-sysc locally to test
your patch ;-)

It works fine on R-Mobile APE6, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 97031db26ae4..57a7f18e16e4 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3441,8 +3441,12 @@  static __maybe_unused int sci_suspend(struct device *dev)
 {
 	struct sci_port *sport = dev_get_drvdata(dev);
 
-	if (sport)
+	if (sport) {
+		if (uart_console(&sport->port) && !console_suspend_enabled)
+			device_set_wakeup_path(dev);
+
 		uart_suspend_port(&sci_uart_driver, &sport->port);
+	}
 
 	return 0;
 }