| Message ID | cover.1513926033.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | PM /Domain/OPP: Add support to get performance state from DT | expand |
On Thursday, January 18, 2018 7:34:04 AM CET Viresh Kumar wrote: > On 22-12-17, 12:56, Viresh Kumar wrote: > > Hi, > > > > Now that the DT bindings [1] are already Reviewed/Acked by respective > > maintainers, here is the code to start using them. > > > > The first two patches provide helpers in the OPP core, [3-5]/7 update > > the PM domain core to start supporting domain OPP tables, etc, 6/7 > > updates the OPP core to use the new callback provided by the PM domains > > to get performance state and the last one removes the unused helpers > > now. > > > > This is tested on Hikey620 and works just fine. > > Ping ! Well, whom are you pinging exactly and why? Thanks, Rafael
On 18-01-18, 20:24, Rafael J. Wysocki wrote: > On Thursday, January 18, 2018 7:34:04 AM CET Viresh Kumar wrote: > > On 22-12-17, 12:56, Viresh Kumar wrote: > > > Hi, > > > > > > Now that the DT bindings [1] are already Reviewed/Acked by respective > > > maintainers, here is the code to start using them. > > > > > > The first two patches provide helpers in the OPP core, [3-5]/7 update > > > the PM domain core to start supporting domain OPP tables, etc, 6/7 > > > updates the OPP core to use the new callback provided by the PM domains > > > to get performance state and the last one removes the unused helpers > > > now. > > > > > > This is tested on Hikey620 and works just fine. > > > > Ping ! > > Well, whom are you pinging exactly and why? Ulf and Kevin as its been almost a month since this series is posted and has received no comments at all. -- viresh
On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote: > The "operating-points-v2" property can contain a list of phandles now, > specifically for the power domain providers that provide multiple > domains. > > Add support to parse that. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/opp/of.c | 50 +++++++++++++++++++++++++++++++++++++++++--------- > include/linux/pm_opp.h | 6 ++++++ > 2 files changed, 47 insertions(+), 9 deletions(-) > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index cb716aa2f44b..22c9bd191f62 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -250,20 +250,17 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table); > > /* Returns opp descriptor node for a device node, caller must > * do of_node_put() */ > -static struct device_node *_opp_of_get_opp_desc_node(struct device_node *np) > +static struct device_node *_opp_of_get_opp_desc_node(struct device_node *np, > + int index) > { > - /* > - * There should be only ONE phandle present in "operating-points-v2" > - * property. > - */ > - > - return of_parse_phandle(np, "operating-points-v2", 0); > + /* "operating-points-v2" can be an array for power domain providers */ > + return of_parse_phandle(np, "operating-points-v2", index); > } > > /* Returns opp descriptor node for a device, caller must do of_node_put() */ > struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev) > { > - return _opp_of_get_opp_desc_node(dev->of_node); > + return _opp_of_get_opp_desc_node(dev->of_node, 0); > } > EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_opp_desc_node); > > @@ -509,6 +506,41 @@ int dev_pm_opp_of_add_table(struct device *dev) > } > EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table); > > +/** > + * dev_pm_opp_of_add_table_indexed() - Initialize indexed opp table from device tree > + * @dev: device pointer used to lookup OPP table. > + * @index: Index number. > + * > + * Register the initial OPP table with the OPP library for given device only > + * using the "operating-points-v2" property. > + * > + * Return: > + * 0 On success OR > + * Duplicate OPPs (both freq and volt are same) and opp->available > + * -EEXIST Freq are same and volt are different OR > + * Duplicate OPPs (both freq and volt are same) and !opp->available > + * -ENOMEM Memory allocation failure > + * -ENODEV when 'operating-points' property is not found or is invalid data > + * in device node. > + * -ENODATA when empty 'operating-points' property is found > + * -EINVAL when invalid entries are found in opp-v2 table > + */ > +int dev_pm_opp_of_add_table_indexed(struct device *dev, int index) > +{ > + struct device_node *opp_np; > + int ret; > + > + opp_np = _opp_of_get_opp_desc_node(dev->of_node, index); > + if (!opp_np) > + return -ENODEV; > + > + ret = _of_add_opp_table_v2(dev, opp_np); > + of_node_put(opp_np); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table_indexed); > + > /* CPU device specific helpers */ > > /** > @@ -613,7 +645,7 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, > } > > /* Get OPP descriptor node */ > - tmp_np = _opp_of_get_opp_desc_node(cpu_np); > + tmp_np = _opp_of_get_opp_desc_node(cpu_np, 0); > of_node_put(cpu_np); > if (!tmp_np) { > pr_err("%pOF: Couldn't find opp node\n", cpu_np); > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index 6c2d2e88f066..f042fdeaaa3c 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -303,6 +303,7 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask > > #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) > int dev_pm_opp_of_add_table(struct device *dev); > +int dev_pm_opp_of_add_table_indexed(struct device *dev, int index); > void dev_pm_opp_of_remove_table(struct device *dev); > int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask); > void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask); > @@ -314,6 +315,11 @@ static inline int dev_pm_opp_of_add_table(struct device *dev) > return -ENOTSUPP; > } > > +static inline int dev_pm_opp_of_add_table_indexed(struct device *dev, int index) > +{ > + return -ENOTSUPP; > +} > + > static inline void dev_pm_opp_of_remove_table(struct device *dev) > { > } > -- > 2.15.0.194.g9af6a3dea062 >
On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote: > A device's DT node or its OPP nodes can contain a phandle to other > device's OPP node, in the "required-opp" property. > > This patch implements a routine to find that required OPP from the node > that contains the "required-opp" property. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> [...] > +++ b/include/linux/pm_opp.h > @@ -309,6 +309,7 @@ int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask); > void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask); > int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); > struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev); > +struct dev_pm_opp *of_dev_pm_opp_find_required_opp(struct device *dev, struct device_node *np); I find the name of the exported function a bit "too" self-explainable, opp...opp. :-) However it's consistent with the existing APIs. [...] Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe
On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote: > The power-domain core would be using the OPP core going forward and the > OPP core has the basic requirement of a device structure for its working. According to the OPP core also seems to require the ->dev.of_node to be set. Actually, it seems like that part belongs in patch4 instead, while starting to make use of the opp OF APIs. Could you please move it? > > Add a struct device to the genpd structure and also add a genpd bus type > for the devices. This seems reasonable, however I am wondering if we could simplify this as bit. See more comments below. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/base/power/domain.c | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/pm_domain.h | 1 + > 2 files changed, 38 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 0c80bea05bcb..013c1e206167 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1630,6 +1630,10 @@ static void genpd_lock_init(struct generic_pm_domain *genpd) > } > } > > +static struct bus_type genpd_bus_type = { > + .name = "genpd", > +}; This seems silly. Can't we just avoid having a bus type altogether, thus no need to call bus_register() as well? > + > /** > * pm_genpd_init - Initialize a generic I/O PM domain object. > * @genpd: PM domain object to initialize. > @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > return ret; > } > > + genpd->dev.bus = &genpd_bus_type; > + device_initialize(&genpd->dev); > + dev_set_name(&genpd->dev, "%s", genpd->name); > + > + ret = device_add(&genpd->dev); What's the point of adding the device? Can we skip this step, as I guess the opp core is only using the device as cookie rather actual using it? No? > + if (ret) { > + dev_err(&genpd->dev, "failed to add device: %d\n", ret); > + put_device(&genpd->dev); > + kfree(genpd->free); > + return ret; > + } > + > mutex_lock(&gpd_list_lock); > list_add(&genpd->gpd_list_node, &gpd_list); > mutex_unlock(&gpd_list_lock); > @@ -1724,6 +1740,7 @@ static int genpd_remove(struct generic_pm_domain *genpd) > > list_del(&genpd->gpd_list_node); > genpd_unlock(genpd); > + device_del(&genpd->dev); > cancel_work_sync(&genpd->power_off_work); > kfree(genpd->free); > pr_debug("%s: removed %s\n", __func__, genpd->name); > @@ -1888,6 +1905,7 @@ int of_genpd_add_provider_simple(struct device_node *np, > if (!ret) { > genpd->provider = &np->fwnode; > genpd->has_provider = true; > + genpd->dev.of_node = np; > } > } > > @@ -1924,6 +1942,7 @@ int of_genpd_add_provider_onecell(struct device_node *np, > > data->domains[i]->provider = &np->fwnode; > data->domains[i]->has_provider = true; > + data->domains[i]->dev.of_node = np; > } > > ret = genpd_add_provider(np, data->xlate, data); > @@ -2691,3 +2710,21 @@ static void __exit genpd_debug_exit(void) > } > __exitcall(genpd_debug_exit); > #endif /* CONFIG_DEBUG_FS */ > + > +static int __init pm_genpd_core_init(void) > +{ > + int ret; > + > + ret = bus_register(&genpd_bus_type); > + if (ret) > + pr_err("bus_register failed (%d)\n", ret); > + > + return ret; > +} > +pure_initcall(pm_genpd_core_init); > + > +static void __exit pm_genpd_core_exit(void) > +{ > + bus_unregister(&genpd_bus_type); > +} > +__exitcall(pm_genpd_core_exit); > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 04dbef9847d3..aaacaa35005d 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -49,6 +49,7 @@ struct genpd_power_state { > struct genpd_lock_ops; > > struct generic_pm_domain { > + struct device dev; > struct dev_pm_domain domain; /* PM domain operations */ > struct list_head gpd_list_node; /* Node in the global PM domains list */ > struct list_head master_links; /* Links with PM domain as a master */ > -- > 2.15.0.194.g9af6a3dea062 > Kind regards Uffe
On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote: > This implements of_dev_pm_genpd_get_performance_state() which can be > used from the device drivers or the OPP core to find the performance > state encoded in the "required-opp" property of a node. Different Please add a two newlines after "node". > platforms may encode the performance state differently using the OPP > table (they may simply return value of opp-hz or opp-microvolt, or apply > some algorithm on top of those values) and so a new callback is > implemented to allow platform specific drivers to convert the power > domain OPP to a performance state. Could you perhaps clarify at what typical point(s) the drivers or the OPP core should call this new API? That is useful information in the changelog. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/base/power/domain.c | 48 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pm_domain.h | 11 +++++++++++ > 2 files changed, 59 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 1ad4ad0b0de0..ef43b75982fa 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -2415,6 +2415,54 @@ int of_genpd_parse_idle_states(struct device_node *dn, > } > EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states); > > +/** > + * of_dev_pm_genpd_get_performance_state- Gets performance state of device's > + * power domain corresponding to a DT node's "required-opp" property. > + * > + * @dev: Device for which the performance-state needs to be found. > + * @np: DT node where the "required-opp" property is present. This can be > + * the device node itself (if it doesn't have an OPP table) or a node > + * within the OPP table of a device (if device has an OPP table). > + * @state: Pointer to return performance state. > + * > + * Returns performance state corresponding to the "required-opp" property of > + * a DT node. This calls platform specific genpd->get_performance_state() > + * callback to translate power domain OPP to performance state. > + * > + * Returns performance state on success and 0 on failure. > + */ > +unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev, > + struct device_node *np) First: To clarify the API, perhaps change to "...struct device_node *opp_node)" Second, the function name is confusing. To me, it sounds like an API that will get the dynamically requested performance state for the device and that isn't the case. So, I would rather name it something along the lines of, "of_genpd_opp_to_performance_state()". Can you please consider renaming it? > +{ > + struct generic_pm_domain *genpd; > + struct dev_pm_opp *opp; > + int state = 0; > + > + genpd = dev_to_genpd(dev); > + if (IS_ERR(genpd)) > + return 0; > + > + if (unlikely(!genpd->set_performance_state)) > + return 0; > + > + genpd_lock(genpd); > + > + opp = of_dev_pm_opp_find_required_opp(&genpd->dev, np); > + if (IS_ERR(opp)) { > + state = PTR_ERR(opp); > + goto unlock; > + } > + > + state = genpd->get_performance_state(genpd, opp); > + dev_pm_opp_put(opp); > + > +unlock: > + genpd_unlock(genpd); > + > + return state; > +} > +EXPORT_SYMBOL_GPL(of_dev_pm_genpd_get_performance_state); > + > #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */ > > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index aaacaa35005d..4edbdaa54cec 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -47,6 +47,7 @@ struct genpd_power_state { > }; > > struct genpd_lock_ops; > +struct dev_pm_opp; > > struct generic_pm_domain { > struct device dev; > @@ -68,6 +69,8 @@ struct generic_pm_domain { > unsigned int performance_state; /* Aggregated max performance state */ > int (*power_off)(struct generic_pm_domain *domain); > int (*power_on)(struct generic_pm_domain *domain); > + unsigned int (*get_performance_state)(struct generic_pm_domain *genpd, > + struct dev_pm_opp *opp); According to the comments above, please rename the callback to: "opp_to_performance_state", or whatever better name you can come up with. :-) > int (*set_performance_state)(struct generic_pm_domain *genpd, > unsigned int state); > struct gpd_dev_ops dev_ops; > @@ -244,6 +247,8 @@ extern int of_genpd_add_subdomain(struct of_phandle_args *parent, > extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np); > extern int of_genpd_parse_idle_states(struct device_node *dn, > struct genpd_power_state **states, int *n); > +extern unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev, > + struct device_node *np); > > int genpd_dev_pm_attach(struct device *dev); > #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */ > @@ -279,6 +284,12 @@ static inline int of_genpd_parse_idle_states(struct device_node *dn, > return -ENODEV; > } > > +static inline unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev, > + struct device_node *np) > +{ > + return -ENODEV; > +} > + > static inline int genpd_dev_pm_attach(struct device *dev) > { > return -ENODEV; > -- > 2.15.0.194.g9af6a3dea062 > Mostly minor things, so overall this looks good to me! Kind regards Uffe
On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote: > These helpers aren't used anymore, remove them. I guess they weren't used even before? However, as we now have the new OF based APIs, we can use them instead. Anyway, feel free to add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/opp/core.c | 75 -------------------------------------------------- > drivers/opp/opp.h | 2 -- > include/linux/pm_opp.h | 10 ------- > 3 files changed, 87 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 6194219fb95f..9e3c40437991 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1545,81 +1545,6 @@ 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_register_get_pstate_helper() - Register get_pstate() helper. > - * @dev: Device for which the helper is getting registered. > - * @get_pstate: Helper. > - * > - * TODO: Remove this callback after the same information is available via Device > - * Tree. > - * > - * This allows a platform to initialize the performance states of individual > - * OPPs for its devices, until we get similar information directly from DT. > - * > - * This must be called before the OPPs are initialized for the device. > - */ > -struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev, > - int (*get_pstate)(struct device *dev, unsigned long rate)) > -{ > - struct opp_table *opp_table; > - int ret; > - > - if (!get_pstate) > - return ERR_PTR(-EINVAL); > - > - opp_table = dev_pm_opp_get_opp_table(dev); > - if (!opp_table) > - return ERR_PTR(-ENOMEM); > - > - /* This should be called before OPPs are initialized */ > - if (WARN_ON(!list_empty(&opp_table->opp_list))) { > - ret = -EBUSY; > - goto err; > - } > - > - /* Already have genpd_performance_state set */ > - if (WARN_ON(opp_table->genpd_performance_state)) { > - ret = -EBUSY; > - goto err; > - } > - > - opp_table->genpd_performance_state = true; > - opp_table->get_pstate = get_pstate; > - > - return opp_table; > - > -err: > - dev_pm_opp_put_opp_table(opp_table); > - > - return ERR_PTR(ret); > -} > -EXPORT_SYMBOL_GPL(dev_pm_opp_register_get_pstate_helper); > - > -/** > - * dev_pm_opp_unregister_get_pstate_helper() - Releases resources blocked for > - * get_pstate() helper > - * @opp_table: OPP table returned from dev_pm_opp_register_get_pstate_helper(). > - * > - * Release resources blocked for platform specific get_pstate() helper. > - */ > -void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table) > -{ > - if (!opp_table->genpd_performance_state) { > - pr_err("%s: Doesn't have performance states set\n", > - __func__); > - return; > - } > - > - /* Make sure there are no concurrent readers while updating opp_table */ > - WARN_ON(!list_empty(&opp_table->opp_list)); > - > - opp_table->genpd_performance_state = false; > - opp_table->get_pstate = NULL; > - > - dev_pm_opp_put_opp_table(opp_table); > -} > -EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_get_pstate_helper); > - > /** > * 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/opp.h b/drivers/opp/opp.h > index b181032e6938..ae99295b5382 100644 > --- a/drivers/opp/opp.h > +++ b/drivers/opp/opp.h > @@ -140,7 +140,6 @@ enum opp_table_access { > * @genpd_performance_state: Device's power domain support performance state. > * @set_opp: Platform specific set_opp callback > * @set_opp_data: Data to be passed to set_opp callback > - * @get_pstate: Platform specific get_pstate callback > * @dentry: debugfs dentry pointer of the real device directory (not links). > * @dentry_name: Name of the real dentry. > * > @@ -178,7 +177,6 @@ struct opp_table { > > int (*set_opp)(struct dev_pm_set_opp_data *data); > struct dev_pm_set_opp_data *set_opp_data; > - int (*get_pstate)(struct device *dev, unsigned long rate); > > #ifdef CONFIG_DEBUG_FS > struct dentry *dentry; > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index 70686f434c13..528a7d9cf2ef 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -125,8 +125,6 @@ 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_register_get_pstate_helper(struct device *dev, int (*get_pstate)(struct device *dev, unsigned long rate)); > -void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table); > 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); > @@ -247,14 +245,6 @@ static inline struct opp_table *dev_pm_opp_register_set_opp_helper(struct device > > static inline void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table) {} > > -static inline struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev, > - int (*get_pstate)(struct device *dev, unsigned long rate)) > -{ > - return ERR_PTR(-ENOTSUPP); > -} > - > -static inline void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table) {} > - > static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name) > { > return ERR_PTR(-ENOTSUPP); > -- > 2.15.0.194.g9af6a3dea062 >
On 22-03-18, 10:30, Ulf Hansson wrote: > On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > The power-domain core would be using the OPP core going forward and the > > OPP core has the basic requirement of a device structure for its working. > > According to the OPP core also seems to require the ->dev.of_node to > be set. Actually, it seems like that part belongs in patch4 instead, > while starting to make use of the opp OF APIs. Could you please move > it? You meaning setting of the of_node to the next patch? I can do that if that;s what you want. > > +static struct bus_type genpd_bus_type = { > > + .name = "genpd", > > +}; > > This seems silly. Can't we just avoid having a bus type altogether, > thus no need to call bus_register() as well? I thought we need a bus where we want to add the device, haven't tried with that pointer being empty. Will try that and let you know. > > + > > /** > > * pm_genpd_init - Initialize a generic I/O PM domain object. > > * @genpd: PM domain object to initialize. > > @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > > return ret; > > } > > > > + genpd->dev.bus = &genpd_bus_type; > > + device_initialize(&genpd->dev); > > + dev_set_name(&genpd->dev, "%s", genpd->name); > > + > > + ret = device_add(&genpd->dev); > > What's the point of adding the device? Can we skip this step, as I > guess the opp core is only using the device as cookie rather actual > using it? No? We also use it for the OPP debugfs stuff, so that would be required I believe. We also use it for playing with clk/regulator stuff as well, though we may not use it in genpd case for now. -- viresh
On 22-03-18, 11:18, Ulf Hansson wrote: > On 22 March 2018 at 10:59, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 22-03-18, 10:30, Ulf Hansson wrote: > >> On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote: > >> > + ret = device_add(&genpd->dev); > >> > >> What's the point of adding the device? Can we skip this step, as I > >> guess the opp core is only using the device as cookie rather actual > >> using it? No? > > > > We also use it for the OPP debugfs stuff, so that would be required I believe. > > Right, however, isn't that only using the dev_name(dev), which you > don't need to add the device to make use of. > > Or maybe I missing something around this... So I tested this bit. The code works fine even if the device isn't added (registered), but this looks a bit sloppy to attempt. Please let me know what would you prefer in this case, add the device or not. -- viresh
On 9 April 2018 at 09:53, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 22-03-18, 11:18, Ulf Hansson wrote: >> On 22 March 2018 at 10:59, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> > On 22-03-18, 10:30, Ulf Hansson wrote: >> >> On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@linaro.org> wrote: > >> >> > + ret = device_add(&genpd->dev); >> >> >> >> What's the point of adding the device? Can we skip this step, as I >> >> guess the opp core is only using the device as cookie rather actual >> >> using it? No? >> > >> > We also use it for the OPP debugfs stuff, so that would be required I believe. >> >> Right, however, isn't that only using the dev_name(dev), which you >> don't need to add the device to make use of. >> >> Or maybe I missing something around this... > > So I tested this bit. The code works fine even if the device isn't added > (registered), but this looks a bit sloppy to attempt. > > Please let me know what would you prefer in this case, add the device or not. Well, I don't know if there is policy of how to do this in general. Or perhaps this can be considered as a special case as the device is only going to be used as cookie (at least so far). I suggest we keep it as simple as possible and *not* add (register) the device. Kind regards Uffe
Hi, Now that the DT bindings [1] are already Reviewed/Acked by respective maintainers, here is the code to start using them. The first two patches provide helpers in the OPP core, [3-5]/7 update the PM domain core to start supporting domain OPP tables, etc, 6/7 updates the OPP core to use the new callback provided by the PM domains to get performance state and the last one removes the unused helpers now. This is tested on Hikey620 and works just fine. -- viresh [1] https://lkml.kernel.org/r/cover.1513591822.git.viresh.kumar@linaro.org Viresh Kumar (7): PM / OPP: Implement dev_pm_opp_of_add_table_indexed() PM / OPP: Implement of_dev_pm_opp_find_required_opp() PM / Domain: Add struct device to genpd PM / Domain: Add support to parse domain's OPP table PM / Domain: Implement of_dev_pm_genpd_get_performance_state() PM / OPP: Get performance state using genpd helper PM / OPP: Remove dev_pm_opp_{un}register_get_pstate_helper() drivers/base/power/domain.c | 159 ++++++++++++++++++++++++++++++++++++++++---- drivers/opp/core.c | 82 +---------------------- drivers/opp/of.c | 123 +++++++++++++++++++++++++++++++--- drivers/opp/opp.h | 3 +- include/linux/pm_domain.h | 12 ++++ include/linux/pm_opp.h | 22 +++--- 6 files changed, 284 insertions(+), 117 deletions(-) -- 2.15.0.194.g9af6a3dea062