Re: [PATCH v2 3/3] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Horia Geantă]

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  wrote:
>>>  #include 
>>>  #include 
>>> -#include 
>>> +#include 
>>
>> 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

Re: [PATCH v2 3/3] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 28/06/17 04:20 AM, Arnd Bergmann wrote:
> On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe  wrote:
>>  #include 
>>  #include 
>> -#include 
>> +#include 
> 
> 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

Re: [PATCH v2 3/3] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Arnd Bergmann]

On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe  wrote:
>  #include 
>  #include 
> -#include 
> +#include 

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

[PATCH v2 3/3] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

From: Horia Geantă 

We can now make use of the io-64-nonatomic-hi-lo header to always
provide 64 bit IO operations. So this patch cleans up the extra
CONFIG_64BIT ifdefs.

Signed-off-by: Horia Geantă 
Signed-off-by: Logan Gunthorpe 
Cc: Horia Geantă 
Cc: Dan Douglass 
Cc: Herbert Xu 
Cc: "David S. Miller" 
---
 drivers/crypto/caam/regs.h | 35 +--
 1 file changed, 5 insertions(+), 30 deletions(-)

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 
 #include 
-#include 
+#include 
 
 /*
  * Architecture-specific register access methods
@@ -134,50 +134,25 @@ static inline void clrsetbits_32(void __iomem *reg, u32 
clear, u32 set)
  *base + 0x : 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
-- 
2.11.0