[PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[Thara Gopinath]

cpuinfo.max_freq reflects the maximum supported frequency of cpus in a
cpufreq policy. When cpus support boost frequency and if boost is disabled
during boot up (which is the default), cpuinfo.max_freq does not reflect
boost frequency as the maximum supported frequency till boost is explicitly
enabled via sysfs interface later. This also means that policy reports two
different cpuinfo.max_freq before and after turning on boost.  Fix this by
separating out setting of policy->max and cpuinfo.max_freq in
cpufreq_frequency_table_cpuinfo.

e.g. of the problem. Qualcomm sdm845 supports boost frequency for gold
cluster (cpus 4-7). After boot up (boost disabled),

1.  cat /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2649600
<- This is wrong because boost frequency is

2.  echo 1 > /sys/devices/system/cpu/cpufreq/boost  <- Enable boost cat
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200	<-
max freq reflects boost freq.

3.  echo 0 > /sys/devices/system/cpu/cpufreq/boost <- Disable boost cat
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200	<-
Discrepancy with step 1 as in both cases boost is disabled.

Note that the other way to fix this is to set cpuinfo.max_freq in Soc
cpufreq driver during initialization. Fixing it in
cpufreq_frequency_table_cpuinfo seems more generic solution

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 drivers/cpufreq/freq_table.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index 67e56cf638ef..6784f94124df 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
 	struct cpufreq_frequency_table *pos;
 	unsigned int min_freq = ~0;
 	unsigned int max_freq = 0;
+	unsigned int cpuinfo_max_freq = 0;
 	unsigned int freq;
 
 	cpufreq_for_each_valid_entry(pos, table) {
 		freq = pos->frequency;
 
+		if (freq > cpuinfo_max_freq)
+			cpuinfo_max_freq = freq;
+
 		if (!cpufreq_boost_enabled()
 		    && (pos->flags & CPUFREQ_BOOST_FREQ))
 			continue;
@@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
 	 * If the driver has set its own cpuinfo.max_freq above max_freq, leave
 	 * it as is.
 	 */
-	if (policy->cpuinfo.max_freq < max_freq)
-		policy->max = policy->cpuinfo.max_freq = max_freq;
+	if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
+		policy->cpuinfo.max_freq = cpuinfo_max_freq;
 
 	if (policy->min == ~0)
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[Steev Klimaszewski]

Hi Thara,

On 11/15/21 1:50 PM, Thara Gopinath wrote:
> cpuinfo.max_freq reflects the maximum supported frequency of cpus in a
> cpufreq policy. When cpus support boost frequency and if boost is disabled
> during boot up (which is the default), cpuinfo.max_freq does not reflect
> boost frequency as the maximum supported frequency till boost is explicitly
> enabled via sysfs interface later. This also means that policy reports two
> different cpuinfo.max_freq before and after turning on boost.  Fix this by
> separating out setting of policy->max and cpuinfo.max_freq in
> cpufreq_frequency_table_cpuinfo.
>
> e.g. of the problem. Qualcomm sdm845 supports boost frequency for gold
> cluster (cpus 4-7). After boot up (boost disabled),
>
> 1.  cat /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2649600
> <- This is wrong because boost frequency is
>
> 2.  echo 1 > /sys/devices/system/cpu/cpufreq/boost  <- Enable boost cat
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200	<-
> max freq reflects boost freq.
>
> 3.  echo 0 > /sys/devices/system/cpu/cpufreq/boost <- Disable boost cat
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200	<-
> Discrepancy with step 1 as in both cases boost is disabled.
>
> Note that the other way to fix this is to set cpuinfo.max_freq in Soc
> cpufreq driver during initialization. Fixing it in
> cpufreq_frequency_table_cpuinfo seems more generic solution
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>   drivers/cpufreq/freq_table.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index 67e56cf638ef..6784f94124df 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
>   	struct cpufreq_frequency_table *pos;
>   	unsigned int min_freq = ~0;
>   	unsigned int max_freq = 0;
> +	unsigned int cpuinfo_max_freq = 0;
>   	unsigned int freq;
>   
>   	cpufreq_for_each_valid_entry(pos, table) {
>   		freq = pos->frequency;
>   
> +		if (freq > cpuinfo_max_freq)
> +			cpuinfo_max_freq = freq;
> +
>   		if (!cpufreq_boost_enabled()
>   		    && (pos->flags & CPUFREQ_BOOST_FREQ))
>   			continue;
> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
>   	 * If the driver has set its own cpuinfo.max_freq above max_freq, leave
>   	 * it as is.
>   	 */
> -	if (policy->cpuinfo.max_freq < max_freq)
> -		policy->max = policy->cpuinfo.max_freq = max_freq;
> +	if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
> +		policy->cpuinfo.max_freq = cpuinfo_max_freq;
>   
>   	if (policy->min == ~0)
>   		return -EINVAL;


Something still isn't quite right...

The setup is that I have an rc.local of

#!/bin/sh

echo 1 > /sys/devices/system/cpu/cpufreq/boost

exit 0


After booting and logging in:

steev@limitless:~$ cat 
/sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
825600 2499
<snip>
2649600 38
2745600 31
2841600 1473
2956800 0

After running a "cargo build --release" in an alacritty git checkout:

teev@limitless:~$ cat 
/sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
825600 11220
<snip>
2649600 41
2745600 35
2841600 3065
2956800 0


however...

If I then

steev@limitless:~$ echo 0 | sudo tee /sys/devices/system/cpu/cpufreq/boost
[sudo] password for steev:
0
steev@limitless:~$ echo 1 | sudo tee /sys/devices/system/cpu/cpufreq/boost
1

and run the build again...

steev@limitless:~$ cat 
/sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
825600 21386
<snip>
2649600 45
2745600 38
2841600 3326
2956800 4815

As a workaround, I attempted to jiggle it 1-0-1 in rc.local, however 
that ends up giving

steev@limitless:~$ cat 
/sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
825600 2902
<snip>
2649600 36
2745600 36
2841600 6050
2956800 13

And it doesn't go up, I even tried adding a sleep of 1 second between 
the echo 1/0/1 lines and while 2956800 goes up to 28 (but never uses it) 
it seems like, unless I do it manually once I've logged in, which I'm 
assuming is a lot slower than waiting 1 second between them, it's not 
quite giving us 2956800 "easily".

If the email wasn't clear, please let me know! I tried to explain as 
best I could what I am seeing here.

-- steev

Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[Steev Klimaszewski]

On 11/15/21 7:23 PM, Steev Klimaszewski wrote:
> Hi Thara,
> <snip>
>
> And it doesn't go up, I even tried adding a sleep of 1 second between 
> the echo 1/0/1 lines and while 2956800 goes up to 28 (but never uses 
> it) it seems like, unless I do it manually once I've logged in, which 
> I'm assuming is a lot slower than waiting 1 second between them, it's 
> not quite giving us 2956800 "easily".
>
> If the email wasn't clear, please let me know! I tried to explain as 
> best I could what I am seeing here.
>
> -- steev
>
So after more testing... *sometimes* it works, with the jiggle, but not 
always.  I'm really not sure why though, so if there's any guidance, I'm 
all for it!

-- steev

Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[Viresh Kumar]

On 15-11-21, 19:23, Steev Klimaszewski wrote:
> Hi Thara,
> 
> On 11/15/21 1:50 PM, Thara Gopinath wrote:
> > cpuinfo.max_freq reflects the maximum supported frequency of cpus in a
> > cpufreq policy. When cpus support boost frequency and if boost is disabled
> > during boot up (which is the default), cpuinfo.max_freq does not reflect
> > boost frequency as the maximum supported frequency till boost is explicitly
> > enabled via sysfs interface later. This also means that policy reports two
> > different cpuinfo.max_freq before and after turning on boost.  Fix this by
> > separating out setting of policy->max and cpuinfo.max_freq in
> > cpufreq_frequency_table_cpuinfo.
> > 
> > e.g. of the problem. Qualcomm sdm845 supports boost frequency for gold
> > cluster (cpus 4-7). After boot up (boost disabled),
> > 
> > 1.  cat /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2649600
> > <- This is wrong because boost frequency is
> > 
> > 2.  echo 1 > /sys/devices/system/cpu/cpufreq/boost  <- Enable boost cat
> > /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200	<-
> > max freq reflects boost freq.
> > 
> > 3.  echo 0 > /sys/devices/system/cpu/cpufreq/boost <- Disable boost cat
> > /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200	<-
> > Discrepancy with step 1 as in both cases boost is disabled.
> > 
> > Note that the other way to fix this is to set cpuinfo.max_freq in Soc
> > cpufreq driver during initialization. Fixing it in
> > cpufreq_frequency_table_cpuinfo seems more generic solution
> > 
> > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> > ---
> >   drivers/cpufreq/freq_table.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> > index 67e56cf638ef..6784f94124df 100644
> > --- a/drivers/cpufreq/freq_table.c
> > +++ b/drivers/cpufreq/freq_table.c
> > @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> >   	struct cpufreq_frequency_table *pos;
> >   	unsigned int min_freq = ~0;
> >   	unsigned int max_freq = 0;
> > +	unsigned int cpuinfo_max_freq = 0;
> >   	unsigned int freq;
> >   	cpufreq_for_each_valid_entry(pos, table) {
> >   		freq = pos->frequency;
> > +		if (freq > cpuinfo_max_freq)
> > +			cpuinfo_max_freq = freq;
> > +
> >   		if (!cpufreq_boost_enabled()
> >   		    && (pos->flags & CPUFREQ_BOOST_FREQ))
> >   			continue;
> > @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> >   	 * If the driver has set its own cpuinfo.max_freq above max_freq, leave
> >   	 * it as is.
> >   	 */
> > -	if (policy->cpuinfo.max_freq < max_freq)
> > -		policy->max = policy->cpuinfo.max_freq = max_freq;
> > +	if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
> > +		policy->cpuinfo.max_freq = cpuinfo_max_freq;

You need to keep the check of policy->max here and update policy->max,
else you will never run at boost freq. I think this is what Steev
reported as well ?

So basically something like this:

	if (policy->max < max_freq)
		policy->max = max_freq;

	if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
		policy->cpuinfo.max_freq = cpuinfo_max_freq;

-- 
viresh

Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[Thara Gopinath]

On 11/15/21 10:59 PM, Viresh Kumar wrote:
> On 15-11-21, 19:23, Steev Klimaszewski wrote:
>> Hi Thara,
>>
>> On 11/15/21 1:50 PM, Thara Gopinath wrote:
>>> cpuinfo.max_freq reflects the maximum supported frequency of cpus in a
>>> cpufreq policy. When cpus support boost frequency and if boost is disabled
>>> during boot up (which is the default), cpuinfo.max_freq does not reflect
>>> boost frequency as the maximum supported frequency till boost is explicitly
>>> enabled via sysfs interface later. This also means that policy reports two
>>> different cpuinfo.max_freq before and after turning on boost.  Fix this by
>>> separating out setting of policy->max and cpuinfo.max_freq in
>>> cpufreq_frequency_table_cpuinfo.
>>>
>>> e.g. of the problem. Qualcomm sdm845 supports boost frequency for gold
>>> cluster (cpus 4-7). After boot up (boost disabled),
>>>
>>> 1.  cat /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2649600
>>> <- This is wrong because boost frequency is
>>>
>>> 2.  echo 1 > /sys/devices/system/cpu/cpufreq/boost  <- Enable boost cat
>>> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200	<-
>>> max freq reflects boost freq.
>>>
>>> 3.  echo 0 > /sys/devices/system/cpu/cpufreq/boost <- Disable boost cat
>>> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 2803200	<-
>>> Discrepancy with step 1 as in both cases boost is disabled.
>>>
>>> Note that the other way to fix this is to set cpuinfo.max_freq in Soc
>>> cpufreq driver during initialization. Fixing it in
>>> cpufreq_frequency_table_cpuinfo seems more generic solution
>>>
>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>>> ---
>>>    drivers/cpufreq/freq_table.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
>>> index 67e56cf638ef..6784f94124df 100644
>>> --- a/drivers/cpufreq/freq_table.c
>>> +++ b/drivers/cpufreq/freq_table.c
>>> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
>>>    	struct cpufreq_frequency_table *pos;
>>>    	unsigned int min_freq = ~0;
>>>    	unsigned int max_freq = 0;
>>> +	unsigned int cpuinfo_max_freq = 0;
>>>    	unsigned int freq;
>>>    	cpufreq_for_each_valid_entry(pos, table) {
>>>    		freq = pos->frequency;
>>> +		if (freq > cpuinfo_max_freq)
>>> +			cpuinfo_max_freq = freq;
>>> +
>>>    		if (!cpufreq_boost_enabled()
>>>    		    && (pos->flags & CPUFREQ_BOOST_FREQ))
>>>    			continue;
>>> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
>>>    	 * If the driver has set its own cpuinfo.max_freq above max_freq, leave
>>>    	 * it as is.
>>>    	 */
>>> -	if (policy->cpuinfo.max_freq < max_freq)
>>> -		policy->max = policy->cpuinfo.max_freq = max_freq;
>>> +	if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
>>> +		policy->cpuinfo.max_freq = cpuinfo_max_freq;
> 
> You need to keep the check of policy->max here and update policy->max,
> else you will never run at boost freq. I think this is what Steev
> reported as well ?

Hi Viresh,
	policy->max is unconditionally set to max_freq in the line before "if 
(policy->cpuinfo.max_freq < max_freq)". So this is not the issue Steev 
is reporting.
	policy->max = max_freq


> 
> So basically something like this:
> 
> 	if (policy->max < max_freq)
> 		policy->max = max_freq;
> 
> 	if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
> 		policy->cpuinfo.max_freq = cpuinfo_max_freq;
> 

-- 
Warm Regards
Thara (She/Her/Hers)

Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[Thara Gopinath]

Hi Steev,

Thanks for testing this.

On 11/15/21 8:23 PM, Steev Klimaszewski wrote:

--- snip
>>
>> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
>> index 67e56cf638ef..6784f94124df 100644
>> --- a/drivers/cpufreq/freq_table.c
>> +++ b/drivers/cpufreq/freq_table.c
>> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct 
>> cpufreq_policy *policy,
>>       struct cpufreq_frequency_table *pos;
>>       unsigned int min_freq = ~0;
>>       unsigned int max_freq = 0;
>> +    unsigned int cpuinfo_max_freq = 0;
>>       unsigned int freq;
>>       cpufreq_for_each_valid_entry(pos, table) {
>>           freq = pos->frequency;
>> +        if (freq > cpuinfo_max_freq)
>> +            cpuinfo_max_freq = freq;
>> +
>>           if (!cpufreq_boost_enabled()
>>               && (pos->flags & CPUFREQ_BOOST_FREQ))
>>               continue;
>> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct 
>> cpufreq_policy *policy,
>>        * If the driver has set its own cpuinfo.max_freq above 
>> max_freq, leave
>>        * it as is.
>>        */
>> -    if (policy->cpuinfo.max_freq < max_freq)
>> -        policy->max = policy->cpuinfo.max_freq = max_freq;
>> +    if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
>> +        policy->cpuinfo.max_freq = cpuinfo_max_freq;
>>       if (policy->min == ~0)
>>           return -EINVAL;
> 
> 
> Something still isn't quite right...
> 
> The setup is that I have an rc.local of
> 
> #!/bin/sh
> 
> echo 1 > /sys/devices/system/cpu/cpufreq/boost
> 
> exit 0
> 
> 
> After booting and logging in:
> 
> steev@limitless:~$ cat 
> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
> 825600 2499
> <snip>
> 2649600 38
> 2745600 31
> 2841600 1473
> 2956800 0

Did you try debugging this ? As in did you read back boost and 
cpuinfo_max_freq at this point to ensure that everything is as expected ?


-- 
Warm Regards
Thara (She/Her/Hers)
> 
> After running a "cargo build --release" in an alacritty git checkout:
> 
> teev@limitless:~$ cat 
> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
> 825600 11220
> <snip>
> 2649600 41
> 2745600 35
> 2841600 3065
> 2956800 0
> 
> 
> however...
> 
> If I then
> 
> steev@limitless:~$ echo 0 | sudo tee /sys/devices/system/cpu/cpufreq/boost
> [sudo] password for steev:
> 0
> steev@limitless:~$ echo 1 | sudo tee /sys/devices/system/cpu/cpufreq/boost
> 1
> 
> and run the build again...
> 
> steev@limitless:~$ cat 
> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
> 825600 21386
> <snip>
> 2649600 45
> 2745600 38
> 2841600 3326
> 2956800 4815
> 
> As a workaround, I attempted to jiggle it 1-0-1 in rc.local, however 
> that ends up giving
> 
> steev@limitless:~$ cat 
> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
> 825600 2902
> <snip>
> 2649600 36
> 2745600 36
> 2841600 6050
> 2956800 13
> 
> And it doesn't go up, I even tried adding a sleep of 1 second between 
> the echo 1/0/1 lines and while 2956800 goes up to 28 (but never uses it) 
> it seems like, unless I do it manually once I've logged in, which I'm 
> assuming is a lot slower than waiting 1 second between them, it's not 
> quite giving us 2956800 "easily".
> 
> If the email wasn't clear, please let me know! I tried to explain as 
> best I could what I am seeing here.
> 
> -- steev
> 

Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[Steev Klimaszewski]

On 11/16/21 9:31 AM, Thara Gopinath wrote:
> Hi Steev,
>
> Thanks for testing this.
>
> On 11/15/21 8:23 PM, Steev Klimaszewski wrote:
>
> --- snip
>>>
>>> diff --git a/drivers/cpufreq/freq_table.c 
>>> b/drivers/cpufreq/freq_table.c
>>> index 67e56cf638ef..6784f94124df 100644
>>> --- a/drivers/cpufreq/freq_table.c
>>> +++ b/drivers/cpufreq/freq_table.c
>>> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct 
>>> cpufreq_policy *policy,
>>>       struct cpufreq_frequency_table *pos;
>>>       unsigned int min_freq = ~0;
>>>       unsigned int max_freq = 0;
>>> +    unsigned int cpuinfo_max_freq = 0;
>>>       unsigned int freq;
>>>       cpufreq_for_each_valid_entry(pos, table) {
>>>           freq = pos->frequency;
>>> +        if (freq > cpuinfo_max_freq)
>>> +            cpuinfo_max_freq = freq;
>>> +
>>>           if (!cpufreq_boost_enabled()
>>>               && (pos->flags & CPUFREQ_BOOST_FREQ))
>>>               continue;
>>> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct 
>>> cpufreq_policy *policy,
>>>        * If the driver has set its own cpuinfo.max_freq above 
>>> max_freq, leave
>>>        * it as is.
>>>        */
>>> -    if (policy->cpuinfo.max_freq < max_freq)
>>> -        policy->max = policy->cpuinfo.max_freq = max_freq;
>>> +    if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
>>> +        policy->cpuinfo.max_freq = cpuinfo_max_freq;
>>>       if (policy->min == ~0)
>>>           return -EINVAL;
>>
>>
>> Something still isn't quite right...
>>
>> The setup is that I have an rc.local of
>>
>> #!/bin/sh
>>
>> echo 1 > /sys/devices/system/cpu/cpufreq/boost
>>
>> exit 0
>>
>>
>> After booting and logging in:
>>
>> steev@limitless:~$ cat 
>> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
>> 825600 2499
>> <snip>
>> 2649600 38
>> 2745600 31
>> 2841600 1473
>> 2956800 0
>
> Did you try debugging this ? As in did you read back boost and 
> cpuinfo_max_freq at this point to ensure that everything is as expected ?
>
>
Hi Thara,

I did - sorry I forgot to mention that boost does show 1 for enabled and 
cpuinfo_max_freq is set to 2956800.  However, scaling_max_freq is still 
listed as 2841600 and scaling_available_frequencies still shows 2841600 
as the max available. scaling_boost_freqencies does also list 2956800.

steev@limitless:~$ grep . /sys/devices/system/cpu/cpufreq/policy4/*
/sys/devices/system/cpu/cpufreq/policy4/affected_cpus:4 5 6 7
grep: /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_cur_freq: 
Permission denied
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq:2956800
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_min_freq:825600
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_transition_latency:0
/sys/devices/system/cpu/cpufreq/policy4/related_cpus:4 5 6 7
/sys/devices/system/cpu/cpufreq/policy4/scaling_available_frequencies:825600 
902400 979200 1056000 1209600 1286400 1363200 1459200 1536000 1612800 
1689600 1766400 1843200 1920000 1996800 2092800 2169600 2246400 2323200 
2400000 2476800 2553600 2649600 2745600 2841600
/sys/devices/system/cpu/cpufreq/policy4/scaling_available_governors:ondemand 
conservative powersave userspace performance schedutil
/sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies:2956800
/sys/devices/system/cpu/cpufreq/policy4/scaling_cur_freq:1920000
/sys/devices/system/cpu/cpufreq/policy4/scaling_driver:qcom-cpufreq-hw
/sys/devices/system/cpu/cpufreq/policy4/scaling_governor:schedutil
/sys/devices/system/cpu/cpufreq/policy4/scaling_max_freq:2841600
/sys/devices/system/cpu/cpufreq/policy4/scaling_min_freq:825600
/sys/devices/system/cpu/cpufreq/policy4/scaling_setspeed:<unsupported>

Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[Steev Klimaszewski]

On 11/16/21 10:15 AM, Steev Klimaszewski wrote:
>
> On 11/16/21 9:31 AM, Thara Gopinath wrote:
>> Hi Steev,
>>
>> Thanks for testing this.
>>
>> On 11/15/21 8:23 PM, Steev Klimaszewski wrote:
>>
>> --- snip
>>>>
>>>> diff --git a/drivers/cpufreq/freq_table.c 
>>>> b/drivers/cpufreq/freq_table.c
>>>> index 67e56cf638ef..6784f94124df 100644
>>>> --- a/drivers/cpufreq/freq_table.c
>>>> +++ b/drivers/cpufreq/freq_table.c
>>>> @@ -35,11 +35,15 @@ int cpufreq_frequency_table_cpuinfo(struct 
>>>> cpufreq_policy *policy,
>>>>       struct cpufreq_frequency_table *pos;
>>>>       unsigned int min_freq = ~0;
>>>>       unsigned int max_freq = 0;
>>>> +    unsigned int cpuinfo_max_freq = 0;
>>>>       unsigned int freq;
>>>>       cpufreq_for_each_valid_entry(pos, table) {
>>>>           freq = pos->frequency;
>>>> +        if (freq > cpuinfo_max_freq)
>>>> +            cpuinfo_max_freq = freq;
>>>> +
>>>>           if (!cpufreq_boost_enabled()
>>>>               && (pos->flags & CPUFREQ_BOOST_FREQ))
>>>>               continue;
>>>> @@ -57,8 +61,8 @@ int cpufreq_frequency_table_cpuinfo(struct 
>>>> cpufreq_policy *policy,
>>>>        * If the driver has set its own cpuinfo.max_freq above 
>>>> max_freq, leave
>>>>        * it as is.
>>>>        */
>>>> -    if (policy->cpuinfo.max_freq < max_freq)
>>>> -        policy->max = policy->cpuinfo.max_freq = max_freq;
>>>> +    if (policy->cpuinfo.max_freq < cpuinfo_max_freq)
>>>> +        policy->cpuinfo.max_freq = cpuinfo_max_freq;
>>>>       if (policy->min == ~0)
>>>>           return -EINVAL;
>>>
>>>
>>> Something still isn't quite right...
>>>
>>> The setup is that I have an rc.local of
>>>
>>> #!/bin/sh
>>>
>>> echo 1 > /sys/devices/system/cpu/cpufreq/boost
>>>
>>> exit 0
>>>
>>>
>>> After booting and logging in:
>>>
>>> steev@limitless:~$ cat 
>>> /sys/devices/system/cpu/cpufreq/policy4/stats/time_in_state
>>> 825600 2499
>>> <snip>
>>> 2649600 38
>>> 2745600 31
>>> 2841600 1473
>>> 2956800 0
>>
>> Did you try debugging this ? As in did you read back boost and 
>> cpuinfo_max_freq at this point to ensure that everything is as 
>> expected ?
>>
>>
> Hi Thara,
>
> I did - sorry I forgot to mention that boost does show 1 for enabled 
> and cpuinfo_max_freq is set to 2956800.  However, scaling_max_freq is 
> still listed as 2841600 and scaling_available_frequencies still shows 
> 2841600 as the max available. scaling_boost_freqencies does also list 
> 2956800.
>
> steev@limitless:~$ grep . /sys/devices/system/cpu/cpufreq/policy4/*
> /sys/devices/system/cpu/cpufreq/policy4/affected_cpus:4 5 6 7
> grep: /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_cur_freq: 
> Permission denied
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq:2956800
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_min_freq:825600
> /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_transition_latency:0
> /sys/devices/system/cpu/cpufreq/policy4/related_cpus:4 5 6 7
> /sys/devices/system/cpu/cpufreq/policy4/scaling_available_frequencies:825600 
> 902400 979200 1056000 1209600 1286400 1363200 1459200 1536000 1612800 
> 1689600 1766400 1843200 1920000 1996800 2092800 2169600 2246400 
> 2323200 2400000 2476800 2553600 2649600 2745600 2841600
> /sys/devices/system/cpu/cpufreq/policy4/scaling_available_governors:ondemand 
> conservative powersave userspace performance schedutil
> /sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies:2956800
> /sys/devices/system/cpu/cpufreq/policy4/scaling_cur_freq:1920000
> /sys/devices/system/cpu/cpufreq/policy4/scaling_driver:qcom-cpufreq-hw
> /sys/devices/system/cpu/cpufreq/policy4/scaling_governor:schedutil
> /sys/devices/system/cpu/cpufreq/policy4/scaling_max_freq:2841600
> /sys/devices/system/cpu/cpufreq/policy4/scaling_min_freq:825600
> /sys/devices/system/cpu/cpufreq/policy4/scaling_setspeed:<unsupported>
>
Once it does start working (e.g. I've run echo 0 to turn off boost, and 
then echo 1 to turn it back one)

steev@limitless:~$ grep . /sys/devices/system/cpu/cpufreq/policy4/*
/sys/devices/system/cpu/cpufreq/policy4/affected_cpus:4 5 6 7
grep: /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_cur_freq: 
Permission denied
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq:2956800
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_min_freq:825600
/sys/devices/system/cpu/cpufreq/policy4/cpuinfo_transition_latency:0
/sys/devices/system/cpu/cpufreq/policy4/related_cpus:4 5 6 7
/sys/devices/system/cpu/cpufreq/policy4/scaling_available_frequencies:825600 
902400 979200 1056000 1209600 1286400 1363200 1459200 1536000 1612800 
1689600 1766400 1843200 1920000 1996800 2092800 2169600 2246400 2323200 
2400000 2476800 2553600 2649600 2745600 2841600
/sys/devices/system/cpu/cpufreq/policy4/scaling_available_governors:ondemand 
conservative powersave userspace performance schedutil
/sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies:2956800
/sys/devices/system/cpu/cpufreq/policy4/scaling_cur_freq:1920000
/sys/devices/system/cpu/cpufreq/policy4/scaling_driver:qcom-cpufreq-hw
/sys/devices/system/cpu/cpufreq/policy4/scaling_governor:schedutil
/sys/devices/system/cpu/cpufreq/policy4/scaling_max_freq:2956800
/sys/devices/system/cpu/cpufreq/policy4/scaling_min_freq:825600
/sys/devices/system/cpu/cpufreq/policy4/scaling_setspeed:<unsupported>


Notice that the scaling_max_freq is now 2956800 instead of 2841600 when 
it isn't working.

Sorry for forgetting and sending another mail :(

-- steev

Re: [PATCH] cpufreq: freq_table: Initialize cpuinfo.max_freq to correct max frequency.

[Viresh Kumar]

On 16-11-21, 10:27, Thara Gopinath wrote:
> 	policy->max is unconditionally set to max_freq in the line before "if
> (policy->cpuinfo.max_freq < max_freq)".

So the current code is not correct then :)

-- 
viresh