This patch series contains improvements and changes needed to get a
stable value of current CPU frequency from "cpuinfo_cur_freq" sysfs
node in Tegra241 SoC. All the CPU's in Tegra241 implement the ARM
Activity Monitor Unit (AMU). On reading the sysfs node, the frequency
value is re-constructed using AMU counters.
Sanjay Chandrashekara (1):
cpufreq: use correct unit when verify cur freq
Sumit Gupta (5):
cpufreq: CPPC: make workaround apply code generic
irqchip/gicv3: Export arm_smccc_get_soc_id_xx funcs
cpufreq: CPPC: update sampling window for Tegra241
arm64: cpufeature: Export get_cpu_with_amu_feat func
cpufreq: CPPC: use wq to read amu counters on target cpu
arch/arm64/kernel/cpufeature.c | 1 +
drivers/cpufreq/cppc_cpufreq.c | 99 +++++++++++++++++++++++++++++-----
drivers/cpufreq/cpufreq.c | 2 +-
drivers/firmware/smccc/smccc.c | 2 +
4 files changed, 89 insertions(+), 15 deletions(-)
--
2.17.1
Export arm_smccc_get_soc_id_version() function which is needed
in CPPC_CPUFREQ to check and apply workaround for Tegra241 SoC.
Also, exporting arm_smccc_get_soc_id_revision() function as it
might be needed in future.
Signed-off-by: Sumit Gupta <[email protected]>
---
drivers/firmware/smccc/smccc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
index db818f9dcb8e..b4224da46988 100644
--- a/drivers/firmware/smccc/smccc.c
+++ b/drivers/firmware/smccc/smccc.c
@@ -64,11 +64,13 @@ s32 arm_smccc_get_soc_id_version(void)
{
return smccc_soc_id_version;
}
+EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_version);
s32 arm_smccc_get_soc_id_revision(void)
{
return smccc_soc_id_revision;
}
+EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision);
static int __init smccc_devices_init(void)
{
--
2.17.1
In Tegra241, the Activity Monitor Unit's (AMU) constant counter
(i.e. reference clock counter) increment happens in bursts and
not incremented in the steps of one. For example reference counter
may increment by '0x20' every '32' periods of ARM periphclk. This
quantization of the reference counter is a source of error when
reconstructing the frequency from the AMU counter data. To fix,
increase the observation time interval so the error percentage
becomes less.
Signed-off-by: Sumit Gupta <[email protected]>
---
drivers/cpufreq/cppc_cpufreq.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 15c2cbb7a50e..5e6a132a525e 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -43,12 +43,17 @@ static LIST_HEAD(cpu_data_list);
static bool boost_supported;
+/* default 2usec delay between sampling */
+static unsigned int sampling_delay_us = 2;
+
static void cppc_check_hisi_workaround(void);
+static void cppc_nvidia_workaround(void);
struct cppc_workaround_oem_info {
char oem_id[ACPI_OEM_ID_SIZE + 1];
char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
u32 oem_revision;
+ u32 smcc_soc_id;
void (*apply_wa_func)(void);
};
@@ -63,6 +68,10 @@ static struct cppc_workaround_oem_info wa_info[] = {
.oem_table_id = "HIP08 ",
.oem_revision = 0,
.apply_wa_func = cppc_check_hisi_workaround,
+ }, {
+ .oem_id = "NVIDIA",
+ .smcc_soc_id = 0x036b0241, /* JEP106 code for NVIDIA T241 chip (036b:0241) */
+ .apply_wa_func = cppc_nvidia_workaround,
}
};
@@ -856,7 +865,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
if (ret)
return ret;
- udelay(2); /* 2usec delay between sampling */
+ udelay(sampling_delay_us);
ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
if (ret)
@@ -942,6 +951,11 @@ static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
return cppc_cpufreq_perf_to_khz(cpu_data, desired_perf);
}
+static void cppc_nvidia_workaround(void)
+{
+ sampling_delay_us = 25;
+}
+
static void cppc_check_hisi_workaround(void)
{
/* Overwrite the get() callback */
@@ -953,8 +967,21 @@ static void cppc_apply_workarounds(void)
{
struct acpi_table_header *tbl;
acpi_status status = AE_OK;
+ s32 soc_id;
int i;
+#ifdef CONFIG_HAVE_ARM_SMCCC_DISCOVERY
+ for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
+ if (wa_info[i].smcc_soc_id) {
+ soc_id = arm_smccc_get_soc_id_version();
+ if (wa_info[i].smcc_soc_id == soc_id) {
+ wa_info[i].apply_wa_func();
+ return;
+ }
+ }
+ }
+#endif
+
status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
if (ACPI_FAILURE(status) || !tbl)
return;
--
2.17.1
From: Sanjay Chandrashekara <[email protected]>
cpufreq_verify_current_freq checks if the frequency returned by
the hardware has a slight delta with the valid frequency value
last set and returns "policy->cur" if the delta is within "1 MHz".
In the comparison, "policy->cur" is in "kHz" but it's compared
against HZ_PER_MHZ. So, the comparison range becomes "1 GHz".
Fix this by comparing against KHZ_PER_MHZ instead of HZ_PER_MHZ.
Fixes: f55ae08c8987 ("cpufreq: Avoid unnecessary frequency updates due to mismatch")
Signed-off-by: Sanjay Chandrashekara <[email protected]>
[ sumit gupta: Commit message update ]
Signed-off-by: Sumit Gupta <[email protected]>
---
drivers/cpufreq/cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8b0509f89f1b..6b52ebe5a890 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1732,7 +1732,7 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
* MHz. In such cases it is better to avoid getting into
* unnecessary frequency updates.
*/
- if (abs(policy->cur - new_freq) < HZ_PER_MHZ)
+ if (abs(policy->cur - new_freq) < KHZ_PER_MHZ)
return policy->cur;
cpufreq_out_of_sync(policy, new_freq);
--
2.17.1
Export the get_cpu_with_amu_feat() function for use by
"cppc_cpufreq" to check if any CPU implements ARM's
Activity Monitor Unit (AMU). If AMU is available, then
for re-constructing the current CPU freq from its
counters, queue work on target CPU to read the counters
in a single call instead of reading them in separate
smp calls.
Signed-off-by: Sumit Gupta <[email protected]>
---
arch/arm64/kernel/cpufeature.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index d9345e9c0226..ec31da7043eb 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1917,6 +1917,7 @@ int get_cpu_with_amu_feat(void)
{
return cpumask_any(&amu_cpus);
}
+EXPORT_SYMBOL_GPL(get_cpu_with_amu_feat);
static void cpu_amu_enable(struct arm64_cpu_capabilities const *cap)
{
--
2.17.1
ARM cores which implement the Activity Monitor Unit (AMU)
use Functional Fixed Hardware (FFH) to map AMU counters to
Delivered_Counter and Reference_Counter registers. Each
sysreg is read separately with a smp_call_function_single
call. So, total four IPI's are used, one per register.
Due to this, the AMU's core counter and constant counter
sampling can happen at a non-consistent time interval if
an IPI is handled late. This results in unstable frequency
value from "cpuinfo_cur_req" node sometimes. To fix, queue
work on target CPU to read all counters synchronously in
sequence. This helps to remove the inter-IPI latency and
make sure that both the counters are sampled at a close
time interval.
Without this change we observed that the re-generated value
of CPU Frequency from AMU counters sometimes deviates by
~25% as the counters are read at non-determenistic time.
Currently, kept the change specific to Tegra241. It can be
applied to other SoC's having AMU if same issue is observed.
Signed-off-by: Sumit Gupta <[email protected]>
---
drivers/cpufreq/cppc_cpufreq.c | 53 +++++++++++++++++++++++++++-------
1 file changed, 43 insertions(+), 10 deletions(-)
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 5e6a132a525e..52b93ac6225e 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);
}
@@ -953,7 +977,16 @@ static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
static void cppc_nvidia_workaround(void)
{
+ int cpu;
+
sampling_delay_us = 25;
+
+#ifdef CONFIG_ARM64_AMU_EXTN
+ cpu = get_cpu_with_amu_feat();
+
+ if (cpu < nr_cpu_ids)
+ get_rate_use_wq = true;
+#endif
}
static void cppc_check_hisi_workaround(void)
--
2.17.1
On Tue, Apr 18, 2023 at 1:35 PM Sumit Gupta <[email protected]> wrote:
>
> From: Sanjay Chandrashekara <[email protected]>
>
> cpufreq_verify_current_freq checks if the frequency returned by
> the hardware has a slight delta with the valid frequency value
> last set and returns "policy->cur" if the delta is within "1 MHz".
> In the comparison, "policy->cur" is in "kHz" but it's compared
> against HZ_PER_MHZ. So, the comparison range becomes "1 GHz".
> Fix this by comparing against KHZ_PER_MHZ instead of HZ_PER_MHZ.
>
> Fixes: f55ae08c8987 ("cpufreq: Avoid unnecessary frequency updates due to mismatch")
> Signed-off-by: Sanjay Chandrashekara <[email protected]>
> [ sumit gupta: Commit message update ]
> Signed-off-by: Sumit Gupta <[email protected]>
> ---
> drivers/cpufreq/cpufreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 8b0509f89f1b..6b52ebe5a890 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1732,7 +1732,7 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
> * MHz. In such cases it is better to avoid getting into
> * unnecessary frequency updates.
> */
> - if (abs(policy->cur - new_freq) < HZ_PER_MHZ)
> + if (abs(policy->cur - new_freq) < KHZ_PER_MHZ)
> return policy->cur;
>
> cpufreq_out_of_sync(policy, new_freq);
> --
So this is a fix that can be applied separately from the rest of the
series, isn't it?
On 18/04/23 18:27, Rafael J. Wysocki wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, Apr 18, 2023 at 1:35 PM Sumit Gupta <[email protected]> wrote:
>>
>> From: Sanjay Chandrashekara <[email protected]>
>>
>> cpufreq_verify_current_freq checks if the frequency returned by
>> the hardware has a slight delta with the valid frequency value
>> last set and returns "policy->cur" if the delta is within "1 MHz".
>> In the comparison, "policy->cur" is in "kHz" but it's compared
>> against HZ_PER_MHZ. So, the comparison range becomes "1 GHz".
>> Fix this by comparing against KHZ_PER_MHZ instead of HZ_PER_MHZ.
>>
>> Fixes: f55ae08c8987 ("cpufreq: Avoid unnecessary frequency updates due to mismatch")
>> Signed-off-by: Sanjay Chandrashekara <[email protected]>
>> [ sumit gupta: Commit message update ]
>> Signed-off-by: Sumit Gupta <[email protected]>
>> ---
>> drivers/cpufreq/cpufreq.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 8b0509f89f1b..6b52ebe5a890 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1732,7 +1732,7 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
>> * MHz. In such cases it is better to avoid getting into
>> * unnecessary frequency updates.
>> */
>> - if (abs(policy->cur - new_freq) < HZ_PER_MHZ)
>> + if (abs(policy->cur - new_freq) < KHZ_PER_MHZ)
>> return policy->cur;
>>
>> cpufreq_out_of_sync(policy, new_freq);
>> --
>
> So this is a fix that can be applied separately from the rest of the
> series, isn't it?
Yes.
Thank you,
Sumit Gupta
On Tue, Apr 18, 2023 at 3:32 PM Sumit Gupta <[email protected]> wrote:
>
>
>
> On 18/04/23 18:27, Rafael J. Wysocki wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, Apr 18, 2023 at 1:35 PM Sumit Gupta <[email protected]> wrote:
> >>
> >> From: Sanjay Chandrashekara <[email protected]>
> >>
> >> cpufreq_verify_current_freq checks if the frequency returned by
> >> the hardware has a slight delta with the valid frequency value
> >> last set and returns "policy->cur" if the delta is within "1 MHz".
> >> In the comparison, "policy->cur" is in "kHz" but it's compared
> >> against HZ_PER_MHZ. So, the comparison range becomes "1 GHz".
> >> Fix this by comparing against KHZ_PER_MHZ instead of HZ_PER_MHZ.
> >>
> >> Fixes: f55ae08c8987 ("cpufreq: Avoid unnecessary frequency updates due to mismatch")
> >> Signed-off-by: Sanjay Chandrashekara <[email protected]>
> >> [ sumit gupta: Commit message update ]
> >> Signed-off-by: Sumit Gupta <[email protected]>
> >> ---
> >> drivers/cpufreq/cpufreq.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >> index 8b0509f89f1b..6b52ebe5a890 100644
> >> --- a/drivers/cpufreq/cpufreq.c
> >> +++ b/drivers/cpufreq/cpufreq.c
> >> @@ -1732,7 +1732,7 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
> >> * MHz. In such cases it is better to avoid getting into
> >> * unnecessary frequency updates.
> >> */
> >> - if (abs(policy->cur - new_freq) < HZ_PER_MHZ)
> >> + if (abs(policy->cur - new_freq) < KHZ_PER_MHZ)
> >> return policy->cur;
> >>
> >> cpufreq_out_of_sync(policy, new_freq);
> >> --
> >
> > So this is a fix that can be applied separately from the rest of the
> > series, isn't it?
>
> Yes.
So applied as 6.4 material.
Hi Sumit,
Thank you for the patches!
On Tuesday 18 Apr 2023 at 17:04:59 (+0530), Sumit Gupta wrote:
> ARM cores which implement the Activity Monitor Unit (AMU)
> use Functional Fixed Hardware (FFH) to map AMU counters to
> Delivered_Counter and Reference_Counter registers. Each
> sysreg is read separately with a smp_call_function_single
> call. So, total four IPI's are used, one per register.
> Due to this, the AMU's core counter and constant counter
> sampling can happen at a non-consistent time interval if
> an IPI is handled late. This results in unstable frequency
> value from "cpuinfo_cur_req" node sometimes. To fix, queue
> work on target CPU to read all counters synchronously in
> sequence. This helps to remove the inter-IPI latency and
> make sure that both the counters are sampled at a close
> time interval.
> Without this change we observed that the re-generated value
> of CPU Frequency from AMU counters sometimes deviates by
> ~25% as the counters are read at non-determenistic time.
> Currently, kept the change specific to Tegra241. It can be
> applied to other SoC's having AMU if same issue is observed.
>
To be honest I never liked the need for IPIs but it was the most
generic solution I could find for an FFH implementation that does not
assume a dependency between different reads, which is usecase specific.
Also, for any kind of caching of the counters I'd have to introduce some
logic that would assume we'd always have two consecutive reads - one for
the cycle counter and one for the constant counter, and there should be no
update between them. And then there's the problem of potentially returning
the same values if there's no update between two sets of reads.
The only feasible idea based on caching would be to piggy back on the
frequency invariance engine (FIE) which computes a performance scale
factor on the tick which can be translated to frequency. But the
frequency obtained would be an average frequency for the past 4ms, which
can in turn be at up to 4ms old (or more, if the CPU was idle).
Would something like this work for you?
This could also help with a similar issue described at [1] - not an IPI
related issue, but an issue with similar symptoms.
[1] https://lore.kernel.org/lkml/[email protected]/
Thanks,
Ionela.
> Signed-off-by: Sumit Gupta <[email protected]>
> ---
> drivers/cpufreq/cppc_cpufreq.c | 53 +++++++++++++++++++++++++++-------
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 5e6a132a525e..52b93ac6225e 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);
> }
> @@ -953,7 +977,16 @@ static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
>
> static void cppc_nvidia_workaround(void)
> {
> + int cpu;
> +
> sampling_delay_us = 25;
> +
> +#ifdef CONFIG_ARM64_AMU_EXTN
> + cpu = get_cpu_with_amu_feat();
> +
> + if (cpu < nr_cpu_ids)
> + get_rate_use_wq = true;
> +#endif
> }
>
> static void cppc_check_hisi_workaround(void)
> --
> 2.17.1
>
Hi Ionela,
Thankyou for the inputs.
On 24/04/23 14:02, Ionela Voinescu wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Sumit,
>
> Thank you for the patches!
>
> On Tuesday 18 Apr 2023 at 17:04:59 (+0530), Sumit Gupta wrote:
>> ARM cores which implement the Activity Monitor Unit (AMU)
>> use Functional Fixed Hardware (FFH) to map AMU counters to
>> Delivered_Counter and Reference_Counter registers. Each
>> sysreg is read separately with a smp_call_function_single
>> call. So, total four IPI's are used, one per register.
>> Due to this, the AMU's core counter and constant counter
>> sampling can happen at a non-consistent time interval if
>> an IPI is handled late. This results in unstable frequency
>> value from "cpuinfo_cur_req" node sometimes. To fix, queue
>> work on target CPU to read all counters synchronously in
>> sequence. This helps to remove the inter-IPI latency and
>> make sure that both the counters are sampled at a close
>> time interval.
>> Without this change we observed that the re-generated value
>> of CPU Frequency from AMU counters sometimes deviates by
>> ~25% as the counters are read at non-determenistic time.
>> Currently, kept the change specific to Tegra241. It can be
>> applied to other SoC's having AMU if same issue is observed.
>>
>
> To be honest I never liked the need for IPIs but it was the most
> generic solution I could find for an FFH implementation that does not
> assume a dependency between different reads, which is usecase specific.
>
> Also, for any kind of caching of the counters I'd have to introduce some
> logic that would assume we'd always have two consecutive reads - one for
> the cycle counter and one for the constant counter, and there should be no
> update between them. And then there's the problem of potentially returning
> the same values if there's no update between two sets of reads.
>
> The only feasible idea based on caching would be to piggy back on the
> frequency invariance engine (FIE) which computes a performance scale
> factor on the tick which can be translated to frequency. But the
> frequency obtained would be an average frequency for the past 4ms, which
> can in turn be at up to 4ms old (or more, if the CPU was idle).
>
> Would something like this work for you?
>
> This could also help with a similar issue described at [1] - not an IPI
> related issue, but an issue with similar symptoms.
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> Thanks,
> Ionela.
>
I think yes that will help as it will increase the time period and also
remove the IPI's ?
One thing I am not sure is whether there can be any impact when CPU is
IDLE w.r.t. the delta between the frequency set and get from the counters.
Thank you,
Sumit Gupta
>> Signed-off-by: Sumit Gupta <[email protected]>
>> ---
>> drivers/cpufreq/cppc_cpufreq.c | 53 +++++++++++++++++++++++++++-------
>> 1 file changed, 43 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 5e6a132a525e..52b93ac6225e 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);
>> }
>> @@ -953,7 +977,16 @@ static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu)
>>
>> static void cppc_nvidia_workaround(void)
>> {
>> + int cpu;
>> +
>> sampling_delay_us = 25;
>> +
>> +#ifdef CONFIG_ARM64_AMU_EXTN
>> + cpu = get_cpu_with_amu_feat();
>> +
>> + if (cpu < nr_cpu_ids)
>> + get_rate_use_wq = true;
>> +#endif
>> }
>>
>> static void cppc_check_hisi_workaround(void)
>> --
>> 2.17.1
>>
On 4/18/23 04:34, Sumit Gupta wrote:
> Export arm_smccc_get_soc_id_version() function which is needed
> in CPPC_CPUFREQ to check and apply workaround for Tegra241 SoC.
> Also, exporting arm_smccc_get_soc_id_revision() function as it
> might be needed in future.
>
> Signed-off-by: Sumit Gupta <[email protected]>
The commit subject seems off, maybe you re-used the last one that was
done in the tree (35727af2b15d irqchip/gicv3: Workaround for NVIDIA
erratum T241-FABRIC-4), it should rather be: "firmware: smccc: Export
arm_smccc_get_soc_id_xx funcs"
--
Florian