Re: [PATCH v8 05/22] RISC-V: Define indirect CSR access helpers

On 8/4/26 5:39 PM, Paul Walmsley wrote:
> On Wed, 1 Jul 2026, Atish Patra wrote:
> 
>> From: Atish Patra <[email protected]>
>>
>> The indirect CSR requires multiple instructions to read/write CSR.
>> Add a few helper functions for ease of usage.
>>
>> Signed-off-by: Atish Patra <[email protected]>
> 
> Thanks.  These macros seem better implemented as static inline functions.
> That also nicely aligns the code with what you write in the patch
> description.

I don't think inlining these macros in the following way will work 
because of the following reason. It won't build once anything calls it.

csr_read()/csr_write() stringify the CSR argument straight into the 
inline asm template:

#define csr_read(csr)                                         \
({                                                            \
       register unsigned long __v;                             \
       __asm__ __volatile__ ("csrr %0, " __ASM_STR(csr)        \
                             : "=r" (__v) :                    \
                             : "memory");                      \
       __v;                                                    \
})

so the CSR operand has to be a literal token. With iregcsr as a function
parameter the template becomes "csrr %0, iregcsr", which will result in 
the following compilation error

Error: unknown CSR `iregcsr'

The patch description should be fixed to indicate that these are macros.>
> Also, I renamed this file to change the abbreviation "ind" to "indirect",
> along the lines of this feedback here:
> 
>     https://lore.kernel.org/linux-riscv/CAHk-=whhSLGZAx3N5jJpb4GLFDqH_QvS07D+6BnkPWmCEzTAgw@mail.gmail.com/
> 
> This case is even worse since there are already uses of "csr_index" in
> the codebase, so it's even more unclear what "ind" is supposed to mean.
> 

Agreed on the expanding the abbreviation part and we should change it.

> Updated patch follows.  Please let me know if you have any objections,
> 
> 
> - Paul
> 
> From: Atish Patra <[email protected]>
> 
> RISC-V: Define indirect CSR access helpers
> 
> The indirect CSR requires multiple instructions to read/write CSR.
> Add a few helper functions for ease of usage.
> 
> Signed-off-by: Atish Patra <[email protected]>
> Reviewed-by: Charlie Jenkins <[email protected]>
> Tested-by: Charlie Jenkins <[email protected]>
> Link: https://patch.msgid.link/[email protected]
> [[email protected]: expand "ind" abbreviation; use static inline functions rather than macros]
> Signed-off-by: Paul Walmsley <[email protected]>
> ---
>   arch/riscv/include/asm/csr_indirect.h | 51 +++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
>   create mode 100644 arch/riscv/include/asm/csr_indirect.h
> 
> diff --git a/arch/riscv/include/asm/csr_indirect.h b/arch/riscv/include/asm/csr_indirect.h
> new file mode 100644
> index 000000000000..3cd6a9059455
> --- /dev/null
> +++ b/arch/riscv/include/asm/csr_indirect.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef _ASM_RISCV_CSR_INDIRECT_H
> +#define _ASM_RISCV_CSR_INDIRECT_H
> +
> +#include <linux/types.h>
> +#include <linux/irqflags.h>
> +
> +#include <asm/csr.h>
> +
> +static inline unsigned long csr_indirect_read(u16 iregcsr, u32 iselbase, u32 iseloff)
> +{
> +	unsigned long __value = 0;
> +	unsigned long __flags;
> +
> +	local_irq_save(__flags);
> +	csr_write(CSR_ISELECT, iselbase + iseloff);
> +	__value = csr_read(iregcsr);
> +	local_irq_restore(__flags);
> +
> +	return __value;
> +}
> +
> +static inline void csr_indirect_write(u16 iregcsr, u32 iselbase, u32 iseloff, unsigned long value)
> +{
> +	unsigned long __flags;
> +
> +	local_irq_save(__flags);
> +	csr_write(CSR_ISELECT, iselbase + iseloff);
> +	csr_write(iregcsr, (value));
> +	local_irq_restore(__flags);
> +}
> +
> +static inline unsigned long csr_indirect_warl(u16 iregcsr, u32 iselbase, u32 iseloff,
> +					      unsigned long warl_val)
> +{
> +	unsigned long __old_val = 0, __value = 0;
> +	unsigned long __flags;
> +
> +	local_irq_save(__flags);
> +	csr_write(CSR_ISELECT, iselbase + iseloff);
> +	__old_val = csr_read(iregcsr);
> +	csr_write(iregcsr, warl_val);
> +	__value = csr_read(iregcsr);
> +	csr_write(iregcsr, __old_val);
> +	local_irq_restore(__flags);
> +
> +	return __value;
> +}
> +
> +#endif