Re: [PATCH v2 1/2] cpufreq: CPPC: keep target core awake when reading its cpufreq rate

[Sumit Gupta]

> > > +Ionela, Sumit, Yang,
> > >
> > > Hello Zeng,
> > >
> > > I think solutions around related issues were suggested at:
> > >
> > > [1]
> > > https://lore.kernel.org/all/20230418113459.12860-7-sumitg@xxxxxxxxxx/
> > > [2]
> > > https://lore.kernel.org/all/20230328193846.8757-1-yang@xxxxxxxxxxxxxxxxxxxxxx/
> > > [3] https://lore.kernel.org/all/ZEl1Fms%2FJmdEZsVn@xxxxxxx/
> > >
> > > About this patch, it seems to mean that CPPC counters of CPUx are always
> > > accessed from CPUx, even when they are not AMUs. For instance CPPC
> > > counters could be memory mapped and accessible from any CPU.
> > > cpu_has_amu_feat() should allow to probe if a CPU uses AMUs or not,
> > > and [2] had an implementation using it.
> > >
> > > Another comment about PATCH 2/2 is that if the counters are accessed
> > > through FFH, arm64 version of cpc_read_ffh() is calling
> > > counters_read_on_cpu(), and a comment in counters_read_on_cpu() seems
> > > to specify the function must be called with interrupt enabled.
> > >
> > > I think the best solution so far was the one at [3], suggested by
> > > Ionela,
> > > but it doesn't seem to solve your issue. Indeed, it is not checked
> > > whether
> > > the counters are AMU counters and that they must be remotely read (to
> > > have the CPU awake),
> > >
> > > Regards,
> > > Pierre
> >
> > I think the solution in [1] is simple and solves all the three cases.
> > Also, it provides better accuracy between the set and get frequency as
> > compared to [3].
>
> I don't think [1] patches work for our case. We use mmio instead of AMU.
> Increasing delay could help to mitigate it somehow, buyt 25us is not
> good enough for our case. IIRC the fix proposed by Ionela works for both
> yours and mine.

I have added the CPC_IN_SYSTEM_MEMORY check from [2] in [1].
Could you please test if the below change works for you.

-----------------------------------------
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 7ff269a78c20..67aa09b5f15c 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1315,6 +1315,7 @@ int cppc_get_perf_ctrs(int cpunum, struct 
cppc_perf_fb_ctrs *perf_fb_ctrs)
 	struct cppc_pcc_data *pcc_ss_data = NULL;
 	u64 delivered, reference, ref_perf, ctr_wrap_time;
 	int ret = 0, regs_in_pcc = 0;
+	unsigned long flags;

 	if (!cpc_desc) {
 		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
@@ -1350,8 +1351,17 @@ int cppc_get_perf_ctrs(int cpunum, struct 
cppc_perf_fb_ctrs *perf_fb_ctrs)
 		}
 	}

+	if (CPC_IN_SYSTEM_MEMORY(delivered_reg) &&
+	    CPC_IN_SYSTEM_MEMORY(reference_reg))
+		local_irq_save(flags);
+
 	cpc_read(cpunum, delivered_reg, &delivered);
 	cpc_read(cpunum, reference_reg, &reference);
+
+	if (CPC_IN_SYSTEM_MEMORY(delivered_reg) &&
+	    CPC_IN_SYSTEM_MEMORY(reference_reg))
+		local_irq_restore(flags);
+
 	cpc_read(cpunum, ref_perf_reg, &ref_perf);

 	/*
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 5e6a132a525e..23e690854459 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -46,6 +46,8 @@ static bool boost_supported;
 /* default 2usec delay between sampling */
 static unsigned int sampling_delay_us = 2;

+static bool get_rate_use_wq;
+
 static void cppc_check_hisi_workaround(void);
 static void cppc_nvidia_workaround(void);

@@ -99,6 +101,12 @@ struct cppc_freq_invariance {
 static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
 static struct kthread_worker *kworker_fie;

+struct feedback_ctrs {
+	u32 cpu;
+	struct cppc_perf_fb_ctrs fb_ctrs_t0;
+	struct cppc_perf_fb_ctrs fb_ctrs_t1;
+};
+
 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,
@@ -851,28 +859,44 @@ static int cppc_perf_from_fbctrs(struct 
cppc_cpudata *cpu_data,
 	return (reference_perf * delta_delivered) / delta_reference;
 }

+static int cppc_get_perf_ctrs_sync(void *fb_ctrs)
+{
+	struct feedback_ctrs *ctrs = fb_ctrs;
+	int ret;
+
+	ret = cppc_get_perf_ctrs(ctrs->cpu, &(ctrs->fb_ctrs_t0));
+	if (ret)
+		return ret;
+
+	udelay(sampling_delay_us);
+
+	ret = cppc_get_perf_ctrs(ctrs->cpu, &(ctrs->fb_ctrs_t1));
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
 static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 {
-	struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
 	struct cppc_cpudata *cpu_data = policy->driver_data;
+	struct feedback_ctrs fb_ctrs = {0};
 	u64 delivered_perf;
 	int ret;

 	cpufreq_cpu_put(policy);
+	fb_ctrs.cpu = cpu;

-	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
-	if (ret)
-		return ret;
-
-	udelay(sampling_delay_us);
-
-	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
+	if (get_rate_use_wq)
+		ret = smp_call_on_cpu(cpu, cppc_get_perf_ctrs_sync, &fb_ctrs, false);
+	else
+		ret = cppc_get_perf_ctrs_sync(&fb_ctrs);
 	if (ret)
 		return ret;

-	delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
-					       &fb_ctrs_t1);
+	delivered_perf = cppc_perf_from_fbctrs(cpu_data, &(fb_ctrs.fb_ctrs_t0),
+					       &(fb_ctrs.fb_ctrs_t1));

 	return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
 }
@@ -1002,10 +1026,18 @@ static void cppc_apply_workarounds(void)
 static int __init cppc_cpufreq_init(void)
 {
 	int ret;
+	int cpu;

 	if (!acpi_cpc_valid())
 		return -ENODEV;

+#ifdef CONFIG_ARM64_AMU_EXTN
+	cpu = get_cpu_with_amu_feat();
+
+	if (cpu < nr_cpu_ids)
+		get_rate_use_wq = true;
+#endif

--------------------------------------------

We can add additional check to call smp_call_on_cpu() only when 
CPC_IN_FFH if we want to reduce the scope of calling smp_call_on_cpu.

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 67aa09b5f15c..3d8348911403 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -110,6 +110,11 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
                                (cpc)->cpc_entry.reg.space_id ==        \
                                ACPI_ADR_SPACE_SYSTEM_IO)

+/* Check if a CPC register is in FFH */
+#define CPC_IN_FFH(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&    \
+                               (cpc)->cpc_entry.reg.space_id ==        \
+                               ACPI_ADR_SPACE_FIXED_HARDWARE)
+
 /* Evaluates to True if reg is a NULL register descriptor */
 #define IS_NULL_REG(reg) ((reg)->space_id == 
ACPI_ADR_SPACE_SYSTEM_MEMORY && \
                                (reg)->address == 0 &&                  \
@@ -437,6 +442,29 @@ bool acpi_cpc_valid(void)
 }
 EXPORT_SYMBOL_GPL(acpi_cpc_valid);

+bool acpi_cpc_in_ffh(void)
+{
+       struct cpc_register_resource *delivered_reg, *reference_reg;
+       struct cpc_desc *cpc_ptr;
+       int cpu;
+
+       if (acpi_disabled)
+               return false;
+
+       for_each_possible_cpu(cpu) {
+               cpc_ptr = per_cpu(cpc_desc_ptr, cpu);
+               delivered_reg = &cpc_ptr->cpc_regs[DELIVERED_CTR];
+               reference_reg = &cpc_ptr->cpc_regs[REFERENCE_CTR];
+
+               if (!CPC_IN_FFH(delivered_reg) ||
+                   !CPC_IN_FFH(reference_reg))
+                       return false;
+       }
+
+       return true;
+}
+EXPORT_SYMBOL_GPL(acpi_cpc_in_ffh);
+
 bool cppc_allow_fast_switch(void)
 {
        struct cpc_register_resource *desired_reg;
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 23e690854459..4109e00b957e 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -1034,7 +1034,7 @@ static int __init cppc_cpufreq_init(void)
 #ifdef CONFIG_ARM64_AMU_EXTN
        cpu = get_cpu_with_amu_feat();

-       if (cpu < nr_cpu_ids)
+       if ((cpu < nr_cpu_ids) && acpi_cpc_in_ffh())
                get_rate_use_wq = true;
 #endif

> > This can be merged and can later still be improved in Upstream.
> >
> > If OK, I can send new version by changing the patch to apply for all
> > ARM SoC's with AMU and not specific to Tegra.
> >
> > Thank you,
> > Sumit Gupta