Re: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support

[Ulf Hansson]

On Mon, 14 Aug 2023 at 14:23, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> On Mon, 14 Aug 2023 at 13:52, Peng Fan <peng.fan@xxxxxxx> 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@xxxxxxxxxxx>
> > > wrote:
> > > >
> > > > From: Peng Fan <peng.fan@xxxxxxx>
> > > >
> > > > 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@xxxxxxx>
> > > > ---
> > > >  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