Re: [PATCH 2/2] arm64: topology: read CPPC FFH feedback counters in one operation

[Pengjie Zhang <zhangpengjie2@huawei.com>]

On 6/30/2026 3:34 PM, Beata Michalska wrote:
> On Fri, Apr 10, 2026 at 05:41:45PM +0800, Pengjie Zhang wrote:
>> arm64 implements CPPC FFH feedback-counter reads using AMU counters.
>> Because those counters must be sampled on the target CPU, reading the
>> delivered and reference counters separately widens the observation window
>> between them.
>>
>> Implement the paired FFH feedback-counter read hook on arm64 and sample
>> both AMU counters together before decoding the requested CPC register
>> values.
>>
>> Also factor the FFH bitfield extraction logic into a helper and reuse
>> it from the existing single-counter FFH read path.
>>
>> Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
>> ---
>>   arch/arm64/kernel/topology.c | 75 ++++++++++++++++++++++++++++++++----
>>   1 file changed, 67 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index b32f13358fbb..b90a767b2a1f 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -50,6 +50,16 @@ struct amu_cntr_sample {
>>   	unsigned long	last_scale_update;
>>   };
>>   
>> +struct amu_ffh_ctrs {
>> +	u64 corecnt;
>> +	u64 constcnt;
>> +};
>> +
>> +enum cpc_ffh_ctr_id {
>> +	CPC_FFH_CTR_CORE  = 0x0,
>> +	CPC_FFH_CTR_CONST = 0x1,
>> +};
>> +
> Those should probably go under the #ifdef CONFIG_ACPI_CPPC_LIB section.

yes

>>   static DEFINE_PER_CPU_SHARED_ALIGNED(struct amu_cntr_sample, cpu_amu_samples);
>>   
>>   void update_freq_counters_refs(void)
>> @@ -397,7 +407,7 @@ static void cpu_read_constcnt(void *val)
>>   }
>>   
>>   static inline
>> -int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val)
>> +int counters_read_on_cpu(int cpu, smp_call_func_t func, void *val)
>>   {
>>   	/*
>>   	 * Abort call on counterless CPU.
>> @@ -447,24 +457,73 @@ bool cpc_ffh_supported(void)
>>   	return true;
>>   }
>>   
>> +static void amu_read_core_const_ctrs(void *val)
>> +{
>> +	struct amu_ffh_ctrs *ctrs = val;
>> +
>> +	cpu_read_constcnt(&ctrs->constcnt);
>> +	cpu_read_corecnt(&ctrs->corecnt);
>> +}
>> +
>> +static u64 cpc_ffh_extract_bits(const struct cpc_reg *reg, u64 val)
>> +{
>> +	val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
>> +			   reg->bit_offset);
>> +	val >>= reg->bit_offset;
>> +
>> +	return val;
>> +}
>> +
>> +static bool cpc_ffh_ctr_value(const struct cpc_reg *reg,
>> +			      const struct amu_ffh_ctrs *ctrs, u64 *val)
>> +{
>> +	switch ((u64)reg->address) {
>> +	case CPC_FFH_CTR_CORE:
>> +		*val = ctrs->corecnt;
>> +		break;
>> +	case CPC_FFH_CTR_CONST:
>> +		*val = ctrs->constcnt;
>> +		break;
>> +	default:
>> +		return false;
>> +	}
>> +
>> +	*val = cpc_ffh_extract_bits(reg, *val);
>> +	return true;
>> +}
>> +
>> +int cpc_read_ffh_fb_ctrs(int cpu, struct cpc_reg *reg1, u64 *val1,
>> +			 struct cpc_reg *reg2, u64 *val2)
>> +{
>> +	struct amu_ffh_ctrs ctrs;
>> +	int ret;
>> +
>> +	ret = counters_read_on_cpu(cpu, amu_read_core_const_ctrs, &ctrs);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!cpc_ffh_ctr_value(reg1, &ctrs, val1) ||
>> +	    !cpc_ffh_ctr_value(reg2, &ctrs, val2))
>> +		return -EOPNOTSUPP;
> Right, so there might be an issues with that:
> If you return EOPNOTSUPP here, that would trigger reading the registers again,
> this time one by one. Is that intentional ?
> Also counters_read_on_cpu might also return EOPNOTSUPP, in which case trying
> again to read the counters is pointless.
>
> I'm not entirely sure I understand the condition itself though.
> This will fail if either of the requested registers in not really expected.
> And that should probably be verified upfront ?
>
>
> ---
> BR
> Beata

Right. The following code may be more appropriate.

static bool is_valid_cpc_ffh_reg(const struct cpc_reg *reg)
{
     return reg->address == CPC_FFH_CTR_CORE ||
         reg->address == CPC_FFH_CTR_CONST;
}

int cpc_read_ffh_fb_ctrs(int cpu, struct cpc_reg *reg1, u64 *val1,
              struct cpc_reg *reg2, u64 *val2)
{
     struct amu_ffh_ctrs ctrs;
     int ret;

     if (!is_valid_cpc_ffh_reg(reg1) || !is_valid_cpc_ffh_reg(reg2))
         return -EINVAL;

     ret = counters_read_on_cpu(cpu, amu_read_core_const_ctrs, &ctrs);
     if (ret)
         return ret == -EOPNOTSUPP ? -ENODEV : ret;

     cpc_ffh_ctr_value(reg1, &ctrs, val1);
     cpc_ffh_ctr_value(reg2, &ctrs, val2);

     return 0;
}

thanks,

    Pengjie

>> +
>> +	return 0;
>> +}
>> +
>>   int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
>>   {
>>   	int ret = -EOPNOTSUPP;
>>   
>>   	switch ((u64)reg->address) {
>> -	case 0x0:
>> +	case CPC_FFH_CTR_CORE:
>>   		ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
>>   		break;
>> -	case 0x1:
>> +	case CPC_FFH_CTR_CONST:
>>   		ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
>>   		break;
>>   	}
>>   
>> -	if (!ret) {
>> -		*val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
>> -				    reg->bit_offset);
>> -		*val >>= reg->bit_offset;
>> -	}
>> +	if (!ret)
>> +		*val = cpc_ffh_extract_bits(reg, *val);
>>   
>>   	return ret;
>>   }
>> -- 
>> 2.33.0
>>