Message-ID: <CAHEos+Msmta3o-HvXoigmJGbD48333Kh8tU3j2D=w_G8gL+OQw@mail.gmail.com>
Date: Wed, 13 Aug 2025 00:06:11 -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，

> 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？

Powered by blists - more mailing lists