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
