| Message ID | 20221021153128.44226-12-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm: Enable GICv3 for AArch32 | expand |
On 10/21/22 18:31, Ayan Kumar Halder wrote: Hi Ayan > Defined readq_relaxed()/writeq_relaxed() to read and write 64 bit regs. > This in turn calls readl_relaxed()/writel_relaxed() twice for the lower > and upper 32 bits. > > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > --- > xen/arch/arm/include/asm/arm32/io.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/xen/arch/arm/include/asm/arm32/io.h b/xen/arch/arm/include/asm/arm32/io.h > index 73a879e9fb..6a5f563fbc 100644 > --- a/xen/arch/arm/include/asm/arm32/io.h > +++ b/xen/arch/arm/include/asm/arm32/io.h > @@ -80,10 +80,14 @@ static inline u32 __raw_readl(const volatile void __iomem *addr) > __raw_readw(c)); __r; }) > #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \ > __raw_readl(c)); __r; }) > +#define readq_relaxed(c) ({ u64 __r = (le64_to_cpu(readl_relaxed(c+4)) << 32) | \ > + readl_relaxed(c); __r; }) Maybe you wanted to write sth like (((u64)readl_relaxed((c) + 4) << 32) | readl_relaxed(c)) readl_relaxed returns a u32 value so no byteorder conversions are needed at this stage. Also, you need to add parentheses around macro parameter c because an operator performs on it. > > #define writeb_relaxed(v,c) __raw_writeb(v,c) > #define writew_relaxed(v,c) __raw_writew((__force u16) cpu_to_le16(v),c) > #define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c) > +#define writeq_relaxed(v,c) writel_relaxed(((uint64_t)v&0xffffffff), c); \ > + writel_relaxed((((uint64_t)v)>>32), (c+4)); > > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(); __v; }) > #define readw(c) ({ u16 __v = readw_relaxed(c); __iormb(); __v; }) And writel_relaxed((u32)v, c); writel_relaxed((u32)((v) >> 32), (c) + 4); v is already u64 and writel_relaxed() expects a u32. Here as well, you need to add parentheses around macro parameter c because an operator performs on it. I am wondering if the parts of the register need to be accessed in a specific order.
Hi Ayan, On 21/10/2022 16:31, Ayan Kumar Halder wrote: > Defined readq_relaxed()/writeq_relaxed() to read and write 64 bit regs. > This in turn calls readl_relaxed()/writel_relaxed() twice for the lower > and upper 32 bits. This needs an explanation why we can't use "strd/ldrd". And the same would have to be duplicated in the code below. > > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > --- > xen/arch/arm/include/asm/arm32/io.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/xen/arch/arm/include/asm/arm32/io.h b/xen/arch/arm/include/asm/arm32/io.h > index 73a879e9fb..6a5f563fbc 100644 > --- a/xen/arch/arm/include/asm/arm32/io.h > +++ b/xen/arch/arm/include/asm/arm32/io.h > @@ -80,10 +80,14 @@ static inline u32 __raw_readl(const volatile void __iomem *addr) > __raw_readw(c)); __r; }) > #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \ > __raw_readl(c)); __r; }) > +#define readq_relaxed(c) ({ u64 __r = (le64_to_cpu(readl_relaxed(c+4)) << 32) | \ > + readl_relaxed(c); __r; }) All the read*_relaxed are provide atomic read. This is not guaranteed by your new helper. The name should be different (maybe readq_relaxed_non_atomic()) to make clear of the difference. I also don't quite understand the implementation. The value returned by readl_relaxed() is already in the CPU endianess. So why do you call le64_to_cpu() on top? > > #define writeb_relaxed(v,c) __raw_writeb(v,c) > #define writew_relaxed(v,c) __raw_writew((__force u16) cpu_to_le16(v),c) > #define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c) > +#define writeq_relaxed(v,c) writel_relaxed(((uint64_t)v&0xffffffff), c); \ > + writel_relaxed((((uint64_t)v)>>32), (c+4)); This needs to be surrounded with do { } while (0), otherwise the following would not properly work: if ( foo ) writeq_relaxed(v, c); Similarly, if 'v' is a call, then it will end up to be called twice. > > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(); __v; }) > #define readw(c) ({ u16 __v = readw_relaxed(c); __iormb(); __v; }) Cheers,
diff --git a/xen/arch/arm/include/asm/arm32/io.h b/xen/arch/arm/include/asm/arm32/io.h index 73a879e9fb..6a5f563fbc 100644 --- a/xen/arch/arm/include/asm/arm32/io.h +++ b/xen/arch/arm/include/asm/arm32/io.h @@ -80,10 +80,14 @@ static inline u32 __raw_readl(const volatile void __iomem *addr) __raw_readw(c)); __r; }) #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \ __raw_readl(c)); __r; }) +#define readq_relaxed(c) ({ u64 __r = (le64_to_cpu(readl_relaxed(c+4)) << 32) | \ + readl_relaxed(c); __r; }) #define writeb_relaxed(v,c) __raw_writeb(v,c) #define writew_relaxed(v,c) __raw_writew((__force u16) cpu_to_le16(v),c) #define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c) +#define writeq_relaxed(v,c) writel_relaxed(((uint64_t)v&0xffffffff), c); \ + writel_relaxed((((uint64_t)v)>>32), (c+4)); #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(); __v; }) #define readw(c) ({ u16 __v = readw_relaxed(c); __iormb(); __v; })
Defined readq_relaxed()/writeq_relaxed() to read and write 64 bit regs. This in turn calls readl_relaxed()/writel_relaxed() twice for the lower and upper 32 bits. Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> --- xen/arch/arm/include/asm/arm32/io.h | 4 ++++ 1 file changed, 4 insertions(+)