[4/4] arm64: implement CPPC FFH support using AMUs

Commit Message

Ionela Voinescu Aug. 26, 2020, 1:03 p.m. UTC

If Activity Monitors (AMUs) are present, two of the counters can be used
to implement support for CPPC's (Collaborative Processor Performance
Control) delivered and reference performance monitoring functionality
using FFH (Functional Fixed Hardware).

Given that counters for a certain CPU can only be read from that CPU,
while FFH operations can be called from any CPU for any of the CPUs, use
smp_call_function_single() to provide the requested values.

Therefore, depending on the register addresses, the following values
are returned:
 - 0x0 (DeliveredPerformanceCounterRegister): AMU core counter
 - 0x1 (ReferencePerformanceCounterRegister): AMU constant counter

The use of Activity Monitors is hidden behind the generic
{read,store}_{corecnt,constcnt}() functions.

Read functionality for these two registers represents the only current
FFH support for CPPC. Read operations for other register values or write
operation for all registers are unsupported. Therefore, keep CPPC's FFH
unsupported if no CPUs have valid AMU frequency counters.

Signed-off-by: Ionela Voinescu <[e-mail]>
Cc: Catalin Marinas <[e-mail]>
Cc: Will Deacon <[e-mail]>
---
 arch/arm64/kernel/topology.c | 62 ++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

Comments

Catalin Marinas Sept. 11, 2020, 2:40 p.m. UTC | #1

On Wed, Aug 26, 2020 at 02:03:09PM +0100, Ionela Voinescu wrote:
> +/*
> + * Refer to drivers/acpi/cppc_acpi.c for the description of the functions
> + * below.
> + */
> +bool cpc_ffh_supported(void)
> +{
> +	const struct cpumask *cnt_cpu_mask = cpus_with_amu_counters();
> +	int cpu = nr_cpu_ids;
> +
> +	if (cnt_cpu_mask)
> +		cpu = cpumask_any_and(cnt_cpu_mask, cpu_present_mask);
> +
> +	if ((cpu >= nr_cpu_ids) || !freq_counters_valid(cpu))
> +		return false;
> +
> +	return true;
> +}

IIUC, the only need for the cpumask is this function, the others would
have worked just fine with the existing cpu_has_amu_feat(). So you have
a lot more !cnt_cpu_mask checks now.

I wonder whether instead you could add a new function near
cpu_has_amu_feat(), something like get_cpu_with_amu_feat() and do the
cpumask_any_and() in there.

Ionela Voinescu Sept. 22, 2020, 4:07 p.m. UTC | #2

Hi Catalin,

Sorry for the delayed reply. I took advantage of a last chance for a
holiday before the weather gets bad :).

On Friday 11 Sep 2020 at 15:40:32 (+0100), Catalin Marinas wrote:
> On Wed, Aug 26, 2020 at 02:03:09PM +0100, Ionela Voinescu wrote:
> > +/*
> > + * Refer to drivers/acpi/cppc_acpi.c for the description of the functions
> > + * below.
> > + */
> > +bool cpc_ffh_supported(void)
> > +{
> > +	const struct cpumask *cnt_cpu_mask = cpus_with_amu_counters();
> > +	int cpu = nr_cpu_ids;
> > +
> > +	if (cnt_cpu_mask)
> > +		cpu = cpumask_any_and(cnt_cpu_mask, cpu_present_mask);
> > +
> > +	if ((cpu >= nr_cpu_ids) || !freq_counters_valid(cpu))
> > +		return false;
> > +
> > +	return true;
> > +}
> 
> IIUC, the only need for the cpumask is this function, the others would
> have worked just fine with the existing cpu_has_amu_feat(). So you have
> a lot more !cnt_cpu_mask checks now.
> 
> I wonder whether instead you could add a new function near
> cpu_has_amu_feat(), something like get_cpu_with_amu_feat() and do the
> cpumask_any_and() in there.
> 

Yes, it does look ugly. I went for this because I wanted to avoid adding
new functions to the cpu feature code with new AMU usecase additions,
functions that might just end up doing cpumask operations, and nothing
more. This way there is a single function that basically extracts all the
information that the feature code is able to provide on AMU support and
the user of the counters can then manipulate the mask as it sees fit.

Basically I was thinking that with a potential new usecase we might have
to add yet another function and I did not like that prospect.

From performance point of view, the !cnt_cpu_mask checks wouldn't affect
that much: cpc_ffh_supported() is only called during CPPC probe and
freq_counters_valid() is only called at init.
counters_read_on_cpu() will be called more often (cpufreq .get
function) but not as much as to bring significant overhead.

To make this nicer I can do the following:
 - make the !cnt_cpu_mask unlikely due to CONFIG_ARM64_AMU_EXTN being
   enabled by default.
 - wrap all the cpus_with_amu_counters() uses under:

static inline int amu_cpus_any_and(const struct cpumask *cpus)
{
	const struct cpumask *cnt_cpu_mask = cpus_with_amu_counters();
	int cpu = nr_cpu_ids;

	if (likely(cnt_cpu_mask))
		cpu = cpumask_any_and(cnt_cpu_mask, cpus);

	return cpu;
}

This is to be called as:
 - "if (!freq_counters_valid(amu_cpus_any_and(cpu_present_mask)))"
   in cpc_ffh_supported();
 - "if (amu_cpus_any_and(cpumask_of(cpu)) == cpu)"
   in the other two.

It won't eliminate the useless checks but it will make the code a bit
nicer.

If you don't think it's worth it, I'll go with your suggestion.

Thank you for the review,
Ionela.


> -- 
> Catalin

Patch

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index edc44b46e34f..cb0372de9aa9 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -326,3 +326,65 @@  void topology_scale_freq_tick(void)
 	this_cpu_write(arch_core_cycles_prev, core_cnt);
 	this_cpu_write(arch_const_cycles_prev, const_cnt);
 }
+
+#ifdef CONFIG_ACPI_CPPC_LIB
+#include <acpi/cppc_acpi.h>
+
+static inline
+int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val)
+{
+	const struct cpumask *cnt_cpu_mask = cpus_with_amu_counters();
+
+	if (!cnt_cpu_mask || !cpumask_test_cpu(cpu, cnt_cpu_mask))
+		return -EOPNOTSUPP;
+
+	smp_call_function_single(cpu, func, val, 1);
+
+	return 0;
+}
+
+/*
+ * Refer to drivers/acpi/cppc_acpi.c for the description of the functions
+ * below.
+ */
+bool cpc_ffh_supported(void)
+{
+	const struct cpumask *cnt_cpu_mask = cpus_with_amu_counters();
+	int cpu = nr_cpu_ids;
+
+	if (cnt_cpu_mask)
+		cpu = cpumask_any_and(cnt_cpu_mask, cpu_present_mask);
+
+	if ((cpu >= nr_cpu_ids) || !freq_counters_valid(cpu))
+		return false;
+
+	return true;
+}
+
+int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
+{
+	int ret = -EOPNOTSUPP;
+
+	switch ((u64)reg->address) {
+	case 0x0:
+		ret = counters_read_on_cpu(cpu, store_corecnt, val);
+		break;
+	case 0x1:
+		ret = counters_read_on_cpu(cpu, store_constcnt, val);
+		break;
+	}
+
+	if (!ret) {
+		*val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
+				    reg->bit_offset);
+		*val >>= reg->bit_offset;
+	}
+
+	return ret;
+}
+
+int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
+{
+	return -EOPNOTSUPP;
+}
+#endif /* CONFIG_ACPI_CPPC_LIB */