+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
