[PATCH] ACPI: RISC-V: CPPC: Add CSR_CYCLE for CPPC FFH

[PATCH] ACPI: RISC-V: CPPC: Add CSR_CYCLE for CPPC FFH

[Yunhui Cui]

Add the read of CSR_CYCLE to cppc_ffh_csr_read() to fix the
warning message: "CPPC Cpufreq: cppc_scale_freq_wokrfn: failed
to read perf counters".

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 drivers/acpi/riscv/cppc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
index 4cdff387deff6..c1acaeb18eac3 100644
--- a/drivers/acpi/riscv/cppc.c
+++ b/drivers/acpi/riscv/cppc.c
@@ -69,11 +69,14 @@ 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;
+	case CSR_CYCLE:
+		data->ret.value = csr_read(CSR_CYCLE);
+		data->ret.error = 0;
+		break;
 	default:
 		data->ret.error = -EINVAL;
 		break;
-- 
2.39.2

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

[yunhui cui]

Hi Sunil,

On Thu, May 15, 2025 at 5:44 PM Yunhui Cui <cuiyunhui@bytedance.com> wrote:
>
> Add the read of CSR_CYCLE to cppc_ffh_csr_read() to fix the
> warning message: "CPPC Cpufreq: cppc_scale_freq_wokrfn: failed
> to read perf counters".
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  drivers/acpi/riscv/cppc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
> index 4cdff387deff6..c1acaeb18eac3 100644
> --- a/drivers/acpi/riscv/cppc.c
> +++ b/drivers/acpi/riscv/cppc.c
> @@ -69,11 +69,14 @@ 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;
> +       case CSR_CYCLE:
> +               data->ret.value = csr_read(CSR_CYCLE);
> +               data->ret.error = 0;
> +               break;
>         default:
>                 data->ret.error = -EINVAL;
>                 break;
> --
> 2.39.2
>

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.

Thanks,
Yunhui

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

[Sunil V L]

On Tue, Aug 12, 2025 at 07:25:44PM +0800, yunhui cui wrote:
> Hi Sunil,
> 
> On Thu, May 15, 2025 at 5:44 PM Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> >
> > Add the read of CSR_CYCLE to cppc_ffh_csr_read() to fix the
> > warning message: "CPPC Cpufreq: cppc_scale_freq_wokrfn: failed
> > to read perf counters".
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >  drivers/acpi/riscv/cppc.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
> > index 4cdff387deff6..c1acaeb18eac3 100644
> > --- a/drivers/acpi/riscv/cppc.c
> > +++ b/drivers/acpi/riscv/cppc.c
> > @@ -69,11 +69,14 @@ 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;
> > +       case CSR_CYCLE:
> > +               data->ret.value = csr_read(CSR_CYCLE);
> > +               data->ret.error = 0;
> > +               break;
> >         default:
> >                 data->ret.error = -EINVAL;
> >                 break;
> > --
> > 2.39.2
> >
> 
> 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

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

[yunhui cui]

Hi Sunil,


On Tue, Aug 12, 2025 at 9:15 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Tue, Aug 12, 2025 at 07:25:44PM +0800, yunhui cui wrote:
> > Hi Sunil,
> >
> > On Thu, May 15, 2025 at 5:44 PM Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Add the read of CSR_CYCLE to cppc_ffh_csr_read() to fix the
> > > warning message: "CPPC Cpufreq: cppc_scale_freq_wokrfn: failed
> > > to read perf counters".
> > >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > ---
> > >  drivers/acpi/riscv/cppc.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
> > > index 4cdff387deff6..c1acaeb18eac3 100644
> > > --- a/drivers/acpi/riscv/cppc.c
> > > +++ b/drivers/acpi/riscv/cppc.c
> > > @@ -69,11 +69,14 @@ 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;
> > > +       case CSR_CYCLE:
> > > +               data->ret.value = csr_read(CSR_CYCLE);
> > > +               data->ret.error = 0;
> > > +               break;
> > >         default:
> > >                 data->ret.error = -EINVAL;
> > >                 break;
> > > --
> > > 2.39.2
> > >
> >
> > 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.

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.

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?

Thanks,
Yunhui

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

[Sunil V L]

On Tue, Aug 12, 2025 at 09:32:10PM +0800, yunhui cui wrote:
> Hi Sunil,
> 
> 
> On Tue, Aug 12, 2025 at 9:15 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > On Tue, Aug 12, 2025 at 07:25:44PM +0800, yunhui cui wrote:
> > > Hi Sunil,
> > >
> > > On Thu, May 15, 2025 at 5:44 PM Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > >
> > > > Add the read of CSR_CYCLE to cppc_ffh_csr_read() to fix the
> > > > warning message: "CPPC Cpufreq: cppc_scale_freq_wokrfn: failed
> > > > to read perf counters".
> > > >
> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > ---
> > > >  drivers/acpi/riscv/cppc.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
> > > > index 4cdff387deff6..c1acaeb18eac3 100644
> > > > --- a/drivers/acpi/riscv/cppc.c
> > > > +++ b/drivers/acpi/riscv/cppc.c
> > > > @@ -69,11 +69,14 @@ 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;
> > > > +       case CSR_CYCLE:
> > > > +               data->ret.value = csr_read(CSR_CYCLE);
> > > > +               data->ret.error = 0;
> > > > +               break;
> > > >         default:
> > > >                 data->ret.error = -EINVAL;
> > > >                 break;
> > > > --
> > > > 2.39.2
> > > >
> > >
> > > 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.
>
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.
 
> 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.

Thanks,
Sunil

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

[yunhui cui]

Hi Sunil,

On Tue, Aug 12, 2025 at 10:06 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Tue, Aug 12, 2025 at 09:32:10PM +0800, yunhui cui wrote:
> > Hi Sunil,
> >
> >
> > On Tue, Aug 12, 2025 at 9:15 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> > >
> > > On Tue, Aug 12, 2025 at 07:25:44PM +0800, yunhui cui wrote:
> > > > Hi Sunil,
> > > >
> > > > On Thu, May 15, 2025 at 5:44 PM Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > > >
> > > > > Add the read of CSR_CYCLE to cppc_ffh_csr_read() to fix the
> > > > > warning message: "CPPC Cpufreq: cppc_scale_freq_wokrfn: failed
> > > > > to read perf counters".
> > > > >
> > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > > ---
> > > > >  drivers/acpi/riscv/cppc.c | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
> > > > > index 4cdff387deff6..c1acaeb18eac3 100644
> > > > > --- a/drivers/acpi/riscv/cppc.c
> > > > > +++ b/drivers/acpi/riscv/cppc.c
> > > > > @@ -69,11 +69,14 @@ 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;
> > > > > +       case CSR_CYCLE:
> > > > > +               data->ret.value = csr_read(CSR_CYCLE);
> > > > > +               data->ret.error = 0;
> > > > > +               break;
> > > > >         default:
> > > > >                 data->ret.error = -EINVAL;
> > > > >                 break;
> > > > > --
> > > > > 2.39.2
> > > > >
> > > >
> > > > 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.
> >
> 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?

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

Could you take a look at the "AMU events and event numbers" chapter in
the ARM64 manual?


>
> Thanks,
> Sunil

Thanks,
Yunhui

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

[Sunil V L]

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@ventanamicro.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().

Thanks,
Sunil

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

[yunhui cui]

Hi Sunil,

On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@ventanamicro.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@ventanamicro.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.

Since each vendor will define their own CSRs, why not formalize them
into a specification?

>
> Thanks,
> Sunil

Thanks,
Yunhui

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

[Anup Patel]

On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Sunil,
>
> On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@ventanamicro.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@ventanamicro.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.

>
> Since each vendor will define their own CSRs, why not formalize them
> into a specification?

The _CPC objects in the ACPI table point to platform specific mechanisms
of accessing CPPC CSR so it can point to a vendor specific CSR.

Regards,
Anup

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

[yunhui cui]

Hi Anup,

On Wed, Aug 13, 2025 at 7:12 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Sunil,
> >
> > On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@ventanamicro.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@ventanamicro.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?

Without a specification defining these two counters, the above code
would lack robustness.

>
> >
> > Since each vendor will define their own CSRs, why not formalize them
> > into a specification?
>
> The _CPC objects in the ACPI table point to platform specific mechanisms
> of accessing CPPC CSR so it can point to a vendor specific CSR.
>
> Regards,
> Anup

Thanks,
Yunhui

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

[Anup Patel]

On Thu, Aug 14, 2025 at 9:08 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Anup,
>
> On Wed, Aug 13, 2025 at 7:12 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Hi Sunil,
> > >
> > > On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@ventanamicro.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@ventanamicro.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.

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

Regards,
Anup

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

[yunhui cui]

Hi Anup,

On Thu, Aug 14, 2025 at 1:48 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Thu, Aug 14, 2025 at 9:08 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Anup,
> >
> > On Wed, Aug 13, 2025 at 7:12 PM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@bytedance.com> wrote:
> > > >
> > > > Hi Sunil,
> > > >
> > > > On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@ventanamicro.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@ventanamicro.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

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

[Anup Patel]

On Thu, Aug 14, 2025 at 11:49 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Anup,
>
> On Thu, Aug 14, 2025 at 1:48 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Thu, Aug 14, 2025 at 9:08 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Hi Anup,
> > >
> > > On Wed, Aug 13, 2025 at 7:12 PM Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@bytedance.com> wrote:
> > > > >
> > > > > Hi Sunil,
> > > > >
> > > > > On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@ventanamicro.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@ventanamicro.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;

Yes, the current switch-case needs to replaced by common
csr_read_num() and csr_write_num() implemented in arch/riscv/

The RISC-V CSR space is limited so with it is straightforward
to implement csr_read_num() and csr_write_num() using
macros where each CSR access will turn-out to be roughly
3-4 instructions.

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

I don't think there is any specification gap here. Platforms can either use
SBI CPPC extension or time CSR or other implementation specific CSRs.
All this is abstracted away by RISC-V ACPI FFH.

If data->reg points to invalid / unimplemented CSR then csr_read_num()
and csr_write_num() will cause an illegal instruction trap which is expected
behaviour for a buggy ACPI table.

Regards,
Anup

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

[Jessica Clarke]

On 14 Aug 2025, at 14:37, Anup Patel <apatel@ventanamicro.com> wrote:
> 
> On Thu, Aug 14, 2025 at 11:49 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
>> 
>> Hi Anup,
>> 
>> On Thu, Aug 14, 2025 at 1:48 PM Anup Patel <apatel@ventanamicro.com> wrote:
>>> 
>>> On Thu, Aug 14, 2025 at 9:08 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
>>>> 
>>>> Hi Anup,
>>>> 
>>>> On Wed, Aug 13, 2025 at 7:12 PM Anup Patel <apatel@ventanamicro.com> wrote:
>>>>> 
>>>>> On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@bytedance.com> wrote:
>>>>>> 
>>>>>> Hi Sunil,
>>>>>> 
>>>>>> On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@ventanamicro.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@ventanamicro.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;
> 
> Yes, the current switch-case needs to replaced by common
> csr_read_num() and csr_write_num() implemented in arch/riscv/
> 
> The RISC-V CSR space is limited so with it is straightforward
> to implement csr_read_num() and csr_write_num() using
> macros where each CSR access will turn-out to be roughly
> 3-4 instructions.

12 bits, or 4096 CSRs. Are you saying you want to have a jump table
that dispatches to one of 4096 entry points?

Maybe you can cut that down a bit for S-mode based on the encoding
convention, but that only eliminates 1/4, so you’re still looking at
3072 entry points, perhaps also minus the few that are allocated and
clearly not sensible things to use for this, like stval.

But I think that’s not a reasonable approach to take, and if there is
no CSR in the current RISC-V spec that fits the needs of ACPI then one
needs to be defined so that we don’t need every vendor to invent their
own. If there is a CSR already then that should be the only one that’s
allowed to be used here. If you look at arm64, it hard-codes which
counter to use for each of the two calls it supports. That’s a much
better world to be in.

Jessica

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

[yunhui cui]

Hi Jessica,

On Fri, Aug 15, 2025 at 12:57 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 14 Aug 2025, at 14:37, Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Thu, Aug 14, 2025 at 11:49 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
> >>
> >> Hi Anup,
> >>
> >> On Thu, Aug 14, 2025 at 1:48 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >>>
> >>> On Thu, Aug 14, 2025 at 9:08 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
> >>>>
> >>>> Hi Anup,
> >>>>
> >>>> On Wed, Aug 13, 2025 at 7:12 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >>>>>
> >>>>> On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@bytedance.com> wrote:
> >>>>>>
> >>>>>> Hi Sunil,
> >>>>>>
> >>>>>> On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@ventanamicro.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@ventanamicro.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;
> >
> > Yes, the current switch-case needs to replaced by common
> > csr_read_num() and csr_write_num() implemented in arch/riscv/
> >
> > The RISC-V CSR space is limited so with it is straightforward
> > to implement csr_read_num() and csr_write_num() using
> > macros where each CSR access will turn-out to be roughly
> > 3-4 instructions.
>
> 12 bits, or 4096 CSRs. Are you saying you want to have a jump table
> that dispatches to one of 4096 entry points?
>
> Maybe you can cut that down a bit for S-mode based on the encoding
> convention, but that only eliminates 1/4, so you’re still looking at
> 3072 entry points, perhaps also minus the few that are allocated and
> clearly not sensible things to use for this, like stval.
>
> But I think that’s not a reasonable approach to take, and if there is
> no CSR in the current RISC-V spec that fits the needs of ACPI then one
> needs to be defined so that we don’t need every vendor to invent their
> own. If there is a CSR already then that should be the only one that’s
> allowed to be used here. If you look at arm64, it hard-codes which
> counter to use for each of the two calls it supports. That’s a much
> better world to be in.

Agreed. Because the second operand of the csrr instruction must be a
constant, a switch-case conversion is therefore necessary.

>
> Jessica
>

Thanks,
Yunhui

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

[Anup Patel]

On Thu, Aug 14, 2025 at 10:27 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 14 Aug 2025, at 14:37, Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Thu, Aug 14, 2025 at 11:49 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
> >>
> >> Hi Anup,
> >>
> >> On Thu, Aug 14, 2025 at 1:48 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >>>
> >>> On Thu, Aug 14, 2025 at 9:08 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
> >>>>
> >>>> Hi Anup,
> >>>>
> >>>> On Wed, Aug 13, 2025 at 7:12 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >>>>>
> >>>>> On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@bytedance.com> wrote:
> >>>>>>
> >>>>>> Hi Sunil,
> >>>>>>
> >>>>>> On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@ventanamicro.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@ventanamicro.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;
> >
> > Yes, the current switch-case needs to replaced by common
> > csr_read_num() and csr_write_num() implemented in arch/riscv/
> >
> > The RISC-V CSR space is limited so with it is straightforward
> > to implement csr_read_num() and csr_write_num() using
> > macros where each CSR access will turn-out to be roughly
> > 3-4 instructions.
>
> 12 bits, or 4096 CSRs. Are you saying you want to have a jump table
> that dispatches to one of 4096 entry points?
>
> Maybe you can cut that down a bit for S-mode based on the encoding
> convention, but that only eliminates 1/4, so you’re still looking at
> 3072 entry points, perhaps also minus the few that are allocated and
> clearly not sensible things to use for this, like stval.

Yes, we don't need a switch case for all possible 4096 CSRs in
csr_read_num() and csr_write_num(). For S-mode, it will mostly
contain HPM counters and custom CSRs.

I am already working on a patch along these lines which will be
posted in the next few days.

>
> But I think that’s not a reasonable approach to take, and if there is
> no CSR in the current RISC-V spec that fits the needs of ACPI then one
> needs to be defined so that we don’t need every vendor to invent their
> own. If there is a CSR already then that should be the only one that’s
> allowed to be used here. If you look at arm64, it hard-codes which
> counter to use for each of the two calls it supports. That’s a much
> better world to be in.
>

Like mentioned in this thread, the time CSR can be used as
reference perf counter and for delivered perf counter, we have
multiple options: 1) RPMI CPPC fast channel, 2) SBI CPPC call,
and 3) custom CSR. In fact, the QEMU PoC which we had posted
previously already has RPMI CPPC fast channel for delivered
perf counter which we access from OpenSBI.

If a platform vendor wants to implement a reference perf counter
and delivered perf counter differently then they can always have
custom CSRs.

Regards,
Anup

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

[鞠文龙]

Hi Sunil，

> From: "Sunil V L"<sunilvl@ventanamicro.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@bytedance.com>
> Cc: <rafael@kernel.org>, <lenb@kernel.org>, <paul.walmsley@sifive.com>, <palmer@dabbelt.com>, <aou@eecs.berkeley.edu>, <alex@ghiti.fr>, <linux-acpi@vger.kernel.org>, <linux-riscv@lists.infradead.org>, <linux-kernel@vger.kernel.org>, "Anup Patel"<apatel@ventanamicro.com>, "Rahul Pathak"<rpathak@ventanamicro.com>, <juwenlong@bytedance.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@ventanamicro.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？

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

[Sunil V L]

On Wed, Aug 13, 2025 at 12:06:11AM -0700, 鞠文龙 wrote:
> Hi Sunil，
> 
> > From: "Sunil V L"<sunilvl@ventanamicro.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@bytedance.com>
> > Cc: <rafael@kernel.org>, <lenb@kernel.org>, <paul.walmsley@sifive.com>, <palmer@dabbelt.com>, <aou@eecs.berkeley.edu>, <alex@ghiti.fr>, <linux-acpi@vger.kernel.org>, <linux-riscv@lists.infradead.org>, <linux-kernel@vger.kernel.org>, "Anup Patel"<apatel@ventanamicro.com>, "Rahul Pathak"<rpathak@ventanamicro.com>, <juwenlong@bytedance.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@ventanamicro.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."

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

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@ventanamicro.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@bytedance.com>
> Cc: "yunhui cui"<cuiyunhui@bytedance.com>, <rafael@kernel.org>, <lenb@kernel.org>, <paul.walmsley@sifive.com>, <palmer@dabbelt.com>, <aou@eecs.berkeley.edu>, <alex@ghiti.fr>, <linux-acpi@vger.kernel.org>, <linux-riscv@lists.infradead.org>, <linux-kernel@vger.kernel.org>, "Anup Patel"<apatel@ventanamicro.com>, "Rahul Pathak"<rpathak@ventanamicro.com>
> On Wed, Aug 13, 2025 at 12:06:11AM -0700, 鞠文龙 wrote:

> > Hi Sunil，

> >

> > > From: "Sunil V L"<sunilvl@ventanamicro.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@bytedance.com>

> > > Cc: <rafael@kernel.org>, <lenb@kernel.org>, <paul.walmsley@sifive.com>, <palmer@dabbelt.com>, <aou@eecs.berkeley.edu>, <alex@ghiti.fr>, <linux-acpi@vger.kernel.org>, <linux-riscv@lists.infradead.org>, <linux-kernel@vger.kernel.org>, "Anup Patel"<apatel@ventanamicro.com>, "Rahul Pathak"<rpathak@ventanamicro.com>, <juwenlong@bytedance.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@ventanamicro.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
>