Received-SPF: pass (google.com: domain of linux-kernel+bounces-87229-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240229162520.970986-1-vanshikonda@os.amperecomputing.com>
 <20240229162520.970986-4-vanshikonda@os.amperecomputing.com>
 <CAJZ5v0j+qz0okToszmU=kBjZvD3GJKzwz-ZrweOkgw6A2e3pQQ@mail.gmail.com> <5paw5cvh7klhwasvldyl2coaihntkv4cvmogzyov7qwyohxmvl@lt3auiqhg2nv>
In-Reply-To: <5paw5cvh7klhwasvldyl2coaihntkv4cvmogzyov7qwyohxmvl@lt3auiqhg2nv>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Thu, 29 Feb 2024 19:02:50 +0100
Message-ID: <CAJZ5v0j9t8y9dvTmVXsyUha73a6CBx8iHQNkteY_Bogx27sHLg@mail.gmail.com>
Subject: Re: Re: [PATCH v1 3/3] ACPI: CPPC: Read CPC FFH counters in a single IPI
To: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Huisong Li <lihuisong@huawei.com>, 
	Beata Michalska <beata.michalska@arm.com>, Ionela Voinescu <ionela.voinescu@arm.com>, 
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, 
	linux-arm-kernel@lists.infradead.org, sumitg@nvidia.com, zengheng4@huawei.com, 
	yang@os.amperecomputing.com, will@kernel.org, sudeep.holla@arm.com, 
	liuyonglong@huawei.com, zhanjie9@hisilicon.com, linux-acpi@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 29, 2024 at 7:01=E2=80=AFPM Vanshidhar Konda
<vanshikonda@os.amperecomputing.com> wrote:
>
> On Thu, Feb 29, 2024 at 06:32:59PM +0100, Rafael J. Wysocki wrote:
> >On Thu, Feb 29, 2024 at 5:25=E2=80=AFPM Vanshidhar Konda
> ><vanshikonda@os.amperecomputing.com> 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 ar=
e
> >> 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@os.amperec=
omputing.com/
> >>
> >> Signed-off-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> >> ---
> >>  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, u6=
4 *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 =3D 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 =3D 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 =3D ctrs->core_cnt;
> >> +                       break;
> >> +               case 0x1:
> >> +                       ffh_regs->regs[idx].value =3D 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 =3D=3D  =
      \
> >>                                 ACPI_ADR_SPACE_SYSTEM_IO)
> >>
> >> +#define CPC_IN_FFH(cpc) ((cpc)->type =3D=3D ACPI_TYPE_BUFFER &&      =
      \
> >> +                               (cpc)->cpc_entry.reg.space_id =3D=3D  =
      \
> >> +                               ACPI_ADR_SPACE_FIXED_HARDWARE)
> >> +
> >>  /* Evaluates to True if reg is a NULL register descriptor */
> >>  #define IS_NULL_REG(reg) ((reg)->space_id =3D=3D  ACPI_ADR_SPACE_SYST=
EM_MEMORY && \
> >>                                 (reg)->address =3D=3D 0 &&            =
      \
> >> @@ -974,6 +978,11 @@ int __weak cpc_write_ffh(int cpunum, struct cpc_r=
eg *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 erro=
r
> 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.