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

[Pengjie Zhang]

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@xxxxxxxxxx>
> > ---
> >   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