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

[Nick Hu]

It seems like my last mail didn't send out successfully.

On Wed, Oct 2, 2024 at 9:52 AM Nick Hu <nick.hu@xxxxxxxxxx> wrote:
>
> Hi Andrew
>
> On Thu, Sep 26, 2024 at 3:59 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote:
>>
>> 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@xxxxxxxxxxxxxxxx>
>> > Suggested-by: Zong Li <zong.li@xxxxxxxxxx>
>> > Signed-off-by: Nick Hu <nick.hu@xxxxxxxxxx>
>> > ---
>> >  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
>
Sure I'll drop the patch in the next version


Regards,
Nick