[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Steffen Trumtrar]

Hi!

I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
drivers/crypto/caam driver only works for PowerPC AFAIK.
Actually, there isn't that much to do, to get support for the i.MX6 but
one patch breaks the driver severely:

	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
	crypto: caam - Add definition of rd/wr_reg64 for little endian platform

This patch adds

+#ifdef __LITTLE_ENDIAN
+static inline void wr_reg64(u64 __iomem *reg, u64 data)
+{
+	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
+	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
+}

The wr_reg64 function is only used in one place in the drivers/crypto/caam/jr.c
driver: to write the dma_addr_t to the register. Without that patch everything
works fine on ARM (little endian, 32bit), with that patch the driver will write
0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
Also, from my understanding, the comment above the defines, stating that you
have to first write the numerically-lower and then the numerically-higher address
on 32bit systems doesn't match with the implementation.

What I don't know/understand is if this makes any sense for any PowerPC implementation.

So, the question is, how to fix this? I'd prefer to do it directly in the jr driver
instead of the ifdef-ery.

Something like
	if (sizeof(dma_addr_t) == sizeof(u32))
		wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
	else if (sizeof(dma_addr_t) == sizeof(u64))
		wr_reg64(...)

or just go by DT compatible and then remove the inline function definitions.

As far as I can tell, the compatible wouldn't be needed for anything else in the
jr driver, so maybe that is not optimal. On the other hand the sizeof(..)
solution would only catch little endian on 32bit and not big endian (?!)
I however don't know what combinations actually *have* to be caught, as I don't
know, which exist.

So, what do you think people?

Thanks,
Steffen Trumtrar

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Steffen Trumtrar]

Hi!

I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
drivers/crypto/caam driver only works for PowerPC AFAIK.
Actually, there isn't that much to do, to get support for the i.MX6 but
one patch breaks the driver severely:

	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
	crypto: caam - Add definition of rd/wr_reg64 for little endian platform

This patch adds

+#ifdef __LITTLE_ENDIAN
+static inline void wr_reg64(u64 __iomem *reg, u64 data)
+{
+	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
+	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
+}

The wr_reg64 function is only used in one place in the drivers/crypto/caam/jr.c
driver: to write the dma_addr_t to the register. Without that patch everything
works fine on ARM (little endian, 32bit), with that patch the driver will write
0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
Also, from my understanding, the comment above the defines, stating that you
have to first write the numerically-lower and then the numerically-higher address
on 32bit systems doesn't match with the implementation.

What I don't know/understand is if this makes any sense for any PowerPC implementation.

So, the question is, how to fix this? I'd prefer to do it directly in the jr driver
instead of the ifdef-ery.

Something like
	if (sizeof(dma_addr_t) == sizeof(u32))
		wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
	else if (sizeof(dma_addr_t) == sizeof(u64))
		wr_reg64(...)

or just go by DT compatible and then remove the inline function definitions.

As far as I can tell, the compatible wouldn't be needed for anything else in the
jr driver, so maybe that is not optimal. On the other hand the sizeof(..)
solution would only catch little endian on 32bit and not big endian (?!)
I however don't know what combinations actually *have* to be caught, as I don't
know, which exist.

So, what do you think people?

Thanks,
Steffen Trumtrar

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Russell King - ARM Linux]

On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
> drivers/crypto/caam driver only works for PowerPC AFAIK.
> Actually, there isn't that much to do, to get support for the i.MX6 but
> one patch breaks the driver severely:
> 
> 	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
> 	crypto: caam - Add definition of rd/wr_reg64 for little endian platform

You're not the only one who's hitting problems with this - Jon Nettleton
has been working on it recently.

The way this has been done is fairly yucky to start with: several things
about it are particularly horrid.  The first is the repetitive code
for the BE and LE cases, when all that's actually different is the
register order between the two code cases.

The second thing is the excessive use of masking - I'm pretty sure the
compiler won't complain with the below.

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 378ddc17f60e..ba0fa630a25d 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -84,34 +84,28 @@
 #endif
 
 #ifndef CONFIG_64BIT
-#ifdef __BIG_ENDIAN
-static inline void wr_reg64(u64 __iomem *reg, u64 data)
-{
-	wr_reg32((u32 __iomem *)reg, (data & 0xffffffff00000000ull) >> 32);
-	wr_reg32((u32 __iomem *)reg + 1, data & 0x00000000ffffffffull);
-}
-
-static inline u64 rd_reg64(u64 __iomem *reg)
-{
-	return (((u64)rd_reg32((u32 __iomem *)reg)) << 32) |
-		((u64)rd_reg32((u32 __iomem *)reg + 1));
-}
+#if defined(__BIG_ENDIAN)
+#define REG64_HI32(reg) ((u32 __iomem *)(reg))
+#define REG64_LO32(reg) ((u32 __iomem *)(reg) + 1)
+#elif defined(__LITTLE_ENDIAN)
+#define REG64_HI32(reg) ((u32 __iomem *)(reg) + 1)
+#define REG64_LO32(reg) ((u32 __iomem *)(reg))
 #else
-#ifdef __LITTLE_ENDIAN
+#error Broken endian?
+#endif
+
 static inline void wr_reg64(u64 __iomem *reg, u64 data)
 {
-	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
-	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
+	wr_reg32(REG64_HI32(reg), data >> 32);
+	wr_reg32(REG64_LO32(reg), data);
 }
 
 static inline u64 rd_reg64(u64 __iomem *reg)
 {
-	return (((u64)rd_reg32((u32 __iomem *)reg + 1)) << 32) |
-		((u64)rd_reg32((u32 __iomem *)reg));
+	return ((u64)rd_reg32(REG64_HI32(reg))) << 32 |
+		rd_reg32(REG64_LO32(reg));
 }
 #endif
-#endif
-#endif
 
 /*
  * jr_outentry

The second issue is that apparently, the register order doesn't actually
change for LE devices - in other words, the byte order within each register
does change, but they aren't a 64-bit register, they're two separate 32-bit
registers.  So, they should always be written as such.

So, I'd get rid of the #if defined(__BIG_ENDIAN) stuff and instead have:

+/*
+ * The DMA address register is a pair of 32-bit registers, and should
+ * always be accessed as such.
+ */
+#define REG64_HI32(reg) ((u32 __iomem *)(reg))
+#define REG64_LO32(reg) ((u32 __iomem *)(reg) + 1)


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Russell King - ARM Linux]

On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
> drivers/crypto/caam driver only works for PowerPC AFAIK.
> Actually, there isn't that much to do, to get support for the i.MX6 but
> one patch breaks the driver severely:
> 
> 	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
> 	crypto: caam - Add definition of rd/wr_reg64 for little endian platform

You're not the only one who's hitting problems with this - Jon Nettleton
has been working on it recently.

The way this has been done is fairly yucky to start with: several things
about it are particularly horrid.  The first is the repetitive code
for the BE and LE cases, when all that's actually different is the
register order between the two code cases.

The second thing is the excessive use of masking - I'm pretty sure the
compiler won't complain with the below.

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 378ddc17f60e..ba0fa630a25d 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -84,34 +84,28 @@
 #endif
 
 #ifndef CONFIG_64BIT
-#ifdef __BIG_ENDIAN
-static inline void wr_reg64(u64 __iomem *reg, u64 data)
-{
-	wr_reg32((u32 __iomem *)reg, (data & 0xffffffff00000000ull) >> 32);
-	wr_reg32((u32 __iomem *)reg + 1, data & 0x00000000ffffffffull);
-}
-
-static inline u64 rd_reg64(u64 __iomem *reg)
-{
-	return (((u64)rd_reg32((u32 __iomem *)reg)) << 32) |
-		((u64)rd_reg32((u32 __iomem *)reg + 1));
-}
+#if defined(__BIG_ENDIAN)
+#define REG64_HI32(reg) ((u32 __iomem *)(reg))
+#define REG64_LO32(reg) ((u32 __iomem *)(reg) + 1)
+#elif defined(__LITTLE_ENDIAN)
+#define REG64_HI32(reg) ((u32 __iomem *)(reg) + 1)
+#define REG64_LO32(reg) ((u32 __iomem *)(reg))
 #else
-#ifdef __LITTLE_ENDIAN
+#error Broken endian?
+#endif
+
 static inline void wr_reg64(u64 __iomem *reg, u64 data)
 {
-	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
-	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
+	wr_reg32(REG64_HI32(reg), data >> 32);
+	wr_reg32(REG64_LO32(reg), data);
 }
 
 static inline u64 rd_reg64(u64 __iomem *reg)
 {
-	return (((u64)rd_reg32((u32 __iomem *)reg + 1)) << 32) |
-		((u64)rd_reg32((u32 __iomem *)reg));
+	return ((u64)rd_reg32(REG64_HI32(reg))) << 32 |
+		rd_reg32(REG64_LO32(reg));
 }
 #endif
-#endif
-#endif
 
 /*
  * jr_outentry

The second issue is that apparently, the register order doesn't actually
change for LE devices - in other words, the byte order within each register
does change, but they aren't a 64-bit register, they're two separate 32-bit
registers.  So, they should always be written as such.

So, I'd get rid of the #if defined(__BIG_ENDIAN) stuff and instead have:

+/*
+ * The DMA address register is a pair of 32-bit registers, and should
+ * always be accessed as such.
+ */
+#define REG64_HI32(reg) ((u32 __iomem *)(reg))
+#define REG64_LO32(reg) ((u32 __iomem *)(reg) + 1)


-- 
FTTC broadband for 0.8mile line: currently@10.5Mbps down 400kbps up
according to speedtest.net.

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Jon Nettleton]

[-- Attachment #1: Type: text/plain, Size: 2700 bytes --]

On Mon, Jun 15, 2015 at 5:59 PM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Hi!
>
> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
> drivers/crypto/caam driver only works for PowerPC AFAIK.
> Actually, there isn't that much to do, to get support for the i.MX6 but
> one patch breaks the driver severely:
>
>         commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
>         crypto: caam - Add definition of rd/wr_reg64 for little endian platform
>
> This patch adds
>
> +#ifdef __LITTLE_ENDIAN
> +static inline void wr_reg64(u64 __iomem *reg, u64 data)
> +{
> +       wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
> +       wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
> +}
>
> The wr_reg64 function is only used in one place in the drivers/crypto/caam/jr.c
> driver: to write the dma_addr_t to the register. Without that patch everything
> works fine on ARM (little endian, 32bit), with that patch the driver will write
> 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
> Also, from my understanding, the comment above the defines, stating that you
> have to first write the numerically-lower and then the numerically-higher address
> on 32bit systems doesn't match with the implementation.
>
> What I don't know/understand is if this makes any sense for any PowerPC implementation.
>
> So, the question is, how to fix this? I'd prefer to do it directly in the jr driver
> instead of the ifdef-ery.
>
> Something like
>         if (sizeof(dma_addr_t) == sizeof(u32))
>                 wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
>         else if (sizeof(dma_addr_t) == sizeof(u64))
>                 wr_reg64(...)
>
> or just go by DT compatible and then remove the inline function definitions.
>
> As far as I can tell, the compatible wouldn't be needed for anything else in the
> jr driver, so maybe that is not optimal. On the other hand the sizeof(..)
> solution would only catch little endian on 32bit and not big endian (?!)
> I however don't know what combinations actually *have* to be caught, as I don't
> know, which exist.
>
> So, what do you think people?

Funny enough I tackled this problem over the weekend as well.  My
approach was to switch the driver over to use the *_relaxed() io
functions and then special case the bits missing from the various
ARCHs.  Basically adding setbits32 and clrbits32 for !PPC
architectures and letting PPC and ARM share a writeq/readq set of
functions.  I left the existing LITTLE_ENDIAN special case until I
could verify if it was needed, or had been tested.

For reference, the patch against 4.1-rc5/6 is attached.

-Jon

[-- Attachment #2: 0001-crypto-caam-use-generic-_relaxed-io-functions.patch --]
[-- Type: application/octet-stream, Size: 14156 bytes --]

From 7fc64b43b214ab3b39504a86a3561cc6b70c9555 Mon Sep 17 00:00:00 2001
From: Jon Nettleton <jon.nettleton@gmail.com>
Date: Fri, 12 Jun 2015 14:51:02 +0200
Subject: [PATCH] crypto: caam: use generic *_relaxed io functions

In preparation for supporting ARM architectures this removes much
of the various defines for the different architecture combinations
by relying on the arch specific definitions.  The two ommissions
are the PPC specific bits that need to be added and the 64-bit
readq/writeq implementations.  Note this change reverts commit
ef94b1d834aace7101de77c3a7c2631b9ae9c5f6 as that change does not
work for ARM little endian systems.
---
 drivers/crypto/caam/ctrl.c | 46 ++++++++++++++++++-------------------
 drivers/crypto/caam/jr.c   | 34 ++++++++++++++--------------
 drivers/crypto/caam/regs.h | 56 +++++++++++++++-------------------------------
 3 files changed, 58 insertions(+), 78 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index efba4cc..dddce65 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -90,7 +90,7 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc,
 	if (ctrlpriv->virt_en == 1) {
 		setbits32(&ctrl->deco_rsr, DECORSR_JR0);
 
-		while (!(rd_reg32(&ctrl->deco_rsr) & DECORSR_VALID) &&
+		while (!(readl_relaxed(&ctrl->deco_rsr) & DECORSR_VALID) &&
 		       --timeout)
 			cpu_relax();
 
@@ -99,7 +99,7 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc,
 
 	setbits32(&ctrl->deco_rq, DECORR_RQD0ENABLE);
 
-	while (!(rd_reg32(&ctrl->deco_rq) & DECORR_DEN0) &&
+	while (!(readl_relaxed(&ctrl->deco_rq) & DECORR_DEN0) &&
 								 --timeout)
 		cpu_relax();
 
@@ -110,7 +110,7 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc,
 	}
 
 	for (i = 0; i < desc_len(desc); i++)
-		wr_reg32(&deco->descbuf[i], *(desc + i));
+		writel_relaxed(*(desc + i), &deco->descbuf[i]);
 
 	flags = DECO_JQCR_WHL;
 	/*
@@ -121,11 +121,11 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc,
 		flags |= DECO_JQCR_FOUR;
 
 	/* Instruct the DECO to execute it */
-	wr_reg32(&deco->jr_ctl_hi, flags);
+	writel_relaxed(flags, &deco->jr_ctl_hi);
 
 	timeout = 10000000;
 	do {
-		deco_dbg_reg = rd_reg32(&deco->desc_dbg);
+		deco_dbg_reg = readl_relaxed(&deco->desc_dbg);
 		/*
 		 * If an error occured in the descriptor, then
 		 * the DECO status field will be set to 0x0D
@@ -136,7 +136,7 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc,
 		cpu_relax();
 	} while ((deco_dbg_reg & DESC_DBG_DECO_STAT_VALID) && --timeout);
 
-	*status = rd_reg32(&deco->op_status_hi) &
+	*status = readl_relaxed(&deco->op_status_hi) &
 		  DECO_OP_STATUS_HI_ERR_MASK;
 
 	if (ctrlpriv->virt_en == 1)
@@ -206,7 +206,7 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
 		 * without any error (HW optimizations for later
 		 * CAAM eras), then try again.
 		 */
-		rdsta_val = rd_reg32(&ctrl->r4tst[0].rdsta) & RDSTA_IFMASK;
+		rdsta_val = readl_relaxed(&ctrl->r4tst[0].rdsta) & RDSTA_IFMASK;
 		if (status || !(rdsta_val & (1 << sh_idx)))
 			ret = -EAGAIN;
 		if (ret)
@@ -334,7 +334,7 @@ static void kick_trng(struct platform_device *pdev, int ent_delay)
 	 * time trying to set the values controlling the sample
 	 * frequency, the function simply returns.
 	 */
-	val = (rd_reg32(&r4tst->rtsdctl) & RTSDCTL_ENT_DLY_MASK)
+	val = (readl_relaxed(&r4tst->rtsdctl) & RTSDCTL_ENT_DLY_MASK)
 	      >> RTSDCTL_ENT_DLY_SHIFT;
 	if (ent_delay <= val) {
 		/* put RNG4 into run mode */
@@ -342,16 +342,16 @@ static void kick_trng(struct platform_device *pdev, int ent_delay)
 		return;
 	}
 
-	val = rd_reg32(&r4tst->rtsdctl);
+	val = readl_relaxed(&r4tst->rtsdctl);
 	val = (val & ~RTSDCTL_ENT_DLY_MASK) |
 	      (ent_delay << RTSDCTL_ENT_DLY_SHIFT);
-	wr_reg32(&r4tst->rtsdctl, val);
+	writel_relaxed(val, &r4tst->rtsdctl);
 	/* min. freq. count, equal to 1/4 of the entropy sample length */
-	wr_reg32(&r4tst->rtfrqmin, ent_delay >> 2);
+	writel_relaxed(ent_delay >> 2, &r4tst->rtfrqmin);
 	/* disable maximum frequency count */
-	wr_reg32(&r4tst->rtfrqmax, RTFRQMAX_DISABLE);
+	writel_relaxed(RTFRQMAX_DISABLE, &r4tst->rtfrqmax);
 	/* read the control register */
-	val = rd_reg32(&r4tst->rtmctl);
+	val = readl_relaxed(&r4tst->rtmctl);
 	/*
 	 * select raw sampling in both entropy shifter
 	 * and statistical checker
@@ -360,7 +360,7 @@ static void kick_trng(struct platform_device *pdev, int ent_delay)
 	/* put RNG4 into run mode */
 	clrbits32(&val, RTMCTL_PRGM);
 	/* write back the control register */
-	wr_reg32(&r4tst->rtmctl, val);
+	writel_relaxed(val, &r4tst->rtmctl);
 }
 
 /**
@@ -416,7 +416,7 @@ static int caam_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 	/* Finding the page size for using the CTPR_MS register */
-	comp_params = rd_reg32(&ctrl->perfmon.comp_parms_ms);
+	comp_params = readl_relaxed(&ctrl->perfmon.comp_parms_ms);
 	pg_size = (comp_params & CTPR_MS_PG_SZ_MASK) >> CTPR_MS_PG_SZ_SHIFT;
 
 	/* Allocating the BLOCK_OFFSET based on the supported page size on
@@ -451,7 +451,7 @@ static int caam_probe(struct platform_device *pdev)
 	 *  Read the Compile Time paramters and SCFGR to determine
 	 * if Virtualization is enabled for this platform
 	 */
-	scfgr = rd_reg32(&ctrl->scfgr);
+	scfgr = readl_relaxed(&ctrl->scfgr);
 
 	ctrlpriv->virt_en = 0;
 	if (comp_params & CTPR_MS_VIRT_EN_INCL) {
@@ -523,7 +523,7 @@ static int caam_probe(struct platform_device *pdev)
 
 	/* Check to see if QI present. If so, enable */
 	ctrlpriv->qi_present =
-			!!(rd_reg32(&ctrl->perfmon.comp_parms_ms) &
+			!!(readl_relaxed(&ctrl->perfmon.comp_parms_ms) &
 			   CTPR_MS_QI_MASK);
 	if (ctrlpriv->qi_present) {
 		ctrlpriv->qi = (struct caam_queue_if __force *)
@@ -531,7 +531,7 @@ static int caam_probe(struct platform_device *pdev)
 				 BLOCK_OFFSET * QI_BLOCK_NUMBER
 			       );
 		/* This is all that's required to physically enable QI */
-		wr_reg32(&ctrlpriv->qi->qi_control_lo, QICTL_DQEN);
+		writel_relaxed(QICTL_DQEN, &ctrlpriv->qi->qi_control_lo);
 	}
 
 	/* If no QI and no rings specified, quit and go home */
@@ -541,7 +541,7 @@ static int caam_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	cha_vid_ls = rd_reg32(&ctrl->perfmon.cha_id_ls);
+	cha_vid_ls = readl_relaxed(&ctrl->perfmon.cha_id_ls);
 
 	/*
 	 * If SEC has RNG version >= 4 and RNG state handle has not been
@@ -549,7 +549,7 @@ static int caam_probe(struct platform_device *pdev)
 	 */
 	if ((cha_vid_ls & CHA_ID_LS_RNG_MASK) >> CHA_ID_LS_RNG_SHIFT >= 4) {
 		ctrlpriv->rng4_sh_init =
-			rd_reg32(&ctrl->r4tst[0].rdsta);
+			readl_relaxed(&ctrl->r4tst[0].rdsta);
 		/*
 		 * If the secure keys (TDKEK, JDKEK, TDSK), were already
 		 * generated, signal this to the function that is instantiating
@@ -560,7 +560,7 @@ static int caam_probe(struct platform_device *pdev)
 		ctrlpriv->rng4_sh_init &= RDSTA_IFMASK;
 		do {
 			int inst_handles =
-				rd_reg32(&ctrl->r4tst[0].rdsta) &
+				readl_relaxed(&ctrl->r4tst[0].rdsta) &
 								RDSTA_IFMASK;
 			/*
 			 * If either SH were instantiated by somebody else
@@ -610,8 +610,8 @@ static int caam_probe(struct platform_device *pdev)
 
 	/* NOTE: RTIC detection ought to go here, around Si time */
 
-	caam_id = (u64)rd_reg32(&ctrl->perfmon.caam_id_ms) << 32 |
-		  (u64)rd_reg32(&ctrl->perfmon.caam_id_ls);
+	caam_id = (u64)readl_relaxed(&ctrl->perfmon.caam_id_ms) << 32 |
+		  readl_relaxed(&ctrl->perfmon.caam_id_ls);
 
 	/* Report "alive" for developer to see */
 	dev_info(dev, "device ID = 0x%016llx (Era %d)\n", caam_id,
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index b8b5d47..ae1ddc2 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -34,12 +34,12 @@ static int caam_reset_hw_jr(struct device *dev)
 	setbits32(&jrp->rregs->rconfig_lo, JRCFG_IMSK);
 
 	/* initiate flush (required prior to reset) */
-	wr_reg32(&jrp->rregs->jrcommand, JRCR_RESET);
-	while (((rd_reg32(&jrp->rregs->jrintstatus) & JRINT_ERR_HALT_MASK) ==
+	writel_relaxed(JRCR_RESET, &jrp->rregs->jrcommand);
+	while (((readl_relaxed(&jrp->rregs->jrintstatus) & JRINT_ERR_HALT_MASK) ==
 		JRINT_ERR_HALT_INPROGRESS) && --timeout)
 		cpu_relax();
 
-	if ((rd_reg32(&jrp->rregs->jrintstatus) & JRINT_ERR_HALT_MASK) !=
+	if ((readl_relaxed(&jrp->rregs->jrintstatus) & JRINT_ERR_HALT_MASK) !=
 	    JRINT_ERR_HALT_COMPLETE || timeout == 0) {
 		dev_err(dev, "failed to flush job ring %d\n", jrp->ridx);
 		return -EIO;
@@ -47,8 +47,8 @@ static int caam_reset_hw_jr(struct device *dev)
 
 	/* initiate reset */
 	timeout = 100000;
-	wr_reg32(&jrp->rregs->jrcommand, JRCR_RESET);
-	while ((rd_reg32(&jrp->rregs->jrcommand) & JRCR_RESET) && --timeout)
+	writel_relaxed(JRCR_RESET, &jrp->rregs->jrcommand);
+	while ((readl_relaxed(&jrp->rregs->jrcommand) & JRCR_RESET) && --timeout)
 		cpu_relax();
 
 	if (timeout == 0) {
@@ -79,8 +79,8 @@ int caam_jr_shutdown(struct device *dev)
 	free_irq(jrp->irq, dev);
 
 	/* Free rings */
-	inpbusaddr = rd_reg64(&jrp->rregs->inpring_base);
-	outbusaddr = rd_reg64(&jrp->rregs->outring_base);
+	inpbusaddr = readq_relaxed(&jrp->rregs->inpring_base);
+	outbusaddr = readq_relaxed(&jrp->rregs->outring_base);
 	dma_free_coherent(dev, sizeof(dma_addr_t) * JOBR_DEPTH,
 			  jrp->inpring, inpbusaddr);
 	dma_free_coherent(dev, sizeof(struct jr_outentry) * JOBR_DEPTH,
@@ -132,7 +132,7 @@ static irqreturn_t caam_jr_interrupt(int irq, void *st_dev)
 	 * Check the output ring for ready responses, kick
 	 * tasklet if jobs done.
 	 */
-	irqstate = rd_reg32(&jrp->rregs->jrintstatus);
+	irqstate = readl_relaxed(&jrp->rregs->jrintstatus);
 	if (!irqstate)
 		return IRQ_NONE;
 
@@ -150,7 +150,7 @@ static irqreturn_t caam_jr_interrupt(int irq, void *st_dev)
 	setbits32(&jrp->rregs->rconfig_lo, JRCFG_IMSK);
 
 	/* Have valid interrupt at this point, just ACK and trigger */
-	wr_reg32(&jrp->rregs->jrintstatus, irqstate);
+	writel_relaxed(irqstate, &jrp->rregs->jrintstatus);
 
 	preempt_disable();
 	tasklet_schedule(&jrp->irqtask);
@@ -169,7 +169,7 @@ static void caam_jr_dequeue(unsigned long devarg)
 	u32 *userdesc, userstatus;
 	void *userarg;
 
-	while (rd_reg32(&jrp->rregs->outring_used)) {
+	while (readl_relaxed(&jrp->rregs->outring_used)) {
 
 		head = ACCESS_ONCE(jrp->head);
 
@@ -203,7 +203,7 @@ static void caam_jr_dequeue(unsigned long devarg)
 		userstatus = jrp->outring[hw_idx].jrstatus;
 
 		/* set done */
-		wr_reg32(&jrp->rregs->outring_rmvd, 1);
+		writel_relaxed(1, &jrp->rregs->outring_rmvd);
 
 		jrp->out_ring_read_index = (jrp->out_ring_read_index + 1) &
 					   (JOBR_DEPTH - 1);
@@ -335,7 +335,7 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
 	head = jrp->head;
 	tail = ACCESS_ONCE(jrp->tail);
 
-	if (!rd_reg32(&jrp->rregs->inpring_avail) ||
+	if (!readl_relaxed(&jrp->rregs->inpring_avail) ||
 	    CIRC_SPACE(head, tail, JOBR_DEPTH) <= 0) {
 		spin_unlock_bh(&jrp->inplock);
 		dma_unmap_single(dev, desc_dma, desc_size, DMA_TO_DEVICE);
@@ -357,7 +357,7 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
 				    (JOBR_DEPTH - 1);
 	jrp->head = (head + 1) & (JOBR_DEPTH - 1);
 
-	wr_reg32(&jrp->rregs->inpring_jobadd, 1);
+	writel_relaxed(1, &jrp->rregs->inpring_jobadd);
 
 	spin_unlock_bh(&jrp->inplock);
 
@@ -416,10 +416,10 @@ static int caam_jr_init(struct device *dev)
 	jrp->head = 0;
 	jrp->tail = 0;
 
-	wr_reg64(&jrp->rregs->inpring_base, inpbusaddr);
-	wr_reg64(&jrp->rregs->outring_base, outbusaddr);
-	wr_reg32(&jrp->rregs->inpring_size, JOBR_DEPTH);
-	wr_reg32(&jrp->rregs->outring_size, JOBR_DEPTH);
+	writeq_relaxed(inpbusaddr, &jrp->rregs->inpring_base);
+	writeq_relaxed(outbusaddr, &jrp->rregs->outring_base);
+	writel_relaxed(JOBR_DEPTH, &jrp->rregs->inpring_size);
+	writel_relaxed(JOBR_DEPTH, &jrp->rregs->outring_size);
 
 	jrp->ringsize = JOBR_DEPTH;
 
diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 378ddc1..0f7d3b8 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -65,52 +65,32 @@
  *
  */
 
-#ifdef __BIG_ENDIAN
-#define wr_reg32(reg, data) out_be32(reg, data)
-#define rd_reg32(reg) in_be32(reg)
-#ifdef CONFIG_64BIT
-#define wr_reg64(reg, data) out_be64(reg, data)
-#define rd_reg64(reg) in_be64(reg)
-#endif
+#ifndef CONFIG_64BIT
+#if defined(__BIG_ENDIAN) || defined (CONFIG_ARM)
+#define REG64_HI32(reg) ((u32 __iomem *)(reg))
+#define REG64_LO32(reg) ((u32 __iomem *)(reg) + 1)
 #else
-#ifdef __LITTLE_ENDIAN
-#define wr_reg32(reg, data) __raw_writel(data, reg)
-#define rd_reg32(reg) __raw_readl(reg)
-#ifdef CONFIG_64BIT
-#define wr_reg64(reg, data) __raw_writeq(data, reg)
-#define rd_reg64(reg) __raw_readq(reg)
-#endif
-#endif
+#define REG64_HI32(reg) ((u32 __iomem *)(reg) + 1)
+#define REG64_LO32(reg) ((u32 __iomem *)(reg))
 #endif
 
-#ifndef CONFIG_64BIT
-#ifdef __BIG_ENDIAN
-static inline void wr_reg64(u64 __iomem *reg, u64 data)
-{
-	wr_reg32((u32 __iomem *)reg, (data & 0xffffffff00000000ull) >> 32);
-	wr_reg32((u32 __iomem *)reg + 1, data & 0x00000000ffffffffull);
-}
-
-static inline u64 rd_reg64(u64 __iomem *reg)
+static inline void writeq(u64 data, u64 __iomem *reg)
 {
-	return (((u64)rd_reg32((u32 __iomem *)reg)) << 32) |
-		((u64)rd_reg32((u32 __iomem *)reg + 1));
+	writel_relaxed(data >> 32, REG64_HI32(reg));
+	writel_relaxed(data, REG64_LO32(reg));
 }
-#else
-#ifdef __LITTLE_ENDIAN
-static inline void wr_reg64(u64 __iomem *reg, u64 data)
+ 
+static inline u64 readq(u64 __iomem *reg)
 {
-	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
-	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
+	return ((u64)readl_relaxed(REG64_HI32(reg))) << 32 |
+		readl_relaxed(REG64_LO32(reg));
 }
-
-static inline u64 rd_reg64(u64 __iomem *reg)
-{
-	return (((u64)rd_reg32((u32 __iomem *)reg + 1)) << 32) |
-		((u64)rd_reg32((u32 __iomem *)reg));
-}
-#endif
 #endif
+
+/* These are common macros for Power add them if they aren't defined*/
+#ifndef CONFIG_PPC
+#define setbits32(_addr, _v) writel((readl(_addr) | (_v)), (_addr))
+#define clrbits32(_addr, _v) writel((readl(_addr) & ~(_v)), (_addr))
 #endif
 
 /*
-- 
1.8.3.1

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Jon Nettleton]

On Mon, Jun 15, 2015 at 5:59 PM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Hi!
>
> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
> drivers/crypto/caam driver only works for PowerPC AFAIK.
> Actually, there isn't that much to do, to get support for the i.MX6 but
> one patch breaks the driver severely:
>
>         commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
>         crypto: caam - Add definition of rd/wr_reg64 for little endian platform
>
> This patch adds
>
> +#ifdef __LITTLE_ENDIAN
> +static inline void wr_reg64(u64 __iomem *reg, u64 data)
> +{
> +       wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
> +       wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
> +}
>
> The wr_reg64 function is only used in one place in the drivers/crypto/caam/jr.c
> driver: to write the dma_addr_t to the register. Without that patch everything
> works fine on ARM (little endian, 32bit), with that patch the driver will write
> 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
> Also, from my understanding, the comment above the defines, stating that you
> have to first write the numerically-lower and then the numerically-higher address
> on 32bit systems doesn't match with the implementation.
>
> What I don't know/understand is if this makes any sense for any PowerPC implementation.
>
> So, the question is, how to fix this? I'd prefer to do it directly in the jr driver
> instead of the ifdef-ery.
>
> Something like
>         if (sizeof(dma_addr_t) == sizeof(u32))
>                 wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
>         else if (sizeof(dma_addr_t) == sizeof(u64))
>                 wr_reg64(...)
>
> or just go by DT compatible and then remove the inline function definitions.
>
> As far as I can tell, the compatible wouldn't be needed for anything else in the
> jr driver, so maybe that is not optimal. On the other hand the sizeof(..)
> solution would only catch little endian on 32bit and not big endian (?!)
> I however don't know what combinations actually *have* to be caught, as I don't
> know, which exist.
>
> So, what do you think people?

Funny enough I tackled this problem over the weekend as well.  My
approach was to switch the driver over to use the *_relaxed() io
functions and then special case the bits missing from the various
ARCHs.  Basically adding setbits32 and clrbits32 for !PPC
architectures and letting PPC and ARM share a writeq/readq set of
functions.  I left the existing LITTLE_ENDIAN special case until I
could verify if it was needed, or had been tested.

For reference, the patch against 4.1-rc5/6 is attached.

-Jon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-crypto-caam-use-generic-_relaxed-io-functions.patch
Type: application/octet-stream
Size: 14156 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150615/de0b9a37/attachment-0001.obj>

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Russell King - ARM Linux]

On Mon, Jun 15, 2015 at 06:33:17PM +0200, Jon Nettleton wrote:
> Funny enough I tackled this problem over the weekend as well.  My
> approach was to switch the driver over to use the *_relaxed() io
> functions and then special case the bits missing from the various
> ARCHs.  Basically adding setbits32 and clrbits32 for !PPC
> architectures and letting PPC and ARM share a writeq/readq set of
> functions.  I left the existing LITTLE_ENDIAN special case until I
> could verify if it was needed, or had been tested.

I'll follow up here with what I've mentioned elsewhere, and some further
thoughts.

I think this shows the dangers of using struct { } to define register
offsets.  Let's start here:

/*
 * caam_job_ring - direct job ring setup
 * 1-4 possible per instantiation, base + 1000/2000/3000/4000
 * Padded out to 0x1000
 */
struct caam_job_ring {
        /* Input ring */
        u64 inpring_base;       /* IRBAx -  Input desc ring baseaddr */
        u32 rsvd1;

Apparently, this is a CPU-endian 64-bit value (it's not defined using
le64 or be64 which would "fix" it's endian.)

The second question, which comes up in light of the breakage that's
being reported is: is this really a 64-bit register, or is it a pair
of 32-bit registers side-by-side?

The documentation I'm looking at doesn't document the register at
base + 0x1000, but documents the one at base + 0x1004, and the one
at 0x1004 is given the name "IRBAR0_LS", which presumably stands
for "input ring base address register 0, least significant".

As the code originally stood for PPC, IRBAR0_LS is also at 0x1004,
but appears to be big endian.

On ARM, IRBAR0_LS appears at the same address, but is little endian.

This is *not* a 64-bit register at all, but is a pair of 32-bit
registers side by side.  Moreover, readq() should not be used - no
amount of arch mangling could ever produce a sane readq() which
coped with this.

So, the CAAM code is buggy in this regard: using readq() here when
endian-portability is required is wrong.  It's got to be two 32-bit
reads or two 32-bit writes in the appropriate endian.

Also, note that there's a big difference between __raw_readl() and
readl_relaxed().  readl_relaxed() is always little-endian.  __raw_readl()
is god-knows-what-the-archtecture-decides endian.  Switching PPC
drivers from __raw_readl() to readl_relaxed() is really not a good
idea unless someone from the PPC camp reviews and tests the code.

So, what I'd suggest is just fixing rd_reg64() and wr_reg64() to do
the right thing: keeping the two 32-bit words in the same order
irrespective of the endian-ness, and staying with the __raw_*
accessors until PPC people can look at this.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Russell King - ARM Linux]

On Mon, Jun 15, 2015 at 06:33:17PM +0200, Jon Nettleton wrote:
> Funny enough I tackled this problem over the weekend as well.  My
> approach was to switch the driver over to use the *_relaxed() io
> functions and then special case the bits missing from the various
> ARCHs.  Basically adding setbits32 and clrbits32 for !PPC
> architectures and letting PPC and ARM share a writeq/readq set of
> functions.  I left the existing LITTLE_ENDIAN special case until I
> could verify if it was needed, or had been tested.

I'll follow up here with what I've mentioned elsewhere, and some further
thoughts.

I think this shows the dangers of using struct { } to define register
offsets.  Let's start here:

/*
 * caam_job_ring - direct job ring setup
 * 1-4 possible per instantiation, base + 1000/2000/3000/4000
 * Padded out to 0x1000
 */
struct caam_job_ring {
        /* Input ring */
        u64 inpring_base;       /* IRBAx -  Input desc ring baseaddr */
        u32 rsvd1;

Apparently, this is a CPU-endian 64-bit value (it's not defined using
le64 or be64 which would "fix" it's endian.)

The second question, which comes up in light of the breakage that's
being reported is: is this really a 64-bit register, or is it a pair
of 32-bit registers side-by-side?

The documentation I'm looking at doesn't document the register at
base + 0x1000, but documents the one at base + 0x1004, and the one
at 0x1004 is given the name "IRBAR0_LS", which presumably stands
for "input ring base address register 0, least significant".

As the code originally stood for PPC, IRBAR0_LS is also at 0x1004,
but appears to be big endian.

On ARM, IRBAR0_LS appears at the same address, but is little endian.

This is *not* a 64-bit register at all, but is a pair of 32-bit
registers side by side.  Moreover, readq() should not be used - no
amount of arch mangling could ever produce a sane readq() which
coped with this.

So, the CAAM code is buggy in this regard: using readq() here when
endian-portability is required is wrong.  It's got to be two 32-bit
reads or two 32-bit writes in the appropriate endian.

Also, note that there's a big difference between __raw_readl() and
readl_relaxed().  readl_relaxed() is always little-endian.  __raw_readl()
is god-knows-what-the-archtecture-decides endian.  Switching PPC
drivers from __raw_readl() to readl_relaxed() is really not a good
idea unless someone from the PPC camp reviews and tests the code.

So, what I'd suggest is just fixing rd_reg64() and wr_reg64() to do
the right thing: keeping the two 32-bit words in the same order
irrespective of the endian-ness, and staying with the __raw_*
accessors until PPC people can look at this.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Herbert Xu]

On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
> Hi!
> 
> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
> drivers/crypto/caam driver only works for PowerPC AFAIK.
> Actually, there isn't that much to do, to get support for the i.MX6 but
> one patch breaks the driver severely:
> 
> 	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
> 	crypto: caam - Add definition of rd/wr_reg64 for little endian platform
> 
> This patch adds
> 
> +#ifdef __LITTLE_ENDIAN
> +static inline void wr_reg64(u64 __iomem *reg, u64 data)
> +{
> +	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
> +	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
> +}
> 
> The wr_reg64 function is only used in one place in the drivers/crypto/caam/jr.c
> driver: to write the dma_addr_t to the register. Without that patch everything
> works fine on ARM (little endian, 32bit), with that patch the driver will write
> 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
> Also, from my understanding, the comment above the defines, stating that you
> have to first write the numerically-lower and then the numerically-higher address
> on 32bit systems doesn't match with the implementation.
> 
> What I don't know/understand is if this makes any sense for any PowerPC implementation.
> 
> So, the question is, how to fix this? I'd prefer to do it directly in the jr driver
> instead of the ifdef-ery.
> 
> Something like
> 	if (sizeof(dma_addr_t) == sizeof(u32))
> 		wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
> 	else if (sizeof(dma_addr_t) == sizeof(u64))
> 		wr_reg64(...)
> 
> or just go by DT compatible and then remove the inline function definitions.
> 
> As far as I can tell, the compatible wouldn't be needed for anything else in the
> jr driver, so maybe that is not optimal. On the other hand the sizeof(..)
> solution would only catch little endian on 32bit and not big endian (?!)
> I however don't know what combinations actually *have* to be caught, as I don't
> know, which exist.
> 
> So, what do you think people?

I'm adding a couple of CCs.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Herbert Xu]

On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
> Hi!
> 
> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
> drivers/crypto/caam driver only works for PowerPC AFAIK.
> Actually, there isn't that much to do, to get support for the i.MX6 but
> one patch breaks the driver severely:
> 
> 	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
> 	crypto: caam - Add definition of rd/wr_reg64 for little endian platform
> 
> This patch adds
> 
> +#ifdef __LITTLE_ENDIAN
> +static inline void wr_reg64(u64 __iomem *reg, u64 data)
> +{
> +	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
> +	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
> +}
> 
> The wr_reg64 function is only used in one place in the drivers/crypto/caam/jr.c
> driver: to write the dma_addr_t to the register. Without that patch everything
> works fine on ARM (little endian, 32bit), with that patch the driver will write
> 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
> Also, from my understanding, the comment above the defines, stating that you
> have to first write the numerically-lower and then the numerically-higher address
> on 32bit systems doesn't match with the implementation.
> 
> What I don't know/understand is if this makes any sense for any PowerPC implementation.
> 
> So, the question is, how to fix this? I'd prefer to do it directly in the jr driver
> instead of the ifdef-ery.
> 
> Something like
> 	if (sizeof(dma_addr_t) == sizeof(u32))
> 		wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
> 	else if (sizeof(dma_addr_t) == sizeof(u64))
> 		wr_reg64(...)
> 
> or just go by DT compatible and then remove the inline function definitions.
> 
> As far as I can tell, the compatible wouldn't be needed for anything else in the
> jr driver, so maybe that is not optimal. On the other hand the sizeof(..)
> solution would only catch little endian on 32bit and not big endian (?!)
> I however don't know what combinations actually *have* to be caught, as I don't
> know, which exist.
> 
> So, what do you think people?

I'm adding a couple of CCs.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Victoria Milhoan]

All,

Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.

Thanks,
Victoria

-----Original Message-----
From: linux-crypto-owner@vger.kernel.org [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Herbert Xu
Sent: Monday, June 15, 2015 3:05 PM
To: Steffen Trumtrar
Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-crypto@vger.kernel.org; Gupta Ruchika-R66431; kernel@pengutronix.de; Geanta Neag Horia Ioan-B05471; Kim Phillips
Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
> Hi!
> 
> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current 
> drivers/crypto/caam driver only works for PowerPC AFAIK.
> Actually, there isn't that much to do, to get support for the i.MX6 
> but one patch breaks the driver severely:
> 
> 	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
> 	crypto: caam - Add definition of rd/wr_reg64 for little endian 
> platform
> 
> This patch adds
> 
> +#ifdef __LITTLE_ENDIAN
> +static inline void wr_reg64(u64 __iomem *reg, u64 data) {
> +	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
> +	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull); }
> 
> The wr_reg64 function is only used in one place in the 
> drivers/crypto/caam/jr.c
> driver: to write the dma_addr_t to the register. Without that patch 
> everything works fine on ARM (little endian, 32bit), with that patch 
> the driver will write 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
> Also, from my understanding, the comment above the defines, stating 
> that you have to first write the numerically-lower and then the 
> numerically-higher address on 32bit systems doesn't match with the implementation.
> 
> What I don't know/understand is if this makes any sense for any PowerPC implementation.
> 
> So, the question is, how to fix this? I'd prefer to do it directly in 
> the jr driver instead of the ifdef-ery.
> 
> Something like
> 	if (sizeof(dma_addr_t) == sizeof(u32))
> 		wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
> 	else if (sizeof(dma_addr_t) == sizeof(u64))
> 		wr_reg64(...)
> 
> or just go by DT compatible and then remove the inline function definitions.
> 
> As far as I can tell, the compatible wouldn't be needed for anything 
> else in the jr driver, so maybe that is not optimal. On the other hand 
> the sizeof(..) solution would only catch little endian on 32bit and 
> not big endian (?!) I however don't know what combinations actually 
> *have* to be caught, as I don't know, which exist.
> 
> So, what do you think people?

I'm adding a couple of CCs.

Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Victoria Milhoan]

All,

Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.

Thanks,
Victoria

-----Original Message-----
From: linux-crypto-owner@vger.kernel.org [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Herbert Xu
Sent: Monday, June 15, 2015 3:05 PM
To: Steffen Trumtrar
Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-crypto@vger.kernel.org; Gupta Ruchika-R66431; kernel@pengutronix.de; Geanta Neag Horia Ioan-B05471; Kim Phillips
Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
> Hi!
> 
> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current 
> drivers/crypto/caam driver only works for PowerPC AFAIK.
> Actually, there isn't that much to do, to get support for the i.MX6 
> but one patch breaks the driver severely:
> 
> 	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
> 	crypto: caam - Add definition of rd/wr_reg64 for little endian 
> platform
> 
> This patch adds
> 
> +#ifdef __LITTLE_ENDIAN
> +static inline void wr_reg64(u64 __iomem *reg, u64 data) {
> +	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
> +	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull); }
> 
> The wr_reg64 function is only used in one place in the 
> drivers/crypto/caam/jr.c
> driver: to write the dma_addr_t to the register. Without that patch 
> everything works fine on ARM (little endian, 32bit), with that patch 
> the driver will write 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
> Also, from my understanding, the comment above the defines, stating 
> that you have to first write the numerically-lower and then the 
> numerically-higher address on 32bit systems doesn't match with the implementation.
> 
> What I don't know/understand is if this makes any sense for any PowerPC implementation.
> 
> So, the question is, how to fix this? I'd prefer to do it directly in 
> the jr driver instead of the ifdef-ery.
> 
> Something like
> 	if (sizeof(dma_addr_t) == sizeof(u32))
> 		wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
> 	else if (sizeof(dma_addr_t) == sizeof(u64))
> 		wr_reg64(...)
> 
> or just go by DT compatible and then remove the inline function definitions.
> 
> As far as I can tell, the compatible wouldn't be needed for anything 
> else in the jr driver, so maybe that is not optimal. On the other hand 
> the sizeof(..) solution would only catch little endian on 32bit and 
> not big endian (?!) I however don't know what combinations actually 
> *have* to be caught, as I don't know, which exist.
> 
> So, what do you think people?

I'm adding a couple of CCs.

Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Victoria Milhoan]

All,

Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.

Thanks,
Victoria

-----Original Message-----
From: linux-crypto-owner@vger.kernel.org [mailto:linux-crypto-owner at vger.kernel.org] On Behalf Of Herbert Xu
Sent: Monday, June 15, 2015 3:05 PM
To: Steffen Trumtrar
Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-crypto at vger.kernel.org; Gupta Ruchika-R66431; kernel at pengutronix.de; Geanta Neag Horia Ioan-B05471; Kim Phillips
Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
> Hi!
> 
> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current 
> drivers/crypto/caam driver only works for PowerPC AFAIK.
> Actually, there isn't that much to do, to get support for the i.MX6 
> but one patch breaks the driver severely:
> 
> 	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
> 	crypto: caam - Add definition of rd/wr_reg64 for little endian 
> platform
> 
> This patch adds
> 
> +#ifdef __LITTLE_ENDIAN
> +static inline void wr_reg64(u64 __iomem *reg, u64 data) {
> +	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
> +	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull); }
> 
> The wr_reg64 function is only used in one place in the 
> drivers/crypto/caam/jr.c
> driver: to write the dma_addr_t to the register. Without that patch 
> everything works fine on ARM (little endian, 32bit), with that patch 
> the driver will write 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
> Also, from my understanding, the comment above the defines, stating 
> that you have to first write the numerically-lower and then the 
> numerically-higher address on 32bit systems doesn't match with the implementation.
> 
> What I don't know/understand is if this makes any sense for any PowerPC implementation.
> 
> So, the question is, how to fix this? I'd prefer to do it directly in 
> the jr driver instead of the ifdef-ery.
> 
> Something like
> 	if (sizeof(dma_addr_t) == sizeof(u32))
> 		wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
> 	else if (sizeof(dma_addr_t) == sizeof(u64))
> 		wr_reg64(...)
> 
> or just go by DT compatible and then remove the inline function definitions.
> 
> As far as I can tell, the compatible wouldn't be needed for anything 
> else in the jr driver, so maybe that is not optimal. On the other hand 
> the sizeof(..) solution would only catch little endian on 32bit and 
> not big endian (?!) I however don't know what combinations actually 
> *have* to be caught, as I don't know, which exist.
> 
> So, what do you think people?

I'm adding a couple of CCs.

Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo at vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Steffen Trumtrar]

Hi!

On Mon, Jun 15, 2015 at 05:28:16PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
> > I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
> > drivers/crypto/caam driver only works for PowerPC AFAIK.
> > Actually, there isn't that much to do, to get support for the i.MX6 but
> > one patch breaks the driver severely:
> > 
> > 	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
> > 	crypto: caam - Add definition of rd/wr_reg64 for little endian platform
> 
> You're not the only one who's hitting problems with this - Jon Nettleton
> has been working on it recently.
> 
> The way this has been done is fairly yucky to start with: several things
> about it are particularly horrid.  The first is the repetitive code
> for the BE and LE cases, when all that's actually different is the
> register order between the two code cases.
> 
> The second thing is the excessive use of masking - I'm pretty sure the
> compiler won't complain with the below.
> 
> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> index 378ddc17f60e..ba0fa630a25d 100644
> --- a/drivers/crypto/caam/regs.h
> +++ b/drivers/crypto/caam/regs.h
> @@ -84,34 +84,28 @@
>  #endif
>  
>  #ifndef CONFIG_64BIT
> -#ifdef __BIG_ENDIAN
> -static inline void wr_reg64(u64 __iomem *reg, u64 data)
> -{
> -	wr_reg32((u32 __iomem *)reg, (data & 0xffffffff00000000ull) >> 32);
> -	wr_reg32((u32 __iomem *)reg + 1, data & 0x00000000ffffffffull);
> -}
> -
> -static inline u64 rd_reg64(u64 __iomem *reg)
> -{
> -	return (((u64)rd_reg32((u32 __iomem *)reg)) << 32) |
> -		((u64)rd_reg32((u32 __iomem *)reg + 1));
> -}
> +#if defined(__BIG_ENDIAN)
> +#define REG64_HI32(reg) ((u32 __iomem *)(reg))
> +#define REG64_LO32(reg) ((u32 __iomem *)(reg) + 1)
> +#elif defined(__LITTLE_ENDIAN)
> +#define REG64_HI32(reg) ((u32 __iomem *)(reg) + 1)
> +#define REG64_LO32(reg) ((u32 __iomem *)(reg))
>  #else
> -#ifdef __LITTLE_ENDIAN
> +#error Broken endian?
> +#endif
> +
>  static inline void wr_reg64(u64 __iomem *reg, u64 data)
>  {
> -	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
> -	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
> +	wr_reg32(REG64_HI32(reg), data >> 32);
> +	wr_reg32(REG64_LO32(reg), data);
>  }
>  
>  static inline u64 rd_reg64(u64 __iomem *reg)
>  {
> -	return (((u64)rd_reg32((u32 __iomem *)reg + 1)) << 32) |
> -		((u64)rd_reg32((u32 __iomem *)reg));
> +	return ((u64)rd_reg32(REG64_HI32(reg))) << 32 |
> +		rd_reg32(REG64_LO32(reg));
>  }
>  #endif
> -#endif
> -#endif
>  
>  /*
>   * jr_outentry
> 
> The second issue is that apparently, the register order doesn't actually
> change for LE devices - in other words, the byte order within each register
> does change, but they aren't a 64-bit register, they're two separate 32-bit
> registers.  So, they should always be written as such.
> 
> So, I'd get rid of the #if defined(__BIG_ENDIAN) stuff and instead have:
> 
> +/*
> + * The DMA address register is a pair of 32-bit registers, and should
> + * always be accessed as such.
> + */
> +#define REG64_HI32(reg) ((u32 __iomem *)(reg))
> +#define REG64_LO32(reg) ((u32 __iomem *)(reg) + 1)
> 

Thanks for all your explanations (in the other mail, too).
I, personally, like this approach the best; at least in the current state
of the driver.

Thanks,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Steffen Trumtrar]

Hi!

On Mon, Jun 15, 2015 at 05:28:16PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
> > I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
> > drivers/crypto/caam driver only works for PowerPC AFAIK.
> > Actually, there isn't that much to do, to get support for the i.MX6 but
> > one patch breaks the driver severely:
> > 
> > 	commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
> > 	crypto: caam - Add definition of rd/wr_reg64 for little endian platform
> 
> You're not the only one who's hitting problems with this - Jon Nettleton
> has been working on it recently.
> 
> The way this has been done is fairly yucky to start with: several things
> about it are particularly horrid.  The first is the repetitive code
> for the BE and LE cases, when all that's actually different is the
> register order between the two code cases.
> 
> The second thing is the excessive use of masking - I'm pretty sure the
> compiler won't complain with the below.
> 
> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> index 378ddc17f60e..ba0fa630a25d 100644
> --- a/drivers/crypto/caam/regs.h
> +++ b/drivers/crypto/caam/regs.h
> @@ -84,34 +84,28 @@
>  #endif
>  
>  #ifndef CONFIG_64BIT
> -#ifdef __BIG_ENDIAN
> -static inline void wr_reg64(u64 __iomem *reg, u64 data)
> -{
> -	wr_reg32((u32 __iomem *)reg, (data & 0xffffffff00000000ull) >> 32);
> -	wr_reg32((u32 __iomem *)reg + 1, data & 0x00000000ffffffffull);
> -}
> -
> -static inline u64 rd_reg64(u64 __iomem *reg)
> -{
> -	return (((u64)rd_reg32((u32 __iomem *)reg)) << 32) |
> -		((u64)rd_reg32((u32 __iomem *)reg + 1));
> -}
> +#if defined(__BIG_ENDIAN)
> +#define REG64_HI32(reg) ((u32 __iomem *)(reg))
> +#define REG64_LO32(reg) ((u32 __iomem *)(reg) + 1)
> +#elif defined(__LITTLE_ENDIAN)
> +#define REG64_HI32(reg) ((u32 __iomem *)(reg) + 1)
> +#define REG64_LO32(reg) ((u32 __iomem *)(reg))
>  #else
> -#ifdef __LITTLE_ENDIAN
> +#error Broken endian?
> +#endif
> +
>  static inline void wr_reg64(u64 __iomem *reg, u64 data)
>  {
> -	wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
> -	wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull);
> +	wr_reg32(REG64_HI32(reg), data >> 32);
> +	wr_reg32(REG64_LO32(reg), data);
>  }
>  
>  static inline u64 rd_reg64(u64 __iomem *reg)
>  {
> -	return (((u64)rd_reg32((u32 __iomem *)reg + 1)) << 32) |
> -		((u64)rd_reg32((u32 __iomem *)reg));
> +	return ((u64)rd_reg32(REG64_HI32(reg))) << 32 |
> +		rd_reg32(REG64_LO32(reg));
>  }
>  #endif
> -#endif
> -#endif
>  
>  /*
>   * jr_outentry
> 
> The second issue is that apparently, the register order doesn't actually
> change for LE devices - in other words, the byte order within each register
> does change, but they aren't a 64-bit register, they're two separate 32-bit
> registers.  So, they should always be written as such.
> 
> So, I'd get rid of the #if defined(__BIG_ENDIAN) stuff and instead have:
> 
> +/*
> + * The DMA address register is a pair of 32-bit registers, and should
> + * always be accessed as such.
> + */
> +#define REG64_HI32(reg) ((u32 __iomem *)(reg))
> +#define REG64_LO32(reg) ((u32 __iomem *)(reg) + 1)
> 

Thanks for all your explanations (in the other mail, too).
I, personally, like this approach the best; at least in the current state
of the driver.

Thanks,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Jon Nettleton]

Victoria,

I was hoping you would join the conversation.  I know you have a
series of patches in Freescale's 3.14 git repository.  Have you
updated those for mainline and published them for review and inclusion
in the upstream kernel?  If yes to any could you post a link?

-Jon

On Tue, Jun 16, 2015 at 5:27 AM, Victoria Milhoan
<Vicki.Milhoan@freescale.com> wrote:
> All,
>
> Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.
>
> Thanks,
> Victoria
>
> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Herbert Xu
> Sent: Monday, June 15, 2015 3:05 PM
> To: Steffen Trumtrar
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-crypto@vger.kernel.org; Gupta Ruchika-R66431; kernel@pengutronix.de; Geanta Neag Horia Ioan-B05471; Kim Phillips
> Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC
>
> On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
>> Hi!
>>
>> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
>> drivers/crypto/caam driver only works for PowerPC AFAIK.
>> Actually, there isn't that much to do, to get support for the i.MX6
>> but one patch breaks the driver severely:
>>
>>       commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
>>       crypto: caam - Add definition of rd/wr_reg64 for little endian
>> platform
>>
>> This patch adds
>>
>> +#ifdef __LITTLE_ENDIAN
>> +static inline void wr_reg64(u64 __iomem *reg, u64 data) {
>> +     wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
>> +     wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull); }
>>
>> The wr_reg64 function is only used in one place in the
>> drivers/crypto/caam/jr.c
>> driver: to write the dma_addr_t to the register. Without that patch
>> everything works fine on ARM (little endian, 32bit), with that patch
>> the driver will write 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
>> Also, from my understanding, the comment above the defines, stating
>> that you have to first write the numerically-lower and then the
>> numerically-higher address on 32bit systems doesn't match with the implementation.
>>
>> What I don't know/understand is if this makes any sense for any PowerPC implementation.
>>
>> So, the question is, how to fix this? I'd prefer to do it directly in
>> the jr driver instead of the ifdef-ery.
>>
>> Something like
>>       if (sizeof(dma_addr_t) == sizeof(u32))
>>               wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
>>       else if (sizeof(dma_addr_t) == sizeof(u64))
>>               wr_reg64(...)
>>
>> or just go by DT compatible and then remove the inline function definitions.
>>
>> As far as I can tell, the compatible wouldn't be needed for anything
>> else in the jr driver, so maybe that is not optimal. On the other hand
>> the sizeof(..) solution would only catch little endian on 32bit and
>> not big endian (?!) I however don't know what combinations actually
>> *have* to be caught, as I don't know, which exist.
>>
>> So, what do you think people?
>
> I'm adding a couple of CCs.
>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Jon Nettleton]

Victoria,

I was hoping you would join the conversation.  I know you have a
series of patches in Freescale's 3.14 git repository.  Have you
updated those for mainline and published them for review and inclusion
in the upstream kernel?  If yes to any could you post a link?

-Jon

On Tue, Jun 16, 2015 at 5:27 AM, Victoria Milhoan
<Vicki.Milhoan@freescale.com> wrote:
> All,
>
> Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.
>
> Thanks,
> Victoria
>
> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Herbert Xu
> Sent: Monday, June 15, 2015 3:05 PM
> To: Steffen Trumtrar
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-crypto@vger.kernel.org; Gupta Ruchika-R66431; kernel@pengutronix.de; Geanta Neag Horia Ioan-B05471; Kim Phillips
> Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC
>
> On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
>> Hi!
>>
>> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
>> drivers/crypto/caam driver only works for PowerPC AFAIK.
>> Actually, there isn't that much to do, to get support for the i.MX6
>> but one patch breaks the driver severely:
>>
>>       commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
>>       crypto: caam - Add definition of rd/wr_reg64 for little endian
>> platform
>>
>> This patch adds
>>
>> +#ifdef __LITTLE_ENDIAN
>> +static inline void wr_reg64(u64 __iomem *reg, u64 data) {
>> +     wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
>> +     wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull); }
>>
>> The wr_reg64 function is only used in one place in the
>> drivers/crypto/caam/jr.c
>> driver: to write the dma_addr_t to the register. Without that patch
>> everything works fine on ARM (little endian, 32bit), with that patch
>> the driver will write 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
>> Also, from my understanding, the comment above the defines, stating
>> that you have to first write the numerically-lower and then the
>> numerically-higher address on 32bit systems doesn't match with the implementation.
>>
>> What I don't know/understand is if this makes any sense for any PowerPC implementation.
>>
>> So, the question is, how to fix this? I'd prefer to do it directly in
>> the jr driver instead of the ifdef-ery.
>>
>> Something like
>>       if (sizeof(dma_addr_t) == sizeof(u32))
>>               wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
>>       else if (sizeof(dma_addr_t) == sizeof(u64))
>>               wr_reg64(...)
>>
>> or just go by DT compatible and then remove the inline function definitions.
>>
>> As far as I can tell, the compatible wouldn't be needed for anything
>> else in the jr driver, so maybe that is not optimal. On the other hand
>> the sizeof(..) solution would only catch little endian on 32bit and
>> not big endian (?!) I however don't know what combinations actually
>> *have* to be caught, as I don't know, which exist.
>>
>> So, what do you think people?
>
> I'm adding a couple of CCs.
>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Jon Nettleton]

Victoria,

I was hoping you would join the conversation.  I know you have a
series of patches in Freescale's 3.14 git repository.  Have you
updated those for mainline and published them for review and inclusion
in the upstream kernel?  If yes to any could you post a link?

-Jon

On Tue, Jun 16, 2015 at 5:27 AM, Victoria Milhoan
<Vicki.Milhoan@freescale.com> wrote:
> All,
>
> Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.
>
> Thanks,
> Victoria
>
> -----Original Message-----
> From: linux-crypto-owner at vger.kernel.org [mailto:linux-crypto-owner at vger.kernel.org] On Behalf Of Herbert Xu
> Sent: Monday, June 15, 2015 3:05 PM
> To: Steffen Trumtrar
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-crypto at vger.kernel.org; Gupta Ruchika-R66431; kernel at pengutronix.de; Geanta Neag Horia Ioan-B05471; Kim Phillips
> Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC
>
> On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
>> Hi!
>>
>> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current
>> drivers/crypto/caam driver only works for PowerPC AFAIK.
>> Actually, there isn't that much to do, to get support for the i.MX6
>> but one patch breaks the driver severely:
>>
>>       commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
>>       crypto: caam - Add definition of rd/wr_reg64 for little endian
>> platform
>>
>> This patch adds
>>
>> +#ifdef __LITTLE_ENDIAN
>> +static inline void wr_reg64(u64 __iomem *reg, u64 data) {
>> +     wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
>> +     wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull); }
>>
>> The wr_reg64 function is only used in one place in the
>> drivers/crypto/caam/jr.c
>> driver: to write the dma_addr_t to the register. Without that patch
>> everything works fine on ARM (little endian, 32bit), with that patch
>> the driver will write 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
>> Also, from my understanding, the comment above the defines, stating
>> that you have to first write the numerically-lower and then the
>> numerically-higher address on 32bit systems doesn't match with the implementation.
>>
>> What I don't know/understand is if this makes any sense for any PowerPC implementation.
>>
>> So, the question is, how to fix this? I'd prefer to do it directly in
>> the jr driver instead of the ifdef-ery.
>>
>> Something like
>>       if (sizeof(dma_addr_t) == sizeof(u32))
>>               wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
>>       else if (sizeof(dma_addr_t) == sizeof(u64))
>>               wr_reg64(...)
>>
>> or just go by DT compatible and then remove the inline function definitions.
>>
>> As far as I can tell, the compatible wouldn't be needed for anything
>> else in the jr driver, so maybe that is not optimal. On the other hand
>> the sizeof(..) solution would only catch little endian on 32bit and
>> not big endian (?!) I however don't know what combinations actually
>> *have* to be caught, as I don't know, which exist.
>>
>> So, what do you think people?
>
> I'm adding a couple of CCs.
>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo at vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Steffen Trumtrar]

Hi!

On Tue, Jun 16, 2015 at 03:27:54AM +0000, Victoria Milhoan wrote:
> All,
> 
> Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.
> 

Could you maybe bounce me that thread?
I can't find it in a format that allows me to reply to the patches.

Thanks,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Steffen Trumtrar]

Hi!

On Tue, Jun 16, 2015 at 03:27:54AM +0000, Victoria Milhoan wrote:
> All,
> 
> Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.
> 

Could you maybe bounce me that thread?
I can't find it in a format that allows me to reply to the patches.

Thanks,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Steffen Trumtrar]

Hi!

On Tue, Jun 16, 2015 at 03:27:54AM +0000, Victoria Milhoan wrote:
> All,
> 
> Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.
> 

Could you maybe bounce me that thread?
I can't find it in a format that allows me to reply to the patches.

Thanks,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Victoria Milhoan]

Jon,

The patches published to the mailing list yesterday for comment (titled crypto: caam - Add i.MX6 support to the Freescale CAAM driver) are based on the patches in the Freescale 3.14 git repository which enable i.MX6 support. The initial patch set is intended to provide a functional driver, based on the mainline kernel, which supports both QorIQ and i.MX6. 

Thanks,
Victoria

-----Original Message-----
From: Jon Nettleton [mailto:jon.nettleton@gmail.com] 
Sent: Tuesday, June 16, 2015 1:11 AM
To: Milhoan Victoria-B42089
Cc: Herbert Xu; Steffen Trumtrar; Kim Phillips; linux-kernel@vger.kernel.org; Gupta Ruchika-R66431; linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de; Geanta Neag Horia Ioan-B05471; linux-crypto@vger.kernel.org
Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

Victoria,

I was hoping you would join the conversation.  I know you have a series of patches in Freescale's 3.14 git repository.  Have you updated those for mainline and published them for review and inclusion in the upstream kernel?  If yes to any could you post a link?

-Jon

On Tue, Jun 16, 2015 at 5:27 AM, Victoria Milhoan <Vicki.Milhoan@freescale.com> wrote:
> All,
>
> Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.
>
> Thanks,
> Victoria
>
> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org 
> [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Herbert Xu
> Sent: Monday, June 15, 2015 3:05 PM
> To: Steffen Trumtrar
> Cc: linux-kernel@vger.kernel.org; 
> linux-arm-kernel@lists.infradead.org; linux-crypto@vger.kernel.org; 
> Gupta Ruchika-R66431; kernel@pengutronix.de; Geanta Neag Horia 
> Ioan-B05471; Kim Phillips
> Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC
>
> On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
>> Hi!
>>
>> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current 
>> drivers/crypto/caam driver only works for PowerPC AFAIK.
>> Actually, there isn't that much to do, to get support for the i.MX6 
>> but one patch breaks the driver severely:
>>
>>       commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
>>       crypto: caam - Add definition of rd/wr_reg64 for little endian 
>> platform
>>
>> This patch adds
>>
>> +#ifdef __LITTLE_ENDIAN
>> +static inline void wr_reg64(u64 __iomem *reg, u64 data) {
>> +     wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
>> +     wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull); }
>>
>> The wr_reg64 function is only used in one place in the 
>> drivers/crypto/caam/jr.c
>> driver: to write the dma_addr_t to the register. Without that patch 
>> everything works fine on ARM (little endian, 32bit), with that patch 
>> the driver will write 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
>> Also, from my understanding, the comment above the defines, stating 
>> that you have to first write the numerically-lower and then the 
>> numerically-higher address on 32bit systems doesn't match with the implementation.
>>
>> What I don't know/understand is if this makes any sense for any PowerPC implementation.
>>
>> So, the question is, how to fix this? I'd prefer to do it directly in 
>> the jr driver instead of the ifdef-ery.
>>
>> Something like
>>       if (sizeof(dma_addr_t) == sizeof(u32))
>>               wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
>>       else if (sizeof(dma_addr_t) == sizeof(u64))
>>               wr_reg64(...)
>>
>> or just go by DT compatible and then remove the inline function definitions.
>>
>> As far as I can tell, the compatible wouldn't be needed for anything 
>> else in the jr driver, so maybe that is not optimal. On the other 
>> hand the sizeof(..) solution would only catch little endian on 32bit 
>> and not big endian (?!) I however don't know what combinations 
>> actually
>> *have* to be caught, as I don't know, which exist.
>>
>> So, what do you think people?
>
> I'm adding a couple of CCs.
>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: 
> http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-crypto" in the body of a message to majordomo@vger.kernel.org 
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Victoria Milhoan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4897 bytes --]

Jon,

The patches published to the mailing list yesterday for comment (titled crypto: caam - Add i.MX6 support to the Freescale CAAM driver) are based on the patches in the Freescale 3.14 git repository which enable i.MX6 support. The initial patch set is intended to provide a functional driver, based on the mainline kernel, which supports both QorIQ and i.MX6. 

Thanks,
Victoria

-----Original Message-----
From: Jon Nettleton [mailto:jon.nettleton@gmail.com] 
Sent: Tuesday, June 16, 2015 1:11 AM
To: Milhoan Victoria-B42089
Cc: Herbert Xu; Steffen Trumtrar; Kim Phillips; linux-kernel@vger.kernel.org; Gupta Ruchika-R66431; linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de; Geanta Neag Horia Ioan-B05471; linux-crypto@vger.kernel.org
Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

Victoria,

I was hoping you would join the conversation.  I know you have a series of patches in Freescale's 3.14 git repository.  Have you updated those for mainline and published them for review and inclusion in the upstream kernel?  If yes to any could you post a link?

-Jon

On Tue, Jun 16, 2015 at 5:27 AM, Victoria Milhoan <Vicki.Milhoan@freescale.com> wrote:
> All,
>
> Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.
>
> Thanks,
> Victoria
>
> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org 
> [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Herbert Xu
> Sent: Monday, June 15, 2015 3:05 PM
> To: Steffen Trumtrar
> Cc: linux-kernel@vger.kernel.org; 
> linux-arm-kernel@lists.infradead.org; linux-crypto@vger.kernel.org; 
> Gupta Ruchika-R66431; kernel@pengutronix.de; Geanta Neag Horia 
> Ioan-B05471; Kim Phillips
> Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC
>
> On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
>> Hi!
>>
>> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current 
>> drivers/crypto/caam driver only works for PowerPC AFAIK.
>> Actually, there isn't that much to do, to get support for the i.MX6 
>> but one patch breaks the driver severely:
>>
>>       commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
>>       crypto: caam - Add definition of rd/wr_reg64 for little endian 
>> platform
>>
>> This patch adds
>>
>> +#ifdef __LITTLE_ENDIAN
>> +static inline void wr_reg64(u64 __iomem *reg, u64 data) {
>> +     wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
>> +     wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull); }
>>
>> The wr_reg64 function is only used in one place in the 
>> drivers/crypto/caam/jr.c
>> driver: to write the dma_addr_t to the register. Without that patch 
>> everything works fine on ARM (little endian, 32bit), with that patch 
>> the driver will write 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
>> Also, from my understanding, the comment above the defines, stating 
>> that you have to first write the numerically-lower and then the 
>> numerically-higher address on 32bit systems doesn't match with the implementation.
>>
>> What I don't know/understand is if this makes any sense for any PowerPC implementation.
>>
>> So, the question is, how to fix this? I'd prefer to do it directly in 
>> the jr driver instead of the ifdef-ery.
>>
>> Something like
>>       if (sizeof(dma_addr_t) == sizeof(u32))
>>               wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
>>       else if (sizeof(dma_addr_t) == sizeof(u64))
>>               wr_reg64(...)
>>
>> or just go by DT compatible and then remove the inline function definitions.
>>
>> As far as I can tell, the compatible wouldn't be needed for anything 
>> else in the jr driver, so maybe that is not optimal. On the other 
>> hand the sizeof(..) solution would only catch little endian on 32bit 
>> and not big endian (?!) I however don't know what combinations 
>> actually
>> *have* to be caught, as I don't know, which exist.
>>
>> So, what do you think people?
>
> I'm adding a couple of CCs.
>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: 
> http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-crypto" in the body of a message to majordomo@vger.kernel.org 
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Victoria Milhoan]

Jon,

The patches published to the mailing list yesterday for comment (titled crypto: caam - Add i.MX6 support to the Freescale CAAM driver) are based on the patches in the Freescale 3.14 git repository which enable i.MX6 support. The initial patch set is intended to provide a functional driver, based on the mainline kernel, which supports both QorIQ and i.MX6. 

Thanks,
Victoria

-----Original Message-----
From: Jon Nettleton [mailto:jon.nettleton at gmail.com] 
Sent: Tuesday, June 16, 2015 1:11 AM
To: Milhoan Victoria-B42089
Cc: Herbert Xu; Steffen Trumtrar; Kim Phillips; linux-kernel at vger.kernel.org; Gupta Ruchika-R66431; linux-arm-kernel at lists.infradead.org; kernel at pengutronix.de; Geanta Neag Horia Ioan-B05471; linux-crypto at vger.kernel.org
Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

Victoria,

I was hoping you would join the conversation.  I know you have a series of patches in Freescale's 3.14 git repository.  Have you updated those for mainline and published them for review and inclusion in the upstream kernel?  If yes to any could you post a link?

-Jon

On Tue, Jun 16, 2015 at 5:27 AM, Victoria Milhoan <Vicki.Milhoan@freescale.com> wrote:
> All,
>
> Freescale has been adding i.MX6 support to the CAAM driver and testing on both i.MX6 and QorIQ platforms. The patch series is now available for review. Your feedback for the provided patches is appreciated.
>
> Thanks,
> Victoria
>
> -----Original Message-----
> From: linux-crypto-owner at vger.kernel.org 
> [mailto:linux-crypto-owner at vger.kernel.org] On Behalf Of Herbert Xu
> Sent: Monday, June 15, 2015 3:05 PM
> To: Steffen Trumtrar
> Cc: linux-kernel at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; linux-crypto at vger.kernel.org; 
> Gupta Ruchika-R66431; kernel at pengutronix.de; Geanta Neag Horia 
> Ioan-B05471; Kim Phillips
> Subject: Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC
>
> On Mon, Jun 15, 2015 at 05:59:07PM +0200, Steffen Trumtrar wrote:
>> Hi!
>>
>> I'm working on CAAM support for the ARM-based i.MX6 SoCs. The current 
>> drivers/crypto/caam driver only works for PowerPC AFAIK.
>> Actually, there isn't that much to do, to get support for the i.MX6 
>> but one patch breaks the driver severely:
>>
>>       commit ef94b1d834aace7101de77c3a7c2631b9ae9c5f6
>>       crypto: caam - Add definition of rd/wr_reg64 for little endian 
>> platform
>>
>> This patch adds
>>
>> +#ifdef __LITTLE_ENDIAN
>> +static inline void wr_reg64(u64 __iomem *reg, u64 data) {
>> +     wr_reg32((u32 __iomem *)reg + 1, (data & 0xffffffff00000000ull) >> 32);
>> +     wr_reg32((u32 __iomem *)reg, data & 0x00000000ffffffffull); }
>>
>> The wr_reg64 function is only used in one place in the 
>> drivers/crypto/caam/jr.c
>> driver: to write the dma_addr_t to the register. Without that patch 
>> everything works fine on ARM (little endian, 32bit), with that patch 
>> the driver will write 0's into the register that holds the DMA address (the numerically-higher) -> kernel hangs.
>> Also, from my understanding, the comment above the defines, stating 
>> that you have to first write the numerically-lower and then the 
>> numerically-higher address on 32bit systems doesn't match with the implementation.
>>
>> What I don't know/understand is if this makes any sense for any PowerPC implementation.
>>
>> So, the question is, how to fix this? I'd prefer to do it directly in 
>> the jr driver instead of the ifdef-ery.
>>
>> Something like
>>       if (sizeof(dma_addr_t) == sizeof(u32))
>>               wr_reg32(&jrp->rregs->inpring_base + 1, inpbusaddr);
>>       else if (sizeof(dma_addr_t) == sizeof(u64))
>>               wr_reg64(...)
>>
>> or just go by DT compatible and then remove the inline function definitions.
>>
>> As far as I can tell, the compatible wouldn't be needed for anything 
>> else in the jr driver, so maybe that is not optimal. On the other 
>> hand the sizeof(..) solution would only catch little endian on 32bit 
>> and not big endian (?!) I however don't know what combinations 
>> actually
>> *have* to be caught, as I don't know, which exist.
>>
>> So, what do you think people?
>
> I'm adding a couple of CCs.
>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: 
> http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-crypto" in the body of a message to majordomo at vger.kernel.org 
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Horia Geantă]

On 6/15/2015 8:18 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 15, 2015 at 06:33:17PM +0200, Jon Nettleton wrote:
>> Funny enough I tackled this problem over the weekend as well.  My
>> approach was to switch the driver over to use the *_relaxed() io
>> functions and then special case the bits missing from the various
>> ARCHs.  Basically adding setbits32 and clrbits32 for !PPC
>> architectures and letting PPC and ARM share a writeq/readq set of
>> functions.  I left the existing LITTLE_ENDIAN special case until I
>> could verify if it was needed, or had been tested.
> 
> I'll follow up here with what I've mentioned elsewhere, and some further
> thoughts.
> 
> I think this shows the dangers of using struct { } to define register
> offsets.  Let's start here:
> 
> /*
>  * caam_job_ring - direct job ring setup
>  * 1-4 possible per instantiation, base + 1000/2000/3000/4000
>  * Padded out to 0x1000
>  */
> struct caam_job_ring {
>         /* Input ring */
>         u64 inpring_base;       /* IRBAx -  Input desc ring baseaddr */
>         u32 rsvd1;
> 
> Apparently, this is a CPU-endian 64-bit value (it's not defined using
> le64 or be64 which would "fix" it's endian.)

The IP in question (CAAM) has different endianness, depending on the
integration on the SoC. Would it be ok to #define a sec64 type which
would translate either to be64 or le64?

> 
> The second question, which comes up in light of the breakage that's
> being reported is: is this really a 64-bit register, or is it a pair
> of 32-bit registers side-by-side?
> 
> The documentation I'm looking at doesn't document the register at
> base + 0x1000, but documents the one at base + 0x1004, and the one
> at 0x1004 is given the name "IRBAR0_LS", which presumably stands
> for "input ring base address register 0, least significant".
> 
> As the code originally stood for PPC, IRBAR0_LS is also at 0x1004,
> but appears to be big endian.
> 
> On ARM, IRBAR0_LS appears at the same address, but is little endian.

In some cases, CAAM endianness does not match CPU endianness (for e.g.
i.MX). We're talking about default endianness here, both for CAAM and
for CPU - no CONFIG_CPU_{BIG,LITTLE}_ENDIAN.
ARCH	CPU	CAAM	SoC
PPC	BIG	BIG	P4080, T4240 etc.
ARM	LITTLE	LITTLE	LS1021A, LS2085A etc.
ARM	LITTLE	BIG	i.MX6, i.MX7, LS1043A

> This is *not* a 64-bit register at all, but is a pair of 32-bit
> registers side by side.  Moreover, readq() should not be used - no
> amount of arch mangling could ever produce a sane readq() which
> coped with this.
> 
> So, the CAAM code is buggy in this regard: using readq() here when
> endian-portability is required is wrong.  It's got to be two 32-bit
> reads or two 32-bit writes in the appropriate endian.
> 
> Also, note that there's a big difference between __raw_readl() and
> readl_relaxed().  readl_relaxed() is always little-endian.  __raw_readl()
> is god-knows-what-the-archtecture-decides endian.  Switching PPC
> drivers from __raw_readl() to readl_relaxed() is really not a good
> idea unless someone from the PPC camp reviews and tests the code.
> 
> So, what I'd suggest is just fixing rd_reg64() and wr_reg64() to do
> the right thing: keeping the two 32-bit words in the same order
> irrespective of the endian-ness, and staying with the __raw_*
> accessors until PPC people can look at this.

Agree that the I/O accessors need to be revisited.
Unfortunately, the proposed change does not work for LE CAAM (LS1021A).
I'll send a fix.

Thanks,
Horia

[BUG?] crypto: caam: little/big endianness on ARM vs PPC

[Horia Geantă]

On 6/15/2015 8:18 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 15, 2015 at 06:33:17PM +0200, Jon Nettleton wrote:
>> Funny enough I tackled this problem over the weekend as well.  My
>> approach was to switch the driver over to use the *_relaxed() io
>> functions and then special case the bits missing from the various
>> ARCHs.  Basically adding setbits32 and clrbits32 for !PPC
>> architectures and letting PPC and ARM share a writeq/readq set of
>> functions.  I left the existing LITTLE_ENDIAN special case until I
>> could verify if it was needed, or had been tested.
> 
> I'll follow up here with what I've mentioned elsewhere, and some further
> thoughts.
> 
> I think this shows the dangers of using struct { } to define register
> offsets.  Let's start here:
> 
> /*
>  * caam_job_ring - direct job ring setup
>  * 1-4 possible per instantiation, base + 1000/2000/3000/4000
>  * Padded out to 0x1000
>  */
> struct caam_job_ring {
>         /* Input ring */
>         u64 inpring_base;       /* IRBAx -  Input desc ring baseaddr */
>         u32 rsvd1;
> 
> Apparently, this is a CPU-endian 64-bit value (it's not defined using
> le64 or be64 which would "fix" it's endian.)

The IP in question (CAAM) has different endianness, depending on the
integration on the SoC. Would it be ok to #define a sec64 type which
would translate either to be64 or le64?

> 
> The second question, which comes up in light of the breakage that's
> being reported is: is this really a 64-bit register, or is it a pair
> of 32-bit registers side-by-side?
> 
> The documentation I'm looking at doesn't document the register at
> base + 0x1000, but documents the one at base + 0x1004, and the one
> at 0x1004 is given the name "IRBAR0_LS", which presumably stands
> for "input ring base address register 0, least significant".
> 
> As the code originally stood for PPC, IRBAR0_LS is also at 0x1004,
> but appears to be big endian.
> 
> On ARM, IRBAR0_LS appears at the same address, but is little endian.

In some cases, CAAM endianness does not match CPU endianness (for e.g.
i.MX). We're talking about default endianness here, both for CAAM and
for CPU - no CONFIG_CPU_{BIG,LITTLE}_ENDIAN.
ARCH	CPU	CAAM	SoC
PPC	BIG	BIG	P4080, T4240 etc.
ARM	LITTLE	LITTLE	LS1021A, LS2085A etc.
ARM	LITTLE	BIG	i.MX6, i.MX7, LS1043A

> This is *not* a 64-bit register at all, but is a pair of 32-bit
> registers side by side.  Moreover, readq() should not be used - no
> amount of arch mangling could ever produce a sane readq() which
> coped with this.
> 
> So, the CAAM code is buggy in this regard: using readq() here when
> endian-portability is required is wrong.  It's got to be two 32-bit
> reads or two 32-bit writes in the appropriate endian.
> 
> Also, note that there's a big difference between __raw_readl() and
> readl_relaxed().  readl_relaxed() is always little-endian.  __raw_readl()
> is god-knows-what-the-archtecture-decides endian.  Switching PPC
> drivers from __raw_readl() to readl_relaxed() is really not a good
> idea unless someone from the PPC camp reviews and tests the code.
> 
> So, what I'd suggest is just fixing rd_reg64() and wr_reg64() to do
> the right thing: keeping the two 32-bit words in the same order
> irrespective of the endian-ness, and staying with the __raw_*
> accessors until PPC people can look at this.

Agree that the I/O accessors need to be revisited.
Unfortunately, the proposed change does not work for LE CAAM (LS1021A).
I'll send a fix.

Thanks,
Horia