[PATCH v3 0/2] cppc_cpufreq: Rework ->get() error handling when cores are idle

[PATCH v3 0/2] cppc_cpufreq: Rework ->get() error handling when cores are idle

[Jie Zhan]

CPPC feedback counters can be unchanged or 0 when cores are idle, e.g.
clock-gated or power-gated.  In such case, get the latest desired perf for
calculating frequency.

Also, the HiSilicon CPPC workaround can now be removed as it can be handled
by the common code.

---

v3:
- Merge patch 1 & 2, tidy up the logic, and reduce duplicate code
- Return 0 in cppc_perf_from_fbctrs() if the feedback counters are
  unchanged rather than return a cached desired perf
- Return early in cppc_scale_freq_workfn() if the feedback counters are
  unchanged

v2:
- Try reading the lastest desired perf first before using the cached one
- Do the same handling logic when feedback counters are unchanged
- Remove hisilicon workaround

Discussions:
v1: https://lore.kernel.org/all/20240819035147.2239767-1-zhanjie9@hisilicon.com/
v2: https://lore.kernel.org/all/20240912072231.439332-1-zhanjie9@hisilicon.com/

Jie Zhan (2):
  cppc_cpufreq: Use desired perf if feedback ctrs are 0 or unchanged
  cppc_cpufreq: Remove HiSilicon CPPC workaround

 drivers/cpufreq/cppc_cpufreq.c | 120 +++++++++++----------------------
 1 file changed, 39 insertions(+), 81 deletions(-)

-- 
2.33.0

[PATCH v3 1/2] cppc_cpufreq: Use desired perf if feedback ctrs are 0 or unchanged

[Jie Zhan]

The CPPC performance feedback counters could be 0 or unchanged when the
target cpu is in a low-power idle state, e.g. power-gated or clock-gated.

When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
cpufreq_online() get a false error and fail to generate a cpufreq policy.

When the counters are unchanged, the existing cppc_perf_from_fbctrs()
returns a cached desired perf, but some platforms may update the real
frequency back to the desired perf reg.

For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
to reflect the frequency; if failed, return the cached desired perf.

Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.")
Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
Reviewed-by: Zeng Heng <zengheng4@huawei.com>
Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index bafa32dd375d..e55192303a9f 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
 
 	perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
 				     &fb_ctrs);
+	if (!perf)
+		return;
+
 	cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
 
 	perf <<= SCHED_CAPACITY_SHIFT;
@@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
 
 	/* Check to avoid divide-by zero and invalid delivered_perf */
 	if (!delta_reference || !delta_delivered)
-		return cpu_data->perf_ctrls.desired_perf;
+		return 0;
 
 	return (reference_perf * delta_delivered) / delta_reference;
 }
 
+static int cppc_get_perf_ctrs_sample(int cpu,
+				     struct cppc_perf_fb_ctrs *fb_ctrs_t0,
+				     struct cppc_perf_fb_ctrs *fb_ctrs_t1)
+{
+	int ret;
+
+	ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
+	if (ret)
+		return ret;
+
+	udelay(2); /* 2usec delay between sampling */
+
+	return cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
+}
+
 static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 {
 	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
@@ -746,18 +764,29 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 
 	cpufreq_cpu_put(policy);
 
-	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
-	if (ret)
-		return 0;
-
-	udelay(2); /* 2usec delay between sampling */
-
-	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
-	if (ret)
-		return 0;
+	ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
+	if (ret) {
+		if (ret == -EFAULT)
+			goto out_invalid_counters;
+		else
+			return 0;
+	}
 
 	delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
 					       &fb_ctrs_t1);
+	if (!delivered_perf)
+		goto out_invalid_counters;
+
+	return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
+
+out_invalid_counters:
+	/*
+	 * Feedback counters could be unchanged or 0 when a cpu enters a
+	 * low-power idle state, e.g. clock-gated or power-gated.
+	 * Get the lastest or cached desired perf for reflecting frequency.
+	 */
+	if (cppc_get_desired_perf(cpu, &delivered_perf))
+		delivered_perf = cpu_data->perf_ctrls.desired_perf;
 
 	return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
 }
-- 
2.33.0

Re: [PATCH v3 1/2] cppc_cpufreq: Use desired perf if feedback ctrs are 0 or unchanged

[lihuisong (C)]

Hi Jie,

LGTM except for some trivial,
Reviewed-by: Huisong Li <lihuisong@huawei.com>


在 2024/9/19 16:45, Jie Zhan 写道:
> The CPPC performance feedback counters could be 0 or unchanged when the
> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>
> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>
> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
> returns a cached desired perf, but some platforms may update the real
> frequency back to the desired perf reg.
>
> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
> to reflect the frequency; if failed, return the cached desired perf.
>
> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.")
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> Reviewed-by: Zeng Heng <zengheng4@huawei.com>
> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
> ---
>   drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>   1 file changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..e55192303a9f 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
>   
>   	perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
>   				     &fb_ctrs);
> +	if (!perf)
> +		return;
> +
>   	cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
>   
>   	perf <<= SCHED_CAPACITY_SHIFT;
> @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>   
>   	/* Check to avoid divide-by zero and invalid delivered_perf */
Now this comment can be removed, right?
>   	if (!delta_reference || !delta_delivered)
> -		return cpu_data->perf_ctrls.desired_perf;
> +		return 0;
>   
>   	return (reference_perf * delta_delivered) / delta_reference;
>   }
>   
> +static int cppc_get_perf_ctrs_sample(int cpu,
> +				     struct cppc_perf_fb_ctrs *fb_ctrs_t0,
> +				     struct cppc_perf_fb_ctrs *fb_ctrs_t1)
> +{
> +	int ret;
> +
> +	ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
> +	if (ret)
> +		return ret;
> +
> +	udelay(2); /* 2usec delay between sampling */
> +
> +	return cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
> +}
> +
>   static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>   {
>   	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> @@ -746,18 +764,29 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>   
>   	cpufreq_cpu_put(policy);
>   
> -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
> -	if (ret)
> -		return 0;
> -
> -	udelay(2); /* 2usec delay between sampling */
> -
> -	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
> -	if (ret)
> -		return 0;
> +	ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
> +	if (ret) {
> +		if (ret == -EFAULT)
> +			goto out_invalid_counters;
suggest that add some comments for ret == -EFAULT case.
Because this error code depands on the implementation of cppc_get_perf_ctrs.
If add a new exception case which also return -EFAULT, then this switch 
is unreasonable.
> +		else
> +			return 0;
> +	}
>   
>   	delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>   					       &fb_ctrs_t1);
> +	if (!delivered_perf)
> +		goto out_invalid_counters;
> +
> +	return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> +
> +out_invalid_counters:
> +	/*
> +	 * Feedback counters could be unchanged or 0 when a cpu enters a
> +	 * low-power idle state, e.g. clock-gated or power-gated.
> +	 * Get the lastest or cached desired perf for reflecting frequency.
> +	 */
> +	if (cppc_get_desired_perf(cpu, &delivered_perf))
> +		delivered_perf = cpu_data->perf_ctrls.desired_perf;
>   
>   	return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
>   }

Re: [PATCH v3 1/2] cppc_cpufreq: Use desired perf if feedback ctrs are 0 or unchanged

[Jie Zhan]

On 25/09/2024 17:28, lihuisong (C) wrote:
> Hi Jie,
> 
> LGTM except for some trivial,
> Reviewed-by: Huisong Li <lihuisong@huawei.com>

Thanks.

> 
> 
> 在 2024/9/19 16:45, Jie Zhan 写道:
>> The CPPC performance feedback counters could be 0 or unchanged when the
>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>>
>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
>> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>>
>> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
>> returns a cached desired perf, but some platforms may update the real
>> frequency back to the desired perf reg.
>>
>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
>> to reflect the frequency; if failed, return the cached desired perf.
>>
>> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.")
>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
>> Reviewed-by: Zeng Heng <zengheng4@huawei.com>
>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>>   1 file changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index bafa32dd375d..e55192303a9f 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
>>         perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
>>                        &fb_ctrs);
>> +    if (!perf)
>> +        return;
>> +
>>       cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
>>         perf <<= SCHED_CAPACITY_SHIFT;
>> @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>         /* Check to avoid divide-by zero and invalid delivered_perf */
> Now this comment can be removed, right?

Didn't notice this comment, but, having a check, I think it still fits.
'!delta_reference' avoids divide-by zero, and '!delta_delivered' checks
invalid delivered_perf.

So I think we just leave it unchanged.

>>       if (!delta_reference || !delta_delivered)
>> -        return cpu_data->perf_ctrls.desired_perf;
>> +        return 0;
>>         return (reference_perf * delta_delivered) / delta_reference;
>>   }
>>   +static int cppc_get_perf_ctrs_sample(int cpu,
>> +                     struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>> +                     struct cppc_perf_fb_ctrs *fb_ctrs_t1)
>> +{
>> +    int ret;
>> +
>> +    ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
>> +    if (ret)
>> +        return ret;
>> +
>> +    udelay(2); /* 2usec delay between sampling */
>> +
>> +    return cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
>> +}
>> +
>>   static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>   {
>>       struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>> @@ -746,18 +764,29 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>         cpufreq_cpu_put(policy);
>>   -    ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>> -    if (ret)
>> -        return 0;
>> -
>> -    udelay(2); /* 2usec delay between sampling */
>> -
>> -    ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>> -    if (ret)
>> -        return 0;
>> +    ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
>> +    if (ret) {
>> +        if (ret == -EFAULT)
>> +            goto out_invalid_counters;
> suggest that add some comments for ret == -EFAULT case.
> Because this error code depands on the implementation of cppc_get_perf_ctrs.
> If add a new exception case which also return -EFAULT, then this switch is unreasonable.

Sure. What about adding the following comment:

/* -EFAULT indicates that any of the associated CPPC regs is 0. */

Thanks,
Jie

Re: [PATCH v3 1/2] cppc_cpufreq: Use desired perf if feedback ctrs are 0 or unchanged

[lihuisong (C)]

在 2024/9/26 10:57, Jie Zhan 写道:
>
> On 25/09/2024 17:28, lihuisong (C) wrote:
>> Hi Jie,
>>
>> LGTM except for some trivial,
>> Reviewed-by: Huisong Li <lihuisong@huawei.com>
> Thanks.
>
>>
>> 在 2024/9/19 16:45, Jie Zhan 写道:
>>> The CPPC performance feedback counters could be 0 or unchanged when the
>>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>>>
>>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
>>> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>>>
>>> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
>>> returns a cached desired perf, but some platforms may update the real
>>> frequency back to the desired perf reg.
>>>
>>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
>>> to reflect the frequency; if failed, return the cached desired perf.
>>>
>>> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.")
>>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
>>> Reviewed-by: Zeng Heng <zengheng4@huawei.com>
>>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
>>> ---
>>>    drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>>>    1 file changed, 39 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>> index bafa32dd375d..e55192303a9f 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
>>>          perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
>>>                         &fb_ctrs);
>>> +    if (!perf)
>>> +        return;
>>> +
>>>        cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
>>>          perf <<= SCHED_CAPACITY_SHIFT;
>>> @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>>          /* Check to avoid divide-by zero and invalid delivered_perf */
>> Now this comment can be removed, right?
> Didn't notice this comment, but, having a check, I think it still fits.
> '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks
> invalid delivered_perf.
The comment  "avoid divide-by zero" is just for the below code: 
"(reference_perf * delta_delivered) / delta_reference".
So It is also useful, but I think It's obvious and it doesn't make much 
sense.

The comment "avoid invalid delivered_perf" is for the return value.
Now this func return zero which can't count as a valid delivered_perf, 
right?
>
> So I think we just leave it unchanged.
>
>>>        if (!delta_reference || !delta_delivered)
>>> -        return cpu_data->perf_ctrls.desired_perf;
>>> +        return 0;
>>>          return (reference_perf * delta_delivered) / delta_reference;
>>>    }
>>>    +static int cppc_get_perf_ctrs_sample(int cpu,
>>> +                     struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>>> +                     struct cppc_perf_fb_ctrs *fb_ctrs_t1)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    udelay(2); /* 2usec delay between sampling */
>>> +
>>> +    return cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
>>> +}
>>> +
>>>    static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>>    {
>>>        struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>>> @@ -746,18 +764,29 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>>          cpufreq_cpu_put(policy);
>>>    -    ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>>> -    if (ret)
>>> -        return 0;
>>> -
>>> -    udelay(2); /* 2usec delay between sampling */
>>> -
>>> -    ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>>> -    if (ret)
>>> -        return 0;
>>> +    ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
>>> +    if (ret) {
>>> +        if (ret == -EFAULT)
>>> +            goto out_invalid_counters;
>> suggest that add some comments for ret == -EFAULT case.
>> Because this error code depands on the implementation of cppc_get_perf_ctrs.
>> If add a new exception case which also return -EFAULT, then this switch is unreasonable.
> Sure. What about adding the following comment:
>
> /* -EFAULT indicates that any of the associated CPPC regs is 0. */
Ack
> .

Re: [PATCH v3 1/2] cppc_cpufreq: Use desired perf if feedback ctrs are 0 or unchanged

[Jie Zhan]

On 26/09/2024 14:07, lihuisong (C) wrote:
> 
> 在 2024/9/26 10:57, Jie Zhan 写道:
>>
>> On 25/09/2024 17:28, lihuisong (C) wrote:
>>> Hi Jie,
>>>
>>> LGTM except for some trivial,
>>> Reviewed-by: Huisong Li <lihuisong@huawei.com>
>> Thanks.
>>
>>>
>>> 在 2024/9/19 16:45, Jie Zhan 写道:
>>>> The CPPC performance feedback counters could be 0 or unchanged when the
>>>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>>>>
>>>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
>>>> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>>>>
>>>> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
>>>> returns a cached desired perf, but some platforms may update the real
>>>> frequency back to the desired perf reg.
>>>>
>>>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
>>>> to reflect the frequency; if failed, return the cached desired perf.
>>>>
>>>> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.")
>>>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
>>>> Reviewed-by: Zeng Heng <zengheng4@huawei.com>
>>>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
>>>> ---
>>>>    drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>>>>    1 file changed, 39 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>>> index bafa32dd375d..e55192303a9f 100644
>>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>>> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
>>>>          perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
>>>>                         &fb_ctrs);
>>>> +    if (!perf)
>>>> +        return;
>>>> +
>>>>        cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
>>>>          perf <<= SCHED_CAPACITY_SHIFT;
>>>> @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>>>          /* Check to avoid divide-by zero and invalid delivered_perf */
>>> Now this comment can be removed, right?
>> Didn't notice this comment, but, having a check, I think it still fits.
>> '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks
>> invalid delivered_perf.
> The comment  "avoid divide-by zero" is just for the below code: "(reference_perf * delta_delivered) / delta_reference".
> So It is also useful, but I think It's obvious and it doesn't make much sense.
> 
> The comment "avoid invalid delivered_perf" is for the return value.
> Now this func return zero which can't count as a valid delivered_perf, right?

so, what about this?

/*
 * Avoid divide-by zero and unchanged feedback counters.
 * Leave it for callers to handle.
 */

>>
>> So I think we just leave it unchanged.
>>

...

Re: [PATCH v3 1/2] cppc_cpufreq: Use desired perf if feedback ctrs are 0 or unchanged

[lihuisong (C)]

在 2024/9/26 16:44, Jie Zhan 写道:
>
> On 26/09/2024 14:07, lihuisong (C) wrote:
>> 在 2024/9/26 10:57, Jie Zhan 写道:
>>> On 25/09/2024 17:28, lihuisong (C) wrote:
>>>> Hi Jie,
>>>>
>>>> LGTM except for some trivial,
>>>> Reviewed-by: Huisong Li <lihuisong@huawei.com>
>>> Thanks.
>>>
>>>> 在 2024/9/19 16:45, Jie Zhan 写道:
>>>>> The CPPC performance feedback counters could be 0 or unchanged when the
>>>>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated.
>>>>>
>>>>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes
>>>>> cpufreq_online() get a false error and fail to generate a cpufreq policy.
>>>>>
>>>>> When the counters are unchanged, the existing cppc_perf_from_fbctrs()
>>>>> returns a cached desired perf, but some platforms may update the real
>>>>> frequency back to the desired perf reg.
>>>>>
>>>>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf
>>>>> to reflect the frequency; if failed, return the cached desired perf.
>>>>>
>>>>> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.")
>>>>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
>>>>> Reviewed-by: Zeng Heng <zengheng4@huawei.com>
>>>>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
>>>>> ---
>>>>>     drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++-------
>>>>>     1 file changed, 39 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>>>> index bafa32dd375d..e55192303a9f 100644
>>>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>>>> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
>>>>>           perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
>>>>>                          &fb_ctrs);
>>>>> +    if (!perf)
>>>>> +        return;
>>>>> +
>>>>>         cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
>>>>>           perf <<= SCHED_CAPACITY_SHIFT;
>>>>> @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>>>>>           /* Check to avoid divide-by zero and invalid delivered_perf */
>>>> Now this comment can be removed, right?
>>> Didn't notice this comment, but, having a check, I think it still fits.
>>> '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks
>>> invalid delivered_perf.
>> The comment  "avoid divide-by zero" is just for the below code: "(reference_perf * delta_delivered) / delta_reference".
>> So It is also useful, but I think It's obvious and it doesn't make much sense.
>>
>> The comment "avoid invalid delivered_perf" is for the return value.
>> Now this func return zero which can't count as a valid delivered_perf, right?
> so, what about this?
>
> /*
>   * Avoid divide-by zero and unchanged feedback counters.
>   * Leave it for callers to handle.
>   */
good.
>>> So I think we just leave it unchanged.
>>>
> ...
> .

[PATCH v3 2/2] cppc_cpufreq: Remove HiSilicon CPPC workaround

[Jie Zhan]

Since commit 6c8d750f9784 ("cpufreq / cppc: Work around for Hisilicon CPPC
cpufreq"), we introduce a workround for HiSilicon platforms that do not
support performance feedback counters, whereas they can get the actual
frequency from the desired perf register.  Later on, FIE is disabled in
that workaround as well.

Now the workround can be handled by the common code.  Desired perf would be
read and converted to frequency if feedback counters don't change.  FIE
would be disabled if the CPPC regs are in PCC region.

Hence, the workaround is no longer needed and can be safely removed, in an
effort to consolidate the driver procedure.

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 71 ----------------------------------
 1 file changed, 71 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index e55192303a9f..0e95ad2303ea 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -36,24 +36,6 @@ static LIST_HEAD(cpu_data_list);
 
 static bool boost_supported;
 
-struct cppc_workaround_oem_info {
-	char oem_id[ACPI_OEM_ID_SIZE + 1];
-	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
-	u32 oem_revision;
-};
-
-static struct cppc_workaround_oem_info wa_info[] = {
-	{
-		.oem_id		= "HISI  ",
-		.oem_table_id	= "HIP07   ",
-		.oem_revision	= 0,
-	}, {
-		.oem_id		= "HISI  ",
-		.oem_table_id	= "HIP08   ",
-		.oem_revision	= 0,
-	}
-};
-
 static struct cpufreq_driver cppc_cpufreq_driver;
 
 static enum {
@@ -78,7 +60,6 @@ struct cppc_freq_invariance {
 static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
 static struct kthread_worker *kworker_fie;
 
-static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu);
 static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
 				 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
 				 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
@@ -841,57 +822,6 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
 	.name = "cppc_cpufreq",
 };
 
-/*
- * HISI platform does not support delivered performance counter and
- * reference performance counter. It can calculate the performance using the
- * platform specific mechanism. We reuse the desired performance register to
- * store the real performance calculated by the platform.
- */
-static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
-{
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-	struct cppc_cpudata *cpu_data;
-	u64 desired_perf;
-	int ret;
-
-	if (!policy)
-		return -ENODEV;
-
-	cpu_data = policy->driver_data;
-
-	cpufreq_cpu_put(policy);
-
-	ret = cppc_get_desired_perf(cpu, &desired_perf);
-	if (ret < 0)
-		return -EIO;
-
-	return cppc_perf_to_khz(&cpu_data->perf_caps, desired_perf);
-}
-
-static void cppc_check_hisi_workaround(void)
-{
-	struct acpi_table_header *tbl;
-	acpi_status status = AE_OK;
-	int i;
-
-	status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
-	if (ACPI_FAILURE(status) || !tbl)
-		return;
-
-	for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
-		if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
-		    !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
-		    wa_info[i].oem_revision == tbl->oem_revision) {
-			/* Overwrite the get() callback */
-			cppc_cpufreq_driver.get = hisi_cppc_cpufreq_get_rate;
-			fie_disabled = FIE_DISABLED;
-			break;
-		}
-	}
-
-	acpi_put_table(tbl);
-}
-
 static int __init cppc_cpufreq_init(void)
 {
 	int ret;
@@ -899,7 +829,6 @@ static int __init cppc_cpufreq_init(void)
 	if (!acpi_cpc_valid())
 		return -ENODEV;
 
-	cppc_check_hisi_workaround();
 	cppc_freq_invariance_init();
 	populate_efficiency_class();
 
-- 
2.33.0

Re: [PATCH v3 2/2] cppc_cpufreq: Remove HiSilicon CPPC workaround

[Xiongfeng Wang]

Reviewed-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>

On 2024/9/19 16:45, Jie Zhan wrote:
> Since commit 6c8d750f9784 ("cpufreq / cppc: Work around for Hisilicon CPPC
> cpufreq"), we introduce a workround for HiSilicon platforms that do not
> support performance feedback counters, whereas they can get the actual
> frequency from the desired perf register.  Later on, FIE is disabled in
> that workaround as well.
> 
> Now the workround can be handled by the common code.  Desired perf would be
> read and converted to frequency if feedback counters don't change.  FIE
> would be disabled if the CPPC regs are in PCC region.
> 
> Hence, the workaround is no longer needed and can be safely removed, in an
> effort to consolidate the driver procedure.
> 
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 71 ----------------------------------
>  1 file changed, 71 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index e55192303a9f..0e95ad2303ea 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -36,24 +36,6 @@ static LIST_HEAD(cpu_data_list);
>  
>  static bool boost_supported;
>  
> -struct cppc_workaround_oem_info {
> -	char oem_id[ACPI_OEM_ID_SIZE + 1];
> -	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> -	u32 oem_revision;
> -};
> -
> -static struct cppc_workaround_oem_info wa_info[] = {
> -	{
> -		.oem_id		= "HISI  ",
> -		.oem_table_id	= "HIP07   ",
> -		.oem_revision	= 0,
> -	}, {
> -		.oem_id		= "HISI  ",
> -		.oem_table_id	= "HIP08   ",
> -		.oem_revision	= 0,
> -	}
> -};
> -
>  static struct cpufreq_driver cppc_cpufreq_driver;
>  
>  static enum {
> @@ -78,7 +60,6 @@ struct cppc_freq_invariance {
>  static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
>  static struct kthread_worker *kworker_fie;
>  
> -static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu);
>  static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>  				 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>  				 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
> @@ -841,57 +822,6 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
>  	.name = "cppc_cpufreq",
>  };
>  
> -/*
> - * HISI platform does not support delivered performance counter and
> - * reference performance counter. It can calculate the performance using the
> - * platform specific mechanism. We reuse the desired performance register to
> - * store the real performance calculated by the platform.
> - */
> -static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
> -{
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> -	struct cppc_cpudata *cpu_data;
> -	u64 desired_perf;
> -	int ret;
> -
> -	if (!policy)
> -		return -ENODEV;
> -
> -	cpu_data = policy->driver_data;
> -
> -	cpufreq_cpu_put(policy);
> -
> -	ret = cppc_get_desired_perf(cpu, &desired_perf);
> -	if (ret < 0)
> -		return -EIO;
> -
> -	return cppc_perf_to_khz(&cpu_data->perf_caps, desired_perf);
> -}
> -
> -static void cppc_check_hisi_workaround(void)
> -{
> -	struct acpi_table_header *tbl;
> -	acpi_status status = AE_OK;
> -	int i;
> -
> -	status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
> -	if (ACPI_FAILURE(status) || !tbl)
> -		return;
> -
> -	for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> -		if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> -		    !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> -		    wa_info[i].oem_revision == tbl->oem_revision) {
> -			/* Overwrite the get() callback */
> -			cppc_cpufreq_driver.get = hisi_cppc_cpufreq_get_rate;
> -			fie_disabled = FIE_DISABLED;
> -			break;
> -		}
> -	}
> -
> -	acpi_put_table(tbl);
> -}
> -
>  static int __init cppc_cpufreq_init(void)
>  {
>  	int ret;
> @@ -899,7 +829,6 @@ static int __init cppc_cpufreq_init(void)
>  	if (!acpi_cpc_valid())
>  		return -ENODEV;
>  
> -	cppc_check_hisi_workaround();
>  	cppc_freq_invariance_init();
>  	populate_efficiency_class();
>  
> 

Re: [PATCH v3 2/2] cppc_cpufreq: Remove HiSilicon CPPC workaround

[Jie Zhan]

On 25/09/2024 14:30, Xiongfeng Wang wrote:
> Reviewed-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> 

Thanks, will pick it up.

Re: [PATCH v3 2/2] cppc_cpufreq: Remove HiSilicon CPPC workaround

[lihuisong (C)]

LGTM,

Reviewed-by: Huisong Li <lihuisong@huawei.com>

在 2024/9/19 16:45, Jie Zhan 写道:
> Since commit 6c8d750f9784 ("cpufreq / cppc: Work around for Hisilicon CPPC
> cpufreq"), we introduce a workround for HiSilicon platforms that do not
> support performance feedback counters, whereas they can get the actual
> frequency from the desired perf register.  Later on, FIE is disabled in
> that workaround as well.
>
> Now the workround can be handled by the common code.  Desired perf would be
> read and converted to frequency if feedback counters don't change.  FIE
> would be disabled if the CPPC regs are in PCC region.
>
> Hence, the workaround is no longer needed and can be safely removed, in an
> effort to consolidate the driver procedure.
>
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> ---
>   drivers/cpufreq/cppc_cpufreq.c | 71 ----------------------------------
>   1 file changed, 71 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index e55192303a9f..0e95ad2303ea 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -36,24 +36,6 @@ static LIST_HEAD(cpu_data_list);
>   
>   static bool boost_supported;
>   
> -struct cppc_workaround_oem_info {
> -	char oem_id[ACPI_OEM_ID_SIZE + 1];
> -	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> -	u32 oem_revision;
> -};
> -
> -static struct cppc_workaround_oem_info wa_info[] = {
> -	{
> -		.oem_id		= "HISI  ",
> -		.oem_table_id	= "HIP07   ",
> -		.oem_revision	= 0,
> -	}, {
> -		.oem_id		= "HISI  ",
> -		.oem_table_id	= "HIP08   ",
> -		.oem_revision	= 0,
> -	}
> -};
> -
>   static struct cpufreq_driver cppc_cpufreq_driver;
>   
>   static enum {
> @@ -78,7 +60,6 @@ struct cppc_freq_invariance {
>   static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
>   static struct kthread_worker *kworker_fie;
>   
> -static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu);
>   static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
>   				 struct cppc_perf_fb_ctrs *fb_ctrs_t0,
>   				 struct cppc_perf_fb_ctrs *fb_ctrs_t1);
> @@ -841,57 +822,6 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
>   	.name = "cppc_cpufreq",
>   };
>   
> -/*
> - * HISI platform does not support delivered performance counter and
> - * reference performance counter. It can calculate the performance using the
> - * platform specific mechanism. We reuse the desired performance register to
> - * store the real performance calculated by the platform.
> - */
> -static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
> -{
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> -	struct cppc_cpudata *cpu_data;
> -	u64 desired_perf;
> -	int ret;
> -
> -	if (!policy)
> -		return -ENODEV;
> -
> -	cpu_data = policy->driver_data;
> -
> -	cpufreq_cpu_put(policy);
> -
> -	ret = cppc_get_desired_perf(cpu, &desired_perf);
> -	if (ret < 0)
> -		return -EIO;
> -
> -	return cppc_perf_to_khz(&cpu_data->perf_caps, desired_perf);
> -}
> -
> -static void cppc_check_hisi_workaround(void)
> -{
> -	struct acpi_table_header *tbl;
> -	acpi_status status = AE_OK;
> -	int i;
> -
> -	status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
> -	if (ACPI_FAILURE(status) || !tbl)
> -		return;
> -
> -	for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> -		if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> -		    !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> -		    wa_info[i].oem_revision == tbl->oem_revision) {
> -			/* Overwrite the get() callback */
> -			cppc_cpufreq_driver.get = hisi_cppc_cpufreq_get_rate;
> -			fie_disabled = FIE_DISABLED;
> -			break;
> -		}
> -	}
> -
> -	acpi_put_table(tbl);
> -}
> -
>   static int __init cppc_cpufreq_init(void)
>   {
>   	int ret;
> @@ -899,7 +829,6 @@ static int __init cppc_cpufreq_init(void)
>   	if (!acpi_cpc_valid())
>   		return -ENODEV;
>   
> -	cppc_check_hisi_workaround();
>   	cppc_freq_invariance_init();
>   	populate_efficiency_class();
>   

Re: [PATCH v3 2/2] cppc_cpufreq: Remove HiSilicon CPPC workaround

[Jie Zhan]

On 25/09/2024 17:36, lihuisong (C) wrote:
> LGTM,
> 
> Reviewed-by: Huisong Li <lihuisong@huawei.com>

Thanks, will pick it up.