RE: [PATCH V3 4/8] genpd: imx: scu-pd: do not power off console if no_console_suspend

[Peng Fan]

> Subject: Re: [PATCH V3 4/8] genpd: imx: scu-pd: do not power off console if
> no_console_suspend
> 
> On Mon, 31 Jul 2023 at 08:43, Peng Fan (OSS) <peng.fan@xxxxxxxxxxx>
> wrote:
> >
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > Do not power off console if no_console_suspend
> 
> Perhaps extend this a bit to let the reader understand this is about leaving
> the serial device's corresponding PM domain on.

ok

> 
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> 
> A comment below - that doesn't need to stop this from being applied
> though.
> 
> > ---
> >  drivers/genpd/imx/scu-pd.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
> > index 08583a10ac62..d69da79d3130 100644
> > --- a/drivers/genpd/imx/scu-pd.c
> > +++ b/drivers/genpd/imx/scu-pd.c
> > @@ -52,6 +52,7 @@
> >   */
> >
> >  #include <dt-bindings/firmware/imx/rsrc.h>
> > +#include <linux/console.h>
> >  #include <linux/firmware/imx/sci.h>
> >  #include <linux/firmware/imx/svc/rm.h>  #include <linux/io.h> @@
> > -324,6 +325,10 @@ static int imx_sc_pd_power(struct
> generic_pm_domain *domain, bool power_on)
> >         msg.resource = pd->rsrc;
> >         msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON :
> > IMX_SC_PM_PW_MODE_LP;
> >
> > +       /* keep uart console power on for no_console_suspend */
> > +       if (imx_con_rsrc == pd->rsrc && !console_suspend_enabled &&
> > + !power_on)
> 
> As I indicated above, I don't mind this, but I also think this is a rather generic
> problem that you are trying to solve here.
> 
> In principle, I think it should be the serial driver's responsibility to check the
> console_suspend_enabled flag. Based upon that, it should inform upper
> layers (genpd) that its device may need to stay powered on during system
> suspend. Quite similar to how we deal with system wakeups. To make this
> work we could do the following instead of $subject patch.
> 
> 1. The serial driver should call device_set_wakeup_path() (the name of that
> function is a bit confusing in this regard, but let's discuss that separately) in
> its ->suspend() callback.
> 2. Set the GENPD_FLAG_ACTIVE_WAKEUP (again the name is a bit confusing
> in this regard) for the corresponding genpd provider.

Thanks for your suggestion, I will try it. For the current v3 4/8, I will keep it in
V4. For your suggested method, I will post in a separate patch after I finish
and verified.

Thanks,
Peng.

> 
> In this way, genpd will keep the PM domain powered on during system
> suspend.
> 
> > +               return -EBUSY;
> > +
> >         ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
> >         if (ret)
> >                 dev_err(&domain->dev, "failed to power %s resource %d
> > ret %d\n",
> > --
> > 2.37.1
> >
> 
> Kind regards
> Uffe