| Message ID | 20190717222340.137578-3-saravanak@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add required-opps support to devfreq passive gov | expand |
On 17-07-19, 15:23, Saravana Kannan wrote: > Add a function that allows looking up required OPPs given a source OPP > table, destination OPP table and the source OPP. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/opp/core.c | 54 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/pm_opp.h | 11 +++++++++ > 2 files changed, 65 insertions(+) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 438fcd134d93..72c055a3f6b7 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table) > } > EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd); > > +/** > + * dev_pm_opp_xlate_opp() - Find required OPP for src_table OPP. > + * @src_table: OPP table which has dst_table as one of its required OPP table. > + * @dst_table: Required OPP table of the src_table. > + * @pstate: OPP of the src_table. You should use @ before parameters in the comments as well ? Just like you did that below. > + * > + * This function returns the OPP (present in @dst_table) pointed out by the > + * "required-opps" property of the OPP (present in @src_table). > + * > + * The callers are required to call dev_pm_opp_put() for the returned OPP after > + * use. > + * > + * Return: destination table OPP on success, otherwise NULL on errors. > + */ > +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table, Please name it dev_pm_opp_xlate_required_opp(). > + struct opp_table *dst_table, > + struct dev_pm_opp *src_opp) > +{ > + struct dev_pm_opp *opp, *dest_opp = NULL; > + int i; > + > + if (!src_table || !dst_table || !src_opp) > + return NULL; > + > + for (i = 0; i < src_table->required_opp_count; i++) { > + if (src_table->required_opp_tables[i]->np == dst_table->np) Why can't we just compare the table pointers instead ? Yeah, I know that's how I wrote that in the other xlate function, but I am confused now :) > + break; > + } > + > + if (unlikely(i == src_table->required_opp_count)) { > + pr_err("%s: Couldn't find matching OPP table (%p: %p)\n", > + __func__, src_table, dst_table); > + return NULL; > + } > + > + mutex_lock(&src_table->lock); > + > + list_for_each_entry(opp, &src_table->opp_list, node) { > + if (opp == src_opp) { > + dest_opp = opp->required_opps[i]; > + dev_pm_opp_get(dest_opp); > + goto unlock; > + } > + } > + > + pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table, > + dst_table); > + > +unlock: > + mutex_unlock(&src_table->lock); > + > + return dest_opp; > +} > + > /** > * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table. > * @src_table: OPP table which has dst_table as one of its required OPP table. > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index af5021f27cb7..36f52b9cf24a 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -131,6 +131,9 @@ 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); > 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); > +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table, > + struct opp_table *dst_table, > + struct dev_pm_opp *src_opp); > 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); > @@ -304,6 +307,14 @@ static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table > return -ENOTSUPP; > } > > +static inline struct dev_pm_opp *dev_pm_opp_xlate_opp( > + struct opp_table *src_table, > + struct opp_table *dst_table, > + struct dev_pm_opp *src_opp) > +{ > + return NULL; > +} > + Keep the order of declaring routines same, so this goes before the other xlate routine. > static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > { > return -ENOTSUPP; > -- > 2.22.0.510.g264f2c817a-goog
On Tue, Jul 23, 2019 at 2:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 17-07-19, 15:23, Saravana Kannan wrote: > > Add a function that allows looking up required OPPs given a source OPP > > table, destination OPP table and the source OPP. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/opp/core.c | 54 ++++++++++++++++++++++++++++++++++++++++++ > > include/linux/pm_opp.h | 11 +++++++++ > > 2 files changed, 65 insertions(+) > > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > > index 438fcd134d93..72c055a3f6b7 100644 > > --- a/drivers/opp/core.c > > +++ b/drivers/opp/core.c > > @@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table) > > } > > EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd); > > > > +/** > > + * dev_pm_opp_xlate_opp() - Find required OPP for src_table OPP. > > + * @src_table: OPP table which has dst_table as one of its required OPP table. > > + * @dst_table: Required OPP table of the src_table. > > + * @pstate: OPP of the src_table. > > You should use @ before parameters in the comments as well ? Just like > you did that below. And I should probably be deleting the @pstate phantom parameter :) > > + * > > + * This function returns the OPP (present in @dst_table) pointed out by the > > + * "required-opps" property of the OPP (present in @src_table). > > + * > > + * The callers are required to call dev_pm_opp_put() for the returned OPP after > > + * use. > > + * > > + * Return: destination table OPP on success, otherwise NULL on errors. > > + */ > > +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table, > > Please name it dev_pm_opp_xlate_required_opp(). Ok > > > + struct opp_table *dst_table, > > + struct dev_pm_opp *src_opp) > > +{ > > + struct dev_pm_opp *opp, *dest_opp = NULL; > > + int i; > > + > > + if (!src_table || !dst_table || !src_opp) > > + return NULL; > > + > > + for (i = 0; i < src_table->required_opp_count; i++) { > > + if (src_table->required_opp_tables[i]->np == dst_table->np) > > Why can't we just compare the table pointers instead ? Yeah, I know > that's how I wrote that in the other xlate function, but I am confused > now :) I almost said "not sure. Let me just compare pointers". I think (not sure) it has to do with the same OPP table being used to create multiple OPP table copies if the "shared OPP table" flag isn't set? Can you confirm if this makes sense? If so, I can add a comment patch that adds comments to the existing code and then copies it into this function in this patch. > > + break; > > + } > > + > > + if (unlikely(i == src_table->required_opp_count)) { > > + pr_err("%s: Couldn't find matching OPP table (%p: %p)\n", > > + __func__, src_table, dst_table); > > + return NULL; > > + } > > + > > + mutex_lock(&src_table->lock); > > + > > + list_for_each_entry(opp, &src_table->opp_list, node) { > > + if (opp == src_opp) { > > + dest_opp = opp->required_opps[i]; > > + dev_pm_opp_get(dest_opp); > > + goto unlock; > > + } > > + } > > + > > + pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table, > > + dst_table); > > + > > +unlock: > > + mutex_unlock(&src_table->lock); > > + > > + return dest_opp; > > +} > > + > > /** > > * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table. > > * @src_table: OPP table which has dst_table as one of its required OPP table. > > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > > index af5021f27cb7..36f52b9cf24a 100644 > > --- a/include/linux/pm_opp.h > > +++ b/include/linux/pm_opp.h > > @@ -131,6 +131,9 @@ 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); > > 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); > > +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table, > > + struct opp_table *dst_table, > > + struct dev_pm_opp *src_opp); > > 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); > > @@ -304,6 +307,14 @@ static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table > > return -ENOTSUPP; > > } > > > > +static inline struct dev_pm_opp *dev_pm_opp_xlate_opp( > > + struct opp_table *src_table, > > + struct opp_table *dst_table, > > + struct dev_pm_opp *src_opp) > > +{ > > + return NULL; > > +} > > + > > Keep the order of declaring routines same, so this goes before the > other xlate routine. Will do. -Saravana > > static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > > { > > return -ENOTSUPP; > > -- > > 2.22.0.510.g264f2c817a-goog > > -- > viresh
On 23-07-19, 17:23, Saravana Kannan wrote: > On Tue, Jul 23, 2019 at 2:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 17-07-19, 15:23, Saravana Kannan wrote: > > > Add a function that allows looking up required OPPs given a source OPP > > > table, destination OPP table and the source OPP. > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > --- > > > drivers/opp/core.c | 54 ++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/pm_opp.h | 11 +++++++++ > > > 2 files changed, 65 insertions(+) > > > > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > > > index 438fcd134d93..72c055a3f6b7 100644 > > > --- a/drivers/opp/core.c > > > +++ b/drivers/opp/core.c > > > @@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table) > > > } > > > EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd); > > > > > > +/** > > > + * dev_pm_opp_xlate_opp() - Find required OPP for src_table OPP. > > > + * @src_table: OPP table which has dst_table as one of its required OPP table. > > > + * @dst_table: Required OPP table of the src_table. > > > + * @pstate: OPP of the src_table. > > > > You should use @ before parameters in the comments as well ? Just like > > you did that below. > > And I should probably be deleting the @pstate phantom parameter :) > > > > + * > > > + * This function returns the OPP (present in @dst_table) pointed out by the > > > + * "required-opps" property of the OPP (present in @src_table). > > > + * > > > + * The callers are required to call dev_pm_opp_put() for the returned OPP after > > > + * use. > > > + * > > > + * Return: destination table OPP on success, otherwise NULL on errors. > > > + */ > > > +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table, > > > > Please name it dev_pm_opp_xlate_required_opp(). > > Ok > > > > > > + struct opp_table *dst_table, > > > + struct dev_pm_opp *src_opp) > > > +{ > > > + struct dev_pm_opp *opp, *dest_opp = NULL; > > > + int i; > > > + > > > + if (!src_table || !dst_table || !src_opp) > > > + return NULL; > > > + > > > + for (i = 0; i < src_table->required_opp_count; i++) { > > > + if (src_table->required_opp_tables[i]->np == dst_table->np) > > > > Why can't we just compare the table pointers instead ? Yeah, I know > > that's how I wrote that in the other xlate function, but I am confused > > now :) > > I almost said "not sure. Let me just compare pointers". > I think (not sure) it has to do with the same OPP table being used to > create multiple OPP table copies if the "shared OPP table" flag isn't > set? > Can you confirm if this makes sense? If so, I can add a comment patch > that adds comments to the existing code and then copies it into this > function in this patch. Right, that was the reason but we also need to fix ... > > > + break; > > > + } > > > + > > > + if (unlikely(i == src_table->required_opp_count)) { > > > + pr_err("%s: Couldn't find matching OPP table (%p: %p)\n", > > > + __func__, src_table, dst_table); > > > + return NULL; > > > + } > > > + > > > + mutex_lock(&src_table->lock); > > > + > > > + list_for_each_entry(opp, &src_table->opp_list, node) { > > > + if (opp == src_opp) { ... this as well. We must be comparing node pointers here as well. > > > + dest_opp = opp->required_opps[i]; > > > + dev_pm_opp_get(dest_opp); > > > + goto unlock; > > > + } > > > + } > > > + > > > + pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table, > > > + dst_table); > > > + > > > +unlock: > > > + mutex_unlock(&src_table->lock); > > > + > > > + return dest_opp; > > > +} > > > + > > > /** > > > * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table. > > > * @src_table: OPP table which has dst_table as one of its required OPP table. > > > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > > > index af5021f27cb7..36f52b9cf24a 100644 > > > --- a/include/linux/pm_opp.h > > > +++ b/include/linux/pm_opp.h > > > @@ -131,6 +131,9 @@ 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); > > > 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); > > > +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table, > > > + struct opp_table *dst_table, > > > + struct dev_pm_opp *src_opp); > > > 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); > > > @@ -304,6 +307,14 @@ static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table > > > return -ENOTSUPP; > > > } > > > > > > +static inline struct dev_pm_opp *dev_pm_opp_xlate_opp( > > > + struct opp_table *src_table, > > > + struct opp_table *dst_table, > > > + struct dev_pm_opp *src_opp) > > > +{ > > > + return NULL; > > > +} > > > + > > > > Keep the order of declaring routines same, so this goes before the > > other xlate routine. > > Will do. > > -Saravana > > > > static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) > > > { > > > return -ENOTSUPP; > > > -- > > > 2.22.0.510.g264f2c817a-goog > > > > -- > > viresh
On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 23-07-19, 17:23, Saravana Kannan wrote: > > On Tue, Jul 23, 2019 at 2:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 17-07-19, 15:23, Saravana Kannan wrote: > > > > Add a function that allows looking up required OPPs given a source OPP > > > > table, destination OPP table and the source OPP. > > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > --- > > > > drivers/opp/core.c | 54 ++++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/pm_opp.h | 11 +++++++++ > > > > 2 files changed, 65 insertions(+) > > > > > > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > > > > index 438fcd134d93..72c055a3f6b7 100644 > > > > --- a/drivers/opp/core.c > > > > +++ b/drivers/opp/core.c > > > > @@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table) > > > > } > > > > EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd); > > > > > > > > +/** > > > > + * dev_pm_opp_xlate_opp() - Find required OPP for src_table OPP. > > > > + * @src_table: OPP table which has dst_table as one of its required OPP table. > > > > + * @dst_table: Required OPP table of the src_table. > > > > + * @pstate: OPP of the src_table. > > > > > > You should use @ before parameters in the comments as well ? Just like > > > you did that below. > > > > And I should probably be deleting the @pstate phantom parameter :) > > > > > > + * > > > > + * This function returns the OPP (present in @dst_table) pointed out by the > > > > + * "required-opps" property of the OPP (present in @src_table). > > > > + * > > > > + * The callers are required to call dev_pm_opp_put() for the returned OPP after > > > > + * use. > > > > + * > > > > + * Return: destination table OPP on success, otherwise NULL on errors. > > > > + */ > > > > +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table, > > > > > > Please name it dev_pm_opp_xlate_required_opp(). > > > > Ok > > > > > > > > > + struct opp_table *dst_table, > > > > + struct dev_pm_opp *src_opp) > > > > +{ > > > > + struct dev_pm_opp *opp, *dest_opp = NULL; > > > > + int i; > > > > + > > > > + if (!src_table || !dst_table || !src_opp) > > > > + return NULL; > > > > + > > > > + for (i = 0; i < src_table->required_opp_count; i++) { > > > > + if (src_table->required_opp_tables[i]->np == dst_table->np) > > > > > > Why can't we just compare the table pointers instead ? Yeah, I know > > > that's how I wrote that in the other xlate function, but I am confused > > > now :) > > > > I almost said "not sure. Let me just compare pointers". > > I think (not sure) it has to do with the same OPP table being used to > > create multiple OPP table copies if the "shared OPP table" flag isn't > > set? > > Can you confirm if this makes sense? If so, I can add a comment patch > > that adds comments to the existing code and then copies it into this > > function in this patch. > > Right, that was the reason but we also need to fix ... I know I gave that explanation but I'm still a bit confused by the existing logic. If the same DT OPP table is used to create multiple in memory OPP tables, how do you device which in memory OPP table is the right one to point to? > > > > > + break; > > > > + } > > > > + > > > > + if (unlikely(i == src_table->required_opp_count)) { > > > > + pr_err("%s: Couldn't find matching OPP table (%p: %p)\n", > > > > + __func__, src_table, dst_table); > > > > + return NULL; > > > > + } > > > > + > > > > + mutex_lock(&src_table->lock); > > > > + > > > > + list_for_each_entry(opp, &src_table->opp_list, node) { > > > > + if (opp == src_opp) { > > ... this as well. We must be comparing node pointers here as well. Not really, if an in memory OPP entry is not part of an in memory OPP table list, I don't think it should be considered part of the OPP table just because the node pointer is the same. I think that's explicitly wrong and the above code is correct as is. -Saravana
On 24-07-19, 20:46, Saravana Kannan wrote: > On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 23-07-19, 17:23, Saravana Kannan wrote: > > > I almost said "not sure. Let me just compare pointers". > > > I think (not sure) it has to do with the same OPP table being used to > > > create multiple OPP table copies if the "shared OPP table" flag isn't > > > set? > > > Can you confirm if this makes sense? If so, I can add a comment patch > > > that adds comments to the existing code and then copies it into this > > > function in this patch. > > > > Right, that was the reason but we also need to fix ... > > I know I gave that explanation but I'm still a bit confused by the > existing logic. If the same DT OPP table is used to create multiple in > memory OPP tables, how do you device which in memory OPP table is the > right one to point to? This is a bit broken actually, we don't see any problems right now but may eventually have to fix it someday. We pick the first in-memory OPP table that was created using the DT OPP table. This is done because the DT doesn't provide any explicit linking to the required-opp device right now. Right now the required-opps is only used for power domains and so it is working fine. It may work fine for your case as well. But once we have a case we want to use required-opps in a single OPP table for both power-domains and master/slave thing you are proposing, we may see more problems. > > > > > + break; > > > > > + } > > > > > + > > > > > + if (unlikely(i == src_table->required_opp_count)) { > > > > > + pr_err("%s: Couldn't find matching OPP table (%p: %p)\n", > > > > > + __func__, src_table, dst_table); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + mutex_lock(&src_table->lock); > > > > > + > > > > > + list_for_each_entry(opp, &src_table->opp_list, node) { > > > > > + if (opp == src_opp) { > > > > ... this as well. We must be comparing node pointers here as well. > > Not really, if an in memory OPP entry is not part of an in memory OPP > table list, I don't think it should be considered part of the OPP > table just because the node pointer is the same. I think that's > explicitly wrong and the above code is correct as is. I understand what you are saying, but because we match the very first OPP table that was there in the list we need to match the DT node here as well. Or somehow we make sure to have the correct in-memory OPP table being pointed by the required-opp-table array. Then we don't need the node pointer anywhere here.
On Wed, Jul 24, 2019 at 10:38 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 24-07-19, 20:46, Saravana Kannan wrote: > > On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > On 23-07-19, 17:23, Saravana Kannan wrote: > > > > > I almost said "not sure. Let me just compare pointers". > > > > I think (not sure) it has to do with the same OPP table being used to > > > > create multiple OPP table copies if the "shared OPP table" flag isn't > > > > set? > > > > Can you confirm if this makes sense? If so, I can add a comment patch > > > > that adds comments to the existing code and then copies it into this > > > > function in this patch. > > > > > > Right, that was the reason but we also need to fix ... > > > > I know I gave that explanation but I'm still a bit confused by the > > existing logic. If the same DT OPP table is used to create multiple in > > memory OPP tables, how do you device which in memory OPP table is the > > right one to point to? > > This is a bit broken actually, we don't see any problems right now but > may eventually have to fix it someday. > > We pick the first in-memory OPP table that was created using the DT > OPP table. This is done because the DT doesn't provide any explicit > linking to the required-opp device right now. > > Right now the required-opps is only used for power domains and so it > is working fine. It may work fine for your case as well. But once we > have a case we want to use required-opps in a single OPP table for > both power-domains and master/slave thing you are proposing, we may > see more problems. > > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + if (unlikely(i == src_table->required_opp_count)) { > > > > > > + pr_err("%s: Couldn't find matching OPP table (%p: %p)\n", > > > > > > + __func__, src_table, dst_table); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + mutex_lock(&src_table->lock); > > > > > > + > > > > > > + list_for_each_entry(opp, &src_table->opp_list, node) { > > > > > > + if (opp == src_opp) { > > > > > > ... this as well. We must be comparing node pointers here as well. > > > > Not really, if an in memory OPP entry is not part of an in memory OPP > > table list, I don't think it should be considered part of the OPP > > table just because the node pointer is the same. I think that's > > explicitly wrong and the above code is correct as is. > > I understand what you are saying, but because we match the very first > OPP table that was there in the list we need to match the DT node here > as well. > > Or somehow we make sure to have the correct in-memory OPP table being > pointed by the required-opp-table array. Then we don't need the node > pointer anywhere here. Ah, right. I'll fix this. -Saravana
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 438fcd134d93..72c055a3f6b7 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1883,6 +1883,60 @@ void dev_pm_opp_detach_genpd(struct opp_table *opp_table) } EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd); +/** + * dev_pm_opp_xlate_opp() - Find required OPP for src_table OPP. + * @src_table: OPP table which has dst_table as one of its required OPP table. + * @dst_table: Required OPP table of the src_table. + * @pstate: OPP of the src_table. + * + * This function returns the OPP (present in @dst_table) pointed out by the + * "required-opps" property of the OPP (present in @src_table). + * + * The callers are required to call dev_pm_opp_put() for the returned OPP after + * use. + * + * Return: destination table OPP on success, otherwise NULL on errors. + */ +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table, + struct opp_table *dst_table, + struct dev_pm_opp *src_opp) +{ + struct dev_pm_opp *opp, *dest_opp = NULL; + int i; + + if (!src_table || !dst_table || !src_opp) + return NULL; + + for (i = 0; i < src_table->required_opp_count; i++) { + if (src_table->required_opp_tables[i]->np == dst_table->np) + break; + } + + if (unlikely(i == src_table->required_opp_count)) { + pr_err("%s: Couldn't find matching OPP table (%p: %p)\n", + __func__, src_table, dst_table); + return NULL; + } + + mutex_lock(&src_table->lock); + + list_for_each_entry(opp, &src_table->opp_list, node) { + if (opp == src_opp) { + dest_opp = opp->required_opps[i]; + dev_pm_opp_get(dest_opp); + goto unlock; + } + } + + pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table, + dst_table); + +unlock: + mutex_unlock(&src_table->lock); + + return dest_opp; +} + /** * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table. * @src_table: OPP table which has dst_table as one of its required OPP table. diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index af5021f27cb7..36f52b9cf24a 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -131,6 +131,9 @@ 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); 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); +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table, + struct opp_table *dst_table, + struct dev_pm_opp *src_opp); 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); @@ -304,6 +307,14 @@ static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table return -ENOTSUPP; } +static inline struct dev_pm_opp *dev_pm_opp_xlate_opp( + struct opp_table *src_table, + struct opp_table *dst_table, + struct dev_pm_opp *src_opp) +{ + return NULL; +} + static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) { return -ENOTSUPP;
Add a function that allows looking up required OPPs given a source OPP table, destination OPP table and the source OPP. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/opp/core.c | 54 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 11 +++++++++ 2 files changed, 65 insertions(+)