On Thu, Feb 29, 2024 at 7:01 PM Vanshidhar Konda <vanshikonda@xxxxxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, Feb 29, 2024 at 06:32:59PM +0100, Rafael J. Wysocki wrote: > >On Thu, Feb 29, 2024 at 5:25 PM Vanshidhar Konda > ><vanshikonda@xxxxxxxxxxxxxxxxxxxxxx> wrote: > >> > >> The CPPC driver reads delivered and reference counters using cpc_read > >> one at a time. This leads to inaccurate measurement of CPU frequency > >> discussed in [1]. If the firmware indicates that both the registers are > >> in the FFH interface the kernel can read the registers together in a > >> single IPI. This has two benefits: > >> 1. Reduces the number of IPIs needed to read the two registers > >> 2. The two registers will be read in close proximity resulting in more > >> accurate CPU frequency measurement > >> > >> [1]: https://lore.kernel.org/all/20230328193846.8757-1-yang@xxxxxxxxxxxxxxxxxxxxxx/ > >> > >> Signed-off-by: Vanshidhar Konda <vanshikonda@xxxxxxxxxxxxxxxxxxxxxx> > >> --- > >> arch/arm64/kernel/topology.c | 37 ++++++++++++++++++++++++++++++++++++ > >> drivers/acpi/cppc_acpi.c | 32 +++++++++++++++++++++++++++---- > >> include/acpi/cppc_acpi.h | 13 +++++++++++++ > >> 3 files changed, 78 insertions(+), 4 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > >> index 8905eb0c681f..8207565f43ee 100644 > >> --- a/arch/arm64/kernel/topology.c > >> +++ b/arch/arm64/kernel/topology.c > >> @@ -421,6 +421,43 @@ int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val) > >> return ret; > >> } > >> > >> +static void cpc_update_freq_counters(void *info) > >> +{ > >> + update_freq_counters_refs(); > >> +} > >> + > >> +int cpc_read_regs_ffh(int cpu, struct ffh_cpc_reg_values *ffh_regs) > >> +{ > >> + struct amu_counters *ctrs = per_cpu_ptr(&cpu_samples, cpu); > >> + int idx; > >> + > >> + if (!cpc_ffh_supported() || !freq_counters_valid(cpu)) > >> + return -EOPNOTSUPP; > >> + > >> + if (WARN_ON_ONCE(irqs_disabled())) > >> + return -EPERM; > >> + > >> + if (!idle_cpu(cpu)) > >> + smp_call_function_single(cpu, cpc_update_freq_counters, NULL, 1); > >> + > >> + for (idx = 0; idx < MAX_NUM_CPC_REGS_FFH; idx++) { > >> + > >> + if (!ffh_regs->regs[idx].reg) > >> + continue; > >> + > >> + switch ((u64)(ffh_regs->regs[idx].reg->address)) { > >> + case 0x0: > >> + ffh_regs->regs[idx].value = ctrs->core_cnt; > >> + break; > >> + case 0x1: > >> + ffh_regs->regs[idx].value = ctrs->const_cnt; > >> + break; > >> + } > >> + } > >> + > >> + 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 d155a86a8614..55ffb1915e4f 100644 > >> --- a/drivers/acpi/cppc_acpi.c > >> +++ b/drivers/acpi/cppc_acpi.c > >> @@ -113,6 +113,10 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr); > >> (cpc)->cpc_entry.reg.space_id == \ > >> ACPI_ADR_SPACE_SYSTEM_IO) > >> > >> +#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 && \ > >> @@ -974,6 +978,11 @@ int __weak cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val) > >> return -ENOTSUPP; > >> } > >> > >> +int __weak cpc_read_regs_ffh(int cpu, struct ffh_cpc_reg_values *regs) > >> +{ > >> + return -ENOTSUPP; > >> +} > > > >This might return a bool value. > > > >Is there any case in which the caller would handle different error > >codes differently? > > > I kept this similar to cpc_read_ffh(). I didn't see any usage of the error > codes. The return value of cpc_read_ffh() is returned only from cpc_read(), > but I didn't see anyone consuming the return value of cpc_read(). > Additionally, checkpatch complains about using -ENOTSUPP and suggests > replacing it with -EOPNOTSUPP. Does it make sense to update > cpc_read/write_ffh() as well to switch to return type bool? Probably, but in a separate patch.
