| Message ID | 20210831135450.26070-5-digetx@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | NVIDIA Tegra power management patches for 5.16 | expand |
On Tue, 31 Aug 2021 at 15:57, Dmitry Osipenko <digetx@gmail.com> 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@gmail.com> > --- > 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
01.09.2021 19:59, Ulf Hansson пишет: >> +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. It's not obvious what you're wanting to improve, I'm probably missing yours point. So indeed will be good to see the code sample, thanks.
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; +} + 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 */
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. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/base/power/domain.c | 90 +++++++++++++++++++++++++++++++------ include/linux/pm_domain.h | 2 + 2 files changed, 79 insertions(+), 13 deletions(-)