Message ID | 20170627230204.16410-4-logang@deltatee.com (mailing list archive) |
---|---|
State | Not Applicable |
Delegated to: | Herbert Xu |
Headers | show |
On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe <logang@deltatee.com> wrote: > #include <linux/types.h> > #include <linux/bitops.h> > -#include <linux/io.h> > +#include <linux/io-64-nonatomic-hi-lo.h> Here you include the hi-lo variant unconditionally. > -#else /* CONFIG_64BIT */ > -static inline void wr_reg64(void __iomem *reg, u64 data) > -{ > -#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX > - if (caam_little_end) { > - wr_reg32((u32 __iomem *)(reg) + 1, data >> 32); > - wr_reg32((u32 __iomem *)(reg), data); > - } else > #endif > - { > - wr_reg32((u32 __iomem *)(reg), data >> 32); > - wr_reg32((u32 __iomem *)(reg) + 1, data); > - } > + iowrite64be(data, reg); > } However, the #else path here uses lo-hi instead. I guess we have to decide how to define iowrite64be_lo_hi() first: it could either byteswap the 64-bit value first, then write the two halves, or it could write the two halves, doing a 32-bit byte swap on each. Arnd
On 28/06/17 04:20 AM, Arnd Bergmann wrote: > On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe <logang@deltatee.com> wrote: >> #include <linux/types.h> >> #include <linux/bitops.h> >> -#include <linux/io.h> >> +#include <linux/io-64-nonatomic-hi-lo.h> > > Here you include the hi-lo variant unconditionally. > >> -#else /* CONFIG_64BIT */ >> -static inline void wr_reg64(void __iomem *reg, u64 data) >> -{ >> -#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX >> - if (caam_little_end) { >> - wr_reg32((u32 __iomem *)(reg) + 1, data >> 32); >> - wr_reg32((u32 __iomem *)(reg), data); >> - } else >> #endif >> - { >> - wr_reg32((u32 __iomem *)(reg), data >> 32); >> - wr_reg32((u32 __iomem *)(reg) + 1, data); >> - } >> + iowrite64be(data, reg); >> } > > However, the #else path here uses lo-hi instead. I guess we have > to decide how to define iowrite64be_lo_hi() first: it could > either byteswap the 64-bit value first, then write the two halves, > or it could write the two halves, doing a 32-bit byte swap on > each. Ok, I studied this a bit more: The lo_hi/hi_lo functions seem to always refer to the data being written or read not to the address operated on. So, in the v3 version of this set, which I'm working on, I've defined: static inline void iowrite64_hi_lo(u64 val, void __iomem *addr) { iowrite32(val >> 32, addr + sizeof(u32)); iowrite32(val, addr); } static inline void iowrite64be_hi_lo(u64 val, void __iomem *addr) { iowrite32be(val >> 32, addr); iowrite32be(val, addr + sizeof(u32)); } So the two hi_lo functions match both paths of the #if and thus, I believe, the patch will be correct in v3 without changes. Thanks, Logan
On 6/28/2017 7:51 PM, Logan Gunthorpe wrote: > > > On 28/06/17 04:20 AM, Arnd Bergmann wrote: >> On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe <logang@deltatee.com> wrote: >>> #include <linux/types.h> >>> #include <linux/bitops.h> >>> -#include <linux/io.h> >>> +#include <linux/io-64-nonatomic-hi-lo.h> >> >> Here you include the hi-lo variant unconditionally. >> Arnd, thanks for spotting this. This was not in the patch I signed off. The lo-hi variant should be used instead for CAAM, see further below. >>> -#else /* CONFIG_64BIT */ >>> -static inline void wr_reg64(void __iomem *reg, u64 data) >>> -{ >>> -#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX >>> - if (caam_little_end) { >>> - wr_reg32((u32 __iomem *)(reg) + 1, data >> 32); >>> - wr_reg32((u32 __iomem *)(reg), data); >>> - } else >>> #endif >>> - { >>> - wr_reg32((u32 __iomem *)(reg), data >> 32); >>> - wr_reg32((u32 __iomem *)(reg) + 1, data); >>> - } >>> + iowrite64be(data, reg); >>> } >> >> However, the #else path here uses lo-hi instead. I guess we have >> to decide how to define iowrite64be_lo_hi() first: it could >> either byteswap the 64-bit value first, then write the two halves, >> or it could write the two halves, doing a 32-bit byte swap on >> each. > > Ok, I studied this a bit more: > > The lo_hi/hi_lo functions seem to always refer to the data being written > or read not to the address operated on. So, in the v3 version of this > set, which I'm working on, I've defined: > > static inline void iowrite64_hi_lo(u64 val, void __iomem *addr) > { > iowrite32(val >> 32, addr + sizeof(u32)); > iowrite32(val, addr); > } > > static inline void iowrite64be_hi_lo(u64 val, void __iomem *addr) > { > iowrite32be(val >> 32, addr); > iowrite32be(val, addr + sizeof(u32)); > } > > So the two hi_lo functions match both paths of the #if and thus, I > believe, the patch will be correct in v3 without changes. > To be consistent with CAAM engine HW spec: in case of 64-bit registers, irrespective of device endianness, the lower address should be read from / written to first, followed by the upper address. Indeed the I/O accessors in CAAM driver currently don't follow the spec, however this is a good opportunity to fix the code. I don't consider this requires a separate patch, as we haven't noticed any problem. I'd say a simple note in the commit message mentioning the change (lo-hi r/w order replacing hi-lo for little endian case) is enough. Thanks, Horia
diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h index 84d2f838a063..fdcf719ecfbe 100644 --- a/drivers/crypto/caam/regs.h +++ b/drivers/crypto/caam/regs.h @@ -9,7 +9,7 @@ #include <linux/types.h> #include <linux/bitops.h> -#include <linux/io.h> +#include <linux/io-64-nonatomic-hi-lo.h> /* * Architecture-specific register access methods @@ -134,50 +134,25 @@ static inline void clrsetbits_32(void __iomem *reg, u32 clear, u32 set) * base + 0x0000 : least-significant 32 bits * base + 0x0004 : most-significant 32 bits */ -#ifdef CONFIG_64BIT static inline void wr_reg64(void __iomem *reg, u64 data) { +#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX if (caam_little_end) iowrite64(data, reg); else - iowrite64be(data, reg); -} - -static inline u64 rd_reg64(void __iomem *reg) -{ - if (caam_little_end) - return ioread64(reg); - else - return ioread64be(reg); -} - -#else /* CONFIG_64BIT */ -static inline void wr_reg64(void __iomem *reg, u64 data) -{ -#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX - if (caam_little_end) { - wr_reg32((u32 __iomem *)(reg) + 1, data >> 32); - wr_reg32((u32 __iomem *)(reg), data); - } else #endif - { - wr_reg32((u32 __iomem *)(reg), data >> 32); - wr_reg32((u32 __iomem *)(reg) + 1, data); - } + iowrite64be(data, reg); } static inline u64 rd_reg64(void __iomem *reg) { #ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX if (caam_little_end) - return ((u64)rd_reg32((u32 __iomem *)(reg) + 1) << 32 | - (u64)rd_reg32((u32 __iomem *)(reg))); + return ioread64(reg); else #endif - return ((u64)rd_reg32((u32 __iomem *)(reg)) << 32 | - (u64)rd_reg32((u32 __iomem *)(reg) + 1)); + return ioread64be(reg); } -#endif /* CONFIG_64BIT */ #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT #ifdef CONFIG_SOC_IMX7D