Re: [PATCH v2 2/2] cpufreq: CPPC: Dont read counters for idle CPUs

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

On Mon, Aug 04, 2025 at 01:55:37PM -0700, Prashant Malani wrote:
> On Fri, 1 Aug 2025 at 02:16, Prashant Malani <pmalani@google.com> wrote:
> > On Thu, 31 Jul 2025 at 13:31, Beata Michalska <beata.michalska@arm.com> wrote:
> > > Thank you for the info, but I'm exploring ways that will not increase the time
> > > window between the reads.
> >
> > IMO this issue is intractable on non-RT OSes like Linux (at least,
> > Linux when it is not compiled for RT), since we basically need to
> > ensure atomicity for the reading of both ref and del registers together.
> > We can't disable preemption here, since some of
> > the code paths (like PCC regs) acquire semaphores [2].
> 
> Actually, minor correction here. The PCC path is not the issue.
> It's the ffh read path[3], which requires interrupts to be enabled.
> Larger point still stands.
> 
> [3] https://elixir.bootlin.com/linux/v6.16-rc7/source/arch/arm64/kernel/topology.c#L451
> 
> BR,
> 
> -Prashant

Would you mind giving it a go and see whether that improves things on your end ?
Note that this is a quick and semi-dirty hack though.

---
BR
Beata

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 5d07ee85bdae..65adb78a9a87 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -479,6 +479,11 @@ bool cpc_ffh_supported(void)
 	return true;
 }
 
+bool cpc_burst_read_supported(void)
+{
+	return cpc_ffh_supported();
+}
+
 int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
 {
 	int ret = -EOPNOTSUPP;
@@ -501,6 +506,61 @@ int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
 	return ret;
 }
 
+
+struct cpc_burst_read {
+	struct cpc_reg_sample *samples;
+	size_t  count;
+};
+
+void counters_burst_read_on_cpu(void *arg)
+{
+	struct cpc_burst_read *desc = arg;
+	u64 value;
+	int i;
+
+	for (i = 0; i < desc->count; ++i) {
+		switch ((u64)desc->samples[i].reg->address) {
+		case 0x0:
+			value = read_corecnt();
+			break;
+		case 0x1:
+			value = read_constcnt();
+			break;
+		}
+		*(u64 *)desc->samples[i].sample_value = value;
+	}
+
+	for (i = 0; i < desc->count; ++i) {
+		struct cpc_reg *reg = desc->samples[i].reg;
+
+		value = *(u64 *)desc->samples[i].sample_value;
+		value &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
+				     reg->bit_offset);
+		value >>= reg->bit_offset;
+		*(u64 *)desc->samples[i].sample_value = value;
+	}
+}
+
+static inline bool cpc_reg_supported(struct cpc_reg *reg)
+{
+	return !((u64)reg->address != 0x0 || (u64)reg->address != 0x1);
+}
+
+int cpc_burst_read_ffh(int cpu, struct cpc_reg_sample *samples, size_t count)
+{
+	struct cpc_burst_read desc = { samples, count };
+	int ret = -EOPNOTSUPP;
+	int i;
+
+	for (i = 0; i < count; ++i) {
+		if (!cpc_reg_supported(samples[i].reg))
+			return ret;
+	}
+
+	smp_call_function_single(cpu, counters_burst_read_on_cpu, &desc, 1);
+	return 0;
+}
+
 int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
 {
 	return -EOPNOTSUPP;
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 6b649031808f..c070627e4a1e 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -617,6 +617,11 @@ bool __weak cpc_supported_by_cpu(void)
 	return false;
 }
 
+bool __weak cpc_burst_read_supported(void)
+{
+	return false;
+}
+
 /**
  * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
  * @pcc_ss_id: PCC Subspace index as in the PCC client ACPI package.
@@ -1077,6 +1082,21 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 	return 0;
 }
 
+static int cpc_burst_read(int cpu, struct cpc_reg_sample *samples, size_t count)
+{
+	int i;
+
+	// Just for now - only ffh
+	if (!cpc_ffh_supported())
+		return -EINVAL;
+
+	for (i = 0; i < count; ++i)
+		if (samples[i].reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE)
+			return -EINVAL;
+	return cpc_burst_read_ffh(cpu, samples, count);
+}
+
+
 static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
 {
 	int ret_val = 0;
@@ -1515,8 +1535,21 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 		}
 	}
 
-	cpc_read(cpunum, delivered_reg, &delivered);
-	cpc_read(cpunum, reference_reg, &reference);
+	// Verify whether the registers can be requested in one go
+	if (delivered_reg->type != ACPI_TYPE_INTEGER &&
+	    reference_reg->type != ACPI_TYPE_INTEGER &&
+	    cpc_burst_read_supported()) {
+
+		struct cpc_reg_sample samples[] = {
+			{ &delivered_reg->cpc_entry.reg, &delivered },
+			{ &reference_reg->cpc_entry.reg, &reference }
+		};
+
+		cpc_burst_read(cpunum, samples, ARRAY_SIZE(samples));
+	} else {
+		cpc_read(cpunum, delivered_reg, &delivered);
+		cpc_read(cpunum, reference_reg, &reference);
+	}
 	cpc_read(cpunum, ref_perf_reg, &ref_perf);
 
 	/*
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 325e9543e08f..f094e275834a 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -52,6 +52,12 @@ struct cpc_reg {
 	u64 address;
 } __packed;
 
+
+struct cpc_reg_sample {
+	struct cpc_reg *reg;
+	void *sample_value;
+};
+
 /*
  * Each entry in the CPC table is either
  * of type ACPI_TYPE_BUFFER or
@@ -165,6 +171,7 @@ extern unsigned int cppc_get_transition_latency(int cpu);
 extern bool cpc_ffh_supported(void);
 extern bool cpc_supported_by_cpu(void);
 extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
+extern int cpc_burst_read_ffh(int cpunum, struct cpc_reg_sample *samples, size_t count);
 extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
 extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf);
 extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
@@ -229,6 +236,10 @@ static inline int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val)
 {
 	return -EOPNOTSUPP;
 }
+static inline int cpc_burst_read_ffh(int cpunum, struct cpc_reg_sample *samples, size_t count)
+{
+	return -EOPNOTSUPP;
+}
 static inline int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
 {
 	return -EOPNOTSUPP;
--