[1/3] PM / devfreq: Actually support providing freq_table

The code in devfreq_add_device() handles the case where a freq_table is
passed by the client, but then requests min and max frequences from
the, in this case absent, opp tables.

Read the min and max frequencies from the frequency table, which has
been built from the opp table if one exists, instead of querying the
opp table.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---

An alternative approach is to clarify in the devfreq code that it's not
possible to pass a freq_table and then in patch 3 create an opp table for the
device in runtime; although the error handling of this becomes non-trivial.

Transitioning the UFSHCD to use opp tables directly is hindered by the fact
that the Qualcomm UFS hardware has two different clocks that needs to be
running at different rates, so we would need a way to describe the two rates in
the opp table. (And would force us to change the DT binding)

 drivers/devfreq/devfreq.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

-- 
2.16.2

Hi,

On 2018년 04월 24일 09:20, Bjorn Andersson wrote:
> The code in devfreq_add_device() handles the case where a freq_table is

> passed by the client, but then requests min and max frequences from

> the, in this case absent, opp tables.

> 

> Read the min and max frequencies from the frequency table, which has

> been built from the opp table if one exists, instead of querying the

> opp table.

> 

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---

> 

> An alternative approach is to clarify in the devfreq code that it's not

> possible to pass a freq_table and then in patch 3 create an opp table for the

> device in runtime; although the error handling of this becomes non-trivial.

> 

> Transitioning the UFSHCD to use opp tables directly is hindered by the fact

> that the Qualcomm UFS hardware has two different clocks that needs to be

> running at different rates, so we would need a way to describe the two rates in

> the opp table. (And would force us to change the DT binding)

> 

>  drivers/devfreq/devfreq.c | 22 ++++------------------

>  1 file changed, 4 insertions(+), 18 deletions(-)

> 

> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c

> index fe2af6aa88fc..086ced50a13d 100644

> --- a/drivers/devfreq/devfreq.c

> +++ b/drivers/devfreq/devfreq.c

> @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct device *dev)

>  

>  static unsigned long find_available_min_freq(struct devfreq *devfreq)

>  {

> -	struct dev_pm_opp *opp;

> -	unsigned long min_freq = 0;

> -

> -	opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq);

> -	if (IS_ERR(opp))

> -		min_freq = 0;

> -	else

> -		dev_pm_opp_put(opp);

> +	struct devfreq_dev_profile *profile = devfreq->profile;

>  

> -	return min_freq;

> +	return profile->freq_table[0];


It is wrong. The thermal framework support the devfreq-cooling device
which uses the dev_pm_opp_enable/disable().

In order to find the correct available min frequency,
the devfreq have to use the OPP function instead of using the first entry
of the freq_table array.

>  }

>  

>  static unsigned long find_available_max_freq(struct devfreq *devfreq)

>  {

> -	struct dev_pm_opp *opp;

> -	unsigned long max_freq = ULONG_MAX;

> -

> -	opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq);

> -	if (IS_ERR(opp))

> -		max_freq = 0;

> -	else

> -		dev_pm_opp_put(opp);

> +	struct devfreq_dev_profile *profile = devfreq->profile;

>  

> -	return max_freq;

> +	return profile->freq_table[profile->max_state - 1];

>  }


ditto.

>  

>  /**

> 



-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Hi,

On 2018년 04월 24일 14:29, Bjorn Andersson wrote:
> On Mon 23 Apr 19:48 PDT 2018, Chanwoo Choi wrote:

> 

>> Hi,

>>

>> On 2018??? 04??? 24??? 09:20, Bjorn Andersson wrote:

>>> The code in devfreq_add_device() handles the case where a freq_table is

>>> passed by the client, but then requests min and max frequences from

>>> the, in this case absent, opp tables.

>>>

>>> Read the min and max frequencies from the frequency table, which has

>>> been built from the opp table if one exists, instead of querying the

>>> opp table.

>>>

>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

>>> ---

>>>

>>> An alternative approach is to clarify in the devfreq code that it's not

>>> possible to pass a freq_table and then in patch 3 create an opp table for the

>>> device in runtime; although the error handling of this becomes non-trivial.

>>>

>>> Transitioning the UFSHCD to use opp tables directly is hindered by the fact

>>> that the Qualcomm UFS hardware has two different clocks that needs to be

>>> running at different rates, so we would need a way to describe the two rates in

>>> the opp table. (And would force us to change the DT binding)

>>>

>>>  drivers/devfreq/devfreq.c | 22 ++++------------------

>>>  1 file changed, 4 insertions(+), 18 deletions(-)

>>>

>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c

>>> index fe2af6aa88fc..086ced50a13d 100644

>>> --- a/drivers/devfreq/devfreq.c

>>> +++ b/drivers/devfreq/devfreq.c

>>> @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct device *dev)

>>>  

>>>  static unsigned long find_available_min_freq(struct devfreq *devfreq)

>>>  {

>>> -	struct dev_pm_opp *opp;

>>> -	unsigned long min_freq = 0;

>>> -

>>> -	opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq);

>>> -	if (IS_ERR(opp))

>>> -		min_freq = 0;

>>> -	else

>>> -		dev_pm_opp_put(opp);

>>> +	struct devfreq_dev_profile *profile = devfreq->profile;

>>>  

>>> -	return min_freq;

>>> +	return profile->freq_table[0];

>>

>> It is wrong. The thermal framework support the devfreq-cooling device

>> which uses the dev_pm_opp_enable/disable().

>>

> 

> Okay, that makes sense. So rather than registering a custom freq_table I

> should register the min and max frequency using dev_pm_opp_add().


Thanks.

> 

>> In order to find the correct available min frequency,

>> the devfreq have to use the OPP function instead of using the first entry

>> of the freq_table array.

>>

> 

> Based on this there seems to be room for cleaning out the freq_table

> from devfreq, to reduce the confusion. I will review this further.


Actually, devfreq must need to have the freq_table[] array. But, freq_table[]
array should be handled in the devfreq core. Now, the devfreq device drivers can
touch the freq_table. I think it is not good.

There is a reason why we have to maintain the freq_table[] as the internal variable.
OPP doesn't provide the OPP API which get the all registered frequencies.
If devfreq-cooling device disables the specific frequency by using dev_pm_oppdisable(),
the user of OPP interface can not get the disabled frequency list.
So, I maintain the freq_table even if using the OPP interface.

And, devfreq-cooling device uses the freq_table directly because released MALi driver
from ARM initializes the freq_table list directly.

I have no any objection for refactoring. Just I'm sharing the issue and current status.

> 

> Thanks,

> Bjorn

> 

> 

> 



-- 
Best Regards,
Chanwoo Choi
Samsung Electronics