[PATCH v3 0/5] crypto: caam - Add i.MX8MQ support

[PATCH v3 0/5] crypto: caam - Add i.MX8MQ support

[Andrey Smirnov]

Everyone:

Picking up where Chris left off (I chatted with him privately
beforehead), this series adds support for i.MX8MQ to CAAM driver. Just
like [v1], this series is i.MX8MQ only.

Feedback is welcome!
Thanks,
Andrey Smirnov

Changes since [v2]:

  - Dropped "crypto: caam - do not initialise clocks on the i.MX8" and
    replaced it with "crypto: caam - simplfy clock initialization" and 
    "crypto: caam - add clock entry for i.MX8MQ"


Changes since [v1]

  - Series reworked to continue using register based interface for
    queueing RNG initialization job, dropping "crypto: caam - use job
    ring for RNG instantiation instead of DECO"

  - Added a patch to share DMA mask selection code

  - Added missing Signed-off-by for authors of original NXP tree
    commits that this sereis is based on

[v2] lore.kernel.org/r/20190607200225.21419-1-andrew.smirnov@gmail.com
[v1] https://patchwork.kernel.org/cover/10825625/


Andrey Smirnov (4):
  crypto: caam - move DMA mask selection into a function
  crypto: caam - always select job ring via RSR on i.MX8MQ
  crypto: caam - simplfy clock initialization
  crypto: caam - add clock entry for i.MX8MQ

Chris Spencer (1):
  crypto: caam - correct DMA address size for the i.MX8

 drivers/crypto/caam/ctrl.c        | 233 +++++++++++++++---------------
 drivers/crypto/caam/desc_constr.h |  24 +--
 drivers/crypto/caam/intern.h      |  29 +++-
 drivers/crypto/caam/jr.c          |  15 +-
 drivers/crypto/caam/pdb.h         |  49 ++++---
 drivers/crypto/caam/regs.h        |  21 ++-
 6 files changed, 193 insertions(+), 178 deletions(-)

-- 
2.21.0

[PATCH v3 1/5] crypto: caam - move DMA mask selection into a function

[Andrey Smirnov]

Exactly the same code to figure out DMA mask is repeated twice in the
driver code. To avoid repetition, move that logic into a standalone
subroutine in intern.h. While at it re-shuffle the code to make it
more readable with early returns.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Spencer <christopher.spencer@sea.co.uk>
Cc: Cory Tusar <cory.tusar@zii.aero>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/caam/ctrl.c   | 11 +----------
 drivers/crypto/caam/intern.h | 20 ++++++++++++++++++++
 drivers/crypto/caam/jr.c     | 15 +--------------
 3 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 4e43ca4d3656..e674d8770cdb 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -688,16 +688,7 @@ static int caam_probe(struct platform_device *pdev)
 			      JRSTART_JR1_START | JRSTART_JR2_START |
 			      JRSTART_JR3_START);
 
-	if (sizeof(dma_addr_t) == sizeof(u64)) {
-		if (caam_dpaa2)
-			ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(49));
-		else if (of_device_is_compatible(nprop, "fsl,sec-v5.0"))
-			ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(40));
-		else
-			ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
-	} else {
-		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
-	}
+	ret = dma_set_mask_and_coherent(dev, caam_get_dma_mask(dev));
 	if (ret) {
 		dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n", ret);
 		goto iounmap_ctrl;
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index 6af84bbc612c..ec25d260fa40 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -10,6 +10,8 @@
 #ifndef INTERN_H
 #define INTERN_H
 
+#include "ctrl.h"
+
 /* Currently comes from Kconfig param as a ^2 (driver-required) */
 #define JOBR_DEPTH (1 << CONFIG_CRYPTO_DEV_FSL_CAAM_RINGSIZE)
 
@@ -215,4 +217,22 @@ DEFINE_SIMPLE_ATTRIBUTE(caam_fops_u32_ro, caam_debugfs_u32_get, NULL, "%llu\n");
 DEFINE_SIMPLE_ATTRIBUTE(caam_fops_u64_ro, caam_debugfs_u64_get, NULL, "%llu\n");
 #endif
 
+static inline u64 caam_get_dma_mask(struct device *dev)
+{
+	struct device_node *nprop = dev->of_node;
+
+	if (sizeof(dma_addr_t) != sizeof(u64))
+		return DMA_BIT_MASK(32);
+
+	if (caam_dpaa2)
+		return DMA_BIT_MASK(49);
+
+	if (of_device_is_compatible(nprop, "fsl,sec-v5.0-job-ring") ||
+	    of_device_is_compatible(nprop, "fsl,sec-v5.0"))
+		return DMA_BIT_MASK(40);
+
+	return DMA_BIT_MASK(36);
+}
+
+
 #endif /* INTERN_H */
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index cea811fed320..4b25b2fa3d02 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -543,20 +543,7 @@ static int caam_jr_probe(struct platform_device *pdev)
 
 	jrpriv->rregs = (struct caam_job_ring __iomem __force *)ctrl;
 
-	if (sizeof(dma_addr_t) == sizeof(u64)) {
-		if (caam_dpaa2)
-			error = dma_set_mask_and_coherent(jrdev,
-							  DMA_BIT_MASK(49));
-		else if (of_device_is_compatible(nprop,
-						 "fsl,sec-v5.0-job-ring"))
-			error = dma_set_mask_and_coherent(jrdev,
-							  DMA_BIT_MASK(40));
-		else
-			error = dma_set_mask_and_coherent(jrdev,
-							  DMA_BIT_MASK(36));
-	} else {
-		error = dma_set_mask_and_coherent(jrdev, DMA_BIT_MASK(32));
-	}
+	error = dma_set_mask_and_coherent(jrdev, caam_get_dma_mask(jrdev));
 	if (error) {
 		dev_err(jrdev, "dma_set_mask_and_coherent failed (%d)\n",
 			error);
-- 
2.21.0

Re: [PATCH v3 1/5] crypto: caam - move DMA mask selection into a function

[Horia Geanta]

On 6/17/2019 7:04 PM, Andrey Smirnov wrote:
> Exactly the same code to figure out DMA mask is repeated twice in the
> driver code. To avoid repetition, move that logic into a standalone
> subroutine in intern.h. While at it re-shuffle the code to make it
> more readable with early returns.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Spencer <christopher.spencer@sea.co.uk>
> Cc: Cory Tusar <cory.tusar@zii.aero>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Horia Geantă <horia.geanta@nxp.com>
> Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
> Cc: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>

Being the 1st patch in the series and not i.MX8-specific, I'd say it should
be merged separately.

Thanks,
Horia

Re: [PATCH v3 1/5] crypto: caam - move DMA mask selection into a function

[Andrey Smirnov]

On Thu, Jun 27, 2019 at 3:50 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>
> On 6/17/2019 7:04 PM, Andrey Smirnov wrote:
> > Exactly the same code to figure out DMA mask is repeated twice in the
> > driver code. To avoid repetition, move that logic into a standalone
> > subroutine in intern.h. While at it re-shuffle the code to make it
> > more readable with early returns.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Chris Spencer <christopher.spencer@sea.co.uk>
> > Cc: Cory Tusar <cory.tusar@zii.aero>
> > Cc: Chris Healy <cphealy@gmail.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Horia Geantă <horia.geanta@nxp.com>
> > Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
> > Cc: Leonard Crestez <leonard.crestez@nxp.com>
> > Cc: linux-crypto@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> Reviewed-by: Horia Geantă <horia.geanta@nxp.com>
>
> Being the 1st patch in the series and not i.MX8-specific, I'd say it should
> be merged separately.
>

Can it be done by cherry picking it from the series or does it have to
be submitted as a separate patch? I am hoping the former is possible
since it'd make life easier for me.

Thanks,
Andrey Smirnov

[PATCH v3 2/5] crypto: caam - correct DMA address size for the i.MX8

[Andrey Smirnov]

From: Chris Spencer <christopher.spencer@sea.co.uk>

The i.MX8 is arm64, but its CAAM DMA address size is 32-bits.

Signed-off-by: Aymen Sghaier <aymen.sghaier@nxp.com>
Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>
Signed-off-by: Chris Spencer <christopher.spencer@sea.co.uk>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Spencer <christopher.spencer@sea.co.uk>
Cc: Cory Tusar <cory.tusar@zii.aero>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/caam/desc_constr.h | 24 +++++++--------
 drivers/crypto/caam/intern.h      |  6 ++--
 drivers/crypto/caam/pdb.h         | 49 ++++++++++++++++---------------
 drivers/crypto/caam/regs.h        | 21 +++++++++++--
 4 files changed, 58 insertions(+), 42 deletions(-)

diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h
index 5988a26a2441..fc8042e63b18 100644
--- a/drivers/crypto/caam/desc_constr.h
+++ b/drivers/crypto/caam/desc_constr.h
@@ -14,7 +14,7 @@
 
 #define IMMEDIATE (1 << 23)
 #define CAAM_CMD_SZ sizeof(u32)
-#define CAAM_PTR_SZ sizeof(dma_addr_t)
+#define CAAM_PTR_SZ sizeof(caam_dma_addr_t)
 #define CAAM_DESC_BYTES_MAX (CAAM_CMD_SZ * MAX_CAAM_DESCSIZE)
 #define DESC_JOB_IO_LEN (CAAM_CMD_SZ * 5 + CAAM_PTR_SZ * 3)
 
@@ -101,9 +101,9 @@ static inline void init_job_desc_pdb(u32 * const desc, u32 options,
 	init_job_desc(desc, (((pdb_len + 1) << HDR_START_IDX_SHIFT)) | options);
 }
 
-static inline void append_ptr(u32 * const desc, dma_addr_t ptr)
+static inline void append_ptr(u32 * const desc, caam_dma_addr_t ptr)
 {
-	dma_addr_t *offset = (dma_addr_t *)desc_end(desc);
+	caam_dma_addr_t *offset = (caam_dma_addr_t *)desc_end(desc);
 
 	*offset = cpu_to_caam_dma(ptr);
 
@@ -111,7 +111,7 @@ static inline void append_ptr(u32 * const desc, dma_addr_t ptr)
 				CAAM_PTR_SZ / CAAM_CMD_SZ);
 }
 
-static inline void init_job_desc_shared(u32 * const desc, dma_addr_t ptr,
+static inline void init_job_desc_shared(u32 * const desc, caam_dma_addr_t ptr,
 					int len, u32 options)
 {
 	PRINT_POS;
@@ -166,7 +166,7 @@ static inline u32 *write_cmd(u32 * const desc, u32 command)
 	return desc + 1;
 }
 
-static inline void append_cmd_ptr(u32 * const desc, dma_addr_t ptr, int len,
+static inline void append_cmd_ptr(u32 * const desc, caam_dma_addr_t ptr, int len,
 				  u32 command)
 {
 	append_cmd(desc, command | len);
@@ -174,7 +174,7 @@ static inline void append_cmd_ptr(u32 * const desc, dma_addr_t ptr, int len,
 }
 
 /* Write length after pointer, rather than inside command */
-static inline void append_cmd_ptr_extlen(u32 * const desc, dma_addr_t ptr,
+static inline void append_cmd_ptr_extlen(u32 * const desc, caam_dma_addr_t ptr,
 					 unsigned int len, u32 command)
 {
 	append_cmd(desc, command);
@@ -239,7 +239,7 @@ APPEND_CMD_LEN(seq_fifo_load, SEQ_FIFO_LOAD)
 APPEND_CMD_LEN(seq_fifo_store, SEQ_FIFO_STORE)
 
 #define APPEND_CMD_PTR(cmd, op) \
-static inline void append_##cmd(u32 * const desc, dma_addr_t ptr, \
+static inline void append_##cmd(u32 * const desc, caam_dma_addr_t ptr, \
 				unsigned int len, u32 options) \
 { \
 	PRINT_POS; \
@@ -250,7 +250,7 @@ APPEND_CMD_PTR(load, LOAD)
 APPEND_CMD_PTR(fifo_load, FIFO_LOAD)
 APPEND_CMD_PTR(fifo_store, FIFO_STORE)
 
-static inline void append_store(u32 * const desc, dma_addr_t ptr,
+static inline void append_store(u32 * const desc, caam_dma_addr_t ptr,
 				unsigned int len, u32 options)
 {
 	u32 cmd_src;
@@ -269,7 +269,7 @@ static inline void append_store(u32 * const desc, dma_addr_t ptr,
 
 #define APPEND_SEQ_PTR_INTLEN(cmd, op) \
 static inline void append_seq_##cmd##_ptr_intlen(u32 * const desc, \
-						 dma_addr_t ptr, \
+						 caam_dma_addr_t ptr, \
 						 unsigned int len, \
 						 u32 options) \
 { \
@@ -293,7 +293,7 @@ APPEND_CMD_PTR_TO_IMM(load, LOAD);
 APPEND_CMD_PTR_TO_IMM(fifo_load, FIFO_LOAD);
 
 #define APPEND_CMD_PTR_EXTLEN(cmd, op) \
-static inline void append_##cmd##_extlen(u32 * const desc, dma_addr_t ptr, \
+static inline void append_##cmd##_extlen(u32 * const desc, caam_dma_addr_t ptr, \
 					 unsigned int len, u32 options) \
 { \
 	PRINT_POS; \
@@ -307,7 +307,7 @@ APPEND_CMD_PTR_EXTLEN(seq_out_ptr, SEQ_OUT_PTR)
  * the size of its type
  */
 #define APPEND_CMD_PTR_LEN(cmd, op, type) \
-static inline void append_##cmd(u32 * const desc, dma_addr_t ptr, \
+static inline void append_##cmd(u32 * const desc, caam_dma_addr_t ptr, \
 				type len, u32 options) \
 { \
 	PRINT_POS; \
@@ -467,7 +467,7 @@ struct alginfo {
 	unsigned int keylen;
 	unsigned int keylen_pad;
 	union {
-		dma_addr_t key_dma;
+		caam_dma_addr_t key_dma;
 		const void *key_virt;
 	};
 	bool key_inline;
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index ec25d260fa40..bf115f8ddb93 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -34,7 +34,7 @@ struct caam_jrentry_info {
 	void (*callbk)(struct device *dev, u32 *desc, u32 status, void *arg);
 	void *cbkarg;	/* Argument per ring entry */
 	u32 *desc_addr_virt;	/* Stored virt addr for postprocessing */
-	dma_addr_t desc_addr_dma;	/* Stored bus addr for done matching */
+	caam_dma_addr_t desc_addr_dma;	/* Stored bus addr for done matching */
 	u32 desc_size;	/* Stored size for postprocessing, header derived */
 };
 
@@ -55,7 +55,7 @@ struct caam_drv_private_jr {
 	spinlock_t inplock ____cacheline_aligned; /* Input ring index lock */
 	u32 inpring_avail;	/* Number of free entries in input ring */
 	int head;			/* entinfo (s/w ring) head index */
-	dma_addr_t *inpring;	/* Base of input ring, alloc DMA-safe */
+	caam_dma_addr_t *inpring;	/* Base of input ring, alloc DMA-safe */
 	int out_ring_read_index;	/* Output index "tail" */
 	int tail;			/* entinfo (s/w ring) tail index */
 	struct jr_outentry *outring;	/* Base of output ring, DMA-safe */
@@ -221,7 +221,7 @@ static inline u64 caam_get_dma_mask(struct device *dev)
 {
 	struct device_node *nprop = dev->of_node;
 
-	if (sizeof(dma_addr_t) != sizeof(u64))
+	if (sizeof(caam_dma_addr_t) != sizeof(u64))
 		return DMA_BIT_MASK(32);
 
 	if (caam_dpaa2)
diff --git a/drivers/crypto/caam/pdb.h b/drivers/crypto/caam/pdb.h
index 810f0bef0652..128d16682feb 100644
--- a/drivers/crypto/caam/pdb.h
+++ b/drivers/crypto/caam/pdb.h
@@ -9,6 +9,7 @@
 #ifndef CAAM_PDB_H
 #define CAAM_PDB_H
 #include "compat.h"
+#include "regs.h"
 
 /*
  * PDB- IPSec ESP Header Modification Options
@@ -507,10 +508,10 @@ struct dsa_verify_pdb {
  */
 struct rsa_pub_pdb {
 	u32		sgf;
-	dma_addr_t	f_dma;
-	dma_addr_t	g_dma;
-	dma_addr_t	n_dma;
-	dma_addr_t	e_dma;
+	caam_dma_addr_t	f_dma;
+	caam_dma_addr_t	g_dma;
+	caam_dma_addr_t	n_dma;
+	caam_dma_addr_t	e_dma;
 	u32		f_len;
 } __packed;
 
@@ -524,10 +525,10 @@ struct rsa_pub_pdb {
  */
 struct rsa_priv_f1_pdb {
 	u32		sgf;
-	dma_addr_t	g_dma;
-	dma_addr_t	f_dma;
-	dma_addr_t	n_dma;
-	dma_addr_t	d_dma;
+	caam_dma_addr_t	g_dma;
+	caam_dma_addr_t	f_dma;
+	caam_dma_addr_t	n_dma;
+	caam_dma_addr_t	d_dma;
 } __packed;
 
 /**
@@ -546,13 +547,13 @@ struct rsa_priv_f1_pdb {
  */
 struct rsa_priv_f2_pdb {
 	u32		sgf;
-	dma_addr_t	g_dma;
-	dma_addr_t	f_dma;
-	dma_addr_t	d_dma;
-	dma_addr_t	p_dma;
-	dma_addr_t	q_dma;
-	dma_addr_t	tmp1_dma;
-	dma_addr_t	tmp2_dma;
+	caam_dma_addr_t	g_dma;
+	caam_dma_addr_t	f_dma;
+	caam_dma_addr_t	d_dma;
+	caam_dma_addr_t	p_dma;
+	caam_dma_addr_t	q_dma;
+	caam_dma_addr_t	tmp1_dma;
+	caam_dma_addr_t	tmp2_dma;
 	u32		p_q_len;
 } __packed;
 
@@ -576,15 +577,15 @@ struct rsa_priv_f2_pdb {
  */
 struct rsa_priv_f3_pdb {
 	u32		sgf;
-	dma_addr_t	g_dma;
-	dma_addr_t	f_dma;
-	dma_addr_t	c_dma;
-	dma_addr_t	p_dma;
-	dma_addr_t	q_dma;
-	dma_addr_t	dp_dma;
-	dma_addr_t	dq_dma;
-	dma_addr_t	tmp1_dma;
-	dma_addr_t	tmp2_dma;
+	caam_dma_addr_t	g_dma;
+	caam_dma_addr_t	f_dma;
+	caam_dma_addr_t	c_dma;
+	caam_dma_addr_t	p_dma;
+	caam_dma_addr_t	q_dma;
+	caam_dma_addr_t	dp_dma;
+	caam_dma_addr_t	dq_dma;
+	caam_dma_addr_t	tmp1_dma;
+	caam_dma_addr_t	tmp2_dma;
 	u32		p_q_len;
 } __packed;
 
diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 8591914d5c51..6e7f8d4f3f2b 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -137,7 +137,7 @@ static inline void clrsetbits_32(void __iomem *reg, u32 clear, u32 set)
  *    base + 0x0000 : least-significant 32 bits
  *    base + 0x0004 : most-significant 32 bits
  */
-#ifdef CONFIG_64BIT
+#if defined(CONFIG_64BIT) && !defined(CONFIG_ARCH_MXC)
 static inline void wr_reg64(void __iomem *reg, u64 data)
 {
 	if (caam_little_end)
@@ -195,7 +195,7 @@ static inline u64 caam_dma64_to_cpu(u64 value)
 	return caam64_to_cpu(value);
 }
 
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+#if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) && !defined(CONFIG_ARCH_MXC)
 #define cpu_to_caam_dma(value) cpu_to_caam_dma64(value)
 #define caam_dma_to_cpu(value) caam_dma64_to_cpu(value)
 #else
@@ -203,12 +203,27 @@ static inline u64 caam_dma64_to_cpu(u64 value)
 #define caam_dma_to_cpu(value) caam32_to_cpu(value)
 #endif /* CONFIG_ARCH_DMA_ADDR_T_64BIT */
 
+/*
+ * On i.MX8 boards the arch is arm64 but the CAAM dma address size is
+ * 32 bits on 8MQ and 36 bits on 8QM and 8QXP.
+ * For 8QM and 8QXP there is a configurable field PS called pointer size
+ * in the MCFGR register to switch between 32 and 64 (default 32)
+ * But this register is only accessible by the SECO and is left to its
+ * default value.
+ * Here we set the CAAM dma address size to 32 bits for all i.MX8
+ */
+#if defined(CONFIG_ARM64) && defined(CONFIG_ARCH_MXC)
+#define caam_dma_addr_t u32
+#else
+#define caam_dma_addr_t dma_addr_t
+#endif
+
 /*
  * jr_outentry
  * Represents each entry in a JobR output ring
  */
 struct jr_outentry {
-	dma_addr_t desc;/* Pointer to completed descriptor */
+	caam_dma_addr_t desc;/* Pointer to completed descriptor */
 	u32 jrstatus;	/* Status for completed descriptor */
 } __packed;
 
-- 
2.21.0

Re: [PATCH v3 2/5] crypto: caam - correct DMA address size for the i.MX8

[Leonard Crestez]

On 6/17/2019 7:04 PM, Andrey Smirnov wrote:
> From: Chris Spencer <christopher.spencer@sea.co.uk>
> 
> The i.MX8 is arm64, but its CAAM DMA address size is 32-bits.

> +/*
> + * On i.MX8 boards the arch is arm64 but the CAAM dma address size is
> + * 32 bits on 8MQ and 36 bits on 8QM and 8QXP.
> + * For 8QM and 8QXP there is a configurable field PS called pointer size
> + * in the MCFGR register to switch between 32 and 64 (default 32)
> + * But this register is only accessible by the SECO and is left to its
> + * default value.
> + * Here we set the CAAM dma address size to 32 bits for all i.MX8
> + */
> +#if defined(CONFIG_ARM64) && defined(CONFIG_ARCH_MXC)
> +#define caam_dma_addr_t u32
> +#else
> +#define caam_dma_addr_t dma_addr_t
> +#endif

Wait, doesn't this break Layerscape? Support for multiple SOC families 
can be enabled at the same time and it is something that we actually 
want to support.

--
Regards,
Leonard

Re: [PATCH v3 2/5] crypto: caam - correct DMA address size for the i.MX8

[Andrey Smirnov]

On Mon, Jun 17, 2019 at 12:24 PM Leonard Crestez
<leonard.crestez@nxp.com> wrote:
>
> On 6/17/2019 7:04 PM, Andrey Smirnov wrote:
> > From: Chris Spencer <christopher.spencer@sea.co.uk>
> >
> > The i.MX8 is arm64, but its CAAM DMA address size is 32-bits.
>
> > +/*
> > + * On i.MX8 boards the arch is arm64 but the CAAM dma address size is
> > + * 32 bits on 8MQ and 36 bits on 8QM and 8QXP.
> > + * For 8QM and 8QXP there is a configurable field PS called pointer size
> > + * in the MCFGR register to switch between 32 and 64 (default 32)
> > + * But this register is only accessible by the SECO and is left to its
> > + * default value.
> > + * Here we set the CAAM dma address size to 32 bits for all i.MX8
> > + */
> > +#if defined(CONFIG_ARM64) && defined(CONFIG_ARCH_MXC)
> > +#define caam_dma_addr_t u32
> > +#else
> > +#define caam_dma_addr_t dma_addr_t
> > +#endif
>
> Wait, doesn't this break Layerscape? Support for multiple SOC families
> can be enabled at the same time and it is something that we actually
> want to support.
>

Ugh, l think you are right. Will fix in next version.

Thanks,
Andrey Smirnov

[PATCH v3 3/5] crypto: caam - always select job ring via RSR on i.MX8MQ

[Andrey Smirnov]

Per feedback from NXP tech support the way to use register based
service interface on i.MX8MQ is to follow the same set of steps
outlined for the case when virtualization is enabled, regardless if it
is. Current version of SRM for i.MX8MQ speaks of DECO DID_MS and DECO
DID_LS registers, but apparently those are not implemented, so the
case when SCFGR[VIRT_EN]=0 should be handles the same as the case when
SCFGR[VIRT_EN]=1

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Spencer <christopher.spencer@sea.co.uk>
Cc: Cory Tusar <cory.tusar@zii.aero>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/caam/ctrl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index e674d8770cdb..e6e2457a573e 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -107,7 +107,12 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc,
 	int i;
 
 
-	if (ctrlpriv->virt_en == 1) {
+	if (ctrlpriv->virt_en == 1 ||
+	    /*
+	     * Apparently on i.MX8MQ it doesn't matter if virt_en == 1
+	     * and the following steps should be performed regardless
+	     */
+	    of_machine_is_compatible("fsl,imx8mq")) {
 		clrsetbits_32(&ctrl->deco_rsr, 0, DECORSR_JR0);
 
 		while (!(rd_reg32(&ctrl->deco_rsr) & DECORSR_VALID) &&
-- 
2.21.0

[PATCH v3 4/5] crypto: caam - simplfy clock initialization

[Andrey Smirnov]

Simplify clock initialization code by converting it to use clk-bulk,
devres and soc_device_match() match table. No functional change
intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Spencer <christopher.spencer@sea.co.uk>
Cc: Cory Tusar <cory.tusar@zii.aero>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/caam/ctrl.c   | 214 +++++++++++++++++------------------
 drivers/crypto/caam/intern.h |   5 -
 2 files changed, 107 insertions(+), 112 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index e6e2457a573e..b9655957d369 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -25,16 +25,6 @@ EXPORT_SYMBOL(caam_dpaa2);
 #include "qi.h"
 #endif
 
-/*
- * i.MX targets tend to have clock control subsystems that can
- * enable/disable clocking to our device.
- */
-static inline struct clk *caam_drv_identify_clk(struct device *dev,
-						char *clk_name)
-{
-	return caam_imx ? devm_clk_get(dev, clk_name) : NULL;
-}
-
 /*
  * Descriptor to instantiate RNG State Handle 0 in normal mode and
  * load the JDKEK, TDKEK and TDSK registers
@@ -347,13 +337,6 @@ static int caam_remove(struct platform_device *pdev)
 	/* Unmap controller region */
 	iounmap(ctrl);
 
-	/* shut clocks off before finalizing shutdown */
-	clk_disable_unprepare(ctrlpriv->caam_ipg);
-	if (ctrlpriv->caam_mem)
-		clk_disable_unprepare(ctrlpriv->caam_mem);
-	clk_disable_unprepare(ctrlpriv->caam_aclk);
-	if (ctrlpriv->caam_emi_slow)
-		clk_disable_unprepare(ctrlpriv->caam_emi_slow);
 	return 0;
 }
 
@@ -502,20 +485,113 @@ static const struct of_device_id caam_match[] = {
 };
 MODULE_DEVICE_TABLE(of, caam_match);
 
+struct clk_bulk_caam {
+	const struct clk_bulk_data *clks;
+	int num_clks;
+};
+
+static const struct clk_bulk_data caam_imx6_clks[] = {
+	{ .id = "ipg" },
+	{ .id = "mem" },
+	{ .id = "aclk" },
+	{ .id = "emi_slow" },
+};
+
+static const struct clk_bulk_caam caam_imx6_clk_data = {
+	.clks = caam_imx6_clks,
+	.num_clks = ARRAY_SIZE(caam_imx6_clks),
+};
+
+static const struct clk_bulk_data caam_imx7_clks[] = {
+	{ .id = "ipg" },
+	{ .id = "aclk" },
+};
+
+static const struct clk_bulk_caam caam_imx7_clk_data = {
+	.clks = caam_imx7_clks,
+	.num_clks = ARRAY_SIZE(caam_imx7_clks),
+};
+
+static const struct clk_bulk_data caam_imx6ul_clks[] = {
+	{ .id = "ipg" },
+	{ .id = "mem" },
+	{ .id = "aclk" },
+};
+
+static const struct clk_bulk_caam caam_imx6ul_clk_data = {
+	.clks = caam_imx6ul_clks,
+	.num_clks = ARRAY_SIZE(caam_imx6ul_clks),
+};
+
+
+static const struct soc_device_attribute imx_soc[] = {
+	{ .soc_id = "i.MX6UL", .data = &caam_imx6ul_clk_data },
+	{ .soc_id = "i.MX6*",  .data = &caam_imx6_clk_data },
+	{ .soc_id = "i.MX7*",  .data = &caam_imx7_clk_data },
+	{ .family = "Freescale i.MX" },
+};
+
+static void disable_clocks(void *private)
+{
+	struct clk_bulk_caam *context = private;
+
+	clk_bulk_disable_unprepare(context->num_clks,
+				   (struct clk_bulk_data *)context->clks);
+}
+
+static int init_clocks(struct device *dev,
+		       const struct clk_bulk_caam *data)
+{
+	struct clk_bulk_data *clks;
+	struct clk_bulk_caam *context;
+	int num_clks;
+	int ret;
+
+	num_clks = data->num_clks;
+	clks = devm_kmemdup(dev, data->clks,
+			    data->num_clks * sizeof(data->clks[0]),
+			    GFP_KERNEL);
+	if (!clks)
+		return -ENOMEM;
+
+	ret = devm_clk_bulk_get(dev, num_clks, clks);
+	if (ret) {
+		dev_err(dev,
+			"Failed to request all necessary clocks\n");
+		return ret;
+	}
+
+	ret = clk_bulk_prepare_enable(num_clks, clks);
+	if (ret) {
+		dev_err(dev,
+			"Failed to prepare/enable all necessary clocks\n");
+		return ret;
+	}
+
+	context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);
+	if (!context)
+		return -ENOMEM;
+
+	context->num_clks = num_clks;
+	context->clks = clks;
+
+	ret = devm_add_action_or_reset(dev, disable_clocks, context);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 /* Probe routine for CAAM top (controller) level */
 static int caam_probe(struct platform_device *pdev)
 {
 	int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
 	u64 caam_id;
-	static const struct soc_device_attribute imx_soc[] = {
-		{.family = "Freescale i.MX"},
-		{},
-	};
+	const struct soc_device_attribute *soc_attr;
 	struct device *dev;
 	struct device_node *nprop, *np;
 	struct caam_ctrl __iomem *ctrl;
 	struct caam_drv_private *ctrlpriv;
-	struct clk *clk;
 #ifdef CONFIG_DEBUG_FS
 	struct caam_perfmon *perfmon;
 #endif
@@ -532,91 +608,25 @@ static int caam_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, ctrlpriv);
 	nprop = pdev->dev.of_node;
 
-	caam_imx = (bool)soc_device_match(imx_soc);
-
-	/* Enable clocking */
-	clk = caam_drv_identify_clk(&pdev->dev, "ipg");
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
-		dev_err(&pdev->dev,
-			"can't identify CAAM ipg clk: %d\n", ret);
-		return ret;
-	}
-	ctrlpriv->caam_ipg = clk;
-
-	if (!of_machine_is_compatible("fsl,imx7d") &&
-	    !of_machine_is_compatible("fsl,imx7s") &&
-	    !of_machine_is_compatible("fsl,imx7ulp")) {
-		clk = caam_drv_identify_clk(&pdev->dev, "mem");
-		if (IS_ERR(clk)) {
-			ret = PTR_ERR(clk);
-			dev_err(&pdev->dev,
-				"can't identify CAAM mem clk: %d\n", ret);
-			return ret;
+	soc_attr = soc_device_match(imx_soc);
+	if (soc_attr) {
+		if (!soc_attr->data) {
+			dev_err(dev, "No clock data provided for i.MX SoC");
+			return -EINVAL;
 		}
-		ctrlpriv->caam_mem = clk;
-	}
 
-	clk = caam_drv_identify_clk(&pdev->dev, "aclk");
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
-		dev_err(&pdev->dev,
-			"can't identify CAAM aclk clk: %d\n", ret);
-		return ret;
-	}
-	ctrlpriv->caam_aclk = clk;
-
-	if (!of_machine_is_compatible("fsl,imx6ul") &&
-	    !of_machine_is_compatible("fsl,imx7d") &&
-	    !of_machine_is_compatible("fsl,imx7s") &&
-	    !of_machine_is_compatible("fsl,imx7ulp")) {
-		clk = caam_drv_identify_clk(&pdev->dev, "emi_slow");
-		if (IS_ERR(clk)) {
-			ret = PTR_ERR(clk);
-			dev_err(&pdev->dev,
-				"can't identify CAAM emi_slow clk: %d\n", ret);
+		ret = init_clocks(dev, soc_attr->data);
+		if (ret)
 			return ret;
-		}
-		ctrlpriv->caam_emi_slow = clk;
-	}
-
-	ret = clk_prepare_enable(ctrlpriv->caam_ipg);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "can't enable CAAM ipg clock: %d\n", ret);
-		return ret;
-	}
-
-	if (ctrlpriv->caam_mem) {
-		ret = clk_prepare_enable(ctrlpriv->caam_mem);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "can't enable CAAM secure mem clock: %d\n",
-				ret);
-			goto disable_caam_ipg;
-		}
-	}
-
-	ret = clk_prepare_enable(ctrlpriv->caam_aclk);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "can't enable CAAM aclk clock: %d\n", ret);
-		goto disable_caam_mem;
-	}
-
-	if (ctrlpriv->caam_emi_slow) {
-		ret = clk_prepare_enable(ctrlpriv->caam_emi_slow);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "can't enable CAAM emi slow clock: %d\n",
-				ret);
-			goto disable_caam_aclk;
-		}
 	}
+	caam_imx = (bool)soc_attr;
 
 	/* Get configuration properties from device tree */
 	/* First, get register page */
 	ctrl = of_iomap(nprop, 0);
 	if (ctrl == NULL) {
 		dev_err(dev, "caam: of_iomap() failed\n");
-		ret = -ENOMEM;
-		goto disable_caam_emi_slow;
+		return -ENOMEM;
 	}
 
 	caam_little_end = !(bool)(rd_reg32(&ctrl->perfmon.status) &
@@ -904,16 +914,6 @@ static int caam_probe(struct platform_device *pdev)
 #endif
 iounmap_ctrl:
 	iounmap(ctrl);
-disable_caam_emi_slow:
-	if (ctrlpriv->caam_emi_slow)
-		clk_disable_unprepare(ctrlpriv->caam_emi_slow);
-disable_caam_aclk:
-	clk_disable_unprepare(ctrlpriv->caam_aclk);
-disable_caam_mem:
-	if (ctrlpriv->caam_mem)
-		clk_disable_unprepare(ctrlpriv->caam_mem);
-disable_caam_ipg:
-	clk_disable_unprepare(ctrlpriv->caam_ipg);
 	return ret;
 }
 
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index bf115f8ddb93..81f73d32a9f8 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -94,11 +94,6 @@ struct caam_drv_private {
 				   Handles of the RNG4 block are initialized
 				   by this driver */
 
-	struct clk *caam_ipg;
-	struct clk *caam_mem;
-	struct clk *caam_aclk;
-	struct clk *caam_emi_slow;
-
 	/*
 	 * debugfs entries for developer view into driver/device
 	 * variables at runtime.
-- 
2.21.0

Re: [PATCH v3 4/5] crypto: caam - simplfy clock initialization

[Leonard Crestez]

On 6/17/2019 7:04 PM, Andrey Smirnov wrote:
> Simplify clock initialization code by converting it to use clk-bulk,
> devres and soc_device_match() match table. No functional change
> intended.

Subject is misspelled.

> +struct clk_bulk_caam {
> +	const struct clk_bulk_data *clks;
> +	int num_clks;
> +};

clks could be an array[0] at the end to avoid an additional allocation.

> +static void disable_clocks(void *private)
> +{
> +	struct clk_bulk_caam *context = private;
> +
> +	clk_bulk_disable_unprepare(context->num_clks,
> +				   (struct clk_bulk_data *)context->clks);
> +}

Not sure using devm for this is worthwhile. Maybe someday CAAM clks will 
be enabled dynamically?

It would be make sense to reference "clk" instead of "clocks".

> +static int init_clocks(struct device *dev,
> +		       const struct clk_bulk_caam *data)
> +{
> +	struct clk_bulk_data *clks;
> +	struct clk_bulk_caam *context;
> +	int num_clks;
> +	int ret;
> +
> +	num_clks = data->num_clks;
> +	clks = devm_kmemdup(dev, data->clks,
> +			    data->num_clks * sizeof(data->clks[0]),
> +			    GFP_KERNEL);
> +	if (!clks)
> +		return -ENOMEM;
> +
> +	ret = devm_clk_bulk_get(dev, num_clks, clks);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to request all necessary clocks\n");
> +		return ret;
> +	}
> +
> +	ret = clk_bulk_prepare_enable(num_clks, clks);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to prepare/enable all necessary clocks\n");
> +		return ret;
> +	}
> +
> +	context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);
> +	if (!context)
> +		return -ENOMEM;

Aren't clks left enabled if this fails? Can move this allocation higher.

> +	context->num_clks = num_clks;
> +	context->clks = clks;
> +
> +	ret = devm_add_action_or_reset(dev, disable_clocks, context);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

>   static int caam_probe(struct platform_device *pdev)
>   {
>   	int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
>   	u64 caam_id;
> -	static const struct soc_device_attribute imx_soc[] = {
> -		{.family = "Freescale i.MX"},
> -		{},
> -	};
> +	const struct soc_device_attribute *soc_attr;

This "soc_attr" is difficult to understand, maybe rename to something 
like "imx_soc_match"?

Re: [PATCH v3 4/5] crypto: caam - simplfy clock initialization

[Andrey Smirnov]

On Mon, Jun 17, 2019 at 12:48 PM Leonard Crestez
<leonard.crestez@nxp.com> wrote:
>
> On 6/17/2019 7:04 PM, Andrey Smirnov wrote:
> > Simplify clock initialization code by converting it to use clk-bulk,
> > devres and soc_device_match() match table. No functional change
> > intended.
>
> Subject is misspelled.
>

Will fix.

> > +struct clk_bulk_caam {
> > +     const struct clk_bulk_data *clks;
> > +     int num_clks;
> > +};
>
> clks could be an array[0] at the end to avoid an additional allocation.
>

struct clk_bulk_caam is also used to declare caam_imx*_clk_data
variables, where this approach wouldn't work.

> > +static void disable_clocks(void *private)
> > +{
> > +     struct clk_bulk_caam *context = private;
> > +
> > +     clk_bulk_disable_unprepare(context->num_clks,
> > +                                (struct clk_bulk_data *)context->clks);
> > +}
>
> Not sure using devm for this is worthwhile. Maybe someday CAAM clks will
> be enabled dynamically?
>

I don't see a reason why this code can't be changed _if_ and when that
ever happens.

> It would be make sense to reference "clk" instead of "clocks".
>

I am not sure what you reference here. I'd rather we avoid discussing
trivial spelling aspects like this.

> > +static int init_clocks(struct device *dev,
> > +                    const struct clk_bulk_caam *data)
> > +{
> > +     struct clk_bulk_data *clks;
> > +     struct clk_bulk_caam *context;
> > +     int num_clks;
> > +     int ret;
> > +
> > +     num_clks = data->num_clks;
> > +     clks = devm_kmemdup(dev, data->clks,
> > +                         data->num_clks * sizeof(data->clks[0]),
> > +                         GFP_KERNEL);
> > +     if (!clks)
> > +             return -ENOMEM;
> > +
> > +     ret = devm_clk_bulk_get(dev, num_clks, clks);
> > +     if (ret) {
> > +             dev_err(dev,
> > +                     "Failed to request all necessary clocks\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = clk_bulk_prepare_enable(num_clks, clks);
> > +     if (ret) {
> > +             dev_err(dev,
> > +                     "Failed to prepare/enable all necessary clocks\n");
> > +             return ret;
> > +     }
> > +
> > +     context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);
> > +     if (!context)
> > +             return -ENOMEM;
>
> Aren't clks left enabled if this fails? Can move this allocation higher.
>

Good point, will do.

> > +     context->num_clks = num_clks;
> > +     context->clks = clks;
> > +
> > +     ret = devm_add_action_or_reset(dev, disable_clocks, context);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
>
> >   static int caam_probe(struct platform_device *pdev)
> >   {
> >       int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
> >       u64 caam_id;
> > -     static const struct soc_device_attribute imx_soc[] = {
> > -             {.family = "Freescale i.MX"},
> > -             {},
> > -     };
> > +     const struct soc_device_attribute *soc_attr;
>
> This "soc_attr" is difficult to understand, maybe rename to something
> like "imx_soc_match"?

Sure, will do in next version.

Thanks,
Andrey Smirnov

[PATCH v3 5/5] crypto: caam - add clock entry for i.MX8MQ

[Andrey Smirnov]

Add clock entry needed to support i.MX8MQ.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Spencer <christopher.spencer@sea.co.uk>
Cc: Cory Tusar <cory.tusar@zii.aero>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/caam/ctrl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index b9655957d369..888eacc7c17d 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -528,6 +528,7 @@ static const struct soc_device_attribute imx_soc[] = {
 	{ .soc_id = "i.MX6UL", .data = &caam_imx6ul_clk_data },
 	{ .soc_id = "i.MX6*",  .data = &caam_imx6_clk_data },
 	{ .soc_id = "i.MX7*",  .data = &caam_imx7_clk_data },
+	{ .soc_id = "i.MX8MQ", .data = &caam_imx7_clk_data },
 	{ .family = "Freescale i.MX" },
 };
 
-- 
2.21.0