On Wed, Aug 13, 2025 at 12:06:11AM -0700, 鞠文龙 wrote: > Hi Sunil， > > > From: "Sunil V L"<sunilvl@xxxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxx> > > Cc: <rafael@xxxxxxxxxx>, <lenb@xxxxxxxxxx>, <paul.walmsley@xxxxxxxxxx>, <palmer@xxxxxxxxxxx>, <aou@xxxxxxxxxxxxxxxxx>, <alex@xxxxxxxx>, <linux-acpi@xxxxxxxxxxxxxxx>, <linux-riscv@xxxxxxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>, "Anup Patel"<apatel@xxxxxxxxxxxxxxxx>, "Rahul Pathak"<rpathak@xxxxxxxxxxxxxxxx>, <juwenlong@xxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxx> 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." 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. Thanks, Sunil
