| Message ID | 52A32C88.9010000@arkona-technologies.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Matthias Mann <M.Mann@arkona-technologies.de> writes: > Add readq/writeq methods for 32 bit ARM to allow transfering 64 bit words over > PCIe as a single transfer. > > Signed-off-by: Matthias Mann <m.mann@arkona-technologies.de> > --- > This patch creates checkpatch warnings, but I used the style used for the > existing functions. > It is based on branch next/soc of the arm-soc tree. > > arch/arm/include/asm/io.h | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > index 3c597c2..0a8d015 100644 > --- a/arch/arm/include/asm/io.h > +++ b/arch/arm/include/asm/io.h > @@ -94,6 +94,13 @@ static inline void __raw_writel(u32 val, volatile void __iomem *addr) > : "r" (val)); > } > > +static inline void __raw_writeq(u64 val, volatile void __iomem *addr) > +{ > + asm volatile("strd %1, %0" Please use "strd %Q1, %R1, %0" here instead of relying on the non-standard implicit second register operand. Although current gcc versions always allocate 64-bit values in even/odd register pairs, there is no guarantee that this will always be the case (and llvm has no such restriction). In Thumb2, the registers do not need to be consecutive, so implicitly adding 1 to the first register can silently result in incorrect code. For big endian, the register arguments need to be reversed. > + : "+Qo" (*(volatile u64 __force *)addr) The "o" constraint is not safe here. The ldrd/strd instructions have a limited offset range compared to ldr/str, so there is a risk that the compiler-generated address is invalid. Using "Q" forces the address to be a single register, which is always safe. This is why the 16-bit versions of these functions use only "Q". While this is slightly suboptimal, there is unfortunately no constraint describing the limitations of ldrd/strd addressing. > + : "r" (val)); > +}
> Matthias Mann <M.Mann@arkona-technologies.de> writes: > > > Add readq/writeq methods for 32 bit ARM to allow transfering 64 bit words over > > PCIe as a single transfer. > > > > Signed-off-by: Matthias Mann <m.mann@arkona-technologies.de> > > --- > > This patch creates checkpatch warnings, but I used the style used for the > > existing functions. > > It is based on branch next/soc of the arm-soc tree. > > > > arch/arm/include/asm/io.h | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > > index 3c597c2..0a8d015 100644 > > --- a/arch/arm/include/asm/io.h > > +++ b/arch/arm/include/asm/io.h > > @@ -94,6 +94,13 @@ static inline void __raw_writel(u32 val, volatile void __iomem *addr) > > : "r" (val)); > > } > > > > +static inline void __raw_writeq(u64 val, volatile void __iomem *addr) > > +{ > > + asm volatile("strd %1, %0" Fixme, how this will work for ARMv4? Is this supported for systems above v5te or I missed something? ---
On Sat, Dec 07, 2013 at 03:11:20PM +0100, Matthias Mann wrote: > Add readq/writeq methods for 32 bit ARM to allow transfering 64 bit words over > PCIe as a single transfer. Since these asm instructions are not universally implemented, use of these will lead to build time errors if a driver attempts to make use of these on an ARM architecture which doesn't support it. Hence, these need to be conditional - since they first appeared (iirc) in ARMv5, then they should be conditional on #if __LINUX_ARM_ARCH__ >= 5 However, there's another issue here. The use of readq/writeq() is controlled with various preprocessor or configuration symbols: #if BITS_PER_LONG >= 64 #ifdef CONFIG_64BIT #ifndef readq #ifndef writeq I much prefer the latter two, as it means drivers can individually implement the lacking support in a way which is appropriate for the device they're driving - which is especially important if reading a register has side effects. In other words, readq() and writeq() is not something that can be provided by an arch where no 64-bit read/write IO instructions exist. This requires that where an architecture provides them, that they be accompanied by these definitions: #define readq readq #define writeq writeq to prevent the drivers own private definitions of these accessors conflicting.
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 3c597c2..0a8d015 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -94,6 +94,13 @@ static inline void __raw_writel(u32 val, volatile void __iomem *addr) : "r" (val)); } +static inline void __raw_writeq(u64 val, volatile void __iomem *addr) +{ + asm volatile("strd %1, %0" + : "+Qo" (*(volatile u64 __force *)addr) + : "r" (val)); +} + static inline u8 __raw_readb(const volatile void __iomem *addr) { u8 val; @@ -112,6 +119,15 @@ static inline u32 __raw_readl(const volatile void __iomem *addr) return val; } +static inline u64 __raw_readq(const volatile void __iomem *addr) +{ + u64 val; + asm volatile("ldrd %1, %0" + : "+Qo" (*(volatile u64 __force *)addr), + "=r" (val)); + return val; +} + /* * Architecture ioremap implementation. */ @@ -293,18 +309,23 @@ extern void _memset_io(volatile void __iomem *, int, size_t); __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((__force __le64) \ + __raw_readq(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) __raw_writeq((__force u64) cpu_to_le64(v),c) #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(); __v; }) #define readw(c) ({ u16 __v = readw_relaxed(c); __iormb(); __v; }) #define readl(c) ({ u32 __v = readl_relaxed(c); __iormb(); __v; }) +#define readq(c) ({ u64 __v = readq_relaxed(c); __iormb(); __v; }) #define writeb(v,c) ({ __iowmb(); writeb_relaxed(v,c); }) #define writew(v,c) ({ __iowmb(); writew_relaxed(v,c); }) #define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); }) +#define writeq(v,c) ({ __iowmb(); writeq_relaxed(v,c); }) #define readsb(p,d,l) __raw_readsb(p,d,l) #define readsw(p,d,l) __raw_readsw(p,d,l)
Add readq/writeq methods for 32 bit ARM to allow transfering 64 bit words over PCIe as a single transfer. Signed-off-by: Matthias Mann <m.mann@arkona-technologies.de> --- This patch creates checkpatch warnings, but I used the style used for the existing functions. It is based on branch next/soc of the arm-soc tree. arch/arm/include/asm/io.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)