| Message ID | 20210203092400.1791884-1-hsinyi@chromium.org |
|---|---|
| Headers | show |
| Series | Add required-opps support to devfreq passive gov | expand |
Hi Hsin-Yi, Thanks for the patch. I already reviewed this patch. But, I'll check these again and test it. On 2/3/21 6:23 PM, Hsin-Yi Wang wrote: > The devfreq passive governor scales the frequency of a "child" device based > on the current frequency of a "parent" device (not parent/child in the > sense of device hierarchy). As of today, the passive governor requires one > of the following to work correctly: > 1. The parent and child device have the same number of frequencies > 2. The child device driver passes a mapping function to translate from > parent frequency to child frequency. > > When (1) is not true, (2) is the only option right now. But often times, > all that is required is a simple mapping from parent's frequency to child's > frequency. > > Since OPPs already support pointing to other "required-opps", add support > for using that to map from parent device frequency to child device > frequency. That way, every child device driver doesn't have to implement a > separate mapping function anytime (1) isn't true. > > Some common (but not comprehensive) reason for needing a devfreq passive > governor to adjust the frequency of one device based on another are: > > 1. These were the combination of frequencies that were validated/screened > during the manufacturing process. > 2. These are the sensible performance combinations between two devices > interacting with each other. So that when one runs fast the other > doesn't become the bottleneck. > 3. Hardware bugs requiring some kind of frequency ratio between devices. > > For example, the following mapping can't be captured in DT as it stands > today because the parent and child device have different number of OPPs. > But with this patch series, this mapping can be captured cleanly. > > In arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi you have something > like this with the following changes: > > bus_g2d_400: bus0 { > compatible = "samsung,exynos-bus"; > clocks = <&cmu_top CLK_ACLK_G2D_400>; > clock-names = "bus"; > operating-points-v2 = <&bus_g2d_400_opp_table>; > status = "disabled"; > }; > > bus_noc2: bus9 { > compatible = "samsung,exynos-bus"; > clocks = <&cmu_mif CLK_ACLK_BUS2_400>; > clock-names = "bus"; > operating-points-v2 = <&bus_noc2_opp_table>; > status = "disabled"; > }; > > bus_g2d_400_opp_table: opp_table2 { > compatible = "operating-points-v2"; > opp-shared; > > opp-400000000 { > opp-hz = /bits/ 64 <400000000>; > opp-microvolt = <1075000>; > required-opps = <&noc2_400>; > }; > opp-267000000 { > opp-hz = /bits/ 64 <267000000>; > opp-microvolt = <1000000>; > required-opps = <&noc2_200>; > }; > opp-200000000 { > opp-hz = /bits/ 64 <200000000>; > opp-microvolt = <975000>; > required-opps = <&noc2_200>; > }; > opp-160000000 { > opp-hz = /bits/ 64 <160000000>; > opp-microvolt = <962500>; > required-opps = <&noc2_134>; > }; > opp-134000000 { > opp-hz = /bits/ 64 <134000000>; > opp-microvolt = <950000>; > required-opps = <&noc2_134>; > }; > opp-100000000 { > opp-hz = /bits/ 64 <100000000>; > opp-microvolt = <937500>; > required-opps = <&noc2_100>; > }; > }; > > bus_noc2_opp_table: opp_table6 { > compatible = "operating-points-v2"; > > noc2_400: opp-400000000 { > opp-hz = /bits/ 64 <400000000>; > }; > noc2_200: opp-200000000 { > opp-hz = /bits/ 64 <200000000>; > }; > noc2_134: opp-134000000 { > opp-hz = /bits/ 64 <134000000>; > }; > noc2_100: opp-100000000 { > opp-hz = /bits/ 64 <100000000>; > }; > }; > > -Saravana > > v4 -> v5: > - drop patch "OPP: Improve required-opps linking" and rebase to > https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=opp/linux-next > - Compare pointers in dev_pm_opp_xlate_required_opp(). > > v3 -> v4: > - Fixed documentation comments > - Fixed order of functions in .h file > - Renamed the new xlate API > - Caused _set_required_opps() to fail if all required opps tables aren't > linked. > v2 -> v3: > - Rebased onto linux-next. > - Added documentation comment for new fields. > - Added support for lazy required-opps linking. > - Updated Ack/Reviewed-bys. > v1 -> v2: > - Cached OPP table reference in devfreq to avoid looking up every time. > - Renamed variable in passive governor to be more intuitive. > - Updated cover letter with examples. > > Saravana Kannan (3): > OPP: Add function to look up required OPP's for a given OPP > PM / devfreq: Cache OPP table reference in devfreq > PM / devfreq: Add required OPPs support to passive governor > > drivers/devfreq/devfreq.c | 6 ++++ > drivers/devfreq/governor_passive.c | 20 ++++++++--- > drivers/opp/core.c | 58 ++++++++++++++++++++++++++++++ > include/linux/devfreq.h | 2 ++ > include/linux/pm_opp.h | 11 ++++++ > 5 files changed, 92 insertions(+), 5 deletions(-) >
On 03-02-21, 17:24, Hsin-Yi Wang wrote: > From: Saravana Kannan <saravanak@google.com> > > Look at the required OPPs of the "parent" device to determine the OPP that > is required from the slave device managed by the passive governor. This > allows having mappings between a parent device and a slave device even when > they don't have the same number of OPPs. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> > Acked-by: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > --- > drivers/devfreq/governor_passive.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c > index 63332e4a65ae8..8d92b1964f9c3 100644 > --- a/drivers/devfreq/governor_passive.c > +++ b/drivers/devfreq/governor_passive.c > @@ -19,7 +19,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > = (struct devfreq_passive_data *)devfreq->data; > struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent; > unsigned long child_freq = ULONG_MAX; > - struct dev_pm_opp *opp; > + struct dev_pm_opp *opp = NULL, *p_opp = NULL; I would initialize p_opp to ERR_PTR(-ENODEV) to avoid using IS_ERR_OR_NULL. There is no need to initialize opp as well. > int i, count, ret = 0; > > /* > @@ -56,13 +56,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > * list of parent device. Because in this case, *freq is temporary > * value which is decided by ondemand governor. > */ > - opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0); > - if (IS_ERR(opp)) { > - ret = PTR_ERR(opp); > + p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0); > + if (IS_ERR(p_opp)) { > + ret = PTR_ERR(p_opp); > goto out; Perhaps just return from here, the goto is useless here. > } > > - dev_pm_opp_put(opp); > + if (devfreq->opp_table && parent_devfreq->opp_table) > + opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table, > + devfreq->opp_table, p_opp); > + if (opp) { This needs to be part of the above if block itself, else the opp will always be NULL, isn't it ? > + *freq = dev_pm_opp_get_freq(opp); > + dev_pm_opp_put(opp); > + goto out; > + } > > /* > * Get the OPP table's index of decided freqeuncy by governor > @@ -89,6 +96,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > *freq = child_freq; > > out: > + if (!IS_ERR_OR_NULL(opp)) you should be checking for p_opp here, isn't it ? And perhaps we don't need this check as well as p_opp can't be invalid here. > + dev_pm_opp_put(p_opp); > + > return ret; > } > > -- > 2.30.0.365.g02bc693789-goog -- viresh
On 03-02-21, 17:23, Hsin-Yi Wang wrote: > The devfreq passive governor scales the frequency of a "child" device based > on the current frequency of a "parent" device (not parent/child in the > sense of device hierarchy). As of today, the passive governor requires one > of the following to work correctly: > 1. The parent and child device have the same number of frequencies > 2. The child device driver passes a mapping function to translate from > parent frequency to child frequency. > > When (1) is not true, (2) is the only option right now. But often times, > all that is required is a simple mapping from parent's frequency to child's > frequency. > > Since OPPs already support pointing to other "required-opps", add support > for using that to map from parent device frequency to child device > frequency. That way, every child device driver doesn't have to implement a > separate mapping function anytime (1) isn't true. So you guys aren't interested in dev_pm_opp_set_opp() but just the translation of the required-OPPs ? I am fine with most of the stuff and I would like to take it via OPP tree, hope that would be fine ? -- viresh
On 03-02-21, 17:23, Hsin-Yi Wang wrote: > From: Saravana Kannan <saravanak@google.com> > > 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> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > --- > drivers/opp/core.c | 58 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/pm_opp.h | 11 ++++++++ > 2 files changed, 69 insertions(+) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index dc95d29e94c1b..878f066b972cc 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -2398,6 +2398,64 @@ devm_pm_opp_attach_genpd(struct device *dev, const char **names, > } > EXPORT_SYMBOL_GPL(devm_pm_opp_attach_genpd); > > +/** > + * dev_pm_opp_xlate_required_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. > + * > + * 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_required_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 || > + !src_table->required_opp_tables) > + return NULL; > + > + /* required-opps not fully initialized yet */ > + if (lazy_linking_pending(src_table)) > + return NULL; > + > + for (i = 0; i < src_table->required_opp_count; i++) { > + if (src_table->required_opp_tables[i] == dst_table) > + 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 ab1d15ce559db..6f5f72a7f601c 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -156,6 +156,9 @@ struct opp_table *devm_pm_opp_register_set_opp_helper(struct device *dev, int (* > 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); > struct opp_table *devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs); > +struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table, > + struct opp_table *dst_table, > + struct dev_pm_opp *src_opp); > 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); > int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp); > @@ -367,6 +370,14 @@ static inline struct opp_table *devm_pm_opp_attach_genpd(struct device *dev, > return ERR_PTR(-EOPNOTSUPP); > } > > +static inline struct dev_pm_opp *dev_pm_opp_xlate_required_opp( > + struct opp_table *src_table, > + struct opp_table *dst_table, > + struct dev_pm_opp *src_opp) > +{ > + return NULL; > +} Like other routines that return opp *, don't return NULL on errors but a valid error number instead. And follow the declaration format of other routines from this file, don't break lines etc.. > + > static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate) > { > return -EOPNOTSUPP; > -- > 2.30.0.365.g02bc693789-goog -- viresh
On Thu, Feb 4, 2021 at 10:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 03-02-21, 17:24, Hsin-Yi Wang wrote: > > From: Saravana Kannan <saravanak@google.com> > > > > Look at the required OPPs of the "parent" device to determine the OPP that > > is required from the slave device managed by the passive governor. This > > allows having mappings between a parent device and a slave device even when > > they don't have the same number of OPPs. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> > > Acked-by: Chanwoo Choi <cw00.choi@samsung.com> > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > --- > > drivers/devfreq/governor_passive.c | 20 +++++++++++++++----- > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c > > index 63332e4a65ae8..8d92b1964f9c3 100644 > > --- a/drivers/devfreq/governor_passive.c > > +++ b/drivers/devfreq/governor_passive.c > > @@ -19,7 +19,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > > = (struct devfreq_passive_data *)devfreq->data; > > struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent; > > unsigned long child_freq = ULONG_MAX; > > - struct dev_pm_opp *opp; > > + struct dev_pm_opp *opp = NULL, *p_opp = NULL; > > I would initialize p_opp to ERR_PTR(-ENODEV) to avoid using > IS_ERR_OR_NULL. There is no need to initialize opp as well. > > > int i, count, ret = 0; > > > > /* > > @@ -56,13 +56,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > > * list of parent device. Because in this case, *freq is temporary > > * value which is decided by ondemand governor. > > */ > > - opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0); > > - if (IS_ERR(opp)) { > > - ret = PTR_ERR(opp); > > + p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0); > > + if (IS_ERR(p_opp)) { > > + ret = PTR_ERR(p_opp); > > goto out; > > Perhaps just return from here, the goto is useless here. > > > } > > > > - dev_pm_opp_put(opp); > > + if (devfreq->opp_table && parent_devfreq->opp_table) > > + opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table, > > + devfreq->opp_table, p_opp); > > + if (opp) { > > This needs to be part of the above if block itself, else the opp will > always be NULL, isn't it ? > > > + *freq = dev_pm_opp_get_freq(opp); > > + dev_pm_opp_put(opp); > > + goto out; > > + } > > > > /* > > * Get the OPP table's index of decided freqeuncy by governor > > @@ -89,6 +96,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq, > > *freq = child_freq; > > > > out: > > + if (!IS_ERR_OR_NULL(opp)) > > you should be checking for p_opp here, isn't it ? And perhaps we don't > need this check as well as p_opp can't be invalid here. > > > + dev_pm_opp_put(p_opp); > > + > > return ret; > > } > > > > -- > > 2.30.0.365.g02bc693789-goog > Thanks for the review. I'll fix them and send next version > -- > viresh
On Thu, Feb 4, 2021 at 1:41 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 03-02-21, 17:23, Hsin-Yi Wang wrote: > > The devfreq passive governor scales the frequency of a "child" device based > > on the current frequency of a "parent" device (not parent/child in the > > sense of device hierarchy). As of today, the passive governor requires one > > of the following to work correctly: > > 1. The parent and child device have the same number of frequencies > > 2. The child device driver passes a mapping function to translate from > > parent frequency to child frequency. > > > > When (1) is not true, (2) is the only option right now. But often times, > > all that is required is a simple mapping from parent's frequency to child's > > frequency. > > > > Since OPPs already support pointing to other "required-opps", add support > > for using that to map from parent device frequency to child device > > frequency. That way, every child device driver doesn't have to implement a > > separate mapping function anytime (1) isn't true. > > So you guys aren't interested in dev_pm_opp_set_opp() but just the > translation of the required-OPPs ? > I think this series focuses on required-opps. > I am fine with most of the stuff and I would like to take it via OPP > tree, hope that would be fine ? > Sounds good to me, thanks. > -- > viresh
Hi Viresh, On 2/4/21 2:41 PM, Viresh Kumar wrote: > On 03-02-21, 17:23, Hsin-Yi Wang wrote: >> The devfreq passive governor scales the frequency of a "child" device based >> on the current frequency of a "parent" device (not parent/child in the >> sense of device hierarchy). As of today, the passive governor requires one >> of the following to work correctly: >> 1. The parent and child device have the same number of frequencies >> 2. The child device driver passes a mapping function to translate from >> parent frequency to child frequency. >> >> When (1) is not true, (2) is the only option right now. But often times, >> all that is required is a simple mapping from parent's frequency to child's >> frequency. >> >> Since OPPs already support pointing to other "required-opps", add support >> for using that to map from parent device frequency to child device >> frequency. That way, every child device driver doesn't have to implement a >> separate mapping function anytime (1) isn't true. > > So you guys aren't interested in dev_pm_opp_set_opp() but just the > translation of the required-OPPs ? > > I am fine with most of the stuff and I would like to take it via OPP > tree, hope that would be fine ? I agree. Take these patches to OPP tree. -- Best Regards, Chanwoo Choi Samsung Electronics
The devfreq passive governor scales the frequency of a "child" device based on the current frequency of a "parent" device (not parent/child in the sense of device hierarchy). As of today, the passive governor requires one of the following to work correctly: 1. The parent and child device have the same number of frequencies 2. The child device driver passes a mapping function to translate from parent frequency to child frequency. When (1) is not true, (2) is the only option right now. But often times, all that is required is a simple mapping from parent's frequency to child's frequency. Since OPPs already support pointing to other "required-opps", add support for using that to map from parent device frequency to child device frequency. That way, every child device driver doesn't have to implement a separate mapping function anytime (1) isn't true. Some common (but not comprehensive) reason for needing a devfreq passive governor to adjust the frequency of one device based on another are: 1. These were the combination of frequencies that were validated/screened during the manufacturing process. 2. These are the sensible performance combinations between two devices interacting with each other. So that when one runs fast the other doesn't become the bottleneck. 3. Hardware bugs requiring some kind of frequency ratio between devices. For example, the following mapping can't be captured in DT as it stands today because the parent and child device have different number of OPPs. But with this patch series, this mapping can be captured cleanly. In arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi you have something like this with the following changes: bus_g2d_400: bus0 { compatible = "samsung,exynos-bus"; clocks = <&cmu_top CLK_ACLK_G2D_400>; clock-names = "bus"; operating-points-v2 = <&bus_g2d_400_opp_table>; status = "disabled"; }; bus_noc2: bus9 { compatible = "samsung,exynos-bus"; clocks = <&cmu_mif CLK_ACLK_BUS2_400>; clock-names = "bus"; operating-points-v2 = <&bus_noc2_opp_table>; status = "disabled"; }; bus_g2d_400_opp_table: opp_table2 { compatible = "operating-points-v2"; opp-shared; opp-400000000 { opp-hz = /bits/ 64 <400000000>; opp-microvolt = <1075000>; required-opps = <&noc2_400>; }; opp-267000000 { opp-hz = /bits/ 64 <267000000>; opp-microvolt = <1000000>; required-opps = <&noc2_200>; }; opp-200000000 { opp-hz = /bits/ 64 <200000000>; opp-microvolt = <975000>; required-opps = <&noc2_200>; }; opp-160000000 { opp-hz = /bits/ 64 <160000000>; opp-microvolt = <962500>; required-opps = <&noc2_134>; }; opp-134000000 { opp-hz = /bits/ 64 <134000000>; opp-microvolt = <950000>; required-opps = <&noc2_134>; }; opp-100000000 { opp-hz = /bits/ 64 <100000000>; opp-microvolt = <937500>; required-opps = <&noc2_100>; }; }; bus_noc2_opp_table: opp_table6 { compatible = "operating-points-v2"; noc2_400: opp-400000000 { opp-hz = /bits/ 64 <400000000>; }; noc2_200: opp-200000000 { opp-hz = /bits/ 64 <200000000>; }; noc2_134: opp-134000000 { opp-hz = /bits/ 64 <134000000>; }; noc2_100: opp-100000000 { opp-hz = /bits/ 64 <100000000>; }; }; -Saravana v4 -> v5: - drop patch "OPP: Improve required-opps linking" and rebase to https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=opp/linux-next - Compare pointers in dev_pm_opp_xlate_required_opp(). v3 -> v4: - Fixed documentation comments - Fixed order of functions in .h file - Renamed the new xlate API - Caused _set_required_opps() to fail if all required opps tables aren't linked. v2 -> v3: - Rebased onto linux-next. - Added documentation comment for new fields. - Added support for lazy required-opps linking. - Updated Ack/Reviewed-bys. v1 -> v2: - Cached OPP table reference in devfreq to avoid looking up every time. - Renamed variable in passive governor to be more intuitive. - Updated cover letter with examples. Saravana Kannan (3): OPP: Add function to look up required OPP's for a given OPP PM / devfreq: Cache OPP table reference in devfreq PM / devfreq: Add required OPPs support to passive governor drivers/devfreq/devfreq.c | 6 ++++ drivers/devfreq/governor_passive.c | 20 ++++++++--- drivers/opp/core.c | 58 ++++++++++++++++++++++++++++++ include/linux/devfreq.h | 2 ++ include/linux/pm_opp.h | 11 ++++++ 5 files changed, 92 insertions(+), 5 deletions(-)