| Message ID | 027985ce35873cd218298302a1408da06d48458b.1562565567.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | opp: Return genpd virtual devices from dev_pm_opp_attach_genpd() | expand |
On 7/8/2019 11:30 AM, Viresh Kumar wrote: > The cpufreq drivers don't need to do runtime PM operations on the > virtual devices returned by dev_pm_domain_attach_by_name() and so the > virtual devices weren't shared with the callers of > dev_pm_opp_attach_genpd() earlier. > > But the IO device drivers would want to do that. This patch updates the > prototype of dev_pm_opp_attach_genpd() to accept another argument to > return the pointer to the array of genpd virtual devices. > > Reported-by: Rajendra Nayak <rnayak@codeaurora.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > @Rajendra: Can you please test this one ? I have only compile tested it. Sorry for the delay, I seem to have completely missed this patch. I just gave this a try and here are some observations, I have a case where I have one device with 2 power domains, one of them is scale-able (supports perf state) and the other one supports only being turned on and off. 1. In the driver I now need to use dev_pm_domain_attach_by_name/id to attach the power domain which supports only on/off and then use dev_pm_opp_attach_genpd() for the one which supports perf states. 2. My OPP table has only 1 required_opps, so the required_opp_count for the OPP table is 1. Now if my device tree has my scale-able powerdomain at index 1 (it works if its at index 0) then I end up with this error [ 2.858628] ufshcd-qcom 1d84000.ufshc: Index can't be greater than required-opp-count - 1, rpmh_pd (1 : 1) so it looks like a lot of the OPP core today just assumes that if a device has multiple power domains, all of them are scale-able which isn't necessarily true. > > drivers/opp/core.c | 5 ++++- > include/linux/pm_opp.h | 4 ++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 2958cc7bbb58..07b6f1187b3b 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1775,6 +1775,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table) > * dev_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer > * @dev: Consumer device for which the genpd is getting attached. > * @names: Null terminated array of pointers containing names of genpd to attach. > + * @virt_devs: Pointer to return the array of virtual devices. > * > * Multiple generic power domains for a device are supported with the help of > * virtual genpd devices, which are created for each consumer device - genpd > @@ -1789,7 +1790,8 @@ static void _opp_detach_genpd(struct opp_table *opp_table) > * This helper needs to be called once with a list of all genpd to attach. > * Otherwise the original device structure will be used instead by the OPP core. > */ > -struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names) > +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > + const char **names, struct device ***virt_devs) > { > struct opp_table *opp_table; > struct device *virt_dev; > @@ -1850,6 +1852,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names > name++; > } > > + *virt_devs = opp_table->genpd_virt_devs; > mutex_unlock(&opp_table->genpd_virt_dev_lock); > > return opp_table; > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index be570761b77a..7c2fe2952f40 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -131,7 +131,7 @@ 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_attach_genpd(struct device *dev, const char **names); > +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs); > void dev_pm_opp_detach_genpd(struct opp_table *opp_table); > int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate); > int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); > @@ -295,7 +295,7 @@ 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_attach_genpd(struct device *dev, const char **names) > +static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs) > { > return ERR_PTR(-ENOTSUPP); > } >
On Mon, Jul 08, 2019 at 11:30:11AM +0530, Viresh Kumar wrote: > The cpufreq drivers don't need to do runtime PM operations on the > virtual devices returned by dev_pm_domain_attach_by_name() and so the > virtual devices weren't shared with the callers of > dev_pm_opp_attach_genpd() earlier. > > But the IO device drivers would want to do that. This patch updates the > prototype of dev_pm_opp_attach_genpd() to accept another argument to > return the pointer to the array of genpd virtual devices. > > Reported-by: Rajendra Nayak <rnayak@codeaurora.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > @Rajendra: Can you please test this one ? I have only compile tested it. > > drivers/opp/core.c | 5 ++++- > include/linux/pm_opp.h | 4 ++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 2958cc7bbb58..07b6f1187b3b 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1775,6 +1775,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table) > * dev_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer > * @dev: Consumer device for which the genpd is getting attached. > * @names: Null terminated array of pointers containing names of genpd to attach. > + * @virt_devs: Pointer to return the array of virtual devices. > * > * Multiple generic power domains for a device are supported with the help of > * virtual genpd devices, which are created for each consumer device - genpd > @@ -1789,7 +1790,8 @@ static void _opp_detach_genpd(struct opp_table *opp_table) > * This helper needs to be called once with a list of all genpd to attach. > * Otherwise the original device structure will be used instead by the OPP core. > */ > -struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names) > +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > + const char **names, struct device ***virt_devs) > { > struct opp_table *opp_table; > struct device *virt_dev; > @@ -1850,6 +1852,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names > name++; > } > > + *virt_devs = opp_table->genpd_virt_devs; Could we perhaps only do this if (virt_devs), that way callers can send in NULL if they don't care about the genpd virtual devices. Kind regards, Niklas > mutex_unlock(&opp_table->genpd_virt_dev_lock); > > return opp_table; > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index be570761b77a..7c2fe2952f40 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -131,7 +131,7 @@ 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_attach_genpd(struct device *dev, const char **names); > +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs); > void dev_pm_opp_detach_genpd(struct opp_table *opp_table); > int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate); > int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); > @@ -295,7 +295,7 @@ 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_attach_genpd(struct device *dev, const char **names) > +static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs) > { > return ERR_PTR(-ENOTSUPP); > } > -- > 2.21.0.rc0.269.g1a574e7a288b >
On 16-07-19, 12:43, Niklas Cassel wrote: > On Mon, Jul 08, 2019 at 11:30:11AM +0530, Viresh Kumar wrote: > > The cpufreq drivers don't need to do runtime PM operations on the > > virtual devices returned by dev_pm_domain_attach_by_name() and so the > > virtual devices weren't shared with the callers of > > dev_pm_opp_attach_genpd() earlier. > > > > But the IO device drivers would want to do that. This patch updates the > > prototype of dev_pm_opp_attach_genpd() to accept another argument to > > return the pointer to the array of genpd virtual devices. > > > > Reported-by: Rajendra Nayak <rnayak@codeaurora.org> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > @Rajendra: Can you please test this one ? I have only compile tested it. > > > > drivers/opp/core.c | 5 ++++- > > include/linux/pm_opp.h | 4 ++-- > > 2 files changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > > index 2958cc7bbb58..07b6f1187b3b 100644 > > --- a/drivers/opp/core.c > > +++ b/drivers/opp/core.c > > @@ -1775,6 +1775,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table) > > * dev_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer > > * @dev: Consumer device for which the genpd is getting attached. > > * @names: Null terminated array of pointers containing names of genpd to attach. > > + * @virt_devs: Pointer to return the array of virtual devices. > > * > > * Multiple generic power domains for a device are supported with the help of > > * virtual genpd devices, which are created for each consumer device - genpd > > @@ -1789,7 +1790,8 @@ static void _opp_detach_genpd(struct opp_table *opp_table) > > * This helper needs to be called once with a list of all genpd to attach. > > * Otherwise the original device structure will be used instead by the OPP core. > > */ > > -struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names) > > +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > > + const char **names, struct device ***virt_devs) > > { > > struct opp_table *opp_table; > > struct device *virt_dev; > > @@ -1850,6 +1852,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names > > name++; > > } > > > > + *virt_devs = opp_table->genpd_virt_devs; > > Could we perhaps only do this if (virt_devs), that way callers can send in > NULL if they don't care about the genpd virtual devices. That was the idea and I failed to add it :(
On 11-07-19, 15:09, Rajendra Nayak wrote: > Sorry for the delay Same here :) > I seem to have completely missed this patch. > I just gave this a try and here are some observations, > > I have a case where I have one device with 2 power domains, one of them > is scale-able (supports perf state) and the other one supports only being > turned on and off. > > 1. In the driver I now need to use dev_pm_domain_attach_by_name/id to attach the > power domain which supports only on/off and then use dev_pm_opp_attach_genpd() > for the one which supports perf states. > > 2. My OPP table has only 1 required_opps, so the required_opp_count for the OPP table is 1. > Now if my device tree has my scale-able powerdomain at index 1 (it works if its at index 0) > then I end up with this error > > [ 2.858628] ufshcd-qcom 1d84000.ufshc: Index can't be greater than required-opp-count - 1, rpmh_pd (1 : 1) > > so it looks like a lot of the OPP core today just assumes that if a device has multiple power domains, > all of them are scale-able which isn't necessarily true. I don't think a lot of OPP core has these problems, but maybe only this place. I was taking care of this since the beginning just forgot it now. What about this over this commit: diff --git a/drivers/opp/core.c b/drivers/opp/core.c index d76ead4eff4c..1f11f8c92337 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1789,13 +1789,16 @@ static void _opp_detach_genpd(struct opp_table *opp_table) * * This helper needs to be called once with a list of all genpd to attach. * Otherwise the original device structure will be used instead by the OPP core. + * + * The order of entries in the names array must match the order in which + * "required-opps" are added in DT. */ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs) { struct opp_table *opp_table; struct device *virt_dev; - int index, ret = -EINVAL; + int index = 0, ret = -EINVAL; const char **name = names; opp_table = dev_pm_opp_get_opp_table(dev); @@ -1821,14 +1824,6 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, goto unlock; while (*name) { - index = of_property_match_string(dev->of_node, - "power-domain-names", *name); - if (index < 0) { - dev_err(dev, "Failed to find power domain: %s (%d)\n", - *name, index); - goto err; - } - if (index >= opp_table->required_opp_count) { dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n", *name, opp_table->required_opp_count, index); @@ -1849,6 +1844,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, } opp_table->genpd_virt_devs[index] = virt_dev; + index++; name++; }
On 7/17/2019 11:17 AM, Viresh Kumar wrote: > On 11-07-19, 15:09, Rajendra Nayak wrote: >> Sorry for the delay > > Same here :) > >> I seem to have completely missed this patch. >> I just gave this a try and here are some observations, >> >> I have a case where I have one device with 2 power domains, one of them >> is scale-able (supports perf state) and the other one supports only being >> turned on and off. >> >> 1. In the driver I now need to use dev_pm_domain_attach_by_name/id to attach the >> power domain which supports only on/off and then use dev_pm_opp_attach_genpd() >> for the one which supports perf states. >> >> 2. My OPP table has only 1 required_opps, so the required_opp_count for the OPP table is 1. >> Now if my device tree has my scale-able powerdomain at index 1 (it works if its at index 0) >> then I end up with this error >> >> [ 2.858628] ufshcd-qcom 1d84000.ufshc: Index can't be greater than required-opp-count - 1, rpmh_pd (1 : 1) >> >> so it looks like a lot of the OPP core today just assumes that if a device has multiple power domains, >> all of them are scale-able which isn't necessarily true. > > I don't think a lot of OPP core has these problems, but maybe only > this place. I was taking care of this since the beginning just forgot > it now. > > What about this over this commit: Yes, this does seem to fix my concern mentioned in 2. above. > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index d76ead4eff4c..1f11f8c92337 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1789,13 +1789,16 @@ static void _opp_detach_genpd(struct opp_table *opp_table) > * > * This helper needs to be called once with a list of all genpd to attach. > * Otherwise the original device structure will be used instead by the OPP core. > + * > + * The order of entries in the names array must match the order in which > + * "required-opps" are added in DT. > */ > struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > const char **names, struct device ***virt_devs) > { > struct opp_table *opp_table; > struct device *virt_dev; > - int index, ret = -EINVAL; > + int index = 0, ret = -EINVAL; > const char **name = names; > > opp_table = dev_pm_opp_get_opp_table(dev); > @@ -1821,14 +1824,6 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > goto unlock; > > while (*name) { > - index = of_property_match_string(dev->of_node, > - "power-domain-names", *name); > - if (index < 0) { > - dev_err(dev, "Failed to find power domain: %s (%d)\n", > - *name, index); > - goto err; > - } > - > if (index >= opp_table->required_opp_count) { > dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n", > *name, opp_table->required_opp_count, index); > @@ -1849,6 +1844,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > } > > opp_table->genpd_virt_devs[index] = virt_dev; > + index++; > name++; > } > >
On 17-07-19, 15:34, Rajendra Nayak wrote: > > > On 7/17/2019 11:17 AM, Viresh Kumar wrote: > > On 11-07-19, 15:09, Rajendra Nayak wrote: > > > Sorry for the delay > > > > Same here :) > > > > > I seem to have completely missed this patch. > > > I just gave this a try and here are some observations, > > > > > > I have a case where I have one device with 2 power domains, one of them > > > is scale-able (supports perf state) and the other one supports only being > > > turned on and off. > > > > > > 1. In the driver I now need to use dev_pm_domain_attach_by_name/id to attach the > > > power domain which supports only on/off and then use dev_pm_opp_attach_genpd() > > > for the one which supports perf states. > > > > > > 2. My OPP table has only 1 required_opps, so the required_opp_count for the OPP table is 1. > > > Now if my device tree has my scale-able powerdomain at index 1 (it works if its at index 0) > > > then I end up with this error > > > > > > [ 2.858628] ufshcd-qcom 1d84000.ufshc: Index can't be greater than required-opp-count - 1, rpmh_pd (1 : 1) > > > > > > so it looks like a lot of the OPP core today just assumes that if a device has multiple power domains, > > > all of them are scale-able which isn't necessarily true. > > > > I don't think a lot of OPP core has these problems, but maybe only > > this place. I was taking care of this since the beginning just forgot > > it now. > > > > What about this over this commit: > > Yes, this does seem to fix my concern mentioned in 2. above. Great. I will include your Tested-by:, Lemme know if you have any objections.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 2958cc7bbb58..07b6f1187b3b 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1775,6 +1775,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table) * dev_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer * @dev: Consumer device for which the genpd is getting attached. * @names: Null terminated array of pointers containing names of genpd to attach. + * @virt_devs: Pointer to return the array of virtual devices. * * Multiple generic power domains for a device are supported with the help of * virtual genpd devices, which are created for each consumer device - genpd @@ -1789,7 +1790,8 @@ static void _opp_detach_genpd(struct opp_table *opp_table) * This helper needs to be called once with a list of all genpd to attach. * Otherwise the original device structure will be used instead by the OPP core. */ -struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names) +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, + const char **names, struct device ***virt_devs) { struct opp_table *opp_table; struct device *virt_dev; @@ -1850,6 +1852,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names name++; } + *virt_devs = opp_table->genpd_virt_devs; mutex_unlock(&opp_table->genpd_virt_dev_lock); return opp_table; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index be570761b77a..7c2fe2952f40 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -131,7 +131,7 @@ 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_attach_genpd(struct device *dev, const char **names); +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs); void dev_pm_opp_detach_genpd(struct opp_table *opp_table); int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate); int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); @@ -295,7 +295,7 @@ 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_attach_genpd(struct device *dev, const char **names) +static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs) { return ERR_PTR(-ENOTSUPP); }
The cpufreq drivers don't need to do runtime PM operations on the virtual devices returned by dev_pm_domain_attach_by_name() and so the virtual devices weren't shared with the callers of dev_pm_opp_attach_genpd() earlier. But the IO device drivers would want to do that. This patch updates the prototype of dev_pm_opp_attach_genpd() to accept another argument to return the pointer to the array of genpd virtual devices. Reported-by: Rajendra Nayak <rnayak@codeaurora.org> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- @Rajendra: Can you please test this one ? I have only compile tested it. drivers/opp/core.c | 5 ++++- include/linux/pm_opp.h | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-)