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

From: Ulf Hansson
Date: Mon Aug 14 2023 - 07:26:38 EST


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?

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

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