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; })
 
 #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; })
-- 
2.17.1

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,

-- 
Julien Grall

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.

-- 
Xenia