On Mon, Aug 18, 2025 at 08:06:00PM +0530, 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@xxxxxxxxxxxxxxxx>
> Reviewed-by: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx>
> ---
> arch/riscv/include/asm/csr.h | 3 +
> arch/riscv/kernel/Makefile | 1 +
> arch/riscv/kernel/csr.c | 165 +++++++++++++++++++++++++++++++++++
> drivers/acpi/riscv/cppc.c | 17 ++--
> drivers/perf/riscv_pmu.c | 54 ++----------
> 5 files changed, 184 insertions(+), 56 deletions(-)
> create mode 100644 arch/riscv/kernel/csr.c
>
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index 6fed42e37705..1540626b3540 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -575,6 +575,9 @@
> : "memory"); \
> })
>
> +extern unsigned long csr_read_num(unsigned long csr_num, int *out_err);
> +extern void csr_write_num(unsigned long csr_num, unsigned long val, int *out_err);
I think it's more consistent to directly return the error code, and for
csr_read_num, we could pass out the read value by a pointer. e.g.
int csr_read_num(unsigned long csr_num, unsigned long *val);
int csr_write_num(unsigned long csr_num, unsigned long val);
This allows the caller to eliminate a variable for temporarily storing
the error code if they use it just after the invokation, and fits the
common convention of Linux better.
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* _ASM_RISCV_CSR_H */
...
> 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;
> - }
> + data->ret.value = csr_read_num(data->reg, &err);
> + data->ret.error = err;
> }
>
> static void cppc_ffh_csr_write(void *write_data)
> {
> struct sbi_cppc_data *data = (struct sbi_cppc_data *)write_data;
> + int err;
>
> - data->ret.error = -EINVAL;
> + csr_write_num(data->reg, data->val, &err);
> + data->ret.error = err;
> }
This could be simplified as
data->ret.error = csr_write_num(data->reg, data->val);
and variable err could be dropped.
Best regards,
Yao Zi
