| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPDyKFrAZ0176wZT0iGZ2-sT09KRqxCN2dOm_0-+J3pdESYctw@mail.gmail.com>
Date: Mon, 14 Aug 2023 14:33:33 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Peng Fan <peng.fan@....com>
Cc: "Peng Fan (OSS)" <peng.fan@....nxp.com>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"festevam@...il.com" <festevam@...il.com>,
dl-linux-imx <linux-imx@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support
On Mon, 14 Aug 2023 at 14:23, Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> On Mon, 14 Aug 2023 at 13:52, Peng Fan <peng.fan@....com> wrote:
> >
> > > Subject: Re: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support
> > >
> > > On Mon, 14 Aug 2023 at 12:37, Peng Fan (OSS) <peng.fan@....nxp.com>
> > > wrote:
> > > >
> > > > From: Peng Fan <peng.fan@....com>
> > > >
> > > > Add multi states support, this is to support devices could run in LP
> > > > mode when runtime suspend, and OFF mode when system suspend.
> > >
> > > For my understanding, is there a functional problem to support OFF at
> > > runtime suspend too?
> >
> > In OFF mode, the HW state is lost, so the clks that exported by this(Subsystem)
> > genpd is lost. While in LF mode, no need handle clks recover.
> >
> >
> > Such as subsystem LSIO has clks output, has GPIO, has LPUART.
> >
> > The clks are in drivers/clk/imx/clk-imx8qxp*, which relies on the scu pd.
> >
> > If scu-pd is off, the clks will lose state.
>
> Thanks for clarifying, much appreciated! So it sounds like it's the
> clock provider(s) that has these requirements then. Can we let the
> clock provider set a QoS latency constraint for its device that is
> attached to the genpd then? To prevent the deeper OFF state?
>
> Another option would be to enable runtime PM support for the clock
> provider (to manage the save/restore from runtime PM callbacks), but
> whether that's feasible sounds like a separate discussion.
>
> >
> > >
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@....com>
> > > > ---
> > > > drivers/genpd/imx/scu-pd.c | 48
> > > > ++++++++++++++++++++++++++++++++++++--
> > > > 1 file changed, 46 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
> > > > index 2f693b67ddb4..30da101119eb 100644
> > > > --- a/drivers/genpd/imx/scu-pd.c
> > > > +++ b/drivers/genpd/imx/scu-pd.c
> > > > @@ -65,6 +65,12 @@
> > > > #include <linux/pm_domain.h>
> > > > #include <linux/slab.h>
> > > >
> > > > +enum {
> > > > + PD_STATE_LP,
> > > > + PD_STATE_OFF,
> > > > + PD_STATE_MAX
> > > > +};
> > > > +
> > > > /* SCU Power Mode Protocol definition */ struct
> > > > imx_sc_msg_req_set_resource_power_mode {
> > > > struct imx_sc_rpc_msg hdr;
> > > > @@ -368,7 +374,8 @@ static int imx_sc_pd_power(struct
> > > generic_pm_domain *domain, bool power_on)
> > > > hdr->size = 2;
> > > >
> > > > msg.resource = pd->rsrc;
> > > > - msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON :
> > > IMX_SC_PM_PW_MODE_LP;
> > > > + msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : pd-
> > > >pd.state_idx ?
> > > > + IMX_SC_PM_PW_MODE_OFF : 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) @@ -412,11 +419,33 @@ static struct generic_pm_domain
> > > *imx_scu_pd_xlate(struct of_phandle_args *spec,
> > > > return domain;
> > > > }
> > > >
> > > > +static bool imx_sc_pd_suspend_ok(struct device *dev) {
> > > > + /* Always true */
> > > > + return true;
> > > > +}
> > > > +
> > > > +static bool imx_sc_pd_power_down_ok(struct dev_pm_domain *pd) {
> > > > + struct generic_pm_domain *genpd = pd_to_genpd(pd);
> > > > +
> > > > + /* For runtime suspend, choose LP mode */
> > > > + genpd->state_idx = 0;
> > > > +
> > > > + return true;
> > > > +}
> > >
> > > I am wondering if we couldn't use the simple_qos_governor here instead. In
> > > principle it looks like we want a QoS latency constraint to be set during
> > > runtime, to prevent the OFF state.
> >
> > LP mode indeed could save resume time, but the major problem is to avoid
> > save/restore clks.
>
> Okay. So it still sounds like a QoS latency constraint (for the clock
> provider) sounds like the correct thing to do.
>
> If/when the clock provider gets runtime PM support, we can remove the
> QoS latency constraints. That should work, right?
>
> > >
> > > During system wide suspend, the deepest state is always selected by genpd.
> > >
> > > > +
> > > > +struct dev_power_governor imx_sc_pd_qos_governor = {
> > > > + .suspend_ok = imx_sc_pd_suspend_ok,
> > > > + .power_down_ok = imx_sc_pd_power_down_ok, };
> > > > +
> > > > static struct imx_sc_pm_domain *
> > > > imx_scu_add_pm_domain(struct device *dev, int idx,
> > > > const struct imx_sc_pd_range *pd_ranges) {
> > > > struct imx_sc_pm_domain *sc_pd;
> > > > + struct genpd_power_state *states;
> > > > bool is_off;
> > > > int mode, ret;
> > > >
> > > > @@ -427,9 +456,22 @@ imx_scu_add_pm_domain(struct device *dev, int
> > > idx,
> > > > if (!sc_pd)
> > > > return ERR_PTR(-ENOMEM);
> > > >
> > > > + states = devm_kcalloc(dev, PD_STATE_MAX, sizeof(*states),
> > > GFP_KERNEL);
> > > > + if (!states) {
> > > > + devm_kfree(dev, sc_pd);
> > > > + return ERR_PTR(-ENOMEM);
> > > > + }
> > > > +
> > > > sc_pd->rsrc = pd_ranges->rsrc + idx;
> > > > sc_pd->pd.power_off = imx_sc_pd_power_off;
> > > > sc_pd->pd.power_on = imx_sc_pd_power_on;
> > > > + states[PD_STATE_LP].power_off_latency_ns = 25000;
> > > > + states[PD_STATE_LP].power_on_latency_ns = 25000;
> > > > + states[PD_STATE_OFF].power_off_latency_ns = 2500000;
> > > > + states[PD_STATE_OFF].power_on_latency_ns = 2500000;
> > >
> > > We should probably describe these in DT instead? The domain-idle-states
> > > bindings allows us to do this. See
> > > Documentation/devicetree/bindings/power/domain-idle-state.yaml.
> >
> > The scu-pd is a firmware function node, there is no sub-genpd node inside it.
> >
> > Just like scmi pd, there is no sub-genpd in it.
>
> Not sure I got your point. We don't need a sub-genpd node to describe
> this. This is how it could look like:
>
> domain-idle-states {
> domain_retention: domain-retention {
> compatible = "domain-idle-state";
> entry-latency-us = <25>;
> exit-latency-us = <25>;
> };
> domain_off: domain-off {
> compatible = "domain-idle-state";
> entry-latency-us = <2500>;
> exit-latency-us = <2500>;
> };
> };
>
> power-controller {
> compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";
> #power-domain-cells = <1>;
> domain-idle-states = <&domain_retention>, <&domain_off>;
> };
Ahh, now I think I got your point. The domain-idle-states need a
corresponding power-domain specifier too, right?
Can we do something along the lines of the below:
domain-idle-states = <&domain_retention domain-id>, <&domain_off domain-id>;
Anyway, I don't have a strong opinion about moving this to the DT, if
you want to keep the values in the code, that works too.
Kind regards
Uffe
Powered by blists - more mailing lists