| Message ID | 20240926065422.226518-2-nick.hu@sifive.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Support SSTC while PM operations | expand |
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
It seems like my last mail didn't send out successfully. On Wed, Oct 2, 2024 at 9:52 AM Nick Hu <nick.hu@sifive.com> wrote: > > Hi Andrew > > On Thu, Sep 26, 2024 at 3:59 PM Andrew Jones <ajones@ventanamicro.com> 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@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 > Sure I'll drop the patch in the next version Regards, Nick
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); \
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(+)