Re: [PATCH v10 4/8] PM: domains: Add dev_get_performance_state() callback

[Ulf Hansson]

On Tue, 31 Aug 2021 at 15:57, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> Add optional dev_get_performance_state() callback that retrieves
> performance state of a device attached to a power domain. The new callback
> returns performance states of the given device, which should be read out
> from hardware.
>
> GENPD core assumes that initially performance state of all devices is
> zero and consumer device driver is responsible for management of the
> performance state. GENPD core now manages performance state across RPM
> suspend/resume of consumer devices by dropping and restoring the state,
> this allowed to move a part of performance management code into GENPD
> core out of consumer drivers. The part that hasn't been moved to GENPD
> core yet is the initialization of the performance state.
>
> Hardware may be preprogrammed to a specific performance state which
> isn't zero initially, hence the PD's performance state is inconsistent
> with hardware at the init time. This requires each consumer driver to
> explicitly sync the state. For some platforms this is a boilerplate code
> replicated by each driver.
>
> The new callback allows GENPD core to initialize the performance state,
> allowing to remove the remaining boilerplate code from consumer drivers.
> Now, when consumer device is resumed for the first time, the performance
> state is either already set by GENPD core or it will be set in accordance
> to the hardware state that was retrieved using the new callback when device
> was attached to a power domain. Still, consumer drivers are free to override
> the initial performance state configured by GENPD, if necessary.

Thanks for improving the commit message, it makes much better sense now!

>
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
>  drivers/base/power/domain.c | 90 +++++++++++++++++++++++++++++++------
>  include/linux/pm_domain.h   |  2 +
>  2 files changed, 79 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 5db704f02e71..598528abce01 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2644,12 +2644,85 @@ static void genpd_dev_pm_sync(struct device *dev)
>         genpd_queue_power_off_work(pd);
>  }
>
> +static int genpd_dev_get_current_performance_state(struct device *dev,
> +                                                  struct device *base_dev,
> +                                                  unsigned int index,
> +                                                  bool *state_default,
> +                                                  bool *suspended)
> +{
> +       struct generic_pm_domain *pd = dev_to_genpd(dev);
> +       int ret;
> +
> +       ret = of_get_required_opp_performance_state(dev->of_node, index);
> +       if (ret < 0 && ret != -ENODEV && ret != -EOPNOTSUPP) {
> +               dev_err(dev, "failed to get required performance state for power-domain %s: %d\n",
> +                       pd->name, ret);
> +
> +               return ret;
> +       } else if (ret >= 0) {
> +               *state_default = true;
> +
> +               return ret;
> +       }
> +
> +       if (pd->dev_get_performance_state) {
> +               ret = pd->dev_get_performance_state(pd, base_dev);
> +               if (ret >= 0)
> +                       *suspended = pm_runtime_status_suspended(base_dev);
> +               else
> +                       dev_err(dev, "failed to get performance state of %s for power-domain %s: %d\n",
> +                               dev_name(base_dev), pd->name, ret);
> +
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int genpd_dev_initialize_performance_state(struct device *dev,
> +                                                 struct device *base_dev,
> +                                                 unsigned int index)
> +{
> +       struct generic_pm_domain *pd = dev_to_genpd(dev);
> +       bool state_default = false, suspended = false;
> +       int pstate, err;
> +
> +       pstate = genpd_dev_get_current_performance_state(dev, base_dev, index,
> +                                                        &state_default,
> +                                                        &suspended);
> +       if (pstate <= 0)
> +               return pstate;
> +
> +       /*
> +        * If device is suspended, then performance state will be applied
> +        * on RPM-resume. This prevents potential extra power consumption
> +        * if device won't be resumed soon.
> +        *
> +        * If device is known to be active at the moment, then the performance
> +        * state should be updated immediately to sync state with hardware.
> +        */
> +       if (suspended) {
> +               dev_gpd_data(dev)->rpm_pstate = pstate;
> +       } else {
> +               err = dev_pm_genpd_set_performance_state(dev, pstate);
> +               if (err) {
> +                       dev_err(dev, "failed to set performance state for power-domain %s: %d\n",
> +                               pd->name, err);
> +                       return err;
> +               }
> +
> +               if (state_default)
> +                       dev_gpd_data(dev)->default_pstate = pstate;
> +       }
> +
> +       return 0;
> +}

As I kind of indicated in my previous reply on the earlier version, I
think the above code can be made a lot less complicated. Although,
perhaps I may be missing some points.

Anyway, I will post a couple patches, later this evening or tomorrow,
to show more exactly what I had in mind. Let's see if that can work
for you.

> +
>  static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>                                  unsigned int index, bool power_on)
>  {
>         struct of_phandle_args pd_args;
>         struct generic_pm_domain *pd;
> -       int pstate;
>         int ret;
>
>         ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> @@ -2693,22 +2766,13 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>                 return -EPROBE_DEFER;
>         }
>
> -       /* Set the default performance state */
> -       pstate = of_get_required_opp_performance_state(dev->of_node, index);
> -       if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) {
> -               ret = pstate;
> +       ret = genpd_dev_initialize_performance_state(dev, base_dev, index);
> +       if (ret)
>                 goto err;
> -       } else if (pstate > 0) {
> -               ret = dev_pm_genpd_set_performance_state(dev, pstate);
> -               if (ret)
> -                       goto err;
> -               dev_gpd_data(dev)->default_pstate = pstate;
> -       }
> +
>         return 1;
>
>  err:
> -       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> -               pd->name, ret);
>         genpd_remove_device(pd, dev);
>         return ret;
>  }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 67017c9390c8..0a4dd4986f34 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -133,6 +133,8 @@ struct generic_pm_domain {
>                                                  struct dev_pm_opp *opp);
>         int (*set_performance_state)(struct generic_pm_domain *genpd,
>                                      unsigned int state);
> +       int (*dev_get_performance_state)(struct generic_pm_domain *genpd,
> +                                        struct device *dev);
>         struct gpd_dev_ops dev_ops;
>         s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
>         ktime_t next_wakeup;    /* Maintained by the domain governor */
> --
> 2.32.0
>

Kind regards
Uffe