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

  From: Peng Fan <peng.fan@nxp.com>

Add multi states support, this is to support devices could run in LP mode
when runtime suspend, and OFF mode when system suspend.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/genpd/imx/scu-pd.c | 48 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)
  

On Mon, 14 Aug 2023 at 12:37, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.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?

>
> Signed-off-by: Peng Fan <peng.fan@nxp.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.

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.

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
  

> 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@oss.nxp.com>
> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.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.

> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.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.
> 
> 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
  

On Mon, 14 Aug 2023 at 13:52, Peng Fan <peng.fan@nxp.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@oss.nxp.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.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@nxp.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>;
};

[...]

Kind regards
Uffe
  

On Mon, 14 Aug 2023 at 14:23, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 14 Aug 2023 at 13:52, Peng Fan <peng.fan@nxp.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@oss.nxp.com>
> > > wrote:
> > > >
> > > > From: Peng Fan <peng.fan@nxp.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@nxp.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