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

[Beata Michalska <beata.michalska@arm.com>]

On Thu, Jul 09, 2026 at 01:11:13AM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 7/8/26 3:28 AM, 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.
> > 
> > Tested-by: Sumit Gupta <sumitg@nvidia.com>
> > Reviewed-by: Sumit Gupta <sumitg@nvidia.com>
> > Tested-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> > Reviewed-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> > Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
> > ---
> >   arch/arm64/kernel/topology.c | 92 ++++++++++++++++++++++++++++++++----
> >   1 file changed, 84 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index b32f13358fbb..d28438f8b83f 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -373,6 +373,16 @@ core_initcall(init_amu_fie);
> >   #ifdef CONFIG_ACPI_CPPC_LIB
> >   #include <acpi/cppc_acpi.h>
> > +struct amu_ffh_ctrs {
> > +	u64 corecnt;
> > +	u64 constcnt;
> > +};
> > +
> > +enum cpc_ffh_ctr_id {
> > +	CPC_FFH_CTR_CORE  = 0x0,
> > +	CPC_FFH_CTR_CONST = 0x1,
> > +};
> > +
> >   static void cpu_read_corecnt(void *val)
> >   {
> >   	/*
> > @@ -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,90 @@ 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() incurs slight latency due to the
> > +	 * ARM64_WORKAROUND_2457168 check. Read it first to minimize
> > +	 * the sampling skew between the const and core counters.
> > +	 */
> > +	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 void 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;
> > +	}
> > +
> > +	*val = cpc_ffh_extract_bits(reg, *val);
> > +}
> > +
> > +static bool is_amu_ctr_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_amu_ctr_reg(reg1) || !is_amu_ctr_reg(reg2))
> > +		return -EINVAL;
> > +
> > +	ret = counters_read_on_cpu(cpu, amu_read_core_const_ctrs, &ctrs);
> > +	if (ret) {
> > +		/*
> > +		 * If AMU is unsupported (-EOPNOTSUPP), translate the error
> > +		 * to -ENODEV. This explicitly tells the generic CPPC layer
> > +		 * to abort immediately and avoid falling back to pointless
> > +		 * single-counter reads.
> > +		 */
> > +		return ret == -EOPNOTSUPP ? -ENODEV : ret;
> > +	}
> > +
> > +	cpc_ffh_ctr_value(reg1, &ctrs, val1);
> > +	cpc_ffh_ctr_value(reg2, &ctrs, val2);
> > +
> > +	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;
> >   }
> 
> 
> So, more a nitpik that only applies if this set gets respun, but:
> 
> I don't think this FFH counter logic belongs in the arm64 topology.c file,
> its not really topology related.

I agree. Those bits should be moved.
That said, I think it would be best to land this first,
as it addresses a rather long-standing issue.
We can do the cleanup later. If needed, I'd pick it up soon'ish.

---
BR
Beata
> 
> 
> 
> 
>