Hi Jessica, On Fri, Aug 15, 2025 at 12:57 AM Jessica Clarke <jrtc27@xxxxxxxxxx> wrote: > > On 14 Aug 2025, at 14:37, Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote: > > > > On Thu, Aug 14, 2025 at 11:49 AM yunhui cui <cuiyunhui@xxxxxxxxxxxxx> wrote: > >> > >> Hi Anup, > >> > >> On Thu, Aug 14, 2025 at 1:48 PM Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote: > >>> > >>> On Thu, Aug 14, 2025 at 9:08 AM yunhui cui <cuiyunhui@xxxxxxxxxxxxx> wrote: > >>>> > >>>> Hi Anup, > >>>> > >>>> On Wed, Aug 13, 2025 at 7:12 PM Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote: > >>>>> > >>>>> On Wed, Aug 13, 2025 at 12:14 PM yunhui cui <cuiyunhui@xxxxxxxxxxxxx> wrote: > >>>>>> > >>>>>> Hi Sunil, > >>>>>> > >>>>>> On Wed, Aug 13, 2025 at 1:28 PM Sunil V L <sunilvl@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> wrote: > >>>>>>>>> > >>>>>>> [...] > >>>>>>>>>>>> > >>>>>>>>>>>> The purpose of cppc_ffh_csr_read() is to calculate the actual > >>>>>>>>>>>> frequency of the CPU, which is delta_CSR_CYCLE/delta_CSR_XXX. > >>>>>>>>>>>> > >>>>>>>>>>>> CSR_XXX should be a reference clock and does not count during WFI > >>>>>>>>>>>> (Wait For Interrupt). > >>>>>>>>>>>> > >>>>>>>>>>>> Similar solutions include: x86's aperf/mperf, and ARM64's AMU with > >>>>>>>>>>>> registers SYS_AMEVCNTR0_CORE_EL0/SYS_AMEVCNTR0_CONST_EL0. > >>>>>>>>>>>> > >>>>>>>>>>>> However, we know that CSR_TIME in the current code does count during > >>>>>>>>>>>> WFI. So, is this design unreasonable? > >>>>>>>>>>>> > >>>>>>>>>>>> Should we consider proposing an extension to support such a dedicated > >>>>>>>>>>>> counter (a reference clock that does not count during WFI)? This way, > >>>>>>>>>>>> the value can be obtained directly in S-mode without trapping to > >>>>>>>>>>>> M-mode, especially since reading this counter is very frequent. > >>>>>>>>>>>> > >>>>>>>>>>> Hi Yunhui, > >>>>>>>>>>> > >>>>>>>>>>> Yes, but we anticipated that vendors might define their own custom CSRs. > >>>>>>>>>>> So, we introduced FFH encoding to accommodate such cases. > >>>>>>>>>>> > >>>>>>>>>>> Thanks, > >>>>>>>>>>> Sunil > >>>>>>>>>> > >>>>>>>>>> As mentioned earlier, it is best to directly read CSR_XXX (a reference > >>>>>>>>>> clock that does not count during WFI) and CSR_CYCLE in S-mode, rather > >>>>>>>>>> than trapping to SBI. > >>>>>>>>>> > >>>>>>>>> No. I meant direct CSR access itself not SBI. Please take a look at > >>>>>>>>> Table 6 of RISC-V FFH spec. > >>>>>>>>> > >>>>>>>>>> drivers/acpi/riscv/cppc.c is a generic driver that is not specific to > >>>>>>>>>> any vendor. Currently, the upstream code already uses CSR_TIME, and > >>>>>>>>>> the logic of CSR_TIME is incorrect. > >>>>>>>>>> > >>>>>>> ACPI spec for "Reference Performance Register" says, > >>>>>>> > >>>>>>> "The Reference Performance Counter Register counts at a fixed rate any > >>>>>>> time the processor is active. It is not affected by changes to Desired > >>>>>>> Performance, processor throttling, etc." > >>>>>>> > >>>>>>>>> CSR_TIME is just an example. It is upto the vendor how _CPC objects are > >>>>>>>>> encoded using FFH. The linux code doesn't mean one should use CSR_TIME > >>>>>>>>> always. > >>>>>>>> > >>>>>>>> First, the example of CSR_TIME is incorrect. What is needed is a > >>>>>>>> CSR_XXX (a reference clock that does not count during WFI). > >>>>>>>> > >>>>>>>> Second, you mentioned that each vendor can customize their own > >>>>>>>> implementations. But should all vendors' CSR_XXX/YYY/... be added to > >>>>>>>> drivers/acpi/riscv/cppc.c? Shouldn’t drivers/acpi/riscv/cppc.c fall > >>>>>>>> under the scope defined by the RISC-V architecture? > >>>>>>>> > >>>>>>> No. One can implement similar to csr_read_num() in opensbi. We didn't > >>>>>>> add it since there was no HW implementing such thing. What I am > >>>>>>> saying is we have FFH encoding to support such case. > >>>>>>> > >>>>>>>>> > >>>>>>>>>> It would be best to promote a specification to support CSR_XXX, just > >>>>>>>>>> like what has been done for x86 and arm64. What do you think? > >>>>>>>>>> > >>>>>>>>> Wouldn't above work? For a standard extension, you may have to provide > >>>>>>>>> more data with actual HW. > >>>>>>>> > >>>>>>>> This won’t work. May I ask how the current upstream code can calculate > >>>>>>>> the actual CPU frequency using CSR_TIME without trapping to SBI? > >>>>>>>> This is a theoretical logical issue. Why is data needed here? > >>>>>>>> > >>>>>>> As I mentioned above, one can implement a generic CSR read without > >>>>>>> trapping to SBI. > >>>>>>> > >>>>>>>> Could you take a look at the "AMU events and event numbers" chapter in > >>>>>>>> the ARM64 manual? > >>>>>>>> > >>>>>>> As-per ACPI spec reference performance counter is not affected by CPU > >>>>>>> state. The RISC-V FFH encoding is sufficiently generic to support this > >>>>>>> requirement, even if the standard CSR_TIME cannot be used. In such > >>>>>>> cases, an alternative CSR can be encodeded, accessed via an OS-level > >>>>>>> abstraction such as csr_read_num(). > >>>>>> > >>>>>> 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