| Message ID | 20240425070936.547100-1-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [RFT,v2] serial: core: Call device_set_awake_path() for console port | expand |
On Thu, Apr 25, 2024 at 9:09 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > In case the UART port is used as a console, no_console_suspend is > available in bootargs and UART port is part of a software-controlled power > domain we need to call device_set_awake_path(). This lets the power > domain core code know that this domain should not be powered off > during system suspend. Otherwise, the UART port power domain is turned off, > nothing is printed while suspending and the suspend/resume process is > blocked. This was detected on the Renesas RZ/G3S SoC while adding support > for power domains. > > Based on code investigation, this issue is present on other SoCs (e.g., > Renesas R-Mobile A1 [1], IMX8QXP [2]) and different SoCs have particular > implementation to handle it. Due to this the patch added the call of > device_set_awake_path() in uart_suspend_port() instead of having it in > the platform specific UART driver. > > [1] https://elixir.bootlin.com/linux/v6.9-rc5/source/drivers/pmdomain/renesas/rmobile-sysc.c#L116 > [2] https://elixir.bootlin.com/linux/v6.9-rc5/source/drivers/pmdomain/imx/scu-pd.c#L357 > > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Works fine on R-Mobile APE6 with console suspend support removed from rmobile-sysc.c Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Thu, Apr 25, 2024 at 10:09:36AM +0300, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > In case the UART port is used as a console, no_console_suspend is > available in bootargs and UART port is part of a software-controlled power > domain we need to call device_set_awake_path(). This lets the power > domain core code know that this domain should not be powered off > during system suspend. Otherwise, the UART port power domain is turned off, > nothing is printed while suspending and the suspend/resume process is > blocked. This was detected on the Renesas RZ/G3S SoC while adding support > for power domains. > > Based on code investigation, this issue is present on other SoCs (e.g., > Renesas R-Mobile A1 [1], IMX8QXP [2]) and different SoCs have particular > implementation to handle it. Due to this the patch added the call of > device_set_awake_path() in uart_suspend_port() instead of having it in > the platform specific UART driver. > [1] https://elixir.bootlin.com/linux/v6.9-rc5/source/drivers/pmdomain/renesas/rmobile-sysc.c#L116 > [2] https://elixir.bootlin.com/linux/v6.9-rc5/source/drivers/pmdomain/imx/scu-pd.c#L357 No need to have the HTTP links into the kernel sources, you may simply refer to the files in the source tree. [1] drivers/pmdomain/renesas/rmobile-sysc.c:L116 [2] drivers/pmdomain/imx/scu-pd.c:L357 > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> The rest makes sense to me as we also have an internal hack to achieve something similar in the case of Intel LPSS (8250_dw). But I like Tony to comment on this, from my perspective it's good: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Thu, Apr 25, 2024 at 12:50:16PM +0300, Andy Shevchenko wrote: > On Thu, Apr 25, 2024 at 10:09:36AM +0300, Claudiu wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> ... > > [1] https://elixir.bootlin.com/linux/v6.9-rc5/source/drivers/pmdomain/renesas/rmobile-sysc.c#L116 > > [2] https://elixir.bootlin.com/linux/v6.9-rc5/source/drivers/pmdomain/imx/scu-pd.c#L357 > > No need to have the HTTP links into the kernel sources, you may simply refer to > the files in the source tree. > > [1] drivers/pmdomain/renesas/rmobile-sysc.c:L116 > [2] drivers/pmdomain/imx/scu-pd.c:L357 Should be without 'L' to follow the `git grep -n` output format.
On Thu, 25 Apr 2024 at 09:09, Claudiu <claudiu.beznea@tuxon.dev> wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > In case the UART port is used as a console, no_console_suspend is > available in bootargs and UART port is part of a software-controlled power > domain we need to call device_set_awake_path(). This lets the power > domain core code know that this domain should not be powered off > during system suspend. Otherwise, the UART port power domain is turned off, > nothing is printed while suspending and the suspend/resume process is > blocked. This was detected on the Renesas RZ/G3S SoC while adding support > for power domains. > > Based on code investigation, this issue is present on other SoCs (e.g., > Renesas R-Mobile A1 [1], IMX8QXP [2]) and different SoCs have particular > implementation to handle it. Due to this the patch added the call of > device_set_awake_path() in uart_suspend_port() instead of having it in > the platform specific UART driver. > > [1] https://elixir.bootlin.com/linux/v6.9-rc5/source/drivers/pmdomain/renesas/rmobile-sysc.c#L116 > [2] https://elixir.bootlin.com/linux/v6.9-rc5/source/drivers/pmdomain/imx/scu-pd.c#L357 > > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> At least conceptually this makes perfect sense to me. If it's the correct place to put it, I trust others to know. Nevertheless, feel free to add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > > Changes in v2: > - used device_set_awake_path() instead of device_set_wakeup_path() > - moved the support in uart_suspend_port() to make it generic for > other drivers > - fixed typos in commit description > - updated the commit description to reflect the new changes and the fact > that support may be applied to other SoCs > - added Suggested-by tag; this was initially proposed by Ulf to move it > in the serial driver then Geert propose to have it more generic in > uart_suspend_port() > > > drivers/tty/serial/serial_core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index d8d797a7a1e3..6270baab668c 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -2409,6 +2409,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport) > uport->ops->stop_rx(uport); > uart_port_unlock_irq(uport); > } > + device_set_awake_path(uport->dev); > goto unlock; > } > > -- > 2.39.2 >
On Thu, Apr 25, 2024 at 12:50:16PM +0300, Andy Shevchenko wrote: > The rest makes sense to me as we also have an internal hack to achieve > something similar in the case of Intel LPSS (8250_dw). > > But I like Tony to comment on this, from my perspective it's good: > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Yes nice, makes sense to me too: Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index d8d797a7a1e3..6270baab668c 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -2409,6 +2409,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport) uport->ops->stop_rx(uport); uart_port_unlock_irq(uport); } + device_set_awake_path(uport->dev); goto unlock; }