Don't limit required_opp_table to genpd only. One possible use case is
cpufreq based devfreq governor, which can use required-opps property to
derive devfreq from cpufreq.
Suggested-by: Chanwoo Choi <[email protected]>
Signed-off-by: Hsin-Yi Wang <[email protected]>
---
This is tested with the non genpd case mt8183-cci with passive
governor[1].
[1] https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
---
drivers/opp/of.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index aa75a1caf08a3..9573facce53a5 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
lazy = true;
continue;
}
-
- /*
- * We only support genpd's OPPs in the "required-opps" for now,
- * as we don't know how much about other cases. Error out if the
- * required OPP doesn't belong to a genpd.
- */
- if (!required_opp_tables[i]->is_genpd) {
- dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
- required_np);
- goto free_required_tables;
- }
}
/* Let's do the linking later on */
@@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
struct dev_pm_opp *opp;
int i, ret;
- /*
- * We only support genpd's OPPs in the "required-opps" for now,
- * as we don't know much about other cases.
- */
- if (!new_table->is_genpd)
- return;
-
mutex_lock(&opp_table_lock);
list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
@@ -873,7 +855,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
return ERR_PTR(-ENOMEM);
ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
- if (ret < 0 && !opp_table->is_genpd) {
+ if (ret < 0) {
dev_err(dev, "%s: opp key field not found\n", __func__);
goto free_opp;
}
--
2.32.0.272.g935e593368-goog
On 16-06-21, 13:33, Hsin-Yi Wang wrote:
> Don't limit required_opp_table to genpd only. One possible use case is
> cpufreq based devfreq governor, which can use required-opps property to
> derive devfreq from cpufreq.
>
> Suggested-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Hsin-Yi Wang <[email protected]>
> ---
> This is tested with the non genpd case mt8183-cci with passive
> governor[1].
> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
> ---
> drivers/opp/of.c | 20 +-------------------
> 1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index aa75a1caf08a3..9573facce53a5 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> lazy = true;
> continue;
> }
> -
> - /*
> - * We only support genpd's OPPs in the "required-opps" for now,
> - * as we don't know how much about other cases. Error out if the
> - * required OPP doesn't belong to a genpd.
> - */
> - if (!required_opp_tables[i]->is_genpd) {
> - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> - required_np);
> - goto free_required_tables;
> - }
> }
>
> /* Let's do the linking later on */
> @@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
> struct dev_pm_opp *opp;
> int i, ret;
>
> - /*
> - * We only support genpd's OPPs in the "required-opps" for now,
> - * as we don't know much about other cases.
> - */
> - if (!new_table->is_genpd)
> - return;
> -
> mutex_lock(&opp_table_lock);
>
> list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
> @@ -873,7 +855,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> return ERR_PTR(-ENOMEM);
>
> ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
> - if (ret < 0 && !opp_table->is_genpd) {
> + if (ret < 0) {
> dev_err(dev, "%s: opp key field not found\n", __func__);
> goto free_opp;
> }
How are you setting the required OPPs ? I mean when someone tries to
change frequency of a device which has some required-opps, who is
configuring those required OPPs ?
--
viresh
On Wed, Jun 16, 2021 at 1:58 PM Viresh Kumar <[email protected]> wrote:
>
> On 16-06-21, 13:33, Hsin-Yi Wang wrote:
> > Don't limit required_opp_table to genpd only. One possible use case is
> > cpufreq based devfreq governor, which can use required-opps property to
> > derive devfreq from cpufreq.
> >
> > Suggested-by: Chanwoo Choi <[email protected]>
> > Signed-off-by: Hsin-Yi Wang <[email protected]>
> > ---
> > This is tested with the non genpd case mt8183-cci with passive
> > governor[1].
> > [1] https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
> > ---
> > drivers/opp/of.c | 20 +-------------------
> > 1 file changed, 1 insertion(+), 19 deletions(-)
> >
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index aa75a1caf08a3..9573facce53a5 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> > lazy = true;
> > continue;
> > }
> > -
> > - /*
> > - * We only support genpd's OPPs in the "required-opps" for now,
> > - * as we don't know how much about other cases. Error out if the
> > - * required OPP doesn't belong to a genpd.
> > - */
> > - if (!required_opp_tables[i]->is_genpd) {
> > - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> > - required_np);
> > - goto free_required_tables;
> > - }
> > }
> >
> > /* Let's do the linking later on */
> > @@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
> > struct dev_pm_opp *opp;
> > int i, ret;
> >
> > - /*
> > - * We only support genpd's OPPs in the "required-opps" for now,
> > - * as we don't know much about other cases.
> > - */
> > - if (!new_table->is_genpd)
> > - return;
> > -
> > mutex_lock(&opp_table_lock);
> >
> > list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
> > @@ -873,7 +855,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> > return ERR_PTR(-ENOMEM);
> >
> > ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
> > - if (ret < 0 && !opp_table->is_genpd) {
> > + if (ret < 0) {
> > dev_err(dev, "%s: opp key field not found\n", __func__);
> > goto free_opp;
> > }
>
> How are you setting the required OPPs ? I mean when someone tries to
> change frequency of a device which has some required-opps, who is
> configuring those required OPPs ?
>
When cpufreq changes, the (cpufreq based) passive governor will
calculate a target devfreq based on that, and the device governor
(mt8183-cci-devfreq) will change the actual opp of the device.
The required-opp is set in the cpufreq table:
cpufreq_opp_table: opp_table0 {
compatible = "operating-points-v2";
opp-shared;
...
opp0_01 {
opp-hz = /bits/ 64 <910000000>;
opp-microvolt = <687500>;
required-opps = <&opp2_01>;
};
...
};
devfreq_opp_table: opp_table2 {
compatible = "operating-points-v2";
opp-shared;
...
opp2_01: opp-338000000 {
opp-hz = /bits/ 64 <338000000>;
opp-microvolt = <687500>;
};
...
};
> --
> viresh
On 16-06-21, 14:25, Hsin-Yi Wang wrote:
> When cpufreq changes, the (cpufreq based) passive governor will
> calculate a target devfreq based on that, and the device governor
> (mt8183-cci-devfreq) will change the actual opp of the device.
>
> The required-opp is set in the cpufreq table:
>
> cpufreq_opp_table: opp_table0 {
> compatible = "operating-points-v2";
> opp-shared;
> ...
> opp0_01 {
> opp-hz = /bits/ 64 <910000000>;
> opp-microvolt = <687500>;
> required-opps = <&opp2_01>;
> };
> ...
> };
>
> devfreq_opp_table: opp_table2 {
> compatible = "operating-points-v2";
> opp-shared;
> ...
> opp2_01: opp-338000000 {
> opp-hz = /bits/ 64 <338000000>;
> opp-microvolt = <687500>;
> };
> ...
> };
Ah, you aren't using dev_pm_opp_set_opp() or dev_pm_opp_set_rate()
interfaces.
Looks okay then.
--
viresh
On 16-06-21, 13:33, Hsin-Yi Wang wrote:
> Don't limit required_opp_table to genpd only. One possible use case is
> cpufreq based devfreq governor, which can use required-opps property to
> derive devfreq from cpufreq.
>
> Suggested-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Hsin-Yi Wang <[email protected]>
> ---
> This is tested with the non genpd case mt8183-cci with passive
> governor[1].
> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
> ---
> drivers/opp/of.c | 20 +-------------------
> 1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index aa75a1caf08a3..9573facce53a5 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> lazy = true;
> continue;
> }
> -
> - /*
> - * We only support genpd's OPPs in the "required-opps" for now,
> - * as we don't know how much about other cases. Error out if the
> - * required OPP doesn't belong to a genpd.
> - */
> - if (!required_opp_tables[i]->is_genpd) {
> - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> - required_np);
> - goto free_required_tables;
> - }
> }
>
> /* Let's do the linking later on */
> @@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
> struct dev_pm_opp *opp;
> int i, ret;
>
> - /*
> - * We only support genpd's OPPs in the "required-opps" for now,
> - * as we don't know much about other cases.
> - */
> - if (!new_table->is_genpd)
> - return;
> -
> mutex_lock(&opp_table_lock);
>
> list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
> @@ -873,7 +855,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> return ERR_PTR(-ENOMEM);
>
> ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
> - if (ret < 0 && !opp_table->is_genpd) {
> + if (ret < 0) {
> dev_err(dev, "%s: opp key field not found\n", __func__);
> goto free_opp;
> }
Plus this and few changes to commit log.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e366218d6736..b335c077f215 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -893,6 +893,16 @@ static int _set_required_opps(struct device *dev,
if (!required_opp_tables)
return 0;
+ /*
+ * We only support genpd's OPPs in the "required-opps" for now, as we
+ * don't know much about other use cases. Error out if the required OPP
+ * doesn't belong to a genpd.
+ */
+ if (unlikely(!required_opp_tables[0]->is_genpd)) {
+ dev_err(dev, "required-opps don't belong to a genpd\n");
+ return -ENOENT;
+ }
+
/* required-opps not fully initialized yet */
if (lazy_linking_pending(opp_table))
return -EBUSY;
Applied. Thanks.
--
viresh
On 6/16/21 4:55 PM, Viresh Kumar wrote:
> On 16-06-21, 13:33, Hsin-Yi Wang wrote:
>> Don't limit required_opp_table to genpd only. One possible use case is
>> cpufreq based devfreq governor, which can use required-opps property to
>> derive devfreq from cpufreq.
>>
>> Suggested-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Hsin-Yi Wang <[email protected]>
>> ---
>> This is tested with the non genpd case mt8183-cci with passive
>> governor[1].
>> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
>> ---
>> drivers/opp/of.c | 20 +-------------------
>> 1 file changed, 1 insertion(+), 19 deletions(-)
>>
>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>> index aa75a1caf08a3..9573facce53a5 100644
>> --- a/drivers/opp/of.c
>> +++ b/drivers/opp/of.c
>> @@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>> lazy = true;
>> continue;
>> }
>> -
>> - /*
>> - * We only support genpd's OPPs in the "required-opps" for now,
>> - * as we don't know how much about other cases. Error out if the
>> - * required OPP doesn't belong to a genpd.
>> - */
>> - if (!required_opp_tables[i]->is_genpd) {
>> - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
>> - required_np);
>> - goto free_required_tables;
>> - }
>> }
>>
>> /* Let's do the linking later on */
>> @@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
>> struct dev_pm_opp *opp;
>> int i, ret;
>>
>> - /*
>> - * We only support genpd's OPPs in the "required-opps" for now,
>> - * as we don't know much about other cases.
>> - */
>> - if (!new_table->is_genpd)
>> - return;
>> -
>> mutex_lock(&opp_table_lock);
>>
>> list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
>> @@ -873,7 +855,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>> return ERR_PTR(-ENOMEM);
>>
>> ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
>> - if (ret < 0 && !opp_table->is_genpd) {
>> + if (ret < 0) {
>> dev_err(dev, "%s: opp key field not found\n", __func__);
>> goto free_opp;
>> }
>
> Plus this and few changes to commit log.
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e366218d6736..b335c077f215 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -893,6 +893,16 @@ static int _set_required_opps(struct device *dev,
> if (!required_opp_tables)
> return 0;
>
> + /*
> + * We only support genpd's OPPs in the "required-opps" for now, as we
> + * don't know much about other use cases. Error out if the required OPP
> + * doesn't belong to a genpd.
> + */
> + if (unlikely(!required_opp_tables[0]->is_genpd)) {
> + dev_err(dev, "required-opps don't belong to a genpd\n");
> + return -ENOENT;
> + }
> +
If you add this checking statement, I think that
when using dev_pm_opp_set_rate with required-opp property, it will be failed.
> /* required-opps not fully initialized yet */
> if (lazy_linking_pending(opp_table))
> return -EBUSY;
>
> Applied. Thanks.
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 16-06-21, 17:47, Chanwoo Choi wrote:
> On 6/16/21 4:55 PM, Viresh Kumar wrote:
> > On 16-06-21, 13:33, Hsin-Yi Wang wrote:
> >> Don't limit required_opp_table to genpd only. One possible use case is
> >> cpufreq based devfreq governor, which can use required-opps property to
> >> derive devfreq from cpufreq.
> >>
> >> Suggested-by: Chanwoo Choi <[email protected]>
> >> Signed-off-by: Hsin-Yi Wang <[email protected]>
> >> ---
> >> This is tested with the non genpd case mt8183-cci with passive
> >> governor[1].
> >> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
> >> ---
> >> drivers/opp/of.c | 20 +-------------------
> >> 1 file changed, 1 insertion(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> >> index aa75a1caf08a3..9573facce53a5 100644
> >> --- a/drivers/opp/of.c
> >> +++ b/drivers/opp/of.c
> >> @@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> >> lazy = true;
> >> continue;
> >> }
> >> -
> >> - /*
> >> - * We only support genpd's OPPs in the "required-opps" for now,
> >> - * as we don't know how much about other cases. Error out if the
> >> - * required OPP doesn't belong to a genpd.
> >> - */
> >> - if (!required_opp_tables[i]->is_genpd) {
> >> - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> >> - required_np);
> >> - goto free_required_tables;
> >> - }
> >> }
> >>
> >> /* Let's do the linking later on */
> >> @@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
> >> struct dev_pm_opp *opp;
> >> int i, ret;
> >>
> >> - /*
> >> - * We only support genpd's OPPs in the "required-opps" for now,
> >> - * as we don't know much about other cases.
> >> - */
> >> - if (!new_table->is_genpd)
> >> - return;
> >> -
> >> mutex_lock(&opp_table_lock);
> >>
> >> list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
> >> @@ -873,7 +855,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> >> return ERR_PTR(-ENOMEM);
> >>
> >> ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
> >> - if (ret < 0 && !opp_table->is_genpd) {
> >> + if (ret < 0) {
> >> dev_err(dev, "%s: opp key field not found\n", __func__);
> >> goto free_opp;
> >> }
> >
> > Plus this and few changes to commit log.
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index e366218d6736..b335c077f215 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -893,6 +893,16 @@ static int _set_required_opps(struct device *dev,
> > if (!required_opp_tables)
> > return 0;
> >
> > + /*
> > + * We only support genpd's OPPs in the "required-opps" for now, as we
> > + * don't know much about other use cases. Error out if the required OPP
> > + * doesn't belong to a genpd.
> > + */
> > + if (unlikely(!required_opp_tables[0]->is_genpd)) {
> > + dev_err(dev, "required-opps don't belong to a genpd\n");
> > + return -ENOENT;
> > + }
> > +
>
> If you add this checking statement, I think that
> when using dev_pm_opp_set_rate with required-opp property, it will be failed.
Yes, that is exactly what I am trying to do here. Hsin already
confirmed that you guys won't use this API, isn't ?
The point here is that the _set_required_opps() function only updates
the performance state of genpds today. So it won't work for you guys
anyway.
--
viresh
On Wed, Jun 16, 2021 at 5:09 PM Viresh Kumar <[email protected]> wrote:
>
> On 16-06-21, 17:47, Chanwoo Choi wrote:
> > On 6/16/21 4:55 PM, Viresh Kumar wrote:
> > > On 16-06-21, 13:33, Hsin-Yi Wang wrote:
> > >> Don't limit required_opp_table to genpd only. One possible use case is
> > >> cpufreq based devfreq governor, which can use required-opps property to
> > >> derive devfreq from cpufreq.
> > >>
> > >> Suggested-by: Chanwoo Choi <[email protected]>
> > >> Signed-off-by: Hsin-Yi Wang <[email protected]>
> > >> ---
> > >> This is tested with the non genpd case mt8183-cci with passive
> > >> governor[1].
> > >> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
> > >> ---
> > >> drivers/opp/of.c | 20 +-------------------
> > >> 1 file changed, 1 insertion(+), 19 deletions(-)
> > >>
> > >> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > >> index aa75a1caf08a3..9573facce53a5 100644
> > >> --- a/drivers/opp/of.c
> > >> +++ b/drivers/opp/of.c
> > >> @@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> > >> lazy = true;
> > >> continue;
> > >> }
> > >> -
> > >> - /*
> > >> - * We only support genpd's OPPs in the "required-opps" for now,
> > >> - * as we don't know how much about other cases. Error out if the
> > >> - * required OPP doesn't belong to a genpd.
> > >> - */
> > >> - if (!required_opp_tables[i]->is_genpd) {
> > >> - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> > >> - required_np);
> > >> - goto free_required_tables;
> > >> - }
> > >> }
> > >>
> > >> /* Let's do the linking later on */
> > >> @@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
> > >> struct dev_pm_opp *opp;
> > >> int i, ret;
> > >>
> > >> - /*
> > >> - * We only support genpd's OPPs in the "required-opps" for now,
> > >> - * as we don't know much about other cases.
> > >> - */
> > >> - if (!new_table->is_genpd)
> > >> - return;
> > >> -
> > >> mutex_lock(&opp_table_lock);
> > >>
> > >> list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
> > >> @@ -873,7 +855,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> > >> return ERR_PTR(-ENOMEM);
> > >>
> > >> ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
> > >> - if (ret < 0 && !opp_table->is_genpd) {
> > >> + if (ret < 0) {
> > >> dev_err(dev, "%s: opp key field not found\n", __func__);
> > >> goto free_opp;
> > >> }
> > >
> > > Plus this and few changes to commit log.
> > >
> > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > index e366218d6736..b335c077f215 100644
> > > --- a/drivers/opp/core.c
> > > +++ b/drivers/opp/core.c
> > > @@ -893,6 +893,16 @@ static int _set_required_opps(struct device *dev,
> > > if (!required_opp_tables)
> > > return 0;
> > >
> > > + /*
> > > + * We only support genpd's OPPs in the "required-opps" for now, as we
> > > + * don't know much about other use cases. Error out if the required OPP
> > > + * doesn't belong to a genpd.
> > > + */
> > > + if (unlikely(!required_opp_tables[0]->is_genpd)) {
> > > + dev_err(dev, "required-opps don't belong to a genpd\n");
> > > + return -ENOENT;
> > > + }
> > > +
> >
> > If you add this checking statement, I think that
> > when using dev_pm_opp_set_rate with required-opp property, it will be failed.
>
> Yes, that is exactly what I am trying to do here. Hsin already
> confirmed that you guys won't use this API, isn't ?
we used clk_set_rate() and regulator_set_voltage().
>
> The point here is that the _set_required_opps() function only updates
> the performance state of genpds today. So it won't work for you guys
> anyway.
>
> --
> viresh
On 6/16/21 6:09 PM, Viresh Kumar wrote:
> On 16-06-21, 17:47, Chanwoo Choi wrote:
>> On 6/16/21 4:55 PM, Viresh Kumar wrote:
>>> On 16-06-21, 13:33, Hsin-Yi Wang wrote:
>>>> Don't limit required_opp_table to genpd only. One possible use case is
>>>> cpufreq based devfreq governor, which can use required-opps property to
>>>> derive devfreq from cpufreq.
>>>>
>>>> Suggested-by: Chanwoo Choi <[email protected]>
>>>> Signed-off-by: Hsin-Yi Wang <[email protected]>
>>>> ---
>>>> This is tested with the non genpd case mt8183-cci with passive
>>>> governor[1].
>>>> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
>>>> ---
>>>> drivers/opp/of.c | 20 +-------------------
>>>> 1 file changed, 1 insertion(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>>>> index aa75a1caf08a3..9573facce53a5 100644
>>>> --- a/drivers/opp/of.c
>>>> +++ b/drivers/opp/of.c
>>>> @@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>>>> lazy = true;
>>>> continue;
>>>> }
>>>> -
>>>> - /*
>>>> - * We only support genpd's OPPs in the "required-opps" for now,
>>>> - * as we don't know how much about other cases. Error out if the
>>>> - * required OPP doesn't belong to a genpd.
>>>> - */
>>>> - if (!required_opp_tables[i]->is_genpd) {
>>>> - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
>>>> - required_np);
>>>> - goto free_required_tables;
>>>> - }
>>>> }
>>>>
>>>> /* Let's do the linking later on */
>>>> @@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
>>>> struct dev_pm_opp *opp;
>>>> int i, ret;
>>>>
>>>> - /*
>>>> - * We only support genpd's OPPs in the "required-opps" for now,
>>>> - * as we don't know much about other cases.
>>>> - */
>>>> - if (!new_table->is_genpd)
>>>> - return;
>>>> -
>>>> mutex_lock(&opp_table_lock);
>>>>
>>>> list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {
>>>> @@ -873,7 +855,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>>>> return ERR_PTR(-ENOMEM);
>>>>
>>>> ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
>>>> - if (ret < 0 && !opp_table->is_genpd) {
>>>> + if (ret < 0) {
>>>> dev_err(dev, "%s: opp key field not found\n", __func__);
>>>> goto free_opp;
>>>> }
>>>
>>> Plus this and few changes to commit log.
>>>
>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>>> index e366218d6736..b335c077f215 100644
>>> --- a/drivers/opp/core.c
>>> +++ b/drivers/opp/core.c
>>> @@ -893,6 +893,16 @@ static int _set_required_opps(struct device *dev,
>>> if (!required_opp_tables)
>>> return 0;
>>>
>>> + /*
>>> + * We only support genpd's OPPs in the "required-opps" for now, as we
>>> + * don't know much about other use cases. Error out if the required OPP
>>> + * doesn't belong to a genpd.
>>> + */
>>> + if (unlikely(!required_opp_tables[0]->is_genpd)) {
>>> + dev_err(dev, "required-opps don't belong to a genpd\n");
>>> + return -ENOENT;
>>> + }
>>> +
>>
>> If you add this checking statement, I think that
>> when using dev_pm_opp_set_rate with required-opp property, it will be failed.
>
> Yes, that is exactly what I am trying to do here. Hsin already
> confirmed that you guys won't use this API, isn't ?
>
> The point here is that the _set_required_opps() function only updates
> the performance state of genpds today. So it won't work for you guys
> anyway.
The devfreq driver(exynos-bus.c) has used the dev_pm_opp_set_rate()
and used the passive governor without the required-opp property.
I have a plan to use the required-opp property
between devfreq drivers (exynos-bus.c) with dev_pm_opp_set_rate().
I'll support them on later if this approach doesn't break the any
rule of required-opp property.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 17-06-21, 10:13, Chanwoo Choi wrote:
> The devfreq driver(exynos-bus.c) has used the dev_pm_opp_set_rate()
> and used the passive governor without the required-opp property.
Which is fine.
> I have a plan to use the required-opp property
> between devfreq drivers (exynos-bus.c) with dev_pm_opp_set_rate().
>
> I'll support them on later if this approach doesn't break the any
> rule of required-opp property.
You will be required to make some changes in core for that I am
afraid. It won't work automatically.
--
viresh
On 6/17/21 12:33 PM, Viresh Kumar wrote:
> On 17-06-21, 10:13, Chanwoo Choi wrote:
>> The devfreq driver(exynos-bus.c) has used the dev_pm_opp_set_rate()
>> and used the passive governor without the required-opp property.
>
> Which is fine.
>
>> I have a plan to use the required-opp property
>> between devfreq drivers (exynos-bus.c) with dev_pm_opp_set_rate().
>>
>> I'll support them on later if this approach doesn't break the any
>> rule of required-opp property.
>
> You will be required to make some changes in core for that I am
> afraid. It won't work automatically.
Do you think that better to use clk_enable/regulator_enable directly
instead of dev_pm_opp_set_rate() for using required-opp property?
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 17-06-21, 13:09, Chanwoo Choi wrote:
> On 6/17/21 12:33 PM, Viresh Kumar wrote:
> > On 17-06-21, 10:13, Chanwoo Choi wrote:
> >> The devfreq driver(exynos-bus.c) has used the dev_pm_opp_set_rate()
> >> and used the passive governor without the required-opp property.
> >
> > Which is fine.
> >
> >> I have a plan to use the required-opp property
> >> between devfreq drivers (exynos-bus.c) with dev_pm_opp_set_rate().
> >>
> >> I'll support them on later if this approach doesn't break the any
> >> rule of required-opp property.
> >
> > You will be required to make some changes in core for that I am
> > afraid. It won't work automatically.
>
> Do you think that better to use clk_enable/regulator_enable directly
> instead of dev_pm_opp_set_rate() for using required-opp property?
No. All I am saying is that the OPP core won't work for your use case
today and may need some updates.
--
viresh
On 6/17/21 1:00 PM, Viresh Kumar wrote:
> On 17-06-21, 13:09, Chanwoo Choi wrote:
>> On 6/17/21 12:33 PM, Viresh Kumar wrote:
>>> On 17-06-21, 10:13, Chanwoo Choi wrote:
>>>> The devfreq driver(exynos-bus.c) has used the dev_pm_opp_set_rate()
>>>> and used the passive governor without the required-opp property.
>>>
>>> Which is fine.
>>>
>>>> I have a plan to use the required-opp property
>>>> between devfreq drivers (exynos-bus.c) with dev_pm_opp_set_rate().
>>>>
>>>> I'll support them on later if this approach doesn't break the any
>>>> rule of required-opp property.
>>>
>>> You will be required to make some changes in core for that I am
>>> afraid. It won't work automatically.
>>
>> Do you think that better to use clk_enable/regulator_enable directly
>> instead of dev_pm_opp_set_rate() for using required-opp property?
>
> No. All I am saying is that the OPP core won't work for your use case
> today and may need some updates.
OK. I'll update them for this case.
--
Best Regards,
Chanwoo Choi
Samsung Electronics