Message-ID: <CAHEos+PdZtqzS++aq-TTXnWbKRmemukeV5-DboSOCnhSQOTRXQ@mail.gmail.com>
Date: Wed, 13 Aug 2025 23:08:33 -0700
From: 鞠文龙 <juwenlong@...edance.com>
To: Sunil V L <sunilvl@...tanamicro.com>
Cc: yunhui cui <cuiyunhui@...edance.com>, rafael@...nel.org, lenb@...nel.org, 
	paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu, 
	alex@...ti.fr, linux-acpi@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Anup Patel <apatel@...tanamicro.com>, 
	Rahul Pathak <rpathak@...tanamicro.com>
Subject: Re: [External] Re: [PATCH] ACPI: RISC-V: CPPC: Add CSR_CYCLE for CPPC FFH

Hi sunil，
 sorry for the last email format error，send it again.



> From: "Sunil V L"<sunilvl@...tanamicro.com>
> Date:  Wed, Aug 13, 2025, 20:10
> Subject:  Re: [External] Re: [PATCH] ACPI: RISC-V: CPPC: Add CSR_CYCLE for CPPC FFH
> To: "鞠文龙"<juwenlong@...edance.com>
> Cc: "yunhui cui"<cuiyunhui@...edance.com>, <rafael@...nel.org>, <lenb@...nel.org>, <paul.walmsley@...ive.com>, <palmer@...belt.com>, <aou@...s.berkeley.edu>, <alex@...ti.fr>, <linux-acpi@...r.kernel.org>, <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, "Anup Patel"<apatel@...tanamicro.com>, "Rahul Pathak"<rpathak@...tanamicro.com>
> On Wed, Aug 13, 2025 at 12:06:11AM -0700, 鞠文龙 wrote:

> > Hi Sunil，

> >

> > > From: "Sunil V L"<sunilvl@...tanamicro.com>

> > > Date:  Wed, Aug 13, 2025, 13:28

> > > Subject:  Re: [External] Re: [PATCH] ACPI: RISC-V: CPPC: Add CSR_CYCLE for CPPC FFH

> > > To: "yunhui cui"<cuiyunhui@...edance.com>

> > > Cc: <rafael@...nel.org>, <lenb@...nel.org>, <paul.walmsley@...ive.com>, <palmer@...belt.com>, <aou@...s.berkeley.edu>, <alex@...ti.fr>, <linux-acpi@...r.kernel.org>, <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, "Anup Patel"<apatel@...tanamicro.com>, "Rahul Pathak"<rpathak@...tanamicro.com>, <juwenlong@...edance.com>

> > > Hi Yunhui,

> > >

> > >

> > > On Wed, Aug 13, 2025 at 11:23:39AM +0800, yunhui cui wrote:

> > > > Hi Sunil,

> > > >

> > > > On Tue, Aug 12, 2025 at 10:06 PM Sunil V L <sunilvl@...tanamicro.com> wrote:

> > > > >

> > > [...]

> > > > > > > >

> > > > > > > > The purpose of cppc_ffh_csr_read() is to calculate the actual

> > > > > > > > frequency of the CPU, which is delta_CSR_CYCLE/delta_CSR_XXX.

> > > > > > > >

> > > > > > > > CSR_XXX should be a reference clock and does not count during WFI

> > > > > > > > (Wait For Interrupt).

> > > > > > > >

> > > > > > > > Similar solutions include: x86's aperf/mperf, and ARM64's AMU with

> > > > > > > > registers SYS_AMEVCNTR0_CORE_EL0/SYS_AMEVCNTR0_CONST_EL0.

> > > > > > > >

> > > > > > > > However, we know that CSR_TIME in the current code does count during

> > > > > > > > WFI. So, is this design unreasonable?

> > > > > > > >

> > > > > > > > Should we consider proposing an extension to support such a dedicated

> > > > > > > > counter (a reference clock that does not count during WFI)? This way,

> > > > > > > > the value can be obtained directly in S-mode without trapping to

> > > > > > > > M-mode, especially since reading this counter is very frequent.

> > > > > > > >

> > > > > > > Hi Yunhui,

> > > > > > >

> > > > > > > Yes, but we anticipated that vendors might define their own custom CSRs.

> > > > > > > So, we introduced FFH encoding to accommodate such cases.

> > > > > > >

> > > > > > > Thanks,

> > > > > > > Sunil

> > > > > >

> > > > > > As mentioned earlier, it is best to directly read CSR_XXX (a reference

> > > > > > clock that does not count during WFI) and CSR_CYCLE in S-mode, rather

> > > > > > than trapping to SBI.

> > > > > >

> > > > > No. I meant direct CSR access itself not SBI. Please take a look at

> > > > > Table 6 of RISC-V FFH spec.

> > > > >

> > > > > > drivers/acpi/riscv/cppc.c is a generic driver that is not specific to

> > > > > > any vendor. Currently, the upstream code already uses CSR_TIME, and

> > > > > > the logic of CSR_TIME is incorrect.

> > > > > >

> > > ACPI spec for "Reference Performance Register" says,

> > >

> > >

> > > "The Reference Performance Counter Register counts at a fixed rate any

> > > time the processor is active. It is not affected by changes to Desired

> > > Performance, processor throttling, etc."

> > >

> > >

> > > > > CSR_TIME is just an example. It is upto the vendor how _CPC objects are

> > > > > encoded using FFH. The linux code doesn't mean one should use CSR_TIME

> > > > > always.

> > > >

> > > > First, the example of CSR_TIME is incorrect. What is needed is a

> > > > CSR_XXX (a reference clock that does not count during WFI).

> > > >

> > > > Second, you mentioned that each vendor can customize their own

> > > > implementations. But should all vendors' CSR_XXX/YYY/... be added to

> > > > drivers/acpi/riscv/cppc.c? Shouldn’t drivers/acpi/riscv/cppc.c fall

> > > > under the scope defined by the RISC-V architecture?

> > > >

> > > No. One can implement similar to csr_read_num() in opensbi. We didn't

> > > add it since there was no HW implementing such thing. What I am

> > > saying is we have FFH encoding to support such case.

> > >

> > >

> > > > >

> > > > > > It would be best to promote a specification to support CSR_XXX, just

> > > > > > like what has been done for x86 and arm64. What do you think?

> > > > > >

> > > > > Wouldn't above work? For a standard extension, you may have to provide

> > > > > more data with actual HW.

> > > >

> > > > This won’t work. May I ask how the current upstream code can calculate

> > > > the actual CPU frequency using CSR_TIME without trapping to SBI?

> > > > This is a theoretical logical issue. Why is data needed here?

> > > >

> > > As I mentioned above, one can implement a generic CSR read without

> > > trapping to SBI.

> > >

> > >

> > > > Could you take a look at the "AMU events and event numbers" chapter in

> > > > the ARM64 manual?

> > > >

> > > As-per ACPI spec reference performance counter is not affected by CPU

> > > state. The RISC-V FFH encoding is sufficiently generic to support this

> > > requirement, even if the standard CSR_TIME cannot be used. In such

> > > cases, an alternative CSR can be encodeded, accessed via an OS-level

> > > abstraction such as csr_read_num().

> >        As-per ACPI Spec，Both Reference performance counter register

> > and Delivered Performance Counter are affected by CPU

> > state。From ACPI Spec，“The Reference Performance Counter Register

> > counts at a fixed rate any time the processor is active.”

> >

> > “The Delivered Performance Counter Register increments any time the

> > processor is active, at a rate proportional to the current performance

> > level, taking into account changes to Desired Performance”

> > “ Processor power states include are designated C0, C1, C2, C3, . . .

> > Cn. The C0 power state is an active power state where the CPU executes

> > instructions. The C1 through Cn power states are processor sleeping

> > states where the processor consumes less power and dissipates less

> > heat than leaving the processor in the C0 state”. So the time csr can

> > not meet this requirements.we need another csr, but not availiable for

> > now.Either implement it as vendor-specific or create a community

> > extension for all？

> >

> It is upto the interpretation. I am not sure what is "active" or

> "etc" in the below statement.

>
> "The Reference Performance Counter Register counts at a fixed rate any

> time the processor is active. It is not affected by changes to Desired

> Performance, processor throttling, etc."
Per ACPI Spec,Per ACPI Spec,"The C0 power state is an active power
state where the CPU executes instructions."
 When we say a processor is active ,it means the processor is in C0,
so the active == C0,i think.
The following is a comparison of different architectural implementations.
 x86
Delivered Performance Counter register----APERF:
This register increments in proportion to the actual number of core
clocks cycles while the core is in C0. The register does not
increment when the core is in the stop-grant state.
Reference Performance Counter register-----MPERF:
This register Incremented by hardware at the P0 frequency while the
core is in C0. This register does not increment when the core is in
the
stop-grant state.
ARM
Delivered Performance Counter register----AMEVCNTR0_EL[0]:
The counter increments on every cycle when the PE is not in WFI or WFE
state. When the PE is in WFI or WFE state, this counter does not
increment.
Reference Performance Counter register-----AMEVCNTR0_EL0[1]:
The counter increments at a constant frequency when the PE is not in
WFI or WFE state, equal to therate of increment of the System counter,
CNTPCT_EL0. When the PE is in WFI or WFE state, thiscounter does not
increment.
RISC V
Delivered Performance Counter register---Not clearly defined.
Reference Performance Counter register---Not clearly defined.
>
> Second, I don't see an issue if both reference and delivered counters

> increment irrespective of idle state because ultimately the ratio

> delta(delivered)/delta(reference) matters which will be same in either

> case.
Theoretically, yes, if both counters  increment irrespective of idle state  .
>
> Thanks,

> Sunil
>

Powered by blists - more mailing lists