diff mbox

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

Message ID 20170627230204.16410-4-logang@deltatee.com (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show

Commit Message

Logan Gunthorpe June 27, 2017, 11:02 p.m. UTC 
From: Horia Geantă <horia.geanta@nxp.com>

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ă <horia.geanta@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Dan Douglass <dan.douglass@nxp.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
 drivers/crypto/caam/regs.h | 35 +++++------------------------------
 1 file changed, 5 insertions(+), 30 deletions(-)

Comments

Arnd Bergmann June 28, 2017, 10:20 a.m. UTC | #1 
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
Logan Gunthorpe June 28, 2017, 4:51 p.m. UTC | #2 
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
Horia Geanta June 29, 2017, 7:52 a.m. UTC | #3 
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 mbox

Patch

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