diff mbox series

opp: of: Allow lazy-linking of required-opps to non genpd

Message ID 20210616053335.4181780-1-hsinyi@chromium.org (mailing list archive)
State New
Delegated to: viresh kumar
Headers show
Series opp: of: Allow lazy-linking of required-opps to non genpd | expand

Commit Message

Hsin-Yi Wang June 16, 2021, 5:33 a.m. UTC
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 <cw00.choi@samsung.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
This is tested with the non genpd case mt8183-cci with passive
governor[1].
[1] https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-2-git-send-email-andrew-sh.cheng@mediatek.com/
---
 drivers/opp/of.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

Comments

Viresh Kumar June 16, 2021, 5:58 a.m. UTC | #1
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 <cw00.choi@samsung.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> This is tested with the non genpd case mt8183-cci with passive
> governor[1].
> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-2-git-send-email-andrew-sh.cheng@mediatek.com/
> ---
>  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 ?
Hsin-Yi Wang June 16, 2021, 6:25 a.m. UTC | #2
On Wed, Jun 16, 2021 at 1:58 PM Viresh Kumar <viresh.kumar@linaro.org> 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 <cw00.choi@samsung.com>
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > This is tested with the non genpd case mt8183-cci with passive
> > governor[1].
> > [1] https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-2-git-send-email-andrew-sh.cheng@mediatek.com/
> > ---
> >  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
Viresh Kumar June 16, 2021, 7:09 a.m. UTC | #3
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 Kumar June 16, 2021, 7:55 a.m. UTC | #4
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 <cw00.choi@samsung.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> This is tested with the non genpd case mt8183-cci with passive
> governor[1].
> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-2-git-send-email-andrew-sh.cheng@mediatek.com/
> ---
>  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.
Chanwoo Choi June 16, 2021, 8:47 a.m. UTC | #5
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 <cw00.choi@samsung.com>
>> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
>> ---
>> This is tested with the non genpd case mt8183-cci with passive
>> governor[1].
>> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-2-git-send-email-andrew-sh.cheng@mediatek.com/
>> ---
>>  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.
>
Viresh Kumar June 16, 2021, 9:09 a.m. UTC | #6
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 <cw00.choi@samsung.com>
> >> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> >> ---
> >> This is tested with the non genpd case mt8183-cci with passive
> >> governor[1].
> >> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-2-git-send-email-andrew-sh.cheng@mediatek.com/
> >> ---
> >>  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.
Hsin-Yi Wang June 16, 2021, 9:20 a.m. UTC | #7
On Wed, Jun 16, 2021 at 5:09 PM Viresh Kumar <viresh.kumar@linaro.org> 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 <cw00.choi@samsung.com>
> > >> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > >> ---
> > >> This is tested with the non genpd case mt8183-cci with passive
> > >> governor[1].
> > >> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-2-git-send-email-andrew-sh.cheng@mediatek.com/
> > >> ---
> > >>  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
Chanwoo Choi June 17, 2021, 1:13 a.m. UTC | #8
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 <cw00.choi@samsung.com>
>>>> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
>>>> ---
>>>> This is tested with the non genpd case mt8183-cci with passive
>>>> governor[1].
>>>> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-2-git-send-email-andrew-sh.cheng@mediatek.com/
>>>> ---
>>>>  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.
Viresh Kumar June 17, 2021, 3:33 a.m. UTC | #9
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 Kumar June 17, 2021, 4 a.m. UTC | #10
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.
Chanwoo Choi June 17, 2021, 4:09 a.m. UTC | #11
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?
Chanwoo Choi June 17, 2021, 4:27 a.m. UTC | #12
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.
diff mbox series

Patch

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;
 	}