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

[Peng Fan]

> 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.

> 
> >
> > 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.
> 
> 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.


Thanks,
Peng.

> 
> Then we have of_genpd_parse_idle_states(), a helper that parses the values.
> 
> > +
> > +       sc_pd->pd.states = states;
> > +       sc_pd->pd.state_count = PD_STATE_MAX;
> >
> >         if (pd_ranges->postfix)
> >                 snprintf(sc_pd->name, sizeof(sc_pd->name), @@ -455,14
> > +497,16 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
> >                          sc_pd->name, sc_pd->rsrc);
> >
> >                 devm_kfree(dev, sc_pd);
> > +               devm_kfree(dev, states);
> >                 return NULL;
> >         }
> >
> > -       ret = pm_genpd_init(&sc_pd->pd, NULL, is_off);
> > +       ret = pm_genpd_init(&sc_pd->pd, &imx_sc_pd_qos_governor,
> > + is_off);
> >         if (ret) {
> >                 dev_warn(dev, "failed to init pd %s rsrc id %d",
> >                          sc_pd->name, sc_pd->rsrc);
> >                 devm_kfree(dev, sc_pd);
> > +               devm_kfree(dev, states);
> >                 return NULL;
> >         }
> >
> > --
> > 2.37.1
> >
> 
> Kind regards
> Uffe