[PATCHv2 0/3] crypto: inside-secure - broaden driver scope

[PATCHv2 0/3] crypto: inside-secure - broaden driver scope

[Pascal van Leeuwen]

This is a first baby step towards making the inside-secure crypto driver
more broadly useful. The current driver only works for Marvell Armada HW
and requires proprietary firmware, only available under NDA from Marvell,
to be installed. This patch set allows the driver to be used with other
hardware and removes the dependence on that proprietary firmware.

changes since v1:
- changed dev_info's into dev_dbg to reduce normal verbosity
- terminate all message strings with \n
- use priv->version field strictly to enumerate device context
- fixed some code & comment style issues
- removed EIP97/197 references from messages
- use #if(IS_ENABLED(CONFIG_PCI)) to remove all PCI related code
- use #if(IS_ENABLED(CONFIG_OF)) to remove all device tree related code
- do not inline the minifw but read it from /lib/firmware instead

Pascal van Leeuwen (3):
  crypto: inside-secure - make driver selectable for non-Marvell
    hardware
  crypto: inside-secure - add support for PCI based FPGA development
    board
  crypto: inside-secure - add support for using the EIP197 without
    vendor firmware

 drivers/crypto/Kconfig                         |  12 +-
 drivers/crypto/inside-secure/safexcel.c        | 748 +++++++++++++++++--------
 drivers/crypto/inside-secure/safexcel.h        |  36 +-
 drivers/crypto/inside-secure/safexcel_cipher.c |  11 -
 drivers/crypto/inside-secure/safexcel_hash.c   |  12 -
 drivers/crypto/inside-secure/safexcel_ring.c   |   3 +-
 6 files changed, 569 insertions(+), 253 deletions(-)

-- 
1.8.3.1

[PATCHv2 1/3] Kconfig: inside-secure - make driver selectable for non-Marvell hardware

[Pascal van Leeuwen]

While being a generic EIP97/EIP197 driver, the driver was only selectable
for Marvell Armada hardware. This fix makes the driver selectable for any
Device Tree supporting kernel configuration, allowing it to be used for
other compatible hardware by just adding the correct device tree entry.

It also allows the driver to be selected for PCI(E) supporting kernel con-
figurations, to be able to use it with PCIE based FPGA development boards
for pre-silicon driver development by both Inside Secure and its IP custo-
mers.

Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 drivers/crypto/Kconfig | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 67af688..0d9f67d 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -716,8 +716,7 @@ source "drivers/crypto/stm32/Kconfig"
 
 config CRYPTO_DEV_SAFEXCEL
 	tristate "Inside Secure's SafeXcel cryptographic engine driver"
-	depends on OF
-	depends on (ARM64 && ARCH_MVEBU) || (COMPILE_TEST && 64BIT)
+	depends on OF || PCI || COMPILE_TEST
 	select CRYPTO_AES
 	select CRYPTO_AUTHENC
 	select CRYPTO_BLKCIPHER
@@ -729,10 +728,11 @@ config CRYPTO_DEV_SAFEXCEL
 	select CRYPTO_SHA256
 	select CRYPTO_SHA512
 	help
-	  This driver interfaces with the SafeXcel EIP-197 cryptographic engine
-	  designed by Inside Secure. Select this if you want to use CBC/ECB
-	  chain mode, AES cipher mode and SHA1/SHA224/SHA256/SHA512 hash
-	  algorithms.
+	  This driver interfaces with the SafeXcel EIP-97 and EIP-197 cryptographic
+	  engines designed by Inside Secure. It currently accelerates DES, 3DES and
+	  AES block ciphers in ECB and CBC mode, as well as SHA1, SHA224, SHA256,
+	  SHA384 and SHA512 hash algorithms for both basic hash and HMAC.
+	  Additionally, it accelerates combined AES-CBC/HMAC-SHA AEAD operations.
 
 config CRYPTO_DEV_ARTPEC6
 	tristate "Support for Axis ARTPEC-6/7 hardware crypto acceleration."
-- 
1.8.3.1

[PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Pascal van Leeuwen]

This patch adds support for a PCIE development board with FPGA from Xilinx,
to facilitate pre-silicon driver development by both Inside Secure and its
IP customers. Since Inside Secure neither produces nor has access to actual
silicon, this is required functionality to allow us to contribute.

Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 drivers/crypto/inside-secure/safexcel.c        | 560 +++++++++++++++++--------
 drivers/crypto/inside-secure/safexcel.h        |  30 +-
 drivers/crypto/inside-secure/safexcel_cipher.c |  11 -
 drivers/crypto/inside-secure/safexcel_hash.c   |  12 -
 drivers/crypto/inside-secure/safexcel_ring.c   |   3 +-
 5 files changed, 413 insertions(+), 203 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index df43a2c..fe9b6d0 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/of_irq.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/workqueue.h>
 
@@ -32,16 +33,17 @@ static void eip197_trc_cache_init(struct safexcel_crypto_priv *priv)
 	u32 val, htable_offset;
 	int i, cs_rc_max, cs_ht_wc, cs_trc_rec_wc, cs_trc_lg_rec_wc;
 
-	if (priv->version == EIP197B) {
-		cs_rc_max = EIP197B_CS_RC_MAX;
-		cs_ht_wc = EIP197B_CS_HT_WC;
-		cs_trc_rec_wc = EIP197B_CS_TRC_REC_WC;
-		cs_trc_lg_rec_wc = EIP197B_CS_TRC_LG_REC_WC;
-	} else {
+	if (priv->version == EIP197D_MRVL) {
 		cs_rc_max = EIP197D_CS_RC_MAX;
 		cs_ht_wc = EIP197D_CS_HT_WC;
 		cs_trc_rec_wc = EIP197D_CS_TRC_REC_WC;
 		cs_trc_lg_rec_wc = EIP197D_CS_TRC_LG_REC_WC;
+	} else {
+		/* Default to minimum "safe" settings */
+		cs_rc_max = EIP197B_CS_RC_MAX;
+		cs_ht_wc = EIP197B_CS_HT_WC;
+		cs_trc_rec_wc = EIP197B_CS_TRC_REC_WC;
+		cs_trc_lg_rec_wc = EIP197B_CS_TRC_LG_REC_WC;
 	}
 
 	/* Enable the record cache memory access */
@@ -145,26 +147,24 @@ static int eip197_load_firmwares(struct safexcel_crypto_priv *priv)
 	int i, j, ret = 0, pe;
 	u32 val;
 
-	switch (priv->version) {
-	case EIP197B:
-		dir = "eip197b";
-		break;
-	case EIP197D:
-		dir = "eip197d";
-		break;
-	default:
+	if (priv->version == EIP97IES_MRVL)
 		/* No firmware is required */
 		return 0;
-	}
+	else if (priv->version == EIP197D_MRVL)
+		dir = "eip197d";
+	else
+		/* Default to minimum EIP197 config */
+		dir = "eip197b";
 
 	for (i = 0; i < FW_NB; i++) {
 		snprintf(fw_path, 31, "inside-secure/%s/%s", dir, fw_name[i]);
 		ret = request_firmware(&fw[i], fw_path, priv->dev);
 		if (ret) {
-			if (priv->version != EIP197B)
+			if (!(priv->version == EIP197B_MRVL))
 				goto release_fw;
 
-			/* Fallback to the old firmware location for the
+			/*
+			 * Fallback to the old firmware location for the
 			 * EIP197b.
 			 */
 			ret = request_firmware(&fw[i], fw_name[i], priv->dev);
@@ -294,6 +294,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
 	u32 version, val;
 	int i, ret, pe;
 
+	dev_dbg(priv->dev, "HW init: using %d pipe(s) and %d ring(s)\n",
+		priv->config.pes, priv->config.rings);
+
 	/* Determine endianess and configure byte swap */
 	version = readl(EIP197_HIA_AIC(priv) + EIP197_HIA_VERSION);
 	val = readl(EIP197_HIA_AIC(priv) + EIP197_HIA_MST_CTRL);
@@ -304,7 +307,7 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
 		val |= (EIP197_MST_CTRL_NO_BYTE_SWAP >> 24);
 
 	/* For EIP197 set maximum number of TX commands to 2^5 = 32 */
-	if (priv->version == EIP197B || priv->version == EIP197D)
+	if (priv->version != EIP97IES_MRVL)
 		val |= EIP197_MST_CTRL_TX_MAX_CMD(5);
 
 	writel(val, EIP197_HIA_AIC(priv) + EIP197_HIA_MST_CTRL);
@@ -330,11 +333,10 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
 		writel(EIP197_DxE_THR_CTRL_RESET_PE,
 		       EIP197_HIA_DFE_THR(priv) + EIP197_HIA_DFE_THR_CTRL(pe));
 
-		if (priv->version == EIP197B || priv->version == EIP197D) {
+		if (priv->version != EIP97IES_MRVL)
 			/* Reset HIA input interface arbiter */
 			writel(EIP197_HIA_RA_PE_CTRL_RESET,
 			       EIP197_HIA_AIC(priv) + EIP197_HIA_RA_PE_CTRL(pe));
-		}
 
 		/* DMA transfer size to use */
 		val = EIP197_HIA_DFE_CFG_DIS_DEBUG;
@@ -357,12 +359,11 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
 		       EIP197_PE_IN_xBUF_THRES_MAX(7),
 		       EIP197_PE(priv) + EIP197_PE_IN_TBUF_THRES(pe));
 
-		if (priv->version == EIP197B || priv->version == EIP197D) {
+		if (priv->version != EIP97IES_MRVL)
 			/* enable HIA input interface arbiter and rings */
 			writel(EIP197_HIA_RA_PE_CTRL_EN |
 			       GENMASK(priv->config.rings - 1, 0),
 			       EIP197_HIA_AIC(priv) + EIP197_HIA_RA_PE_CTRL(pe));
-		}
 
 		/* Data Store Engine configuration */
 
@@ -381,10 +382,11 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
 		       EIP197_HIA_DxE_CFG_MAX_DATA_SIZE(8);
 		val |= EIP197_HIA_DxE_CFG_DATA_CACHE_CTRL(WR_CACHE_3BITS);
 		val |= EIP197_HIA_DSE_CFG_ALWAYS_BUFFERABLE;
-		/* FIXME: instability issues can occur for EIP97 but disabling it impact
-		 * performances.
+		/*
+		 * FIXME: instability issues can occur for EIP97 but disabling
+		 * it impacts performance.
 		 */
-		if (priv->version == EIP197B || priv->version == EIP197D)
+		if (priv->version != EIP97IES_MRVL)
 			val |= EIP197_HIA_DSE_CFG_EN_SINGLE_WR;
 		writel(val, EIP197_HIA_DSE(priv) + EIP197_HIA_DSE_CFG(pe));
 
@@ -479,7 +481,7 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
 	/* Clear any HIA interrupt */
 	writel(GENMASK(30, 20), EIP197_HIA_AIC_G(priv) + EIP197_HIA_AIC_G_ACK);
 
-	if (priv->version == EIP197B || priv->version == EIP197D) {
+	if (priv->version != EIP97IES_MRVL) {
 		eip197_trc_cache_init(priv);
 
 		ret = eip197_load_firmwares(priv);
@@ -514,7 +516,8 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
 	struct safexcel_context *ctx;
 	int ret, nreq = 0, cdesc = 0, rdesc = 0, commands, results;
 
-	/* If a request wasn't properly dequeued because of a lack of resources,
+	/*
+	 * If a request wasn't properly dequeued because of a lack of resources,
 	 * proceeded it first,
 	 */
 	req = priv->ring[ring].req;
@@ -543,7 +546,8 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
 		if (backlog)
 			backlog->complete(backlog, -EINPROGRESS);
 
-		/* In case the send() helper did not issue any command to push
+		/*
+		 * In case the send() helper did not issue any command to push
 		 * to the engine because the input data was cached, continue to
 		 * dequeue other requests as this is valid and not an error.
 		 */
@@ -556,7 +560,8 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
 	}
 
 request_failed:
-	/* Not enough resources to handle all the requests. Bail out and save
+	/*
+	 * Not enough resources to handle all the requests. Bail out and save
 	 * the request and the backlog for the next dequeue call (per-ring).
 	 */
 	priv->ring[ring].req = req;
@@ -711,7 +716,8 @@ static inline void safexcel_handle_result_descriptor(struct safexcel_crypto_priv
 		ndesc = ctx->handle_result(priv, ring, req,
 					   &should_complete, &ret);
 		if (ndesc < 0) {
-			dev_err(priv->dev, "failed to handle result (%d)", ndesc);
+			dev_err(priv->dev, "failed to handle result (%d)\n",
+				ndesc);
 			goto acknowledge;
 		}
 
@@ -731,7 +737,8 @@ static inline void safexcel_handle_result_descriptor(struct safexcel_crypto_priv
 		       EIP197_xDR_PROC_xD_COUNT(tot_descs * priv->config.rd_offset),
 		       EIP197_HIA_RDR(priv, ring) + EIP197_HIA_xDR_PROC_COUNT);
 
-	/* If the number of requests overflowed the counter, try to proceed more
+	/*
+	 * If the number of requests overflowed the counter, try to proceed more
 	 * requests.
 	 */
 	if (nreq == EIP197_xDR_PROC_xD_PKT_MASK)
@@ -783,7 +790,7 @@ static irqreturn_t safexcel_irq_ring(int irq, void *data)
 			 * reinitialized. This should not happen under
 			 * normal circumstances.
 			 */
-			dev_err(priv->dev, "RDR: fatal error.");
+			dev_err(priv->dev, "RDR: fatal error.\n");
 		} else if (likely(stat & EIP197_xDR_THRESH)) {
 			rc = IRQ_WAKE_THREAD;
 		}
@@ -813,23 +820,45 @@ static irqreturn_t safexcel_irq_ring_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static int safexcel_request_ring_irq(struct platform_device *pdev, const char *name,
+static int safexcel_request_ring_irq(void *pdev, int irqid,
+				     int is_pci_dev,
 				     irq_handler_t handler,
 				     irq_handler_t threaded_handler,
 				     struct safexcel_ring_irq_data *ring_irq_priv)
 {
-	int ret, irq = platform_get_irq_byname(pdev, name);
+	int ret, irq;
+	struct device *dev;
+
+	if (IS_ENABLED(CONFIG_PCI) && is_pci_dev) {
+		struct pci_dev *pci_pdev = pdev;
+
+		dev = &pci_pdev->dev;
+		irq = pci_irq_vector(pci_pdev, irqid);
+		if (irq < 0) {
+			dev_err(dev, "unable to get device MSI IRQ %d (err %d)\n",
+				irqid, irq);
+			return irq;
+		}
+	} else if (IS_ENABLED(CONFIG_OF)) {
+		struct platform_device *plf_pdev = pdev;
+		char irq_name[6] = {0}; /* "ringX\0" */
 
-	if (irq < 0) {
-		dev_err(&pdev->dev, "unable to get IRQ '%s'\n", name);
-		return irq;
+		snprintf(irq_name, 6, "ring%d", irqid);
+		dev = &plf_pdev->dev;
+		irq = platform_get_irq_byname(plf_pdev, irq_name);
+
+		if (irq < 0) {
+			dev_err(dev, "unable to get IRQ '%s' (err %d)\n",
+				irq_name, irq);
+			return irq;
+		}
 	}
 
-	ret = devm_request_threaded_irq(&pdev->dev, irq, handler,
+	ret = devm_request_threaded_irq(dev, irq, handler,
 					threaded_handler, IRQF_ONESHOT,
-					dev_name(&pdev->dev), ring_irq_priv);
+					dev_name(dev), ring_irq_priv);
 	if (ret) {
-		dev_err(&pdev->dev, "unable to request IRQ %d\n", irq);
+		dev_err(dev, "unable to request IRQ %d\n", irq);
 		return ret;
 	}
 
@@ -869,9 +898,6 @@ static int safexcel_register_algorithms(struct safexcel_crypto_priv *priv)
 	for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) {
 		safexcel_algs[i]->priv = priv;
 
-		if (!(safexcel_algs[i]->engines & priv->version))
-			continue;
-
 		if (safexcel_algs[i]->type == SAFEXCEL_ALG_TYPE_SKCIPHER)
 			ret = crypto_register_skcipher(&safexcel_algs[i]->alg.skcipher);
 		else if (safexcel_algs[i]->type == SAFEXCEL_ALG_TYPE_AEAD)
@@ -887,9 +913,6 @@ static int safexcel_register_algorithms(struct safexcel_crypto_priv *priv)
 
 fail:
 	for (j = 0; j < i; j++) {
-		if (!(safexcel_algs[j]->engines & priv->version))
-			continue;
-
 		if (safexcel_algs[j]->type == SAFEXCEL_ALG_TYPE_SKCIPHER)
 			crypto_unregister_skcipher(&safexcel_algs[j]->alg.skcipher);
 		else if (safexcel_algs[j]->type == SAFEXCEL_ALG_TYPE_AEAD)
@@ -906,9 +929,6 @@ static void safexcel_unregister_algorithms(struct safexcel_crypto_priv *priv)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) {
-		if (!(safexcel_algs[i]->engines & priv->version))
-			continue;
-
 		if (safexcel_algs[i]->type == SAFEXCEL_ALG_TYPE_SKCIPHER)
 			crypto_unregister_skcipher(&safexcel_algs[i]->alg.skcipher);
 		else if (safexcel_algs[i]->type == SAFEXCEL_ALG_TYPE_AEAD)
@@ -925,22 +945,18 @@ static void safexcel_configure(struct safexcel_crypto_priv *priv)
 	val = readl(EIP197_HIA_AIC_G(priv) + EIP197_HIA_OPTIONS);
 
 	/* Read number of PEs from the engine */
-	switch (priv->version) {
-	case EIP197B:
-	case EIP197D:
-		mask = EIP197_N_PES_MASK;
-		break;
-	default:
+	if (priv->version == EIP97IES_MRVL)
 		mask = EIP97_N_PES_MASK;
-	}
+	else
+		mask = EIP197_N_PES_MASK;
+
 	priv->config.pes = (val >> EIP197_N_PES_OFFSET) & mask;
 
+	priv->config.rings = min_t(u32, val & GENMASK(3, 0), max_rings);
+
 	val = (val & GENMASK(27, 25)) >> 25;
 	mask = BIT(val) - 1;
 
-	val = readl(EIP197_HIA_AIC_G(priv) + EIP197_HIA_OPTIONS);
-	priv->config.rings = min_t(u32, val & GENMASK(3, 0), max_rings);
-
 	priv->config.cd_size = (sizeof(struct safexcel_command_desc) / sizeof(u32));
 	priv->config.cd_offset = (priv->config.cd_size + mask) & ~mask;
 
@@ -952,21 +968,7 @@ static void safexcel_init_register_offsets(struct safexcel_crypto_priv *priv)
 {
 	struct safexcel_register_offsets *offsets = &priv->offsets;
 
-	switch (priv->version) {
-	case EIP197B:
-	case EIP197D:
-		offsets->hia_aic	= EIP197_HIA_AIC_BASE;
-		offsets->hia_aic_g	= EIP197_HIA_AIC_G_BASE;
-		offsets->hia_aic_r	= EIP197_HIA_AIC_R_BASE;
-		offsets->hia_aic_xdr	= EIP197_HIA_AIC_xDR_BASE;
-		offsets->hia_dfe	= EIP197_HIA_DFE_BASE;
-		offsets->hia_dfe_thr	= EIP197_HIA_DFE_THR_BASE;
-		offsets->hia_dse	= EIP197_HIA_DSE_BASE;
-		offsets->hia_dse_thr	= EIP197_HIA_DSE_THR_BASE;
-		offsets->hia_gen_cfg	= EIP197_HIA_GEN_CFG_BASE;
-		offsets->pe		= EIP197_PE_BASE;
-		break;
-	case EIP97IES:
+	if (priv->version == EIP97IES_MRVL) {
 		offsets->hia_aic	= EIP97_HIA_AIC_BASE;
 		offsets->hia_aic_g	= EIP97_HIA_AIC_G_BASE;
 		offsets->hia_aic_r	= EIP97_HIA_AIC_R_BASE;
@@ -977,135 +979,119 @@ static void safexcel_init_register_offsets(struct safexcel_crypto_priv *priv)
 		offsets->hia_dse_thr	= EIP97_HIA_DSE_THR_BASE;
 		offsets->hia_gen_cfg	= EIP97_HIA_GEN_CFG_BASE;
 		offsets->pe		= EIP97_PE_BASE;
-		break;
+	} else {
+		offsets->hia_aic	= EIP197_HIA_AIC_BASE;
+		offsets->hia_aic_g	= EIP197_HIA_AIC_G_BASE;
+		offsets->hia_aic_r	= EIP197_HIA_AIC_R_BASE;
+		offsets->hia_aic_xdr	= EIP197_HIA_AIC_xDR_BASE;
+		offsets->hia_dfe	= EIP197_HIA_DFE_BASE;
+		offsets->hia_dfe_thr	= EIP197_HIA_DFE_THR_BASE;
+		offsets->hia_dse	= EIP197_HIA_DSE_BASE;
+		offsets->hia_dse_thr	= EIP197_HIA_DSE_THR_BASE;
+		offsets->hia_gen_cfg	= EIP197_HIA_GEN_CFG_BASE;
+		offsets->pe		= EIP197_PE_BASE;
 	}
 }
 
-static int safexcel_probe(struct platform_device *pdev)
+/*
+ * Generic part of probe routine, shared by platform and PCI driver
+ *
+ * Assumes IO resources have been mapped, private data mem has been allocated,
+ * clocks have been enabled, device pointer has been assigned etc.
+ *
+ */
+static int safexcel_probe_generic(void *pdev,
+				  struct safexcel_crypto_priv *priv,
+				  int is_pci_dev)
 {
-	struct device *dev = &pdev->dev;
-	struct resource *res;
-	struct safexcel_crypto_priv *priv;
+	struct device *dev = priv->dev;
 	int i, ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
+	priv->context_pool = dmam_pool_create("safexcel-context", dev,
+					      sizeof(struct safexcel_context_record),
+					      1, 0);
+	if (!priv->context_pool)
 		return -ENOMEM;
 
-	priv->dev = dev;
-	priv->version = (enum safexcel_eip_version)of_device_get_match_data(dev);
-
-	if (priv->version == EIP197B || priv->version == EIP197D)
-		priv->flags |= EIP197_TRC_CACHE;
-
 	safexcel_init_register_offsets(priv);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	priv->base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(priv->base)) {
-		dev_err(dev, "failed to get resource\n");
-		return PTR_ERR(priv->base);
-	}
+	if (priv->version != EIP97IES_MRVL)
+		priv->flags |= EIP197_TRC_CACHE;
 
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
-	ret = PTR_ERR_OR_ZERO(priv->clk);
-	/* The clock isn't mandatory */
-	if  (ret != -ENOENT) {
-		if (ret)
-			return ret;
+	safexcel_configure(priv);
 
-		ret = clk_prepare_enable(priv->clk);
-		if (ret) {
-			dev_err(dev, "unable to enable clk (%d)\n", ret);
+	if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) {
+		/*
+		 * Request MSI vectors for global + 1 per ring -
+		 * or just 1 for older dev images
+		 */
+		struct pci_dev *pci_pdev = pdev;
+
+		ret = pci_alloc_irq_vectors(pci_pdev,
+					    priv->config.rings + 1,
+					    priv->config.rings + 1,
+					    PCI_IRQ_MSI | PCI_IRQ_MSIX);
+		if (ret < 0) {
+			dev_err(dev, "Failed to allocate PCI MSI interrupts\n");
 			return ret;
 		}
 	}
 
-	priv->reg_clk = devm_clk_get(&pdev->dev, "reg");
-	ret = PTR_ERR_OR_ZERO(priv->reg_clk);
-	/* The clock isn't mandatory */
-	if  (ret != -ENOENT) {
-		if (ret)
-			goto err_core_clk;
-
-		ret = clk_prepare_enable(priv->reg_clk);
-		if (ret) {
-			dev_err(dev, "unable to enable reg clk (%d)\n", ret);
-			goto err_core_clk;
-		}
-	}
-
-	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
-	if (ret)
-		goto err_reg_clk;
-
-	priv->context_pool = dmam_pool_create("safexcel-context", dev,
-					      sizeof(struct safexcel_context_record),
-					      1, 0);
-	if (!priv->context_pool) {
-		ret = -ENOMEM;
-		goto err_reg_clk;
-	}
-
-	safexcel_configure(priv);
-
+	/* Register the ring IRQ handlers and configure the rings */
 	priv->ring = devm_kcalloc(dev, priv->config.rings,
 				  sizeof(*priv->ring),
 				  GFP_KERNEL);
-	if (!priv->ring) {
-		ret = -ENOMEM;
-		goto err_reg_clk;
-	}
+	if (!priv->ring)
+		return -ENOMEM;
 
 	for (i = 0; i < priv->config.rings; i++) {
-		char irq_name[6] = {0}; /* "ringX\0" */
-		char wq_name[9] = {0}; /* "wq_ringX\0" */
+		char wq_name[9] = {0};
 		int irq;
 		struct safexcel_ring_irq_data *ring_irq;
 
 		ret = safexcel_init_ring_descriptors(priv,
 						     &priv->ring[i].cdr,
 						     &priv->ring[i].rdr);
-		if (ret)
-			goto err_reg_clk;
+		if (ret) {
+			dev_err(dev, "Failed to initialize rings\n");
+			return ret;
+		}
 
 		priv->ring[i].rdr_req = devm_kcalloc(dev,
 			EIP197_DEFAULT_RING_SIZE,
 			sizeof(priv->ring[i].rdr_req),
 			GFP_KERNEL);
-		if (!priv->ring[i].rdr_req) {
-			ret = -ENOMEM;
-			goto err_reg_clk;
-		}
+		if (!priv->ring[i].rdr_req)
+			return -ENOMEM;
 
 		ring_irq = devm_kzalloc(dev, sizeof(*ring_irq), GFP_KERNEL);
-		if (!ring_irq) {
-			ret = -ENOMEM;
-			goto err_reg_clk;
-		}
+		if (!ring_irq)
+			return -ENOMEM;
 
 		ring_irq->priv = priv;
 		ring_irq->ring = i;
 
-		snprintf(irq_name, 6, "ring%d", i);
-		irq = safexcel_request_ring_irq(pdev, irq_name, safexcel_irq_ring,
+		irq = safexcel_request_ring_irq(pdev,
+						EIP197_IRQ_NUMBER(i, is_pci_dev),
+						is_pci_dev,
+						safexcel_irq_ring,
 						safexcel_irq_ring_thread,
 						ring_irq);
 		if (irq < 0) {
-			ret = irq;
-			goto err_reg_clk;
+			dev_err(dev, "Failed to get IRQ ID for ring %d\n", i);
+			return irq;
 		}
 
 		priv->ring[i].work_data.priv = priv;
 		priv->ring[i].work_data.ring = i;
-		INIT_WORK(&priv->ring[i].work_data.work, safexcel_dequeue_work);
+		INIT_WORK(&priv->ring[i].work_data.work,
+			  safexcel_dequeue_work);
 
 		snprintf(wq_name, 9, "wq_ring%d", i);
-		priv->ring[i].workqueue = create_singlethread_workqueue(wq_name);
-		if (!priv->ring[i].workqueue) {
-			ret = -ENOMEM;
-			goto err_reg_clk;
-		}
+		priv->ring[i].workqueue =
+			create_singlethread_workqueue(wq_name);
+		if (!priv->ring[i].workqueue)
+			return -ENOMEM;
 
 		priv->ring[i].requests = 0;
 		priv->ring[i].busy = false;
@@ -1117,28 +1103,21 @@ static int safexcel_probe(struct platform_device *pdev)
 		spin_lock_init(&priv->ring[i].queue_lock);
 	}
 
-	platform_set_drvdata(pdev, priv);
 	atomic_set(&priv->ring_used, 0);
 
 	ret = safexcel_hw_init(priv);
 	if (ret) {
-		dev_err(dev, "EIP h/w init failed (%d)\n", ret);
-		goto err_reg_clk;
+		dev_err(dev, "HW init failed (%d)\n", ret);
+		return ret;
 	}
 
 	ret = safexcel_register_algorithms(priv);
 	if (ret) {
 		dev_err(dev, "Failed to register algorithms (%d)\n", ret);
-		goto err_reg_clk;
+		return ret;
 	}
 
 	return 0;
-
-err_reg_clk:
-	clk_disable_unprepare(priv->reg_clk);
-err_core_clk:
-	clk_disable_unprepare(priv->clk);
-	return ret;
 }
 
 static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
@@ -1160,6 +1139,78 @@ static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
 	}
 }
 
+#if (IS_ENABLED(CONFIG_OF))
+/*
+ * for Device Tree platform driver
+ */
+
+static int safexcel_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct safexcel_crypto_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->version = (enum safexcel_eip_version)of_device_get_match_data(dev);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base)) {
+		dev_err(dev, "failed to get resource\n");
+		return PTR_ERR(priv->base);
+	}
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	ret = PTR_ERR_OR_ZERO(priv->clk);
+	/* The clock isn't mandatory */
+	if  (ret != -ENOENT) {
+		if (ret)
+			return ret;
+
+		ret = clk_prepare_enable(priv->clk);
+		if (ret) {
+			dev_err(dev, "unable to enable clk (%d)\n", ret);
+			return ret;
+		}
+	}
+
+	priv->reg_clk = devm_clk_get(&pdev->dev, "reg");
+	ret = PTR_ERR_OR_ZERO(priv->reg_clk);
+	/* The clock isn't mandatory */
+	if  (ret != -ENOENT) {
+		if (ret)
+			goto err_core_clk;
+
+		ret = clk_prepare_enable(priv->reg_clk);
+		if (ret) {
+			dev_err(dev, "unable to enable reg clk (%d)\n", ret);
+			goto err_core_clk;
+		}
+	}
+
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+	if (ret)
+		goto err_reg_clk;
+
+	/* Generic EIP97/EIP197 device probing */
+	ret = safexcel_probe_generic(pdev, priv, 0);
+	if (ret)
+		goto err_reg_clk;
+
+	return 0;
+
+err_reg_clk:
+	clk_disable_unprepare(priv->reg_clk);
+err_core_clk:
+	clk_disable_unprepare(priv->clk);
+	return ret;
+}
+
 static int safexcel_remove(struct platform_device *pdev)
 {
 	struct safexcel_crypto_priv *priv = platform_get_drvdata(pdev);
@@ -1179,30 +1230,28 @@ static int safexcel_remove(struct platform_device *pdev)
 static const struct of_device_id safexcel_of_match_table[] = {
 	{
 		.compatible = "inside-secure,safexcel-eip97ies",
-		.data = (void *)EIP97IES,
+		.data = (void *)EIP97IES_MRVL,
 	},
 	{
 		.compatible = "inside-secure,safexcel-eip197b",
-		.data = (void *)EIP197B,
+		.data = (void *)EIP197B_MRVL,
 	},
 	{
 		.compatible = "inside-secure,safexcel-eip197d",
-		.data = (void *)EIP197D,
+		.data = (void *)EIP197D_MRVL,
 	},
+	/* For backward compatibility and intended for generic use */
 	{
-		/* Deprecated. Kept for backward compatibility. */
 		.compatible = "inside-secure,safexcel-eip97",
-		.data = (void *)EIP97IES,
+		.data = (void *)EIP97IES_MRVL,
 	},
 	{
-		/* Deprecated. Kept for backward compatibility. */
 		.compatible = "inside-secure,safexcel-eip197",
-		.data = (void *)EIP197B,
+		.data = (void *)EIP197B_MRVL,
 	},
 	{},
 };
 
-
 static struct platform_driver  crypto_safexcel = {
 	.probe		= safexcel_probe,
 	.remove		= safexcel_remove,
@@ -1211,10 +1260,169 @@ static int safexcel_remove(struct platform_device *pdev)
 		.of_match_table = safexcel_of_match_table,
 	},
 };
-module_platform_driver(crypto_safexcel);
+#endif
+
+#if (IS_ENABLED(CONFIG_PCI))
+/*
+ * PCIE devices - i.e. Inside Secure development boards
+ */
+
+static int crypto_is_pci_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *ent)
+{
+	struct device *dev = &pdev->dev;
+	struct safexcel_crypto_priv *priv;
+	void __iomem *pciebase;
+	int rc;
+	u32 val;
+
+	dev_dbg(dev, "Probing PCIE device: vendor %04x, device %04x, subv %04x, subdev %04x, ctxt %lx\n",
+		ent->vendor, ent->device, ent->subvendor,
+		ent->subdevice, ent->driver_data);
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->version = (enum safexcel_eip_version)ent->driver_data;
+
+	pci_set_drvdata(pdev, priv);
+
+	/* enable the device */
+	rc = pcim_enable_device(pdev);
+	if (rc) {
+		dev_err(dev, "Failed to enable PCI device\n");
+		return rc;
+	}
+
+	/* take ownership of PCI BAR0 */
+	rc = pcim_iomap_regions(pdev, 1, "crypto_safexcel");
+	if (rc) {
+		dev_err(dev, "Failed to map IO region for BAR0\n");
+		return rc;
+	}
+	priv->base = pcim_iomap_table(pdev)[0];
+
+	if (priv->version == EIP197_DEVBRD) {
+		dev_dbg(dev, "Device identified as FPGA based development board - applying HW reset\n");
+
+		rc = pcim_iomap_regions(pdev, 4, "crypto_safexcel");
+		if (rc) {
+			dev_err(dev, "Failed to map IO region for BAR4\n");
+			return rc;
+		}
+
+		pciebase = pcim_iomap_table(pdev)[2];
+		val = readl(pciebase + EIP197_XLX_IRQ_BLOCK_ID_ADDR);
+		if ((val >> 16) == EIP197_XLX_IRQ_BLOCK_ID_VALUE) {
+			dev_dbg(dev, "Detected Xilinx PCIE IRQ block version %d, multiple MSI support enabled\n",
+				(val & 0xff));
+
+			/* Setup MSI identity map mapping */
+			writel(EIP197_XLX_USER_VECT_LUT0_IDENT,
+			       pciebase + EIP197_XLX_USER_VECT_LUT0_ADDR);
+			writel(EIP197_XLX_USER_VECT_LUT1_IDENT,
+			       pciebase + EIP197_XLX_USER_VECT_LUT1_ADDR);
+			writel(EIP197_XLX_USER_VECT_LUT2_IDENT,
+			       pciebase + EIP197_XLX_USER_VECT_LUT2_ADDR);
+			writel(EIP197_XLX_USER_VECT_LUT3_IDENT,
+			       pciebase + EIP197_XLX_USER_VECT_LUT3_ADDR);
+
+			/* Enable all device interrupts */
+			writel(GENMASK(31, 0),
+			       pciebase + EIP197_XLX_USER_INT_ENB_MSK);
+		} else {
+			dev_err(dev, "Unrecognised IRQ block identifier %x\n",
+				val);
+			return -ENODEV;
+		}
+
+		/* HW reset FPGA dev board */
+		/* assert reset */
+		writel(1, priv->base + EIP197_XLX_GPIO_BASE);
+		wmb(); /* maintain strict ordering for accesses here */
+		/* deassert reset */
+		writel(0, priv->base + EIP197_XLX_GPIO_BASE);
+		wmb(); /* maintain strict ordering for accesses here */
+	}
+
+	/* enable bus mastering */
+	pci_set_master(pdev);
+
+	/* Generic EIP97/EIP197 device probing */
+	rc = safexcel_probe_generic(pdev, priv, 1);
+	return rc;
+}
+
+void crypto_is_pci_remove(struct pci_dev *pdev)
+{
+	struct safexcel_crypto_priv *priv = pci_get_drvdata(pdev);
+	int i;
+
+	safexcel_unregister_algorithms(priv);
+
+	for (i = 0; i < priv->config.rings; i++)
+		destroy_workqueue(priv->ring[i].workqueue);
+
+	safexcel_hw_reset_rings(priv);
+}
+
+static const struct pci_device_id crypto_is_pci_ids[] = {
+	{
+		PCI_DEVICE_SUB(PCI_VENDOR_ID_XILINX, 0x9038,
+			       0x16ae, 0xc522),
+		/* assume EIP197B for now */
+		.driver_data = EIP197_DEVBRD,
+	},
+	{},
+};
+
+MODULE_DEVICE_TABLE(pci, crypto_is_pci_ids);
+
+static struct pci_driver crypto_is_pci_driver = {
+	.name          = "crypto-safexcel",
+	.id_table      = crypto_is_pci_ids,
+	.probe         = crypto_is_pci_probe,
+	.remove        = crypto_is_pci_remove,
+};
+#endif
+
+static int __init crypto_is_init(void)
+{
+	int rc;
+
+	#if (IS_ENABLED(CONFIG_OF))
+		/* Register platform driver */
+		platform_driver_register(&crypto_safexcel);
+	#endif
+
+	#if (IS_ENABLED(CONFIG_PCI))
+		/* Register PCI driver */
+		rc = pci_register_driver(&crypto_is_pci_driver);
+	#endif
+
+	return 0;
+}
+
+static void __exit crypto_is_exit(void)
+{
+	#if (IS_ENABLED(CONFIG_OF))
+		/* Unregister platform driver */
+		platform_driver_unregister(&crypto_safexcel);
+	#endif
+
+	#if (IS_ENABLED(CONFIG_PCI))
+		/* Unregister PCI driver if successfully registered before */
+		pci_unregister_driver(&crypto_is_pci_driver);
+	#endif
+}
+
+module_init(crypto_is_init);
+module_exit(crypto_is_exit);
 
 MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
 MODULE_AUTHOR("Ofer Heifetz <oferh@marvell.com>");
 MODULE_AUTHOR("Igal Liberman <igall@marvell.com>");
-MODULE_DESCRIPTION("Support for SafeXcel cryptographic engine EIP197");
+MODULE_DESCRIPTION("Support for SafeXcel cryptographic engines: EIP97 & EIP197");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h
index e0c202f..d07bec1 100644
--- a/drivers/crypto/inside-secure/safexcel.h
+++ b/drivers/crypto/inside-secure/safexcel.h
@@ -38,6 +38,27 @@
 	char __##name##_desc[size] CRYPTO_MINALIGN_ATTR; \
 	struct type##_request *name = (void *)__##name##_desc
 
+/* Xilinx dev board base offsets */
+#define EIP197_XLX_GPIO_BASE		0x200000
+#define EIP197_XLX_IRQ_BLOCK_ID_ADDR	0x2000
+#define EIP197_XLX_IRQ_BLOCK_ID_VALUE	0x1fc2
+#define EIP197_XLX_USER_INT_ENB_MSK	0x2004
+#define EIP197_XLX_USER_INT_ENB_SET	0x2008
+#define EIP197_XLX_USER_INT_ENB_CLEAR	0x200c
+#define EIP197_XLX_USER_INT_BLOCK	0x2040
+#define EIP197_XLX_USER_INT_PEND	0x2048
+#define EIP197_XLX_USER_VECT_LUT0_ADDR	0x2080
+#define EIP197_XLX_USER_VECT_LUT0_IDENT	0x03020100
+#define EIP197_XLX_USER_VECT_LUT1_ADDR	0x2084
+#define EIP197_XLX_USER_VECT_LUT1_IDENT	0x07060504
+#define EIP197_XLX_USER_VECT_LUT2_ADDR	0x2088
+#define EIP197_XLX_USER_VECT_LUT2_IDENT	0x0b0a0908
+#define EIP197_XLX_USER_VECT_LUT3_ADDR	0x208c
+#define EIP197_XLX_USER_VECT_LUT3_IDENT	0x0f0e0d0c
+
+/* Helper defines for probe function */
+#define EIP197_IRQ_NUMBER(i, is_pci)	(i + is_pci)
+
 /* Register base offsets */
 #define EIP197_HIA_AIC(priv)		((priv)->base + (priv)->offsets.hia_aic)
 #define EIP197_HIA_AIC_G(priv)		((priv)->base + (priv)->offsets.hia_aic_g)
@@ -581,10 +602,13 @@ struct safexcel_ring {
 	struct crypto_async_request *backlog;
 };
 
+/* EIP integration context flags */
 enum safexcel_eip_version {
-	EIP97IES = BIT(0),
-	EIP197B  = BIT(1),
-	EIP197D  = BIT(2),
+	/* Platform (EIP integration context) specifier */
+	EIP97IES_MRVL,
+	EIP197B_MRVL,
+	EIP197D_MRVL,
+	EIP197_DEVBRD
 };
 
 struct safexcel_register_offsets {
diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index 8cdbdbe..ee8a0c3 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -881,7 +881,6 @@ static void safexcel_aead_cra_exit(struct crypto_tfm *tfm)
 
 struct safexcel_alg_template safexcel_alg_ecb_aes = {
 	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.skcipher = {
 		.setkey = safexcel_skcipher_aes_setkey,
 		.encrypt = safexcel_ecb_aes_encrypt,
@@ -920,7 +919,6 @@ static int safexcel_cbc_aes_decrypt(struct skcipher_request *req)
 
 struct safexcel_alg_template safexcel_alg_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.skcipher = {
 		.setkey = safexcel_skcipher_aes_setkey,
 		.encrypt = safexcel_cbc_aes_encrypt,
@@ -990,7 +988,6 @@ static int safexcel_des_setkey(struct crypto_skcipher *ctfm, const u8 *key,
 
 struct safexcel_alg_template safexcel_alg_cbc_des = {
 	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.skcipher = {
 		.setkey = safexcel_des_setkey,
 		.encrypt = safexcel_cbc_des_encrypt,
@@ -1030,7 +1027,6 @@ static int safexcel_ecb_des_decrypt(struct skcipher_request *req)
 
 struct safexcel_alg_template safexcel_alg_ecb_des = {
 	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.skcipher = {
 		.setkey = safexcel_des_setkey,
 		.encrypt = safexcel_ecb_des_encrypt,
@@ -1093,7 +1089,6 @@ static int safexcel_des3_ede_setkey(struct crypto_skcipher *ctfm,
 
 struct safexcel_alg_template safexcel_alg_cbc_des3_ede = {
 	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.skcipher = {
 		.setkey = safexcel_des3_ede_setkey,
 		.encrypt = safexcel_cbc_des3_ede_encrypt,
@@ -1133,7 +1128,6 @@ static int safexcel_ecb_des3_ede_decrypt(struct skcipher_request *req)
 
 struct safexcel_alg_template safexcel_alg_ecb_des3_ede = {
 	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.skcipher = {
 		.setkey = safexcel_des3_ede_setkey,
 		.encrypt = safexcel_ecb_des3_ede_encrypt,
@@ -1203,7 +1197,6 @@ static int safexcel_aead_sha1_cra_init(struct crypto_tfm *tfm)
 
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.aead = {
 		.setkey = safexcel_aead_aes_setkey,
 		.encrypt = safexcel_aead_encrypt,
@@ -1238,7 +1231,6 @@ static int safexcel_aead_sha256_cra_init(struct crypto_tfm *tfm)
 
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha256_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.aead = {
 		.setkey = safexcel_aead_aes_setkey,
 		.encrypt = safexcel_aead_encrypt,
@@ -1273,7 +1265,6 @@ static int safexcel_aead_sha224_cra_init(struct crypto_tfm *tfm)
 
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha224_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.aead = {
 		.setkey = safexcel_aead_aes_setkey,
 		.encrypt = safexcel_aead_encrypt,
@@ -1308,7 +1299,6 @@ static int safexcel_aead_sha512_cra_init(struct crypto_tfm *tfm)
 
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha512_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.aead = {
 		.setkey = safexcel_aead_aes_setkey,
 		.encrypt = safexcel_aead_encrypt,
@@ -1343,7 +1333,6 @@ static int safexcel_aead_sha384_cra_init(struct crypto_tfm *tfm)
 
 struct safexcel_alg_template safexcel_alg_authenc_hmac_sha384_cbc_aes = {
 	.type = SAFEXCEL_ALG_TYPE_AEAD,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.aead = {
 		.setkey = safexcel_aead_aes_setkey,
 		.encrypt = safexcel_aead_encrypt,
diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
index a80a5e7..2c31536 100644
--- a/drivers/crypto/inside-secure/safexcel_hash.c
+++ b/drivers/crypto/inside-secure/safexcel_hash.c
@@ -802,7 +802,6 @@ static void safexcel_ahash_cra_exit(struct crypto_tfm *tfm)
 
 struct safexcel_alg_template safexcel_alg_sha1 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_sha1_init,
 		.update = safexcel_ahash_update,
@@ -1035,7 +1034,6 @@ static int safexcel_hmac_sha1_setkey(struct crypto_ahash *tfm, const u8 *key,
 
 struct safexcel_alg_template safexcel_alg_hmac_sha1 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_hmac_sha1_init,
 		.update = safexcel_ahash_update,
@@ -1099,7 +1097,6 @@ static int safexcel_sha256_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_sha256 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_sha256_init,
 		.update = safexcel_ahash_update,
@@ -1162,7 +1159,6 @@ static int safexcel_sha224_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_sha224 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_sha224_init,
 		.update = safexcel_ahash_update,
@@ -1218,7 +1214,6 @@ static int safexcel_hmac_sha224_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_hmac_sha224 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_hmac_sha224_init,
 		.update = safexcel_ahash_update,
@@ -1275,7 +1270,6 @@ static int safexcel_hmac_sha256_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_hmac_sha256 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_hmac_sha256_init,
 		.update = safexcel_ahash_update,
@@ -1347,7 +1341,6 @@ static int safexcel_sha512_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_sha512 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_sha512_init,
 		.update = safexcel_ahash_update,
@@ -1418,7 +1411,6 @@ static int safexcel_sha384_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_sha384 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_sha384_init,
 		.update = safexcel_ahash_update,
@@ -1474,7 +1466,6 @@ static int safexcel_hmac_sha512_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_hmac_sha512 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_hmac_sha512_init,
 		.update = safexcel_ahash_update,
@@ -1531,7 +1522,6 @@ static int safexcel_hmac_sha384_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_hmac_sha384 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_hmac_sha384_init,
 		.update = safexcel_ahash_update,
@@ -1591,7 +1581,6 @@ static int safexcel_md5_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_md5 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_md5_init,
 		.update = safexcel_ahash_update,
@@ -1647,7 +1636,6 @@ static int safexcel_hmac_md5_digest(struct ahash_request *areq)
 
 struct safexcel_alg_template safexcel_alg_hmac_md5 = {
 	.type = SAFEXCEL_ALG_TYPE_AHASH,
-	.engines = EIP97IES | EIP197B | EIP197D,
 	.alg.ahash = {
 		.init = safexcel_hmac_md5_init,
 		.update = safexcel_ahash_update,
diff --git a/drivers/crypto/inside-secure/safexcel_ring.c b/drivers/crypto/inside-secure/safexcel_ring.c
index 142bc3f..2402a62 100644
--- a/drivers/crypto/inside-secure/safexcel_ring.c
+++ b/drivers/crypto/inside-secure/safexcel_ring.c
@@ -145,7 +145,8 @@ struct safexcel_command_desc *safexcel_add_cdesc(struct safexcel_crypto_priv *pr
 			(lower_32_bits(context) & GENMASK(31, 2)) >> 2;
 		cdesc->control_data.context_hi = upper_32_bits(context);
 
-		if (priv->version == EIP197B || priv->version == EIP197D)
+		if (priv->version == EIP197B_MRVL ||
+		    priv->version == EIP197D_MRVL)
 			cdesc->control_data.options |= EIP197_OPTION_RC_AUTO;
 
 		/* TODO: large xform HMAC with SHA-384/512 uses refresh = 3 */
-- 
1.8.3.1

Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Antoine Tenart]

Hi Pascal,

On Fri, Jul 26, 2019 at 02:43:24PM +0200, Pascal van Leeuwen wrote:
> +	if (priv->version == EIP197D_MRVL) {

I see you renamed EIP197D to EIP197D_MRVL in the v2. Such a rename
should not be part of this patch, as it has nothing to do with the
engine you're adding support for.

Is there a reason to have this one linked to Marvell? Aren't there other
EIP197 (or EIP97) engines not on Marvell SoCs? (I'm pretty sure I know
at least one).

> -	switch (priv->version) {
> -	case EIP197B:
> -		dir = "eip197b";
> -		break;
> -	case EIP197D:
> -		dir = "eip197d";
> -		break;
> -	default:
> +	if (priv->version == EIP97IES_MRVL)
>  		/* No firmware is required */
>  		return 0;
> -	}
> +	else if (priv->version == EIP197D_MRVL)
> +		dir = "eip197d";
> +	else
> +		/* Default to minimum EIP197 config */
> +		dir = "eip197b";

You're moving the default choice from "no firmware" to being a specific
one.

> -			if (priv->version != EIP197B)
> +			if (!(priv->version == EIP197B_MRVL))

'!=' ?

> -			/* Fallback to the old firmware location for the
> +			/*
> +			 * Fallback to the old firmware location for the

This is actually the expected comment style in net/ and crypto/. (There
are other examples).

>  	/* For EIP197 set maximum number of TX commands to 2^5 = 32 */
> -	if (priv->version == EIP197B || priv->version == EIP197D)
> +	if (priv->version != EIP97IES_MRVL)

I would really prefer having explicit checks here. More engines will be
supported in the future and doing will help. (There are others).

> @@ -869,9 +898,6 @@ static int safexcel_register_algorithms(struct safexcel_crypto_priv *priv)
>  	for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) {
>  		safexcel_algs[i]->priv = priv;
>  
> -		if (!(safexcel_algs[i]->engines & priv->version))
> -			continue;

You should remove the 'engines' flag in a separate patch. I'm really not
sure about this. I don't think all the IS EIP engines support the same
sets of algorithms?

> @@ -925,22 +945,18 @@ static void safexcel_configure(struct safexcel_crypto_priv *priv)
>  	val = readl(EIP197_HIA_AIC_G(priv) + EIP197_HIA_OPTIONS);
>  
>  	/* Read number of PEs from the engine */
> -	switch (priv->version) {
> -	case EIP197B:
> -	case EIP197D:
> -		mask = EIP197_N_PES_MASK;
> -		break;
> -	default:
> +	if (priv->version == EIP97IES_MRVL)
>  		mask = EIP97_N_PES_MASK;
> -	}
> +	else
> +		mask = EIP197_N_PES_MASK;
> +

You also lose readability here.

> +	if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) {

You have extra parenthesis here.

> -	platform_set_drvdata(pdev, priv);

Side note: this is why you had to send the patch fixing rmmod.

>  static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
> @@ -1160,6 +1139,78 @@ static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
>  	}
>  }
>  
> +#if (IS_ENABLED(CONFIG_OF))

No need for the extra parenthesis here.

> +	if (priv->version == EIP197_DEVBRD) {

It seems to me this is mixing an engine version information and a board
were the engine is integrated. Are there differences in the engine
itself, or only in the way it's wired?

We had this discussion on the v1. Your point was that you wanted this
information to be in .data. One solution I proposed then was to use a
struct (with both a 'version' and a 'flag' variable inside) instead of
a single 'version' variable, so that we still can make checks on the
version itself and not on something too specific.

> +static int __init crypto_is_init(void)
> +{
> +	int rc;
> +
> +	#if (IS_ENABLED(CONFIG_OF))
> +		/* Register platform driver */
> +		platform_driver_register(&crypto_safexcel);
> +	#endif

When used in the code directly, you should use:

  if (IS_ENABLED(CONFIG_OF))

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Tuesday, July 30, 2019 11:08 AM
> To: Pascal van Leeuwen <pascalvanl@gmail.com>
> Cc: linux-crypto@vger.kernel.org; antoine.tenart@bootlin.com; herbert@gondor.apana.org.au;
> davem@davemloft.net; Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Subject: Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA
> development board
> 
> Hi Pascal,
> 
> On Fri, Jul 26, 2019 at 02:43:24PM +0200, Pascal van Leeuwen wrote:
> > +	if (priv->version == EIP197D_MRVL) {
> 
> I see you renamed EIP197D to EIP197D_MRVL in the v2. Such a rename
> should not be part of this patch, as it has nothing to do with the
> engine you're adding support for.
> 
I think the rename makes sense within the context of this patch, as the
patch allows it to be run on our development board which may not contain
Marvell hardware (see also the answer below). The rename makes it clear
from the code, at least, that for now only Marvell hardware is supported.

> Is there a reason to have this one linked to Marvell? Aren't there other
> EIP197 (or EIP97) engines not on Marvell SoCs? (I'm pretty sure I know
> at least one).
> 
Yes, there is a very good reason. These flags control features that are
very specific to those three Marvell socs and have nothing to do with 
'generic' EIP97IES's, EIP197B's or EIP197D's (these are just high-level
marketing/sales denominators and do not cover all the intricate config
details of the actual delivery that the driver needs to know about, so
this naive approach would not be maintainable scaling to other devices) 
Therefore, I wanted to make that abundantly clear, hence the prefix.
(Renaming them to the specific Marvell SoC names was another option,
if only I knew which engine ended up in which SoC ...)

While there are many SoC's out there with EIP97 and EIP197 engines,
the driver in its current form will NOT work (properly) on them, for
that there is still much work to be done beyond the patches I already
submitted. I already have the implementation for that (you tested that
already!), but chopping it into bits and submitting it all will take 
a lot more time. But you have to understand that, without that, it's 
totally useless to either me or Verimatrix.

TL;DR: These flags are really just for controlling anything specific
to those particular Marvell instances and nothing else.

> > -	switch (priv->version) {
> > -	case EIP197B:
> > -		dir = "eip197b";
> > -		break;
> > -	case EIP197D:
> > -		dir = "eip197d";
> > -		break;
> > -	default:
> > +	if (priv->version == EIP97IES_MRVL)
> >  		/* No firmware is required */
> >  		return 0;
> > -	}
> > +	else if (priv->version == EIP197D_MRVL)
> > +		dir = "eip197d";
> > +	else
> > +		/* Default to minimum EIP197 config */
> > +		dir = "eip197b";
> 
> You're moving the default choice from "no firmware" to being a specific
> one.
> 
The EIP97 being the exception as the only firmware-less engine.
This makes EIP197_DEVBRD fall through to EIP197B firmware until
my patches supporting other EIP197 configs eventually get merged,
after which this part will change anyway.

> > -			if (priv->version != EIP197B)
> > +			if (!(priv->version == EIP197B_MRVL))
> 
> '!=' ?
> 
Yes, that one looks funny ;-) May have been some global search and
replace going awry. Will fix.

> > -			/* Fallback to the old firmware location for the
> > +			/*
> > +			 * Fallback to the old firmware location for the
> 
> This is actually the expected comment style in net/ and crypto/. (There
> are other examples).
> 
Not according to the Linux coding style (which only makes an exception
for /net) and not in most other crypto code I've seen. So maybe both
styles are allowed(?) and they are certainly both used, but this style
seems to be prevalent ...

> >  	/* For EIP197 set maximum number of TX commands to 2^5 = 32 */
> > -	if (priv->version == EIP197B || priv->version == EIP197D)
> > +	if (priv->version != EIP97IES_MRVL)
> 
> I would really prefer having explicit checks here. More engines will be
> supported in the future and doing will help. (There are others).
> 
Same situation as with the crypto mode: I know for a fact the EIP97
is the *only* configuration that *doesn't* need this code. So why
would I have a long list of configurations there (that will keep
growing indefinitely) that *do* need that code? That will for sure
not improve maintainability ...


> > @@ -869,9 +898,6 @@ static int safexcel_register_algorithms(struct safexcel_crypto_priv
> *priv)
> >  	for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) {
> >  		safexcel_algs[i]->priv = priv;
> >
> > -		if (!(safexcel_algs[i]->engines & priv->version))
> > -			continue;
> 
> You should remove the 'engines' flag in a separate patch. I'm really not
> sure about this. I don't think all the IS EIP engines support the same
> sets of algorithms?
> 
All algorithms provided at this moment are available from all engines
currently supported. So the whole mechanism, so far, is redundant.

This will change as I add support for generic (non-Marvell) engines,
but the approach taken here is not scalable or maintainable. So I will
end up doing it differently, eventually. I don't see the point in
maintaining dead/unused/redundant code I'm about to replace anyway.

> > @@ -925,22 +945,18 @@ static void safexcel_configure(struct safexcel_crypto_priv *priv)
> >  	val = readl(EIP197_HIA_AIC_G(priv) + EIP197_HIA_OPTIONS);
> >
> >  	/* Read number of PEs from the engine */
> > -	switch (priv->version) {
> > -	case EIP197B:
> > -	case EIP197D:
> > -		mask = EIP197_N_PES_MASK;
> > -		break;
> > -	default:
> > +	if (priv->version == EIP97IES_MRVL)
> >  		mask = EIP97_N_PES_MASK;
> > -	}
> > +	else
> > +		mask = EIP197_N_PES_MASK;
> > +
> 
> You also lose readability here.
> 
I don't see why or how. Same reasoning here: the EIP97 is the
only known exception so why bother with a full list of "the rest".
That's just more code to maintain and more error prone.
(and this code, like previous similar cases, will shortly need to
change when adding generic engine support, so this is just (hopefully
very) temporary anyway)

> > +	if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) {
> 
> You have extra parenthesis here.
> 
Our internal coding style (as well as my personal preference) 
actually mandates to put parenthesis around everything so expect
that to happen a lot going forward as I've been doing it like that
for nearly 30 years now.

Does the Linux coding style actually *prohibit* the use of these
"extra" parenthesis? I don't recall reading that anywhere ...

> > -	platform_set_drvdata(pdev, priv);
> 
> Side note: this is why you had to send the patch fixing rmmod.
> 
Ah ... so that's where it accidentally disappeared :-)

> >  static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
> > @@ -1160,6 +1139,78 @@ static void safexcel_hw_reset_rings(struct safexcel_crypto_priv
> *priv)
> >  	}
> >  }
> >
> > +#if (IS_ENABLED(CONFIG_OF))
> 
> No need for the extra parenthesis here.
> 
Same comment as before

> > +	if (priv->version == EIP197_DEVBRD) {
> 
> It seems to me this is mixing an engine version information and a board
> were the engine is integrated. Are there differences in the engine
> itself, or only in the way it's wired?
> 
Actually, no. The way I see it, priv->version does not convey any engine
information, just integration context (i.e. a specific Marvell SoC or, in 
this case, our FPGA dev board), see also my explanation at the beginning.

Conveying engine information through a simple set of flags or some
integer or whatever is just not going to fly. No two of our engines
are ever the same, so that would quickly blow up in your face.

> We had this discussion on the v1. Your point was that you wanted this
> information to be in .data. One solution I proposed then was to use a
> struct (with both a 'version' and a 'flag' variable inside) instead of
> a single 'version' variable, so that we still can make checks on the
> version itself and not on something too specific.
> 
As a result of that discussion, I kept your original version field
as intact as I could, knowing what I know and where I want to go.

But to truly support generic engines, we really need all the stuff
that I added. Because it simply has that many parameters that are
different for each individual instance. But at the same time these
parameters can all be probed from the hardware directly, so
maintaining huge if statements all over the place decoding some 
artificial version field is not the way to go (not maintainable).
Just probe all the parameters from the hardware and use them 
directly where needed ... which my future patch set will do.

> > +static int __init crypto_is_init(void)
> > +{
> > +	int rc;
> > +
> > +	#if (IS_ENABLED(CONFIG_OF))
> > +		/* Register platform driver */
> > +		platform_driver_register(&crypto_safexcel);
> > +	#endif
> 
> When used in the code directly, you should use:
> 
>   if (IS_ENABLED(CONFIG_OF))
> 
Oops, I missed that one, will fix.

> Thanks!
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thanks,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Antoine Tenart]

Hi Pascal,

On Tue, Jul 30, 2019 at 10:20:43AM +0000, Pascal Van Leeuwen wrote:
> > On Fri, Jul 26, 2019 at 02:43:24PM +0200, Pascal van Leeuwen wrote:
> 
> > Is there a reason to have this one linked to Marvell? Aren't there other
> > EIP197 (or EIP97) engines not on Marvell SoCs? (I'm pretty sure I know
> > at least one).
> > 
> Yes, there is a very good reason. These flags control features that are
> very specific to those three Marvell socs and have nothing to do with 
> 'generic' EIP97IES's, EIP197B's or EIP197D's (these are just high-level
> marketing/sales denominators and do not cover all the intricate config
> details of the actual delivery that the driver needs to know about, so
> this naive approach would not be maintainable scaling to other devices) 
> Therefore, I wanted to make that abundantly clear, hence the prefix.
> (Renaming them to the specific Marvell SoC names was another option,
> if only I knew which engine ended up in which SoC ...)
> 
> While there are many SoC's out there with EIP97 and EIP197 engines,
> the driver in its current form will NOT work (properly) on them, for
> that there is still much work to be done beyond the patches I already
> submitted. I already have the implementation for that (you tested that
> already!), but chopping it into bits and submitting it all will take 
> a lot more time. But you have to understand that, without that, it's 
> totally useless to either me or Verimatrix.
> 
> TL;DR: These flags are really just for controlling anything specific
> to those particular Marvell instances and nothing else.

I had this driver running on another non-Marvell SoC with very minor
modifications, so there's then at least one hardware which is similar
enough. In this case I don't see why this should be named "Marvell".

What are the features specific to those Marvell SoC that won't be used
in other integrations? I'm pretty sure there are common features between
all those EIP97 engines on different SoCs.

> > > -	switch (priv->version) {
> > > -	case EIP197B:
> > > -		dir = "eip197b";
> > > -		break;
> > > -	case EIP197D:
> > > -		dir = "eip197d";
> > > -		break;
> > > -	default:
> > > +	if (priv->version == EIP97IES_MRVL)
> > >  		/* No firmware is required */
> > >  		return 0;
> > > -	}
> > > +	else if (priv->version == EIP197D_MRVL)
> > > +		dir = "eip197d";
> > > +	else
> > > +		/* Default to minimum EIP197 config */
> > > +		dir = "eip197b";
> > 
> > You're moving the default choice from "no firmware" to being a specific
> > one.
> > 
> The EIP97 being the exception as the only firmware-less engine.
> This makes EIP197_DEVBRD fall through to EIP197B firmware until
> my patches supporting other EIP197 configs eventually get merged,
> after which this part will change anyway.

We don't know when/in what shape those patches will be merged, so in
the meantime please make the "no firmware" the default choice.

> > > -			/* Fallback to the old firmware location for the
> > > +			/*
> > > +			 * Fallback to the old firmware location for the
> > 
> > This is actually the expected comment style in net/ and crypto/. (There
> > are other examples).
> > 
> Not according to the Linux coding style (which only makes an exception
> for /net) and not in most other crypto code I've seen. So maybe both
> styles are allowed(?) and they are certainly both used, but this style
> seems to be prevalent ...

I agree having non-written rules is not good (I don't make them), but
not everything is described in the documentation or in the coding style.
I don't really care about the comment style when adding new ones, but
those are valid (& recommended) in crypto/ and it just make the patch
bigger.

> > >  	/* For EIP197 set maximum number of TX commands to 2^5 = 32 */
> > > -	if (priv->version == EIP197B || priv->version == EIP197D)
> > > +	if (priv->version != EIP97IES_MRVL)
> > 
> > I would really prefer having explicit checks here. More engines will be
> > supported in the future and doing will help. (There are others).
> > 
> Same situation as with the crypto mode: I know for a fact the EIP97
> is the *only* configuration that *doesn't* need this code. So why
> would I have a long list of configurations there (that will keep
> growing indefinitely) that *do* need that code? That will for sure
> not improve maintainability ...

OK, I won't debate this for hours. At least add a comment, for when
*others* will add support for new hardware (because that really is the
point, *others* might update and modify the driver).

> > > @@ -869,9 +898,6 @@ static int safexcel_register_algorithms(struct safexcel_crypto_priv
> > *priv)
> > >  	for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) {
> > >  		safexcel_algs[i]->priv = priv;
> > >
> > > -		if (!(safexcel_algs[i]->engines & priv->version))
> > > -			continue;
> > 
> > You should remove the 'engines' flag in a separate patch. I'm really not
> > sure about this. I don't think all the IS EIP engines support the same
> > sets of algorithms?
> > 
> All algorithms provided at this moment are available from all engines
> currently supported. So the whole mechanism, so far, is redundant.
> 
> This will change as I add support for generic (non-Marvell) engines,
> but the approach taken here is not scalable or maintainable. So I will
> end up doing it differently, eventually. I don't see the point in
> maintaining dead/unused/redundant code I'm about to replace anyway.

But it's not done yet and we might discuss how you'll handle this. You
can't know for sure you'll end up with a different approach.

At least remove this in a separate patch.

> > > +	if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) {
> > 
> > You have extra parenthesis here.
> > 
> Our internal coding style (as well as my personal preference) 
> actually mandates to put parenthesis around everything so expect
> that to happen a lot going forward as I've been doing it like that
> for nearly 30 years now.
> 
> Does the Linux coding style actually *prohibit* the use of these
> "extra" parenthesis? I don't recall reading that anywhere ...

I don't know if this is a written rule (as many others), but you'll find
plenty of examples of reviews asking not to have extra parenthesis.

> > > +	if (priv->version == EIP197_DEVBRD) {
> > 
> > It seems to me this is mixing an engine version information and a board
> > were the engine is integrated. Are there differences in the engine
> > itself, or only in the way it's wired?
> > 
> Actually, no. The way I see it, priv->version does not convey any engine
> information, just integration context (i.e. a specific Marvell SoC or, in 
> this case, our FPGA dev board), see also my explanation at the beginning.

So that's really the point here :) This variable was introduced to
convey the engine information, not the way it is integrated. There are
EIP97 engines not on Marvell SoC which will just work out of the box (or
with let's say a one liner adding support for using a clock). And the
version could be in both cases something like 'EIP97'.

> Conveying engine information through a simple set of flags or some
> integer or whatever is just not going to fly. No two of our engines
> are ever the same, so that would quickly blow up in your face.

Well, you have more info about this than I do, I can only trust you on
this (it's just weird based on the experience I described just before,
it seems to me the differences are not that big, but again, you know the
h/w better).

I just don't want to end up with:

  if (version == EIP97_MRVL || version == EIP97_XXX || ...)

> > We had this discussion on the v1. Your point was that you wanted this
> > information to be in .data. One solution I proposed then was to use a
> > struct (with both a 'version' and a 'flag' variable inside) instead of
> > a single 'version' variable, so that we still can make checks on the
> > version itself and not on something too specific.
> > 
> As a result of that discussion, I kept your original version field
> as intact as I could, knowing what I know and where I want to go.
> 
> But to truly support generic engines, we really need all the stuff
> that I added. Because it simply has that many parameters that are
> different for each individual instance. But at the same time these
> parameters can all be probed from the hardware directly, so
> maintaining huge if statements all over the place decoding some 
> artificial version field is not the way to go (not maintainable).
> Just probe all the parameters from the hardware and use them 
> directly where needed ... which my future patch set will do.

OK, I do think this would be a good solution :)

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Tuesday, July 30, 2019 3:43 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen
> <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au;
> davem@davemloft.net
> Subject: Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA
> development board
> 
> Hi Pascal,
> 
> On Tue, Jul 30, 2019 at 10:20:43AM +0000, Pascal Van Leeuwen wrote:
> > > On Fri, Jul 26, 2019 at 02:43:24PM +0200, Pascal van Leeuwen wrote:
> >
> > > Is there a reason to have this one linked to Marvell? Aren't there other
> > > EIP197 (or EIP97) engines not on Marvell SoCs? (I'm pretty sure I know
> > > at least one).
> > >
> > Yes, there is a very good reason. These flags control features that are
> > very specific to those three Marvell socs and have nothing to do with
> > 'generic' EIP97IES's, EIP197B's or EIP197D's (these are just high-level
> > marketing/sales denominators and do not cover all the intricate config
> > details of the actual delivery that the driver needs to know about, so
> > this naive approach would not be maintainable scaling to other devices)
> > Therefore, I wanted to make that abundantly clear, hence the prefix.
> > (Renaming them to the specific Marvell SoC names was another option,
> > if only I knew which engine ended up in which SoC ...)
> >
> > While there are many SoC's out there with EIP97 and EIP197 engines,
> > the driver in its current form will NOT work (properly) on them, for
> > that there is still much work to be done beyond the patches I already
> > submitted. I already have the implementation for that (you tested that
> > already!), but chopping it into bits and submitting it all will take
> > a lot more time. But you have to understand that, without that, it's
> > totally useless to either me or Verimatrix.
> >
> > TL;DR: These flags are really just for controlling anything specific
> > to those particular Marvell instances and nothing else.
> 
> I had this driver running on another non-Marvell SoC with very minor
> modifications, so there's then at least one hardware which is similar
> enough. In this case I don't see why this should be named "Marvell".
> 
Just because it more or less appeared to run, does NOT mean that it 
actually works properly, reliably and efficiently across the board. 
Trust me, this is my design and I know it inside out and there is NO 
engine out there equal to any Marvell engine.

> What are the features specific to those Marvell SoC that won't be used
> in other integrations? I'm pretty sure there are common features between
> all those EIP97 engines on different SoCs.
> 
- specific algorithms supported
- number of pipes present
- number of rings present
- number of ring interrupt controllers present
- full featured (TRC) versus simplified record cache (STRC)
- for TRC: size of cache admin & data RAMs, affects configuration
- size of various internal buffers which affects configuration
- width of the databus, which affects descriptor size
- workarounds needed for specific design bugs and/or limitations
- newer features unsupported by now old MRVL engines

That's just the low-hanging fruit. We've *never* delivered the same
engine configuration to different customers. It's always customized.

> > > > -	switch (priv->version) {
> > > > -	case EIP197B:
> > > > -		dir = "eip197b";
> > > > -		break;
> > > > -	case EIP197D:
> > > > -		dir = "eip197d";
> > > > -		break;
> > > > -	default:
> > > > +	if (priv->version == EIP97IES_MRVL)
> > > >  		/* No firmware is required */
> > > >  		return 0;
> > > > -	}
> > > > +	else if (priv->version == EIP197D_MRVL)
> > > > +		dir = "eip197d";
> > > > +	else
> > > > +		/* Default to minimum EIP197 config */
> > > > +		dir = "eip197b";
> > >
> > > You're moving the default choice from "no firmware" to being a specific
> > > one.
> > >
> > The EIP97 being the exception as the only firmware-less engine.
> > This makes EIP197_DEVBRD fall through to EIP197B firmware until
> > my patches supporting other EIP197 configs eventually get merged,
> > after which this part will change anyway.
> 
> We don't know when/in what shape those patches will be merged, so in
> the meantime please make the "no firmware" the default choice.
>
"No firmware" is not possible with an EIP197. Trying to use it without
loading firmware will cause it to hang, which I don't believe is what
you would want. So the alternative would be to return an error, which
is fine by me, so then I'll change it into that as default.
 
> > > > -			/* Fallback to the old firmware location for the
> > > > +			/*
> > > > +			 * Fallback to the old firmware location for the
> > >
> > > This is actually the expected comment style in net/ and crypto/. (There
> > > are other examples).
> > >
> > Not according to the Linux coding style (which only makes an exception
> > for /net) and not in most other crypto code I've seen. So maybe both
> > styles are allowed(?) and they are certainly both used, but this style
> > seems to be prevalent ...
> 
> I agree having non-written rules is not good (I don't make them), but
> not everything is described in the documentation or in the coding style.
> I don't really care about the comment style when adding new ones, but
> those are valid (& recommended) in crypto/ and it just make the patch
> bigger.
> 
Ah, ok, your point being not to touch an already existing comment.
I missed that tiny detail ... I guess that's sort of automatic out of
my desire for consistency. I'll try to suppress those urges :-)

> > > >  	/* For EIP197 set maximum number of TX commands to 2^5 = 32 */
> > > > -	if (priv->version == EIP197B || priv->version == EIP197D)
> > > > +	if (priv->version != EIP97IES_MRVL)
> > >
> > > I would really prefer having explicit checks here. More engines will be
> > > supported in the future and doing will help. (There are others).
> > >
> > Same situation as with the crypto mode: I know for a fact the EIP97
> > is the *only* configuration that *doesn't* need this code. So why
> > would I have a long list of configurations there (that will keep
> > growing indefinitely) that *do* need that code? That will for sure
> > not improve maintainability ...
> 
> OK, I won't debate this for hours. At least add a comment, for when
> *others* will add support for new hardware (because that really is the
> point, *others* might update and modify the driver).
> 
Sure, I can add some comments to clarify these.

> > > > @@ -869,9 +898,6 @@ static int safexcel_register_algorithms(struct
> safexcel_crypto_priv
> > > *priv)
> > > >  	for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) {
> > > >  		safexcel_algs[i]->priv = priv;
> > > >
> > > > -		if (!(safexcel_algs[i]->engines & priv->version))
> > > > -			continue;
> > >
> > > You should remove the 'engines' flag in a separate patch. I'm really not
> > > sure about this. I don't think all the IS EIP engines support the same
> > > sets of algorithms?
> > >
> > All algorithms provided at this moment are available from all engines
> > currently supported. So the whole mechanism, so far, is redundant.
> >
> > This will change as I add support for generic (non-Marvell) engines,
> > but the approach taken here is not scalable or maintainable. So I will
> > end up doing it differently, eventually. I don't see the point in
> > maintaining dead/unused/redundant code I'm about to replace anyway.
> 
> But it's not done yet and we might discuss how you'll handle this. You
> can't know for sure you'll end up with a different approach.
> 
We can discuss all we want, but the old approach for sure won't work
so there's no point in keeping those (effectively redundant up until
now, so why did they even exist?) 'engines' flags. 

> At least remove this in a separate patch.
>
Ok, I can do that

> > > > +	if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) {
> > >
> > > You have extra parenthesis here.
> > >
> > Our internal coding style (as well as my personal preference)
> > actually mandates to put parenthesis around everything so expect
> > that to happen a lot going forward as I've been doing it like that
> > for nearly 30 years now.
> >
> > Does the Linux coding style actually *prohibit* the use of these
> > "extra" parenthesis? I don't recall reading that anywhere ...
> 
> I don't know if this is a written rule (as many others), but you'll find
> plenty of examples of reviews asking not to have extra parenthesis.
>
I can remove them, I was just wondering if there was actually any 
rationale for not wanting to have them (considering that that at least
seems far more error prone). I have this strange desire to want to 
know WHY I have to do something before I do it.

Is it just to show off your intimate knowledge of C operator precedence
and associativity  ;-) (which, incidentally, I'm not too familiar with)
 
> > > > +	if (priv->version == EIP197_DEVBRD) {
> > >
> > > It seems to me this is mixing an engine version information and a board
> > > were the engine is integrated. Are there differences in the engine
> > > itself, or only in the way it's wired?
> > >
> > Actually, no. The way I see it, priv->version does not convey any engine
> > information, just integration context (i.e. a specific Marvell SoC or, in
> > this case, our FPGA dev board), see also my explanation at the beginning.
> 
> So that's really the point here :) This variable was introduced to
> convey the engine information, not the way it is integrated. There are
> EIP97 engines not on Marvell SoC which will just work out of the box
>
No, they will not. Certainly not if the driver becomes more capable.
You are making (dangerous) assumptions you don't have the knowledge
to make. With all due respect, not your fault, you just can't know
as this is not public information.

> (or with let's say a one liner adding support for using a clock)
>
No, that's naive thought. Marvell configs, for example, have a pretty
extensive algorithm loadout and relatively large record cache RAMs
as well as internal buffer RAMs. That will cause problems with most
other engines being a subset of that (which may be reliability 
problems you won't immediately notice(!) ...).

> And the version could be in both cases something like 'EIP97'.
> 
It needs to be something unique. And that will very quickly become
overwhelming as you need to maintain lists of which unique identifier
has which specific features somewhere. Or have huge case and/or 
if-else if statements all over the place.

While all required parameters needed to configure the engine and
the driver can be directly probed from said hardware (including the
difference between an EIP97 and an EIP197, the number of pipes,
rings, everything, all you need is a base address), so why bother?

The only place where this 'version' can still be useful is to 
distinguish the *platform*, which may need some specific initialization 
(e.g. platform clocks, power domains, interrupt controllers and such).

> > Conveying engine information through a simple set of flags or some
> > integer or whatever is just not going to fly. No two of our engines
> > are ever the same, so that would quickly blow up in your face.
> 
> Well, you have more info about this than I do, I can only trust you on
> this (it's just weird based on the experience I described just before,
> it seems to me the differences are not that big, but again, you know the
> h/w better).
> 
Knowing the hardware better is actually a huge understatement, being the
responsible architect and lead designer :-) I specified and implemented
most of the stuff you are talking from the driver with my own two hands.

> I just don't want to end up with:
> 
>   if (version == EIP97_MRVL || version == EIP97_XXX || ...)
> 
That's an interesting remark as that is exactly what I am trying to
*avoid* through the way I'm using the version field and, for me,
contradicts some comments you had earlier?
Oh well, I'll take this statement as confirmation we're more or less
on the same page here. 

> > > We had this discussion on the v1. Your point was that you wanted this
> > > information to be in .data. One solution I proposed then was to use a
> > > struct (with both a 'version' and a 'flag' variable inside) instead of
> > > a single 'version' variable, so that we still can make checks on the
> > > version itself and not on something too specific.
> > >
> > As a result of that discussion, I kept your original version field
> > as intact as I could, knowing what I know and where I want to go.
> >
> > But to truly support generic engines, we really need all the stuff
> > that I added. Because it simply has that many parameters that are
> > different for each individual instance. But at the same time these
> > parameters can all be probed from the hardware directly, so
> > maintaining huge if statements all over the place decoding some
> > artificial version field is not the way to go (not maintainable).
> > Just probe all the parameters from the hardware and use them
> > directly where needed ... which my future patch set will do.
> 
> OK, I do think this would be a good solution :)
> 
I hope so ... as you're not exactly easy to convince and I can't
afford to keep spending this much effort on this for much longer
without getting in trouble with my dayjob.

> Thanks!
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thanks,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Antoine Tenart]

Hi Pascal,

On Tue, Jul 30, 2019 at 04:17:54PM +0000, Pascal Van Leeuwen wrote:
> > From: Antoine Tenart <antoine.tenart@bootlin.com>
> > On Tue, Jul 30, 2019 at 10:20:43AM +0000, Pascal Van Leeuwen wrote:
> > > > On Fri, Jul 26, 2019 at 02:43:24PM +0200, Pascal van Leeuwen wrote:
> > > > > -	switch (priv->version) {
> > > > > -	case EIP197B:
> > > > > -		dir = "eip197b";
> > > > > -		break;
> > > > > -	case EIP197D:
> > > > > -		dir = "eip197d";
> > > > > -		break;
> > > > > -	default:
> > > > > +	if (priv->version == EIP97IES_MRVL)
> > > > >  		/* No firmware is required */
> > > > >  		return 0;
> > > > > -	}
> > > > > +	else if (priv->version == EIP197D_MRVL)
> > > > > +		dir = "eip197d";
> > > > > +	else
> > > > > +		/* Default to minimum EIP197 config */
> > > > > +		dir = "eip197b";
> > > >
> > > > You're moving the default choice from "no firmware" to being a specific
> > > > one.
> > > >
> > > The EIP97 being the exception as the only firmware-less engine.
> > > This makes EIP197_DEVBRD fall through to EIP197B firmware until
> > > my patches supporting other EIP197 configs eventually get merged,
> > > after which this part will change anyway.
> > 
> > We don't know when/in what shape those patches will be merged, so in
> > the meantime please make the "no firmware" the default choice.
> >
> "No firmware" is not possible with an EIP197. Trying to use it without
> loading firmware will cause it to hang, which I don't believe is what
> you would want. So the alternative would be to return an error, which
> is fine by me, so then I'll change it into that as default.

Sure. When you look at this it's just weird to have a specific firmware
tied to an 'else' without having a check for a given version. Having the
"no firmware" option as the default option or not does not change
anything at runtime in practice here. If you prefer throwing an error if
the version isn't supported, I'm OK with it as well.

> > > > > @@ -869,9 +898,6 @@ static int safexcel_register_algorithms(struct
> > safexcel_crypto_priv
> > > > *priv)
> > > > >  	for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) {
> > > > >  		safexcel_algs[i]->priv = priv;
> > > > >
> > > > > -		if (!(safexcel_algs[i]->engines & priv->version))
> > > > > -			continue;
> > > >
> > > > You should remove the 'engines' flag in a separate patch. I'm really not
> > > > sure about this. I don't think all the IS EIP engines support the same
> > > > sets of algorithms?
> > > >
> > > All algorithms provided at this moment are available from all engines
> > > currently supported. So the whole mechanism, so far, is redundant.
> > >
> > > This will change as I add support for generic (non-Marvell) engines,
> > > but the approach taken here is not scalable or maintainable. So I will
> > > end up doing it differently, eventually. I don't see the point in
> > > maintaining dead/unused/redundant code I'm about to replace anyway.
> > 
> > But it's not done yet and we might discuss how you'll handle this. You
> > can't know for sure you'll end up with a different approach.
> > 
> We can discuss all we want, but the old approach for sure won't work
> so there's no point in keeping those (effectively redundant up until
> now, so why did they even exist?) 'engines' flags. 

Yes, I mean my point is that you can't know the future for sure, but I
agree you'll surely end up with a better solution :)

> > > > > +	if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) {
> > > >
> > > > You have extra parenthesis here.
> > > >
> > > Our internal coding style (as well as my personal preference)
> > > actually mandates to put parenthesis around everything so expect
> > > that to happen a lot going forward as I've been doing it like that
> > > for nearly 30 years now.
> > >
> > > Does the Linux coding style actually *prohibit* the use of these
> > > "extra" parenthesis? I don't recall reading that anywhere ...
> > 
> > I don't know if this is a written rule (as many others), but you'll find
> > plenty of examples of reviews asking not to have extra parenthesis.
> >
> I can remove them, I was just wondering if there was actually any 
> rationale for not wanting to have them (considering that that at least
> seems far more error prone). I have this strange desire to want to 
> know WHY I have to do something before I do it.
> 
> Is it just to show off your intimate knowledge of C operator precedence
> and associativity  ;-) (which, incidentally, I'm not too familiar with)

Hehe :) I think this is more a coding style non-written requirement than
something backed up with a technical reason, or it's possible I'm not
aware of the reason. Anyway, this is often asked in reviews, and you
won't find many examples of this in existing code.

> > > > We had this discussion on the v1. Your point was that you wanted this
> > > > information to be in .data. One solution I proposed then was to use a
> > > > struct (with both a 'version' and a 'flag' variable inside) instead of
> > > > a single 'version' variable, so that we still can make checks on the
> > > > version itself and not on something too specific.
> > > >
> > > As a result of that discussion, I kept your original version field
> > > as intact as I could, knowing what I know and where I want to go.
> > >
> > > But to truly support generic engines, we really need all the stuff
> > > that I added. Because it simply has that many parameters that are
> > > different for each individual instance. But at the same time these
> > > parameters can all be probed from the hardware directly, so
> > > maintaining huge if statements all over the place decoding some
> > > artificial version field is not the way to go (not maintainable).
> > > Just probe all the parameters from the hardware and use them
> > > directly where needed ... which my future patch set will do.
> > 
> > OK, I do think this would be a good solution :)
> > 
> I hope so ... as you're not exactly easy to convince and I can't
> afford to keep spending this much effort on this for much longer
> without getting in trouble with my dayjob.

I could say the same (I do think it's good that you want to know the
reason behind requirements and comments). I didn't comment specifically
on the lengthy explanations you gave, but it helped me to understand
your point, how the h/w engines we'll support are designed and
integrated and the reason behind lots of your choices. I know we spent
a lot of time discussing various points and I'm pretty sure this is
because we need to both ramp up (on the diversity of engines and on
the upstream process) but I'm sure this is only temporary.

One of the examples where those discussions will be useful is if/when
we'll have more of compatibles to support in the driver, as those cannot
be removed once merged.

I could continue arguing on the 'version', but let's say we're on the
same page for now and if ever I see a reason to factorize some code
regarding this we can always discuss it later, as this it's internal to
the driver.

Thanks for taking the time :)

Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Wednesday, July 31, 2019 2:13 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen
> <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au;
> davem@davemloft.net
> Subject: Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA
> development board
> 
> > > > > You're moving the default choice from "no firmware" to being a specific
> > > > > one.
> > > > >
> > > > The EIP97 being the exception as the only firmware-less engine.
> > > > This makes EIP197_DEVBRD fall through to EIP197B firmware until
> > > > my patches supporting other EIP197 configs eventually get merged,
> > > > after which this part will change anyway.
> > >
> > > We don't know when/in what shape those patches will be merged, so in
> > > the meantime please make the "no firmware" the default choice.
> > >
> > "No firmware" is not possible with an EIP197. Trying to use it without
> > loading firmware will cause it to hang, which I don't believe is what
> > you would want. So the alternative would be to return an error, which
> > is fine by me, so then I'll change it into that as default.
> 
> Sure. When you look at this it's just weird to have a specific firmware
> tied to an 'else' without having a check for a given version. Having the
> "no firmware" option as the default option or not does not change
> anything at runtime in practice here. If you prefer throwing an error if
> the version isn't supported, I'm OK with it as well.
> 
Agree that it was weird, not loading anything was bad too.
Glad that we both agree on throwing an error :-)

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

RE: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Pascal Van Leeuwen]

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of
> Pascal Van Leeuwen
> Sent: Tuesday, July 30, 2019 12:21 PM
> To: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen <pascalvanl@gmail.com>
> Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
> Subject: RE: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA
> development board
> 
> 
> > > +static int __init crypto_is_init(void)
> > > +{
> > > +	int rc;
> > > +
> > > +	#if (IS_ENABLED(CONFIG_OF))
> > > +		/* Register platform driver */
> > > +		platform_driver_register(&crypto_safexcel);
> > > +	#endif
> >
> > When used in the code directly, you should use:
> >
> >   if (IS_ENABLED(CONFIG_OF))
> >
> Oops, I missed that one, will fix.
> 
Actually, I tried that first, but it doesn't work in this particular case. 
The #if is really necessary here to avoid compile errors due to 
references to datastructures that are within another #if ... (which refer
to functions with the #if etc. so it's a whole dependency chain)

This seemed to be the best way to handle it, but any suggestions that
take into account the big picture are welcome!

> > Thanks!
> > Antoine
> >
> > --
> > Antoine Ténart, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> 
> Thanks,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> www.insidesecure.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Herbert Xu]

On Wed, Jul 31, 2019 at 10:11:46AM +0000, Pascal Van Leeuwen wrote:
>
> > > > +static int __init crypto_is_init(void)
> > > > +{
> > > > +	int rc;
> > > > +
> > > > +	#if (IS_ENABLED(CONFIG_OF))
> > > > +		/* Register platform driver */
> > > > +		platform_driver_register(&crypto_safexcel);
> > > > +	#endif
> > >
> > > When used in the code directly, you should use:
> > >
> > >   if (IS_ENABLED(CONFIG_OF))
> > >
> > Oops, I missed that one, will fix.
> > 
> Actually, I tried that first, but it doesn't work in this particular case. 
> The #if is really necessary here to avoid compile errors due to 
> references to datastructures that are within another #if ... (which refer
> to functions with the #if etc. so it's a whole dependency chain)

If you're going to use a #if then please don't indent it as that's
not the kernel coding style.

I see why you can't use a straight "if" because you've moved
crypto_safexcel inside a #if, but what if you got rid of that
#if too? IOW what would it take to make the probe function compile
with CONFIG_OF off?

In general we want to maximise compiler coverage under all config
options so if we can make it compiler without too much effort that
would be the preferred solution.

Cheers,
-- 
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: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Wednesday, July 31, 2019 1:08 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen
> <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org; davem@davemloft.net
> Subject: Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA
> development board
> 
> On Wed, Jul 31, 2019 at 10:11:46AM +0000, Pascal Van Leeuwen wrote:
> >
> > > > > +static int __init crypto_is_init(void)
> > > > > +{
> > > > > +	int rc;
> > > > > +
> > > > > +	#if (IS_ENABLED(CONFIG_OF))
> > > > > +		/* Register platform driver */
> > > > > +		platform_driver_register(&crypto_safexcel);
> > > > > +	#endif
> > > >
> > > > When used in the code directly, you should use:
> > > >
> > > >   if (IS_ENABLED(CONFIG_OF))
> > > >
> > > Oops, I missed that one, will fix.
> > >
> > Actually, I tried that first, but it doesn't work in this particular case.
> > The #if is really necessary here to avoid compile errors due to
> > references to datastructures that are within another #if ... (which refer
> > to functions with the #if etc. so it's a whole dependency chain)
> 
> If you're going to use a #if then please don't indent it as that's
> not the kernel coding style.
> 
Ok, I can do that

> I see why you can't use a straight "if" because you've moved
> crypto_safexcel inside a #if, but what if you got rid of that
> #if too?
>
Then it won't compile either because it references stuff outside of the 
module that don't exist. Originally (previous version of the patch) I 
had a different solution removing those function bodies using regular 
if's but for some reason I don't remember anymore that didn't fly and 
I had to change it into this.

> IOW what would it take to make the probe function compile
> with CONFIG_OF off?
> 
That's how I came up with the current code using #if's. If anyone has a 
better solution, I'm happy to hear it. But I don't really want to spend
more time on it doing trial-and-error and not getting anywhere.

> In general we want to maximise compiler coverage under all config
> options so if we can make it compiler without too much effort that
> would be the preferred solution.
>
I understand that, and I tried to do that initially, but that wouldn't
work so I ended up with this as a compromise ...

> Cheers,
> --
> 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

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Herbert Xu]

On Wed, Jul 31, 2019 at 11:37:08AM +0000, Pascal Van Leeuwen wrote:
>
> > In general we want to maximise compiler coverage under all config
> > options so if we can make it compiler without too much effort that
> > would be the preferred solution.
> >
> I understand that, and I tried to do that initially, but that wouldn't
> work so I ended up with this as a compromise ...

If it's too hard then #if is the way to go.

Cheers,
-- 
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

[PATCHv2 3/3] crypto: inside-secure - add support for using the EIP197 without vendor firmware

[Pascal van Leeuwen]

Until now, the inside-secure driver required a set of firmware images
supplied by the silicon vendor, typically under NDA, to be present in
/lib/firmware/inside-secure in order to be able to function.
This patch removes the dependence on this official vendor firmware by
falling back to generic "mini" FW - developed specifically for this
driver - that can be provided under GPL 2.0 through linux-firmwares.

Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 drivers/crypto/inside-secure/safexcel.c | 194 ++++++++++++++++++++++++--------
 drivers/crypto/inside-secure/safexcel.h |   6 +
 2 files changed, 153 insertions(+), 47 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index fe9b6d0..503fef0 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -108,44 +108,139 @@ static void eip197_trc_cache_init(struct safexcel_crypto_priv *priv)
 	writel(val, priv->base + EIP197_TRC_PARAMS);
 }
 
-static void eip197_write_firmware(struct safexcel_crypto_priv *priv,
-				  const struct firmware *fw, int pe, u32 ctrl,
-				  u32 prog_en)
+static void eip197_init_firmware(struct safexcel_crypto_priv *priv)
 {
-	const u32 *data = (const u32 *)fw->data;
+	int pe, i;
 	u32 val;
-	int i;
 
-	/* Reset the engine to make its program memory accessible */
-	writel(EIP197_PE_ICE_x_CTRL_SW_RESET |
-	       EIP197_PE_ICE_x_CTRL_CLR_ECC_CORR |
-	       EIP197_PE_ICE_x_CTRL_CLR_ECC_NON_CORR,
-	       EIP197_PE(priv) + ctrl);
+	for (pe = 0; pe < priv->config.pes; pe++) {
+		/* Configure the token FIFO's */
+		writel(3, EIP197_PE(priv) + EIP197_PE_ICE_PUTF_CTRL(pe));
+		writel(0, EIP197_PE(priv) + EIP197_PE_ICE_PPTF_CTRL(pe));
 
-	/* Enable access to the program memory */
-	writel(prog_en, EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
+		/* Clear the ICE scratchpad memory */
+		val = readl(EIP197_PE(priv) + EIP197_PE_ICE_SCRATCH_CTRL(pe));
+		val |= EIP197_PE_ICE_SCRATCH_CTRL_CHANGE_TIMER |
+		       EIP197_PE_ICE_SCRATCH_CTRL_TIMER_EN |
+		       EIP197_PE_ICE_SCRATCH_CTRL_SCRATCH_ACCESS |
+		       EIP197_PE_ICE_SCRATCH_CTRL_CHANGE_ACCESS;
+		writel(val, EIP197_PE(priv) + EIP197_PE_ICE_SCRATCH_CTRL(pe));
+
+		/* clear the scratchpad RAM using 32 bit writes only */
+		for (i = 0; i < EIP197_NUM_OF_SCRATCH_BLOCKS; i++)
+			writel(0, EIP197_PE(priv) +
+				  EIP197_PE_ICE_SCRATCH_RAM(pe) + (i<<2));
+
+		/* Reset the IFPP engine to make its program mem accessible */
+		writel(EIP197_PE_ICE_x_CTRL_SW_RESET |
+		       EIP197_PE_ICE_x_CTRL_CLR_ECC_CORR |
+		       EIP197_PE_ICE_x_CTRL_CLR_ECC_NON_CORR,
+		       EIP197_PE(priv) + EIP197_PE_ICE_FPP_CTRL(pe));
+
+		/* Reset the IPUE engine to make its program mem accessible */
+		writel(EIP197_PE_ICE_x_CTRL_SW_RESET |
+		       EIP197_PE_ICE_x_CTRL_CLR_ECC_CORR |
+		       EIP197_PE_ICE_x_CTRL_CLR_ECC_NON_CORR,
+		       EIP197_PE(priv) + EIP197_PE_ICE_PUE_CTRL(pe));
+
+		/* Enable access to all IFPP program memories */
+		writel(EIP197_PE_ICE_RAM_CTRL_FPP_PROG_EN,
+		       EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
+	}
+
+}
+
+static int eip197_write_firmware(struct safexcel_crypto_priv *priv,
+				  const struct firmware *fw)
+{
+	const u32 *data = (const u32 *)fw->data;
+	int i;
 
 	/* Write the firmware */
 	for (i = 0; i < fw->size / sizeof(u32); i++)
 		writel(be32_to_cpu(data[i]),
 		       priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(u32));
 
-	/* Disable access to the program memory */
-	writel(0, EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
+	return i - 2;
+}
+
+/*
+ * If FW is actual production firmware, then poll for its initialization
+ * to complete and check if it is good for the HW, otherwise just return OK.
+ */
+static bool poll_fw_ready(struct safexcel_crypto_priv *priv, int fpp)
+{
+	int pe, pollcnt;
+	u32 base, pollofs;
 
-	/* Release engine from reset */
-	val = readl(EIP197_PE(priv) + ctrl);
-	val &= ~EIP197_PE_ICE_x_CTRL_SW_RESET;
-	writel(val, EIP197_PE(priv) + ctrl);
+	if (fpp)
+		pollofs  = EIP197_FW_FPP_READY;
+	else
+		pollofs  = EIP197_FW_PUE_READY;
+
+	for (pe = 0; pe < priv->config.pes; pe++) {
+		base = EIP197_PE_ICE_SCRATCH_RAM(pe);
+		pollcnt = EIP197_FW_START_POLLCNT;
+		while (pollcnt &&
+		       (readl(EIP197_PE(priv) + base +
+			      pollofs) != 1)) {
+			pollcnt--;
+			cpu_relax();
+		}
+		if (!pollcnt) {
+			dev_err(priv->dev, "FW(%d) for PE %d failed to start",
+				fpp, pe);
+			return false;
+		}
+	}
+	return true;
+}
+
+static bool eip197_start_firmware(struct safexcel_crypto_priv *priv,
+				  int ipuesz, int ifppsz, int minifw)
+{
+	int pe;
+	u32 val;
+
+	for (pe = 0; pe < priv->config.pes; pe++) {
+		/* Disable access to all program memory */
+		writel(0, EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
+
+		/* Start IFPP microengines */
+		if (minifw)
+			val = 0;
+		else
+			val = (((ifppsz - 1) & 0x7ff0) << 16) | BIT(3);
+		writel(val, EIP197_PE(priv) + EIP197_PE_ICE_FPP_CTRL(pe));
+
+		/* Start IPUE microengines */
+		if (minifw)
+			val = 0;
+		else
+			val = ((ipuesz - 1) & 0x7ff0) << 16 | BIT(3);
+		writel(val, EIP197_PE(priv) + EIP197_PE_ICE_PUE_CTRL(pe));
+	}
+
+	/* For miniFW startup, there is no initialization, so always succeed */
+	if (minifw)
+		return true;
+
+	/* Wait until all the firmwares have properly started up */
+	if (!poll_fw_ready(priv, 1))
+		return false;
+	if (!poll_fw_ready(priv, 0))
+		return false;
+
+	return true;
 }
 
 static int eip197_load_firmwares(struct safexcel_crypto_priv *priv)
 {
 	const char *fw_name[] = {"ifpp.bin", "ipue.bin"};
 	const struct firmware *fw[FW_NB];
-	char fw_path[31], *dir = NULL;
+	char fw_path[37], *dir = NULL;
 	int i, j, ret = 0, pe;
-	u32 val;
+	int ipuesz, ifppsz, minifw = 0;
 
 	if (priv->version == EIP97IES_MRVL)
 		/* No firmware is required */
@@ -156,52 +251,57 @@ static int eip197_load_firmwares(struct safexcel_crypto_priv *priv)
 		/* Default to minimum EIP197 config */
 		dir = "eip197b";
 
+retry_fw:
 	for (i = 0; i < FW_NB; i++) {
-		snprintf(fw_path, 31, "inside-secure/%s/%s", dir, fw_name[i]);
-		ret = request_firmware(&fw[i], fw_path, priv->dev);
+		snprintf(fw_path, 37, "inside-secure/%s/%s", dir, fw_name[i]);
+		ret = firmware_request_nowarn(&fw[i], fw_path, priv->dev);
 		if (ret) {
-			if (!(priv->version == EIP197B_MRVL))
+			if (minifw || !(priv->version == EIP197B_MRVL))
 				goto release_fw;
 
 			/*
 			 * Fallback to the old firmware location for the
 			 * EIP197b.
 			 */
-			ret = request_firmware(&fw[i], fw_name[i], priv->dev);
-			if (ret) {
-				dev_err(priv->dev,
-					"Failed to request firmware %s (%d)\n",
-					fw_name[i], ret);
+			ret = firmware_request_nowarn(&fw[i], fw_name[i],
+						      priv->dev);
+			if (ret)
 				goto release_fw;
-			}
 		}
 	}
 
-	for (pe = 0; pe < priv->config.pes; pe++) {
-		/* Clear the scratchpad memory */
-		val = readl(EIP197_PE(priv) + EIP197_PE_ICE_SCRATCH_CTRL(pe));
-		val |= EIP197_PE_ICE_SCRATCH_CTRL_CHANGE_TIMER |
-		       EIP197_PE_ICE_SCRATCH_CTRL_TIMER_EN |
-		       EIP197_PE_ICE_SCRATCH_CTRL_SCRATCH_ACCESS |
-		       EIP197_PE_ICE_SCRATCH_CTRL_CHANGE_ACCESS;
-		writel(val, EIP197_PE(priv) + EIP197_PE_ICE_SCRATCH_CTRL(pe));
+	eip197_init_firmware(priv);
+
+	ifppsz = eip197_write_firmware(priv, fw[FW_IFPP]);
 
-		memset_io(EIP197_PE(priv) + EIP197_PE_ICE_SCRATCH_RAM(pe), 0,
-			  EIP197_NUM_OF_SCRATCH_BLOCKS * sizeof(u32));
+	/* Enable access to IPUE program memories */
+	for (pe = 0; pe < priv->config.pes; pe++)
+		writel(EIP197_PE_ICE_RAM_CTRL_PUE_PROG_EN,
+		       EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
 
-		eip197_write_firmware(priv, fw[FW_IFPP], pe,
-				      EIP197_PE_ICE_FPP_CTRL(pe),
-				      EIP197_PE_ICE_RAM_CTRL_FPP_PROG_EN);
+	ipuesz = eip197_write_firmware(priv, fw[FW_IPUE]);
 
-		eip197_write_firmware(priv, fw[FW_IPUE], pe,
-				      EIP197_PE_ICE_PUE_CTRL(pe),
-				      EIP197_PE_ICE_RAM_CTRL_PUE_PROG_EN);
+	if (eip197_start_firmware(priv, ipuesz, ifppsz, minifw)) {
+		dev_dbg(priv->dev, "Firmware loaded successfully");
+		return 0;
 	}
 
+	ret = -ENODEV;
+
 release_fw:
 	for (j = 0; j < i; j++)
 		release_firmware(fw[j]);
 
+	if (!minifw) {
+		/* Retry with minifw path */
+		dev_dbg(priv->dev, "Firmware set not (fully) present or init failed, falling back to BCLA mode");
+		dir = "eip197_minifw";
+		minifw = 1;
+		goto retry_fw;
+	}
+
+	dev_dbg(priv->dev, "Firmware load failed.");
+
 	return ret;
 }
 
diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h
index d07bec1..ed47df0 100644
--- a/drivers/crypto/inside-secure/safexcel.h
+++ b/drivers/crypto/inside-secure/safexcel.h
@@ -136,8 +136,10 @@
 #define EIP197_PE_IN_TBUF_THRES(n)		(0x0100 + (0x2000 * (n)))
 #define EIP197_PE_ICE_SCRATCH_RAM(n)		(0x0800 + (0x2000 * (n)))
 #define EIP197_PE_ICE_PUE_CTRL(n)		(0x0c80 + (0x2000 * (n)))
+#define EIP197_PE_ICE_PUTF_CTRL(n)		(0x0d00 + (0x2000 * (n)))
 #define EIP197_PE_ICE_SCRATCH_CTRL(n)		(0x0d04 + (0x2000 * (n)))
 #define EIP197_PE_ICE_FPP_CTRL(n)		(0x0d80 + (0x2000 * (n)))
+#define EIP197_PE_ICE_PPTF_CTRL(n)		(0x0e00 + (0x2000 * (n)))
 #define EIP197_PE_ICE_RAM_CTRL(n)		(0x0ff0 + (0x2000 * (n)))
 #define EIP197_PE_EIP96_TOKEN_CTRL(n)		(0x1000 + (0x2000 * (n)))
 #define EIP197_PE_EIP96_FUNCTION_EN(n)		(0x1004 + (0x2000 * (n)))
@@ -530,6 +532,10 @@ struct safexcel_command_desc {
  * Internal structures & functions
  */
 
+#define EIP197_FW_START_POLLCNT		16
+#define EIP197_FW_PUE_READY		0x14
+#define EIP197_FW_FPP_READY		0x18
+
 enum eip197_fw {
 	FW_IFPP = 0,
 	FW_IPUE,
-- 
1.8.3.1

Re: [PATCHv2 3/3] crypto: inside-secure - add support for using the EIP197 without vendor firmware

[Antoine Tenart]

Hi Pascal,

Thanks for reworking this not to include the firmware blob, the patch
looks good and I only have minor comments.

On Fri, Jul 26, 2019 at 02:43:25PM +0200, Pascal van Leeuwen wrote:
> +
> +static int eip197_write_firmware(struct safexcel_crypto_priv *priv,
> +				  const struct firmware *fw)
> +{
> +	const u32 *data = (const u32 *)fw->data;
> +	int i;
>  
>  	/* Write the firmware */
>  	for (i = 0; i < fw->size / sizeof(u32); i++)
>  		writel(be32_to_cpu(data[i]),
>  		       priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(u32));
>  
> -	/* Disable access to the program memory */
> -	writel(0, EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
> +	return i - 2;

Could you add a comment (or if applicable, a define) for this '- 2'?
What happens if i < 2 ?

> +	for (pe = 0; pe < priv->config.pes; pe++) {
> +		base = EIP197_PE_ICE_SCRATCH_RAM(pe);
> +		pollcnt = EIP197_FW_START_POLLCNT;
> +		while (pollcnt &&
> +		       (readl(EIP197_PE(priv) + base +
> +			      pollofs) != 1)) {
> +			pollcnt--;
> +			cpu_relax();

You can probably use readl_relaxed() here.

> +		}
> +		if (!pollcnt) {
> +			dev_err(priv->dev, "FW(%d) for PE %d failed to start",
> +				fpp, pe);

A \n is missing at the end of the string.

> +static bool eip197_start_firmware(struct safexcel_crypto_priv *priv,
> +				  int ipuesz, int ifppsz, int minifw)
> +{
> +	int pe;
> +	u32 val;
> +
> +	for (pe = 0; pe < priv->config.pes; pe++) {
> +		/* Disable access to all program memory */
> +		writel(0, EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
> +
> +		/* Start IFPP microengines */
> +		if (minifw)
> +			val = 0;
> +		else
> +			val = (((ifppsz - 1) & 0x7ff0) << 16) | BIT(3);

Could you define the mask and the 'BIT(3)'?

> +		writel(val, EIP197_PE(priv) + EIP197_PE_ICE_FPP_CTRL(pe));
> +
> +		/* Start IPUE microengines */
> +		if (minifw)
> +			val = 0;
> +		else
> +			val = ((ipuesz - 1) & 0x7ff0) << 16 | BIT(3);

Ditto.

>  
> +	if (!minifw) {
> +		/* Retry with minifw path */
> +		dev_dbg(priv->dev, "Firmware set not (fully) present or init failed, falling back to BCLA mode");

A \n is missing here.

> +		dir = "eip197_minifw";
> +		minifw = 1;
> +		goto retry_fw;
> +	}
> +
> +	dev_dbg(priv->dev, "Firmware load failed.");

Ditto.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCHv2 3/3] crypto: inside-secure - add support for using the EIP197 without vendor firmware

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Wednesday, July 31, 2019 2:26 PM
> To: Pascal van Leeuwen <pascalvanl@gmail.com>
> Cc: linux-crypto@vger.kernel.org; antoine.tenart@bootlin.com; herbert@gondor.apana.org.au;
> davem@davemloft.net; Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Subject: Re: [PATCHv2 3/3] crypto: inside-secure - add support for using the EIP197
> without vendor firmware
> 
> Hi Pascal,
> 
> Thanks for reworking this not to include the firmware blob, the patch
> looks good and I only have minor comments.
> 
> On Fri, Jul 26, 2019 at 02:43:25PM +0200, Pascal van Leeuwen wrote:
> > +
> > +static int eip197_write_firmware(struct safexcel_crypto_priv *priv,
> > +				  const struct firmware *fw)
> > +{
> > +	const u32 *data = (const u32 *)fw->data;
> > +	int i;
> >
> >  	/* Write the firmware */
> >  	for (i = 0; i < fw->size / sizeof(u32); i++)
> >  		writel(be32_to_cpu(data[i]),
> >  		       priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(u32));
> >
> > -	/* Disable access to the program memory */
> > -	writel(0, EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
> > +	return i - 2;
> 
> Could you add a comment (or if applicable, a define) for this '- 2'?
>
Of course

> What happens if i < 2 ?
> 
Ok, I did not consider that as it can't happen for any kind of legal FW. But it
wouldn't be pretty (neither would 2 itself, BTW). I could throw an error for it
but it wouldn't make that much sense as we don't do any checks on the firm-
ware *contents* either ... So either way, if your firmware file is no good, you
have a problem ...

> > +	for (pe = 0; pe < priv->config.pes; pe++) {
> > +		base = EIP197_PE_ICE_SCRATCH_RAM(pe);
> > +		pollcnt = EIP197_FW_START_POLLCNT;
> > +		while (pollcnt &&
> > +		       (readl(EIP197_PE(priv) + base +
> > +			      pollofs) != 1)) {
> > +			pollcnt--;
> > +			cpu_relax();
> 
> You can probably use readl_relaxed() here.
> 
Ok, will do

> > +		}
> > +		if (!pollcnt) {
> > +			dev_err(priv->dev, "FW(%d) for PE %d failed to start",
> > +				fpp, pe);
> 
> A \n is missing at the end of the string.
> 
Well spotted, will fix

> > +static bool eip197_start_firmware(struct safexcel_crypto_priv *priv,
> > +				  int ipuesz, int ifppsz, int minifw)
> > +{
> > +	int pe;
> > +	u32 val;
> > +
> > +	for (pe = 0; pe < priv->config.pes; pe++) {
> > +		/* Disable access to all program memory */
> > +		writel(0, EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
> > +
> > +		/* Start IFPP microengines */
> > +		if (minifw)
> > +			val = 0;
> > +		else
> > +			val = (((ifppsz - 1) & 0x7ff0) << 16) | BIT(3);
> 
> Could you define the mask and the 'BIT(3)'?
>
i.e. add a define. Sure.

> > +		writel(val, EIP197_PE(priv) + EIP197_PE_ICE_FPP_CTRL(pe));
> > +
> > +		/* Start IPUE microengines */
> > +		if (minifw)
> > +			val = 0;
> > +		else
> > +			val = ((ipuesz - 1) & 0x7ff0) << 16 | BIT(3);
> 
> Ditto.
> 
> >
> > +	if (!minifw) {
> > +		/* Retry with minifw path */
> > +		dev_dbg(priv->dev, "Firmware set not (fully) present or init failed, falling
> back to BCLA mode");
> 
> A \n is missing here.
> 
Yes, thanks

> > +		dir = "eip197_minifw";
> > +		minifw = 1;
> > +		goto retry_fw;
> > +	}
> > +
> > +	dev_dbg(priv->dev, "Firmware load failed.");
> 
> Ditto.
> 
Ack

> Thanks,
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCHv2 3/3] crypto: inside-secure - add support for using the EIP197 without vendor firmware

[Antoine Tenart]

On Wed, Jul 31, 2019 at 02:23:27PM +0000, Pascal Van Leeuwen wrote:
> > From: Antoine Tenart <antoine.tenart@bootlin.com>
> 
> > What happens if i < 2 ?
> > 
> Ok, I did not consider that as it can't happen for any kind of legal FW. But it
> wouldn't be pretty (neither would 2 itself, BTW). I could throw an error for it
> but it wouldn't make that much sense as we don't do any checks on the firm-
> ware *contents* either ... So either way, if your firmware file is no good, you
> have a problem ...

The thing is to avoid doing harm to the kernel if a single driver can't
work as expected, especially when we have an user input (the firmware).
The firmware being a valid one is another topic. But honestly I'm not
sure if a wrong returned value would change anything here, apart from
not probing the driver successfully as we know something went wrong.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCHv2 3/3] crypto: inside-secure - add support for using the EIP197 without vendor firmware

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Wednesday, July 31, 2019 4:46 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Pascal van Leeuwen
> <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au;
> davem@davemloft.net
> Subject: Re: [PATCHv2 3/3] crypto: inside-secure - add support for using the EIP197
> without vendor firmware
> 
> On Wed, Jul 31, 2019 at 02:23:27PM +0000, Pascal Van Leeuwen wrote:
> > > From: Antoine Tenart <antoine.tenart@bootlin.com>
> >
> > > What happens if i < 2 ?
> > >
> > Ok, I did not consider that as it can't happen for any kind of legal FW. But it
> > wouldn't be pretty (neither would 2 itself, BTW). I could throw an error for it
> > but it wouldn't make that much sense as we don't do any checks on the firm-
> > ware *contents* either ... So either way, if your firmware file is no good, you
> > have a problem ...
> 
> The thing is to avoid doing harm to the kernel if a single driver can't
> work as expected, especially when we have an user input (the firmware).
> The firmware being a valid one is another topic. But honestly I'm not
> sure if a wrong returned value would change anything here, apart from
> not probing the driver successfully as we know something went wrong.
> 
Initially I thought it would hang (as the HW would hang for sure), but on
second observation the driver already has a nice time-out mechanism on the
firmware startup, so it would just throw a nice error and bail out. Same
as it would do for a e.g. a corrupted image. So I think this is just fine.

> Thanks,
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com


Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com