Message-ID: <CAEEQ3wm9shktdzUeO5RczE-=qdDUS30TGASOFtnMEcuw7L7jZw@mail.gmail.com>
Date: Thu, 14 Aug 2025 14:19:45 +0800
From: yunhui cui <cuiyunhui@...edance.com>
To: Anup Patel <apatel@...tanamicro.com>
Cc: Sunil V L <sunilvl@...tanamicro.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, Rahul Pathak <rpathak@...tanamicro.com>, 
	juwenlong@...edance.com
Subject: Re: [External] Re: [PATCH] ACPI: RISC-V: CPPC: Add CSR_CYCLE for CPPC FFH

Hi Anup,

On Thu, Aug 14, 2025 at 1:48 PM Anup Patel <apatel@...tanamicro.com> wrote:
>
> On Thu, Aug 14, 2025 at 9:08 AM yunhui cui <cuiyunhui@...edance.com> wrote:
> >
> > Hi Anup,
> >
> > On Wed, Aug 13, 2025 at 7:12 PM Anup Patel <apatel@...tanamicro.com> wrote:
> > >
> > > On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@...edance.com> wrote:
> > > >
> > > > Hi Sunil,
> > > >
> > > > On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@...tanamicro.com> wrote:
> > > > >
> > > > > 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().
> > > >
> > > > So what you're saying is that we should submit a patch like this, right?
> > > >
> > > > diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
> > > > index 440cf9fb91aab..953c259d46c69 100644
> > > > --- a/drivers/acpi/riscv/cppc.c
> > > > +++ b/drivers/acpi/riscv/cppc.c
> > > > @@ -66,16 +66,8 @@ static void cppc_ffh_csr_read(void *read_data)
> > > >  {
> > > >         struct sbi_cppc_data *data = (struct sbi_cppc_data *)read_data;
> > > >
> > > > -       switch (data->reg) {
> > > > -       /* Support only TIME CSR for now */
> > > > -       case CSR_TIME:
> > > > -               data->ret.value = csr_read(CSR_TIME);
> > > > -               data->ret.error = 0;
> > > > -               break;
> > > > -       default:
> > > > -               data->ret.error = -EINVAL;
> > > > -               break;
> > > > -       }
> > > > +       data->ret.value = csr_read_num(data->reg);
> > > > +       data->ret.error = 0;
> > > >  }
> > > >
> > > > If that's the case, the robustness of the code cannot be guaranteed,
> > > > because the range of CSRs from different vendors is unknown.
> > >
> > > ACPI FFH is allows mapping to any CSR.
> >
> > Yes, FFH can map any CSR, and this is not the point of contention.
> >
> > If that's the case, the CSR_TIME used in the current kernel code is
> > inappropriate. Some vendors may design a counter that does not count
> > during WFI, making CSR_TIME irrelevant. Even if counting continues
> > during WFI, are you planning to have one counter operate in S-mode
> > while the other traps to M-mode?
> >
> > In that case, the code would need to be modified as proposed above. Do
> > you agree?
>
> I disagree.
>
> Like Sunil already explained, if an implementation has reference counter
> which does not count during WFI state then for such implementation the
> delivered performance counter should also not increment during WFI
> to maintain the relative delta of increments. This means if an implementation
> uses TIME CSR as reference counter then for such implementation
> the delivered performance counter should increment accordingly. Ultimately,
> what matters is OS being able to correctly compute the performance level
> using reference and delivered performance counters.


For calculating the actual CPU frequency, both implementations are
acceptable where either both counters continue counting during WFI or
both stop counting.
In the current code, how do you read the other counter?
Shouldn't it be modified like this to support it? This way, all
counters can be read directly in S-mode without trapping to M-mode:
+       data->ret.value = csr_read_num(data->reg);
+       data->ret.error = 0;


>
> >
> > Without a specification defining these two counters, the above code
> > would lack robustness.
>
> Can you elaborate the "robustness" part ?
> Do you have data to back your claims ?

Because there is no specification defining address access rules for
data->reg across different vendors' implementations, you cannot write
a validation logic like this:
if (data->reg < CSR_CYCLE || data->reg > CSR_HPMCOUNTER31H) {
    ...
    return -EINVAL;
}
Reading data->reg directly lacks robustness, because data->reg may
potentially hold an invalid value.


>
> Regards,
> Anup

Thanks,
Yunhui

Powered by blists - more mailing lists