[PATCH v3 1/1] RISC-V: Add common csr_read_num() and csr_write_num() functions

[Anup Patel]

On Thu, Oct 30, 2025 at 12:35 AM Paul Walmsley <pjw at kernel.org> wrote:
>
> Hi Anup,
>
> On Tue, 14 Oct 2025, Anup Patel wrote:
>
> > In RISC-V, there is no CSR read/write instruction which takes CSR
> > number via register so add common csr_read_num() and csr_write_num()
> > functions which allow accessing certain CSRs by passing CSR number
> > as parameter. These common functions will be first used by the
> > ACPI CPPC driver and RISC-V PMU driver.
> >
> > Signed-off-by: Anup Patel <apatel at ventanamicro.com>
> > Reviewed-by: Sunil V L <sunilvl at ventanamicro.com>
> > Reviewed-by: Andrew Jones <ajones at ventanamicro.com>
> > Reviewed-by: Atish Patra <atishp at rivosinc.com>
> > Reviewed-by: Nutty Liu <nutty.liu at hotmail.com>
>
> This patch also (silently) removes the CSR number filtering, e.g.
>
> > diff --git a/drivers/acpi/riscv/cppc.c b/drivers/acpi/riscv/cppc.c
> > index 42c1a9052470..fe491937ed25 100644
> > --- a/drivers/acpi/riscv/cppc.c
> > +++ b/drivers/acpi/riscv/cppc.c
> > @@ -65,24 +65,19 @@ static void sbi_cppc_write(void *write_data)
> >  static void cppc_ffh_csr_read(void *read_data)
> >  {
> >       struct sbi_cppc_data *data = (struct sbi_cppc_data *)read_data;
> > +     int err;
> >
> > -     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;
> > -     }
>
> ... the above code, and:

The switch case is incomplete for cppc_ffh_csr_read().
Also, csr_read_num() already does appropriate filtering
so the switch case over here is now redundant.

>
> >  /*
> >   * Read the CSR of a corresponding counter.
> >   */
> >  unsigned long riscv_pmu_ctr_read_csr(unsigned long csr)
> >  {
> > -     if (csr < CSR_CYCLE || csr > CSR_HPMCOUNTER31H ||
> > -        (csr > CSR_HPMCOUNTER31 && csr < CSR_CYCLEH)) {
> > -             pr_err("Invalid performance counter csr %lx\n", csr);
> > -             return -EINVAL;
>
> ... the above code.
>
> I'm thinking that we probably want to keep the CSR number filtering code
> in; at least, I can't think of a good reason to remove it.  Care to add it
> back in?

We can potentially have custom CSRs as hardware counters
hence the CSR filtering over here is already incomplete. Plus,
csr_read_num() already does the CSR filtering and returns
failure for inappropriate CSR number.

Regards,
Anup