| Message ID | 20230731064746.2717684-5-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | genpd: imx: relocate scu-pd and misc update | expand |
On Mon, 31 Jul 2023 at 08:43, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > 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. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> 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. 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
> 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@oss.nxp.com> > wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > 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@nxp.com> > > 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
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) + 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",