Re: [PATCH v2 1/3] riscv: Add csr_read/write_hi_lo support

[Andrew Jones <ajones@ventanamicro.com>]

On Thu, Sep 26, 2024 at 02:54:16PM GMT, Nick Hu wrote:
> In RV32, we may have the need to read both low 32 bit and high 32 bit of
> the CSR. Therefore adding the csr_read_hi_lo() and csr_write_hi_lo() to
> support such case.
> 
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Suggested-by: Zong Li <zong.li@sifive.com>
> Signed-off-by: Nick Hu <nick.hu@sifive.com>
> ---
>  arch/riscv/include/asm/csr.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index 25966995da04..54198284eb22 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -501,6 +501,17 @@
>  	__v;							\
>  })
>  
> +#if __riscv_xlen < 64
> +#define csr_read_hi_lo(csr)					\
> +({								\
> +	u32 hi = csr_read(csr##H);				\
> +	u32 lo = csr_read(csr);					\
> +	lo | ((u64)hi << 32);					\
> +})
> +#else
> +#define csr_read_hi_lo	csr_read
> +#endif
> +
>  #define csr_write(csr, val)					\
>  ({								\
>  	unsigned long __v = (unsigned long)(val);		\
> @@ -509,6 +520,17 @@
>  			      : "memory");			\
>  })
>  
> +#if __riscv_xlen < 64
> +#define csr_write_hi_lo(csr, val)				\
> +({								\
> +	u64 _v = (u64)(val);					\
> +	csr_write(csr##H, (_v) >> 32);				\
> +	csr_write(csr, (_v));					\
> +})
> +#else
> +#define csr_write_hi_lo	csr_write
> +#endif
> +
>  #define csr_read_set(csr, val)					\
>  ({								\
>  	unsigned long __v = (unsigned long)(val);		\
> -- 
> 2.34.1
>

I know I suggested this, but I'm having second thoughts. The nice thing
about the

 csr_write(CSR, ...);
 if (__riscv_xlen < 64)
    csr_write(CSRH, ...);

pattern is that it matches the spec. With this helper we'll have

 csr_write_hi_lo(CSR, ...);

for both rv32 and rv64. That looks odd for rv64 and hides the hi register
access for rv32.

We could avoid the oddness of the helper's name for rv64 if we instead
added csr_write32 and csr_write64 which do the right things, but that
still hides the hi register access for rv32. Hiding the hi register
access is probably fine, though, since we can be pretty certain that
the spec will rarely, if ever, deviate from naming high registers with
the H suffix and/or not keep the upper bits compatible.

In summary, I think I'm in favor of just dropping this patch, keeping
the noisy, but explicit, pattern. Or, if the consensus is to add
helpers, then I'd rather have all csr_<op>32/64 helpers. Then, code
would match the spec by choosing the right helper based on the width
of the CSR being accessed, when the CSR has an explicit width, or still
use the current helpers for xlen-wide CSRs.

Thanks,
drew