| Message ID | cover.1540446493.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | OPP: Support multiple power-domains per device | expand |
On 25 October 2018 at 07:52, Viresh Kumar <viresh.kumar@linaro.org> wrote: > There are several struct device instances that genpd core handles. The > most common one is the consumer device structure, which is named > (correctly) as "dev" within genpd core. The second one is the genpd's > device structure, referenced as genpd->dev. The third one is the virtual > device structures created by the genpd core to represent the consumer > device for multiple power domain case, currently named as genpd_dev. The > naming of these virtual devices isn't very clear or readable and it > looks more like the genpd->dev. > > Rename the virtual device instances within the genpd core as "virt_dev". > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Thanks, this make the code more clear! Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/domain.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 7f38a92b444a..fe9b0527b161 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -2338,7 +2338,7 @@ EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); > struct device *genpd_dev_pm_attach_by_id(struct device *dev, > unsigned int index) > { > - struct device *genpd_dev; > + struct device *virt_dev; > int num_domains; > int ret; > > @@ -2352,31 +2352,31 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev, > return NULL; > > /* Allocate and register device on the genpd bus. */ > - genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL); > - if (!genpd_dev) > + virt_dev = kzalloc(sizeof(*virt_dev), GFP_KERNEL); > + if (!virt_dev) > return ERR_PTR(-ENOMEM); > > - dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev)); > - genpd_dev->bus = &genpd_bus_type; > - genpd_dev->release = genpd_release_dev; > + dev_set_name(virt_dev, "genpd:%u:%s", index, dev_name(dev)); > + virt_dev->bus = &genpd_bus_type; > + virt_dev->release = genpd_release_dev; > > - ret = device_register(genpd_dev); > + ret = device_register(virt_dev); > if (ret) { > - kfree(genpd_dev); > + kfree(virt_dev); > return ERR_PTR(ret); > } > > /* Try to attach the device to the PM domain at the specified index. */ > - ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index, false); > + ret = __genpd_dev_pm_attach(virt_dev, dev->of_node, index, false); > if (ret < 1) { > - device_unregister(genpd_dev); > + device_unregister(virt_dev); > return ret ? ERR_PTR(ret) : NULL; > } > > - pm_runtime_enable(genpd_dev); > - genpd_queue_power_off_work(dev_to_genpd(genpd_dev)); > + pm_runtime_enable(virt_dev); > + genpd_queue_power_off_work(dev_to_genpd(virt_dev)); > > - return genpd_dev; > + return virt_dev; > } > EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id); > > -- > 2.19.1.568.g152ad8e3369a >
On 25 October 2018 at 07:52, Viresh Kumar <viresh.kumar@linaro.org> wrote: > Multiple generic power domains for a consumer device are supported with > the help of virtual devices, which are created for each consumer device > - genpd pair. These are the device structures which are attached to the > power domain and are required by the OPP core to set the performance > state of the genpd. > > The helpers added by this commit are required to be called once for each > of these virtual devices. These are required only if multiple domains > are available for a device, otherwise the actual device structure will > be used instead by the OPP core. > > The new helpers also support the complex cases where the consumer device > wouldn't always require all the domains. For example, a camera may > require only one power domain during normal operations but two during > high resolution operations. The consumer driver can call > dev_pm_opp_put_genpd_virt_dev(high_resolution_genpd_virt_dev) if it is > currently operating in the normal mode and doesn't have any performance > requirements from the genpd which manages high resolution power > requirements. The consumer driver can later call > dev_pm_opp_set_genpd_virt_dev(high_resolution_genpd_virt_dev) once it > switches back to the high resolution mode. > > The new helpers differ from other OPP set/put helpers as the new ones > can be called with OPPs initialized for the table as we may need to call > them on the fly because of the complex case explained above. For this > reason it is possible that the genpd virt_dev structure may be used in > parallel while the new helpers are running and a new mutex is added to > protect against that. We didn't use the existing opp_table->lock mutex > as that is widely used in the OPP core and we will need this lock in the > dev_pm_opp_set_rate() helper while changing OPP and we need to make sure > there is not much contention while doing that as that's the hotpath. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/opp/core.c | 88 ++++++++++++++++++++++++++++++++++++++++++ > drivers/opp/of.c | 16 +++++++- > drivers/opp/opp.h | 4 ++ > include/linux/pm_opp.h | 8 ++++ > 4 files changed, 115 insertions(+), 1 deletion(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 02a69a62dac8..cef2ccda355d 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -823,6 +823,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index) > return NULL; > > mutex_init(&opp_table->lock); > + mutex_init(&opp_table->genpd_virt_dev_lock); > INIT_LIST_HEAD(&opp_table->dev_list); > > opp_dev = _add_opp_dev(dev, opp_table); > @@ -920,6 +921,7 @@ static void _opp_table_kref_release(struct kref *kref) > _remove_opp_dev(opp_dev, opp_table); > } > > + mutex_destroy(&opp_table->genpd_virt_dev_lock); > mutex_destroy(&opp_table->lock); > list_del(&opp_table->node); > kfree(opp_table); > @@ -1602,6 +1604,92 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table) > } > EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper); > > +/** > + * dev_pm_opp_set_genpd_virt_dev - Set virtual genpd device for an index > + * @dev: Consumer device for which the genpd device is getting set. > + * @virt_dev: virtual genpd device. > + * @index: index. > + * > + * Multiple generic power domains for a device are supported with the help of > + * virtual genpd devices, which are created for each consumer device - genpd > + * pair. These are the device structures which are attached to the power domain > + * and are required by the OPP core to set the performance state of the genpd. > + * > + * This helper will normally be called by the consumer driver of the device > + * "dev", as only that has details of the genpd devices. > + * > + * This helper needs to be called once for each of those virtual devices, but > + * only if multiple domains are available for a device. Otherwise the original > + * device structure will be used instead by the OPP core. > + */ > +struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, > + struct device *virt_dev, > + int index) > +{ > + struct opp_table *opp_table; > + > + opp_table = dev_pm_opp_get_opp_table(dev); > + if (!opp_table) > + return ERR_PTR(-ENOMEM); > + > + mutex_lock(&opp_table->genpd_virt_dev_lock); > + > + if (unlikely(!opp_table->genpd_virt_devs || > + index >= opp_table->required_opp_count || > + opp_table->genpd_virt_devs[index])) { > + > + dev_err(dev, "Invalid request to set required device\n"); > + dev_pm_opp_put_opp_table(opp_table); > + mutex_unlock(&opp_table->genpd_virt_dev_lock); > + > + return ERR_PTR(-EINVAL); > + } > + > + opp_table->genpd_virt_devs[index] = virt_dev; > + mutex_unlock(&opp_table->genpd_virt_dev_lock); > + > + return opp_table; > +} > + > +/** > + * dev_pm_opp_put_genpd_virt_dev() - Releases resources blocked for genpd device. > + * @opp_table: OPP table returned by dev_pm_opp_set_genpd_virt_dev(). > + * @virt_dev: virtual genpd device. > + * > + * This releases the resource previously acquired with a call to > + * dev_pm_opp_set_genpd_virt_dev(). The consumer driver shall call this helper > + * if it doesn't want OPP core to update performance state of a power domain > + * anymore. > + */ > +void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, > + struct device *virt_dev) > +{ > + int i; > + > + /* > + * Acquire genpd_virt_dev_lock to make sure virt_dev isn't getting > + * used in parallel. > + */ > + mutex_lock(&opp_table->genpd_virt_dev_lock); > + > + for (i = 0; i < opp_table->required_opp_count; i++) { > + if (opp_table->genpd_virt_devs[i] != virt_dev) > + continue; > + > + opp_table->genpd_virt_devs[i] = NULL; > + dev_pm_opp_put_opp_table(opp_table); > + > + /* Drop the vote */ > + dev_pm_genpd_set_performance_state(virt_dev, 0); > + break; > + } > + > + mutex_unlock(&opp_table->genpd_virt_dev_lock); > + > + if (unlikely(i == opp_table->required_opp_count)) > + dev_err(virt_dev, "Failed to find required device entry\n"); > +} > + > /** > * dev_pm_opp_add() - Add an OPP table from a table definitions > * @dev: device for which we do this operation > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index ffaeefef98ce..71aef28953c2 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -134,6 +134,7 @@ static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np) > static void _opp_table_free_required_tables(struct opp_table *opp_table) > { > struct opp_table **required_opp_tables = opp_table->required_opp_tables; > + struct device **genpd_virt_devs = opp_table->genpd_virt_devs; > int i; > > if (!required_opp_tables) > @@ -147,8 +148,10 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table) > } > > kfree(required_opp_tables); > + kfree(genpd_virt_devs); > > opp_table->required_opp_count = 0; > + opp_table->genpd_virt_devs = NULL; > opp_table->required_opp_tables = NULL; > } > > @@ -161,6 +164,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, > struct device_node *opp_np) > { > struct opp_table **required_opp_tables; > + struct device **genpd_virt_devs = NULL; > struct device_node *required_np, *np; > int count, i; > > @@ -175,11 +179,21 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, > if (!count) > goto put_np; > > + if (count > 1) { > + genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs), > + GFP_KERNEL); > + if (!genpd_virt_devs) > + goto put_np; > + } > + > required_opp_tables = kcalloc(count, sizeof(*required_opp_tables), > GFP_KERNEL); > - if (!required_opp_tables) > + if (!required_opp_tables) { > + kfree(genpd_virt_devs); > goto put_np; > + } > > + opp_table->genpd_virt_devs = genpd_virt_devs; > opp_table->required_opp_tables = required_opp_tables; > opp_table->required_opp_count = count; > > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h > index 24b340ad18d1..8aec38792cae 100644 > --- a/drivers/opp/opp.h > +++ b/drivers/opp/opp.h > @@ -135,6 +135,8 @@ enum opp_table_access { > * @parsed_static_opps: True if OPPs are initialized from DT. > * @shared_opp: OPP is shared between multiple devices. > * @suspend_opp: Pointer to OPP to be used during device suspend. > + * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers. > + * @genpd_virt_devs: List of virtual devices for multiple genpd support. > * @required_opp_tables: List of device OPP tables that are required by OPPs in > * this table. > * @required_opp_count: Number of required devices. > @@ -177,6 +179,8 @@ struct opp_table { > enum opp_table_access shared_opp; > struct dev_pm_opp *suspend_opp; > > + struct mutex genpd_virt_dev_lock; > + struct device **genpd_virt_devs; > struct opp_table **required_opp_tables; > unsigned int required_opp_count; > > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index 5d399eeef172..8fed222c089b 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -126,6 +126,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name); > void dev_pm_opp_put_clkname(struct opp_table *opp_table); > struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data)); > void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table); > +struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, struct device *virt_dev, int index); > +void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev); > int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); > int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask); > int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); > @@ -272,6 +274,12 @@ static inline struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const > > static inline void dev_pm_opp_put_clkname(struct opp_table *opp_table) {} > > +static inline struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, struct device *virt_dev, int index) > +{ > + return ERR_PTR(-ENOTSUPP); > +} > + > +static inline void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev) {} > static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > { > return -ENOTSUPP; > -- > 2.19.1.568.g152ad8e3369a >
Hello, This series improves the OPP core (and a bit of genpd core as well) to support multiple phandles in the "required-opps" property, which are only used for multiple power-domains per device for now. We still don't propagate the changes to master domains for the sub-domains, but this patchset is an important stepping stone for that to happen. Tested on Hikey960 after faking some power domains for CPUs. V2->V3: - Gained an additional patch to properly name the virtual device (1/10) - Renamed the virtual device as virt_dev instead of genpd_dev. - Few helpers renamed as well accordingly. - Use container_of() instead of traversing list of genpd. - Included tags received from Ulf. V1->V2: - Had a discussion at Linaro connect with Ulf regarding the changes V1 did in the genpd core and what his objections are to them. Based on his suggestions many changes are made. - The OPP core still needs the virtual device pointers to set the performance state for multiple domains, but the genpd core doesn't provide them automatically to the OPP core. One of the reasons behind this is to give more power to the consumer drivers which may not want to enable all the genpds at once. - The consumer drivers would now need to call the APIs dev_pm_opp_{set|put}_genpd_device() in order to set/reset these virtual device pointers. - More locking is put in place to protect the genpd device pointers in OPP core. - Reorg of the code at many places to make code less redundant. -- viresh Viresh Kumar (10): PM / Domains: Rename genpd virtual devices as virt_dev OPP: Identify and mark genpd OPP tables OPP: Separate out custom OPP handler specific code OPP: Populate required opp tables from "required-opps" property OPP: Populate OPPs from "required-opps" property PM / Domains: Add genpd_opp_to_performance_state() OPP: Add dev_pm_opp_{set|put}_genpd_virt_dev() helper OPP: Configure all required OPPs OPP: Rename and relocate of_genpd_opp_to_performance_state() OPP: Remove of_dev_pm_opp_find_required_opp() drivers/base/power/domain.c | 70 ++++---- drivers/opp/core.c | 261 ++++++++++++++++++++++-------- drivers/opp/of.c | 313 +++++++++++++++++++++++++++++++----- drivers/opp/opp.h | 20 +++ include/linux/pm_domain.h | 8 +- include/linux/pm_opp.h | 16 +- 6 files changed, 526 insertions(+), 162 deletions(-) -- 2.19.1.568.g152ad8e3369a