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.
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
firmware images
drivers/crypto/Kconfig | 12 +-
drivers/crypto/inside-secure/safexcel.c | 710 ++++++++++++++++++++++++--------
drivers/crypto/inside-secure/safexcel.h | 23 ++
3 files changed, 562 insertions(+), 183 deletions(-)
--
1.8.3.1
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 <[email protected]>
---
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
Until now, the inside-secure driver required a set of firmware images to be
present in /lib/firmware/inside-secure in order to be able to function.
These firmware images were not available to the general public and diffi-
cult to obtain (only under NDA from Marvell). Also, the driver did and does
not use any specific firmware functionality. This patch removes the depen-
dence on those firmware images.
Signed-off-by: Pascal van Leeuwen <[email protected]>
---
drivers/crypto/inside-secure/safexcel.c | 217 +++++++++++++++++++++++++-------
drivers/crypto/inside-secure/safexcel.h | 6 +
2 files changed, 180 insertions(+), 43 deletions(-)
diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index a6a0f48..e1781b2 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -107,44 +107,160 @@ 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));
+
+ /* 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));
+ }
+
+}
- /* Enable access to the program memory */
- writel(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;
+
+ 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 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 (ifppsz)
+ val = (((ifppsz - 1) & 0x7ff0) << 16) | BIT(3);
+ else
+ val = 0;
+ writel(val, EIP197_PE(priv) + EIP197_PE_ICE_FPP_CTRL(pe));
+
+ /* Start IPUE microengines */
+ if (ipuesz)
+ val = ((ipuesz - 1) & 0x7ff0) << 16 | BIT(3);
+ else
+ val = 0;
+ writel(val, EIP197_PE(priv) + EIP197_PE_ICE_PUE_CTRL(pe));
+ }
+
+ /* For miniFW startup, there is no initialization, so always succeed */
+ if ((!ipuesz) && (!ifppsz))
+ 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;
- /* Release engine from reset */
- val = readl(EIP197_PE(priv) + ctrl);
- val &= ~EIP197_PE_ICE_x_CTRL_SW_RESET;
- writel(val, EIP197_PE(priv) + ctrl);
+ return true;
}
static int eip197_load_firmwares(struct safexcel_crypto_priv *priv)
{
+ /*
+ * The embedded one-size-fits-all MiniFW is just for handling TR
+ * prefetch & invalidate. It does not support any FW flows, effectively
+ * turning the EIP197 into a glorified EIP97
+ */
+ const u32 ipue_minifw[] = {
+ 0x24808200, 0x2D008204, 0x2680E208, 0x2780E20C,
+ 0x2200F7FF, 0x38347000, 0x2300F000, 0x15200A80,
+ 0x01699003, 0x60038011, 0x38B57000, 0x0119F04C,
+ 0x01198548, 0x20E64000, 0x20E75000, 0x1E200000,
+ 0x30E11000, 0x103A93FF, 0x60830014, 0x5B8B0000,
+ 0xC0389000, 0x600B0018, 0x2300F000, 0x60800011,
+ 0x90800000, 0x10000000, 0x10000000};
+ const u32 ifpp_minifw[] = {
+ 0x21008000, 0x260087FC, 0xF01CE4C0, 0x60830006,
+ 0x530E0000, 0x90800000, 0x23008004, 0x24808008,
+ 0x2580800C, 0x0D300000, 0x205577FC, 0x30D42000,
+ 0x20DAA7FC, 0x43107000, 0x42220004, 0x00000000,
+ 0x00000000, 0x00000000, 0x00000000, 0x00000000,
+ 0x00060004, 0x20337004, 0x90800000, 0x10000000,
+ 0x10000000};
const char *fw_name[] = {"ifpp.bin", "ipue.bin"};
const struct firmware *fw[FW_NB];
char fw_path[31], *dir = NULL;
int i, j, ret = 0, pe;
- u32 val;
+ int ipuesz, ifppsz;
if (priv->version & EIP197B)
dir = "eip197b";
@@ -156,7 +272,7 @@ static int eip197_load_firmwares(struct safexcel_crypto_priv *priv)
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);
+ ret = firmware_request_nowarn(&fw[i], fw_path, priv->dev);
if (ret) {
if (!(priv->version & EIP197B))
goto release_fw;
@@ -164,42 +280,57 @@ static int eip197_load_firmwares(struct safexcel_crypto_priv *priv)
/* 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)) {
+ dev_info(priv->dev, "EIP197 firmware loaded successfully");
+ return 0;
}
release_fw:
for (j = 0; j < i; j++)
release_firmware(fw[j]);
- return ret;
+ /*
+ * Firmware download failed, fall back to EIP97 BCLA mode
+ * Note that this is not a formally supported mode for the EIP197,
+ * so your mileage may vary
+ */
+ dev_info(priv->dev, "EIP197 firmware set not (fully) present or init failed, falling back to EIP97 BCLA mode");
+
+ eip197_init_firmware(priv);
+
+ for (i = 0; i < sizeof(ifpp_minifw)>>2; i++)
+ writel(ifpp_minifw[i],
+ priv->base + EIP197_CLASSIFICATION_RAMS + (i<<2));
+
+ /* 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));
+
+ for (i = 0; i < sizeof(ipue_minifw)>>2; i++)
+ writel(ipue_minifw[i],
+ priv->base + EIP197_CLASSIFICATION_RAMS + (i<<2));
+
+ eip197_start_firmware(priv, 0, 0);
+ return 0;
}
static int safexcel_hw_setup_cdesc_rings(struct safexcel_crypto_priv *priv)
diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h
index 924270e..380ba72 100644
--- a/drivers/crypto/inside-secure/safexcel.h
+++ b/drivers/crypto/inside-secure/safexcel.h
@@ -128,8 +128,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)))
@@ -522,6 +524,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
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 <[email protected]>
---
drivers/crypto/inside-secure/safexcel.c | 493 +++++++++++++++++++++++---------
drivers/crypto/inside-secure/safexcel.h | 17 ++
2 files changed, 376 insertions(+), 134 deletions(-)
diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index df43a2c..a6a0f48 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,7 +33,7 @@ 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) {
+ 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;
@@ -145,23 +146,19 @@ static int eip197_load_firmwares(struct safexcel_crypto_priv *priv)
int i, j, ret = 0, pe;
u32 val;
- switch (priv->version) {
- case EIP197B:
+ if (priv->version & EIP197B)
dir = "eip197b";
- break;
- case EIP197D:
+ else if (priv->version & EIP197D)
dir = "eip197d";
- break;
- default:
+ else
/* No firmware is required */
return 0;
- }
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))
goto release_fw;
/* Fallback to the old firmware location for the
@@ -294,6 +291,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
u32 version, val;
int i, ret, pe;
+ dev_info(priv->dev, "EIP(1)97 HW init: burst size %d beats, using %d pipe(s) and %d ring(s)",
+ 16, 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 +304,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 & EIP197B) || (priv->version & EIP197D))
val |= EIP197_MST_CTRL_TX_MAX_CMD(5);
writel(val, EIP197_HIA_AIC(priv) + EIP197_HIA_MST_CTRL);
@@ -330,11 +330,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 & EIP197B) || (priv->version & EIP197D))
/* 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 +356,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 & EIP197B) || (priv->version & EIP197D))
/* 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 */
@@ -384,7 +382,7 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
/* FIXME: instability issues can occur for EIP97 but disabling it impact
* performances.
*/
- if (priv->version == EIP197B || priv->version == EIP197D)
+ if ((priv->version & EIP197B) || (priv->version & EIP197D))
val |= EIP197_HIA_DSE_CFG_EN_SINGLE_WR;
writel(val, EIP197_HIA_DSE(priv) + EIP197_HIA_DSE_CFG(pe));
@@ -479,7 +477,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 & EIP197B) || (priv->version & EIP197D)) {
eip197_trc_cache_init(priv);
ret = eip197_load_firmwares(priv);
@@ -813,23 +811,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 (irq < 0) {
- dev_err(&pdev->dev, "unable to get IRQ '%s'\n", name);
- return irq;
+ 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)",
+ irqid, irq);
+ return irq;
+ }
+ } else {
+ struct platform_device *plf_pdev = pdev;
+ char irq_name[6] = {0}; /* "ringX\0" */
+
+ 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'\n (err %d)",
+ 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;
}
@@ -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)
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) {
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,139 +979,131 @@ 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 & EIP197B) || (priv->version & EIP197D))
+ 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 & DEVICE_IS_PCI)) {
+ /*
+ * 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");
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;
+ dev_err(dev, "Failed to allocate ring memory");
+ 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");
+ 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;
+ dev_err(dev, "Failed to allocate RDR async request queue for ring %d",
+ i);
+ return -ENOMEM;
}
ring_irq = devm_kzalloc(dev, sizeof(*ring_irq), GFP_KERNEL);
if (!ring_irq) {
- ret = -ENOMEM;
- goto err_reg_clk;
+ dev_err(dev, "Failed to allocate IRQ data for ring %d",
+ i);
+ 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, 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", 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);
+ priv->ring[i].workqueue =
+ create_singlethread_workqueue(wq_name);
if (!priv->ring[i].workqueue) {
- ret = -ENOMEM;
- goto err_reg_clk;
+ dev_err(dev, "Failed to create work queue for ring %d",
+ i);
+ return -ENOMEM;
}
-
priv->ring[i].requests = 0;
priv->ring[i].busy = false;
-
crypto_init_queue(&priv->ring[i].queue,
EIP197_DEFAULT_RING_SIZE);
@@ -1117,22 +1111,88 @@ 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, "EIP(1)97 h/w init failed (%d)", ret);
+ return ret;
}
ret = safexcel_register_algorithms(priv);
if (ret) {
- dev_err(dev, "Failed to register algorithms (%d)\n", ret);
- goto err_reg_clk;
+ dev_err(dev, "Failed to register algorithms (%d)", ret);
+ return ret;
}
return 0;
+}
+
+/*
+ *
+ * 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);
@@ -1189,13 +1249,12 @@ static int safexcel_remove(struct platform_device *pdev)
.compatible = "inside-secure,safexcel-eip197d",
.data = (void *)EIP197D,
},
+ /* For backward compatibiliry and intended for generic use */
{
- /* Deprecated. Kept for backward compatibility. */
.compatible = "inside-secure,safexcel-eip97",
.data = (void *)EIP97IES,
},
{
- /* Deprecated. Kept for backward compatibility. */
.compatible = "inside-secure,safexcel-eip197",
.data = (void *)EIP197B,
},
@@ -1211,10 +1270,176 @@ static int safexcel_remove(struct platform_device *pdev)
.of_match_table = safexcel_of_match_table,
},
};
-module_platform_driver(crypto_safexcel);
+
+/*
+ *
+ * PCIE devices - i.e. Inside Secure development boards
+ *
+ */
+
+static int crypto_is_pci_probe(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ if (IS_ENABLED(CONFIG_PCI)) {
+ struct device *dev = &pdev->dev;
+ struct safexcel_crypto_priv *priv;
+ void __iomem *pciebase;
+ int rc;
+ u32 val;
+
+ dev_info(dev, "Probing PCIE device: vendor %04x, device %04x, subv %04x, subdev %04x, ctxt %lx",
+ ent->vendor, ent->device, ent->subvendor,
+ ent->subdevice, ent->driver_data);
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ dev_err(dev, "Failed to allocate memory");
+ 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, "pci_enable_device() failed");
+ return rc;
+ }
+
+ /* take ownership of PCI BAR0 */
+ rc = pcim_iomap_regions(pdev, 1, "crypto_safexcel");
+ if (rc) {
+ dev_err(dev, "pcim_iomap_regions() failed for BAR0");
+ return rc;
+ }
+ priv->base = pcim_iomap_table(pdev)[0];
+
+ if (priv->version & XILINX_PCIE) {
+ dev_info(dev, "Device identified as FPGA based development board - applying HW reset");
+
+ rc = pcim_iomap_regions(pdev, 4, "crypto_safexcel");
+ if (rc) {
+ dev_err(dev, "pcim_iomap_regions() failed for BAR4");
+ return rc;
+ }
+
+ pciebase = pcim_iomap_table(pdev)[2];
+ val = readl(pciebase + XILINX_IRQ_BLOCK_ID);
+ if ((val >> 16) == 0x1fc2) {
+ dev_info(dev, "Detected Xilinx PCIE IRQ block version %d, multiple MSI support enabled",
+ (val & 0xff));
+
+ /* Setup MSI identity map mapping */
+ writel(0x03020100,
+ pciebase + XILINX_USER_VECT_LUT0);
+ writel(0x07060504,
+ pciebase + XILINX_USER_VECT_LUT1);
+ writel(0x0b0a0908,
+ pciebase + XILINX_USER_VECT_LUT2);
+ writel(0x0f0e0d0c,
+ pciebase + XILINX_USER_VECT_LUT3);
+
+ /* Enable all device interrupts */
+ writel(GENMASK(31, 0),
+ pciebase + XILINX_USER_INT_ENB_MSK);
+ } else {
+ dev_err(dev, "Unrecognised IRQ block identifier %x",
+ val);
+ return -ENODEV;
+ }
+
+ /* HW reset FPGA dev board */
+ // assert reset
+ writel(1, priv->base + XILINX_GPIO_BASE);
+ wmb(); /* maintain strict ordering for accesses here */
+ // deassert reset
+ writel(0, priv->base + XILINX_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);
+ if (rc)
+ return rc;
+ }
+ return 0;
+}
+
+void crypto_is_pci_remove(struct pci_dev *pdev)
+{
+ if (IS_ENABLED(CONFIG_PCI)) {
+ 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[] = {
+ {
+ .vendor = 0x10ee,
+ .device = 0x9038,
+ .subvendor = 0x16ae,
+ .subdevice = 0xc522,
+ .class = 0x00000000,
+ .class_mask = 0x00000000,
+ // assume EIP197B for now
+ .driver_data = XILINX_PCIE | DEVICE_IS_PCI | EIP197B,
+ },
+ {},
+};
+
+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,
+};
+
+static int __init crypto_is_init(void)
+{
+ int rc;
+ /* Register platform driver */
+ platform_driver_register(&crypto_safexcel);
+
+ if (IS_ENABLED(CONFIG_PCI)) {
+ /* Register PCI driver */
+ rc = pci_register_driver(&crypto_is_pci_driver);
+ }
+
+ return 0;
+}
+
+static void __exit crypto_is_exit(void)
+{
+ /* Unregister platform driver */
+ platform_driver_unregister(&crypto_safexcel);
+
+ if (IS_ENABLED(CONFIG_PCI)) {
+ /* Unregister PCI driver if successfully registered before */
+ pci_unregister_driver(&crypto_is_pci_driver);
+ }
+}
+
+module_init(crypto_is_init);
+module_exit(crypto_is_exit);
MODULE_AUTHOR("Antoine Tenart <[email protected]>");
MODULE_AUTHOR("Ofer Heifetz <[email protected]>");
MODULE_AUTHOR("Igal Liberman <[email protected]>");
-MODULE_DESCRIPTION("Support for SafeXcel cryptographic engine EIP197");
+MODULE_AUTHOR("Pascal van Leeuwen <[email protected]>");
+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..924270e 100644
--- a/drivers/crypto/inside-secure/safexcel.h
+++ b/drivers/crypto/inside-secure/safexcel.h
@@ -38,6 +38,19 @@
char __##name##_desc[size] CRYPTO_MINALIGN_ATTR; \
struct type##_request *name = (void *)__##name##_desc
+/* Xilinx dev board base offsets */
+#define XILINX_GPIO_BASE 0x200000
+#define XILINX_IRQ_BLOCK_ID 0x2000
+#define XILINX_USER_INT_ENB_MSK 0x2004
+#define XILINX_USER_INT_ENB_SET 0x2008
+#define XILINX_USER_INT_ENB_CLEAR 0x200c
+#define XILINX_USER_INT_BLOCK 0x2040
+#define XILINX_USER_INT_PEND 0x2048
+#define XILINX_USER_VECT_LUT0 0x2080
+#define XILINX_USER_VECT_LUT1 0x2084
+#define XILINX_USER_VECT_LUT2 0x2088
+#define XILINX_USER_VECT_LUT3 0x208c
+
/* 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 +594,14 @@ struct safexcel_ring {
struct crypto_async_request *backlog;
};
+/* EIP integration context flags */
enum safexcel_eip_version {
+ /* for Marvell Armada BW compatibility */
EIP97IES = BIT(0),
EIP197B = BIT(1),
EIP197D = BIT(2),
+ XILINX_PCIE = BIT(3),
+ DEVICE_IS_PCI = BIT(4),
};
struct safexcel_register_offsets {
--
1.8.3.1
Hi Pascal,
Thanks for the patch :)
On Tue, Jun 18, 2019 at 07:56:23AM +0200, Pascal van Leeuwen wrote:
>
> /* Fallback to the old firmware location for the
> @@ -294,6 +291,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
>
> + dev_info(priv->dev, "EIP(1)97 HW init: burst size %d beats, using %d pipe(s) and %d ring(s)",
> + 16, priv->config.pes, priv->config.rings);
Adding custom messages in the kernel log has to be done carefully.
Although it's not considered stable it could be difficult to rework
later on. Also, if all driver were to print custom messages the log
would be very hard to read. But you can also argue that a single message
when probing a driver is also done in other drivers.
For this one particularly, the probe could fail later on. So if we were
to add this output, it should be done at the very end of the probe.
> - if (priv->version == EIP197B || priv->version == EIP197D) {
> + if ((priv->version & EIP197B) || (priv->version & EIP197D)) {
You could also use "version & (EIP197B | EIP197D)" (there are other
examples of this).
> -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,
You could probably use the DEVICE_IS_PCI flag instead.
> irq_handler_t handler,
> irq_handler_t threaded_handler,
> struct safexcel_ring_irq_data *ring_irq_priv)
> {
> + dev_err(dev, "unable to get IRQ '%s'\n (err %d)",
> + irq_name, irq);
I think there's a typo here, the \n should be at the end of the string.
> +static int safexcel_probe_generic(void *pdev,
> + struct safexcel_crypto_priv *priv,
> + int is_pci_dev)
> {
> + if (IS_ENABLED(CONFIG_PCI) && (priv->version & DEVICE_IS_PCI)) {
DEVICE_IS_PCI should be set in ->flags, not ->version.
> + /*
> + * 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);
You need a space before and after the | here.
> + if (ret < 0) {
> + dev_err(dev, "Failed to allocate PCI MSI interrupts");
Do not forget the \n at the end of the string.
> + if (ret) {
> + dev_err(dev, "Failed to initialize rings");
Also here, and probably in other places :)
> - snprintf(irq_name, 6, "ring%d", i);
> - irq = safexcel_request_ring_irq(pdev, irq_name, safexcel_irq_ring,
> + irq = safexcel_request_ring_irq(pdev, i + is_pci_dev,
This 'i + is_pci_dev' is hard to read and understand. I would suggest to
use a define, or an helper to get the real irq given if a device is
probed using PCI or not. Something like "safexcel_irq_number(i, priv)"
> + dev_err(dev, "Failed to create work queue for ring %d",
> + i);
> + return -ENOMEM;
> }
> -
Why removing this one blank line?
> priv->ring[i].requests = 0;
> priv->ring[i].busy = false;
> -
Ditto.
> + dev_err(dev, "EIP(1)97 h/w init failed (%d)", ret);
Adding a version information in the log means all the outputs will have
to be updated if we ever support an h/w with a different name.
> +/*
> + *
> + * for Device Tree platform driver
> + *
> + */
Please follow the comment styles (you can remove set in on a single line
here, or at least remove the blank lines). (There are other examples).
> @@ -1189,13 +1249,12 @@ static int safexcel_remove(struct platform_device *pdev)
> .compatible = "inside-secure,safexcel-eip197d",
> .data = (void *)EIP197D,
> },
> + /* For backward compatibiliry and intended for generic use */
> {
> - /* Deprecated. Kept for backward compatibility. */
> .compatible = "inside-secure,safexcel-eip97",
> .data = (void *)EIP97IES,
> },
> {
> - /* Deprecated. Kept for backward compatibility. */
> .compatible = "inside-secure,safexcel-eip197",
> .data = (void *)EIP197B,
> },
I'm not sure about this. The compatible should describe what the
hardware is, and the driver can then decide if it has special things to
do or not. It is not used to configure the driver to be used with a
generic use or not.
Do you have a practical reason to do this?
> +static int crypto_is_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
The alignment is wrong here.
> +{
> + if (IS_ENABLED(CONFIG_PCI)) {
Can this be called with CONFIG_PCI disabled?
> + dev_info(dev, "Probing PCIE device: vendor %04x, device %04x, subv %04x, subdev %04x, ctxt %lx",
> + ent->vendor, ent->device, ent->subvendor,
> + ent->subdevice, ent->driver_data);
Please drop this new message. If the userspace needs it, I believe there
are clean ways to get it.
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + dev_err(dev, "Failed to allocate memory");
> + return -ENOMEM;
No need to have a debug message when returning ENOMEM.
> + /* enable the device */
> + rc = pcim_enable_device(pdev);
> + if (rc) {
> + dev_err(dev, "pci_enable_device() failed");
Please use error messages describing the issue rather than printing the
function names. (There are other examples).
> + if (priv->version & XILINX_PCIE) {
> + dev_info(dev, "Device identified as FPGA based development board - applying HW reset");
> +
> + rc = pcim_iomap_regions(pdev, 4, "crypto_safexcel");
> + if (rc) {
> + dev_err(dev, "pcim_iomap_regions() failed for BAR4");
> + return rc;
> + }
> +
> + pciebase = pcim_iomap_table(pdev)[2];
> + val = readl(pciebase + XILINX_IRQ_BLOCK_ID);
> + if ((val >> 16) == 0x1fc2) {
Try not to use magic values, use defines with a meaningful name.
> + dev_info(dev, "Detected Xilinx PCIE IRQ block version %d, multiple MSI support enabled",
> + (val & 0xff));
Same comments as the other dev_info().
> +
> + /* Setup MSI identity map mapping */
> + writel(0x03020100,
> + pciebase + XILINX_USER_VECT_LUT0);
> + writel(0x07060504,
> + pciebase + XILINX_USER_VECT_LUT1);
> + writel(0x0b0a0908,
> + pciebase + XILINX_USER_VECT_LUT2);
> + writel(0x0f0e0d0c,
> + pciebase + XILINX_USER_VECT_LUT3);
Use defines here.
> + /* HW reset FPGA dev board */
> + // assert reset
> + writel(1, priv->base + XILINX_GPIO_BASE);
> + wmb(); /* maintain strict ordering for accesses here */
> + // deassert reset
> + writel(0, priv->base + XILINX_GPIO_BASE);
> + wmb(); /* maintain strict ordering for accesses here */
It seems the driver here access to a GPIO controller, to assert and
de-assert reset, which is outside the crypto engine i/o range. If true,
this is not acceptable in the upstream kernel: those GPIO or reset
should be accessed through the dedicated in-kernel API and be
implemented in separate drivers (maybe a reset driver here).
> +void crypto_is_pci_remove(struct pci_dev *pdev)
> +{
> + if (IS_ENABLED(CONFIG_PCI)) {
Can this be accessed with CONFIG_PCI disabled?
> +static const struct pci_device_id crypto_is_pci_ids[] = {
> + {
> + .vendor = 0x10ee,
> + .device = 0x9038,
> + .subvendor = 0x16ae,
> + .subdevice = 0xc522,
> + .class = 0x00000000,
> + .class_mask = 0x00000000,
You should use proper defines here, and define the vendor ID in a
generic way in the kernel. You can look for the use of PCI_DEVICE and
PCI_DEVICE_DATA as a starting point.
> + // assume EIP197B for now
> + .driver_data = XILINX_PCIE | DEVICE_IS_PCI | EIP197B,
We do use the data (here and for dt compatibles) to store a flag about
the version. I think XILINX_PCIE and DEVICE_IS_PCI should be flags, and
you should be able to set them using pci_dev->vendor_id (or other ids).
Also, you should prefix XILINX_PCIE and DEVICE_IS_PCI with an "unique"
name, which should be driver-specific to avoid colliding with other
possible global definitions. In this driver we do use EIP197_ and
SAFEXCEL_ a lot for historical reasons, you can pick one :)
> +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,
> +};
More generally, you should protect all the PCI specific functions and
definitions between #ifdef.
> +MODULE_AUTHOR("Pascal van Leeuwen <[email protected]>");
I think this is usually done in a separate patch, when one has made
numerous contributions to a driver.
I tested it on my hardware and the boot tests passed nicely, so it seems
there were no regressions (I'll make more tests though).
Thanks!
Antoine
--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Pascal,
On Tue, Jun 18, 2019 at 07:56:24AM +0200, Pascal van Leeuwen wrote:
>
> static int eip197_load_firmwares(struct safexcel_crypto_priv *priv)
> {
> + /*
> + * The embedded one-size-fits-all MiniFW is just for handling TR
> + * prefetch & invalidate. It does not support any FW flows, effectively
> + * turning the EIP197 into a glorified EIP97
> + */
> + const u32 ipue_minifw[] = {
> + 0x24808200, 0x2D008204, 0x2680E208, 0x2780E20C,
> + 0x2200F7FF, 0x38347000, 0x2300F000, 0x15200A80,
> + 0x01699003, 0x60038011, 0x38B57000, 0x0119F04C,
> + 0x01198548, 0x20E64000, 0x20E75000, 0x1E200000,
> + 0x30E11000, 0x103A93FF, 0x60830014, 0x5B8B0000,
> + 0xC0389000, 0x600B0018, 0x2300F000, 0x60800011,
> + 0x90800000, 0x10000000, 0x10000000};
> + const u32 ifpp_minifw[] = {
> + 0x21008000, 0x260087FC, 0xF01CE4C0, 0x60830006,
> + 0x530E0000, 0x90800000, 0x23008004, 0x24808008,
> + 0x2580800C, 0x0D300000, 0x205577FC, 0x30D42000,
> + 0x20DAA7FC, 0x43107000, 0x42220004, 0x00000000,
> + 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> + 0x00060004, 0x20337004, 0x90800000, 0x10000000,
> + 0x10000000};
What is the license of this firmware? With this patch, it would be
shipped with Linux kernel images and this question is then very
important.
In addition to this, the direction the kernel has taken was to *remove*
binary firmwares from its source code. I'm afraid adding this is a
no-go.
The proper solution I believe would be to support loading this "MiniFW",
which (depending on the license) could be either distributed in the
rootfs and loaded (like what's done currently), or through
CONFIG_EXTRA_FIRMWARE.
This should be discussed first before discussing the implementation of
this particular patch.
Thanks!
Antoine
--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Pascal,
On Tue, Jun 18, 2019 at 07:56:22AM +0200, Pascal van Leeuwen wrote:
> 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 <[email protected]>
This looks good to me, thanks!
Antoine
> ---
> 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
>
--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Pascal,
On Wed, Jun 19, 2019 at 02:15:02PM +0200, Antoine Tenart wrote:
> On Tue, Jun 18, 2019 at 07:56:23AM +0200, Pascal van Leeuwen wrote:
>
> More generally, you should protect all the PCI specific functions and
> definitions between #ifdef.
And I think there are compilation issues if CONFIG_PCI is disabled with
this patch. This approach would probably fix it.
Thanks!
Antoine
--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Antoine,
Thanks for reviewing.
> -----Original Message-----
> From: Antoine Tenart <[email protected]>
> Sent: Wednesday, June 19, 2019 2:15 PM
> To: Pascal van Leeuwen <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Pascal Van Leeuwen <[email protected]>
> Subject: Re: [PATCH 2/3] crypto: inside-secure - add support for PCI based FPGA
> development board
>
> Hi Pascal,
>
> Thanks for the patch :)
>
> On Tue, Jun 18, 2019 at 07:56:23AM +0200, Pascal van Leeuwen wrote:
> >
> > /* Fallback to the old firmware location for the
> > @@ -294,6 +291,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
> >
> > + dev_info(priv->dev, "EIP(1)97 HW init: burst size %d beats, using %d pipe(s) and %d
> ring(s)",
> > + 16, priv->config.pes, priv->config.rings);
>
> Adding custom messages in the kernel log has to be done carefully.
> Although it's not considered stable it could be difficult to rework
> later on. Also, if all driver were to print custom messages the log
> would be very hard to read. But you can also argue that a single message
> when probing a driver is also done in other drivers.
>
Hmm ... don't know what the rules for logging are exactly, but from my
perspective, I'm dealing with a zillion different HW configurations so
some feedback whether the driver detected the *correct* HW parameters -
or actually, whether I stuffed the correct image into my FPGA :o) - is
very convenient to have. And not just for my local development, but also
to debug deployments in the field at customer sites.
Also, looking at my log, there's other drivers that are similarly
(or even more) verbose.
If it's a really considered a problem, I could make it dev_dbg?
Downside of that is I will need to educate my customers into
building debug kernels just to get some basic support feedback ...
> For this one particularly, the probe could fail later on. So if we were
> to add this output, it should be done at the very end of the probe.
>
I'm in doubt about this one. I understand that you want to reduce the
logging in that case, but at the same time that message can convey
information as to WHY the probing fails later on ...
i.e. if it detects, say, 4 pipes on a device that, in fact, only has
2, then that may be the very reason for the FW init to fail later on.
Note that you can only get this message if the EIP(1)97 hardware has
indeed been found. You won't get it if the hardware does not exist.
> > - if (priv->version == EIP197B || priv->version == EIP197D) {
> > + if ((priv->version & EIP197B) || (priv->version & EIP197D)) {
>
> You could also use "version & (EIP197B | EIP197D)" (there are other
> examples of this).
>
Yes, I will fix that. I just lazily changed the == into & :-)
> > -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,
>
> You could probably use the DEVICE_IS_PCI flag instead.
>
I'm not currently passing priv to that function, so I can't access
priv->version directly. I could pass a priv pointer instead and use
the flag, but passing a bool constant seemed to be more efficient?
> > irq_handler_t handler,
> > irq_handler_t threaded_handler,
> > struct safexcel_ring_irq_data *ring_irq_priv)
> > {
>
> > + dev_err(dev, "unable to get IRQ '%s'\n (err %d)",
> > + irq_name, irq);
>
> I think there's a typo here, the \n should be at the end of the string.
>
Yes, will fix, thanks.
> > +static int safexcel_probe_generic(void *pdev,
> > + struct safexcel_crypto_priv *priv,
> > + int is_pci_dev)
> > {
>
> > + if (IS_ENABLED(CONFIG_PCI) && (priv->version & DEVICE_IS_PCI)) {
>
> DEVICE_IS_PCI should be set in ->flags, not ->version.
>
As far as I could tell from the existing code, "version" was
a highlevel indication of the device *type* (EIP97/EIP197B/D)
while flags was more about low-level *features*.
So in my humble opinion, version was the correct location, it
is just a confusing name. (i.e. you can have many *versions*
of an EIP197B, for instance ...)
> > + /*
> > + * 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);
>
> You need a space before and after the | here.
>
Ok, will add (checkpatch did not complain though)
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to allocate PCI MSI interrupts");
>
> Do not forget the \n at the end of the string.
>
Actually, I removed all "\n"'s from my messages as I
understood they are not needed? If they are really needed,
I can add them to all log strings again ... anyone else?
> > + if (ret) {
> > + dev_err(dev, "Failed to initialize rings");
>
> Also here, and probably in other places :)
>
> > - snprintf(irq_name, 6, "ring%d", i);
> > - irq = safexcel_request_ring_irq(pdev, irq_name, safexcel_irq_ring,
> > + irq = safexcel_request_ring_irq(pdev, i + is_pci_dev,
>
> This 'i + is_pci_dev' is hard to read and understand. I would suggest to
> use a define, or an helper to get the real irq given if a device is
> probed using PCI or not. Something like "safexcel_irq_number(i, priv)"
>
OK, I can do that.
> > + dev_err(dev, "Failed to create work queue for ring %d",
> > + i);
> > + return -ENOMEM;
> > }
> > -
>
> Why removing this one blank line?
>
Don't know, I can add it back
> > priv->ring[i].requests = 0;
> > priv->ring[i].busy = false;
> > -
>
> Ditto.
>
Same
> > + dev_err(dev, "EIP(1)97 h/w init failed (%d)", ret);
>
> Adding a version information in the log means all the outputs will have
> to be updated if we ever support an h/w with a different name.
>
Fair enough, then I'll remove all EIP references
> > +/*
> > + *
> > + * for Device Tree platform driver
> > + *
> > + */
>
> Please follow the comment styles (you can remove set in on a single line
> here, or at least remove the blank lines). (There are other examples).
>
Ok, I'll fix that
> > @@ -1189,13 +1249,12 @@ static int safexcel_remove(struct platform_device *pdev)
> > .compatible = "inside-secure,safexcel-eip197d",
> > .data = (void *)EIP197D,
> > },
> > + /* For backward compatibiliry and intended for generic use */
> > {
> > - /* Deprecated. Kept for backward compatibility. */
> > .compatible = "inside-secure,safexcel-eip97",
> > .data = (void *)EIP97IES,
> > },
> > {
> > - /* Deprecated. Kept for backward compatibility. */
> > .compatible = "inside-secure,safexcel-eip197",
> > .data = (void *)EIP197B,
> > },
>
> I'm not sure about this. The compatible should describe what the
> hardware is, and the driver can then decide if it has special things to
> do or not. It is not used to configure the driver to be used with a
> generic use or not.
>
> Do you have a practical reason to do this?
>
I have to admit I don't fully understand how these compatible
strings work. All I wanted to achieve is provide some generic
device tree entry to point to this driver, to be used for
devices other than Marvell. No need to convey b/d that way
(or even eip97/197, for that matter) as that can all be probed.
>
> > +static int crypto_is_pci_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *ent)
>
> The alignment is wrong here.
>
Will fix
> > +{
> > + if (IS_ENABLED(CONFIG_PCI)) {
>
> Can this be called with CONFIG_PCI disabled?
>
I would expect not, but I suppose I was fearing it wouldn't compile
with CONFIG_PCI disabled. Since I only have PCI based systems, I
can't really try this.
> > + dev_info(dev, "Probing PCIE device: vendor %04x, device %04x, subv %04x, subdev
> %04x, ctxt %lx",
> > + ent->vendor, ent->device, ent->subvendor,
> > + ent->subdevice, ent->driver_data);
>
> Please drop this new message. If the userspace needs it, I believe there
> are clean ways to get it.
>
It's not for userspace ...
This is just information to see whether the HW is correctly
detected. Very useful for debugging and providing support.
Again, I can make it a dev_dbg, but I really DO need this.
> > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > + if (!priv) {
> > + dev_err(dev, "Failed to allocate memory");
> > + return -ENOMEM;
>
> No need to have a debug message when returning ENOMEM.
>
OK, will remove
>
> > + /* enable the device */
> > + rc = pcim_enable_device(pdev);
> > + if (rc) {
> > + dev_err(dev, "pci_enable_device() failed");
>
> Please use error messages describing the issue rather than printing the
> function names. (There are other examples).
>
But ... that IS the issue, isn't it? Well, apart from the actual
return code. What message text would you suggest?
> > + if (priv->version & XILINX_PCIE) {
> > + dev_info(dev, "Device identified as FPGA based development board -
> applying HW reset");
> > +
> > + rc = pcim_iomap_regions(pdev, 4, "crypto_safexcel");
> > + if (rc) {
> > + dev_err(dev, "pcim_iomap_regions() failed for BAR4");
> > + return rc;
> > + }
> > +
> > + pciebase = pcim_iomap_table(pdev)[2];
> > + val = readl(pciebase + XILINX_IRQ_BLOCK_ID);
> > + if ((val >> 16) == 0x1fc2) {
>
> Try not to use magic values, use defines with a meaningful name.
>
I can make it a define
> > + dev_info(dev, "Detected Xilinx PCIE IRQ block version %d, multiple
> MSI support enabled",
> > + (val & 0xff));
>
> Same comments as the other dev_info().
>
And same response from my side. When working with lots of different
HW, some of which only half-functional, such logging is really crucial.
> > +
> > + /* Setup MSI identity map mapping */
> > + writel(0x03020100,
> > + pciebase + XILINX_USER_VECT_LUT0);
> > + writel(0x07060504,
> > + pciebase + XILINX_USER_VECT_LUT1);
> > + writel(0x0b0a0908,
> > + pciebase + XILINX_USER_VECT_LUT2);
> > + writel(0x0f0e0d0c,
> > + pciebase + XILINX_USER_VECT_LUT3);
>
> Use defines here.
>
OK
> > + /* HW reset FPGA dev board */
> > + // assert reset
> > + writel(1, priv->base + XILINX_GPIO_BASE);
> > + wmb(); /* maintain strict ordering for accesses here */
> > + // deassert reset
> > + writel(0, priv->base + XILINX_GPIO_BASE);
> > + wmb(); /* maintain strict ordering for accesses here */
>
> It seems the driver here access to a GPIO controller, to assert and
> de-assert reset, which is outside the crypto engine i/o range. If true,
> this is not acceptable in the upstream kernel: those GPIO or reset
> should be accessed through the dedicated in-kernel API and be
> implemented in separate drivers (maybe a reset driver here).
>
Hmmm ... this is some *local* GPIO controller *inside*
the FPGA device (i.e. doesn't even leave the silicon die).
It's inside the range of the (single!) PCIE device, it's not
a seperate device and thus cannot be managed as such ...
Effectively, it's just an MMIO mapped register no different
from any other slave accessible register.
If I had given it a less explicit name, nobody would've cared.
> > +void crypto_is_pci_remove(struct pci_dev *pdev)
> > +{
> > + if (IS_ENABLED(CONFIG_PCI)) {
>
> Can this be accessed with CONFIG_PCI disabled?
>
Again, probably not, but will it compile without this?
> > +static const struct pci_device_id crypto_is_pci_ids[] = {
> > + {
> > + .vendor = 0x10ee,
> > + .device = 0x9038,
> > + .subvendor = 0x16ae,
> > + .subdevice = 0xc522,
> > + .class = 0x00000000,
> > + .class_mask = 0x00000000,
>
> You should use proper defines here, and define the vendor ID in a
> generic way in the kernel. You can look for the use of PCI_DEVICE and
> PCI_DEVICE_DATA as a starting point.
>
Ok, I will look into that
> > + // assume EIP197B for now
> > + .driver_data = XILINX_PCIE | DEVICE_IS_PCI | EIP197B,
>
> We do use the data (here and for dt compatibles) to store a flag about
> the version. I think XILINX_PCIE and DEVICE_IS_PCI should be flags, and
> you should be able to set them using pci_dev->vendor_id (or other ids).
>
But why would I want to do that as this is more convenient?
Basically, the "version" (wrong name, if you ask me) field was used
to identify the device type, so I just extended that to more types.
> Also, you should prefix XILINX_PCIE and DEVICE_IS_PCI with an "unique"
> name, which should be driver-specific to avoid colliding with other
> possible global definitions. In this driver we do use EIP197_ and
> SAFEXCEL_ a lot for historical reasons, you can pick one :)
>
Ok, will do
> > +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,
> > +};
>
> More generally, you should protect all the PCI specific functions and
> definitions between #ifdef.
>
I asked the mailing list and the answer was I should NOT use #ifdef,
but instead use IS_ENABLED to *only* remove relevant function bodies.
Which is exactly what I did (or tried to do, anyway).
> > +MODULE_AUTHOR("Pascal van Leeuwen <[email protected]>");
>
> I think this is usually done in a separate patch, when one has made
> numerous contributions to a driver.
>
OK, I can remove that
> I tested it on my hardware and the boot tests passed nicely, so it seems
> there were no regressions (I'll make more tests though).
>
> Thanks!
> Antoine
>
> --
> Antoine T?nart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
http://www.insidesecure.com
Thanks for the comments!
> -----Original Message-----
> From: Antoine Tenart <[email protected]>
> Sent: Wednesday, June 19, 2019 2:28 PM
> To: Pascal van Leeuwen <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Pascal Van Leeuwen <[email protected]>
> Subject: Re: [PATCH 3/3] crypto: inside-secure - add support for using the EIP197 without
> firmware images
>
> Hi Pascal,
>
> On Tue, Jun 18, 2019 at 07:56:24AM +0200, Pascal van Leeuwen wrote:
> >
> > static int eip197_load_firmwares(struct safexcel_crypto_priv *priv)
> > {
> > + /*
> > + * The embedded one-size-fits-all MiniFW is just for handling TR
> > + * prefetch & invalidate. It does not support any FW flows, effectively
> > + * turning the EIP197 into a glorified EIP97
> > + */
> > + const u32 ipue_minifw[] = {
> > + 0x24808200, 0x2D008204, 0x2680E208, 0x2780E20C,
> > + 0x2200F7FF, 0x38347000, 0x2300F000, 0x15200A80,
> > + 0x01699003, 0x60038011, 0x38B57000, 0x0119F04C,
> > + 0x01198548, 0x20E64000, 0x20E75000, 0x1E200000,
> > + 0x30E11000, 0x103A93FF, 0x60830014, 0x5B8B0000,
> > + 0xC0389000, 0x600B0018, 0x2300F000, 0x60800011,
> > + 0x90800000, 0x10000000, 0x10000000};
> > + const u32 ifpp_minifw[] = {
> > + 0x21008000, 0x260087FC, 0xF01CE4C0, 0x60830006,
> > + 0x530E0000, 0x90800000, 0x23008004, 0x24808008,
> > + 0x2580800C, 0x0D300000, 0x205577FC, 0x30D42000,
> > + 0x20DAA7FC, 0x43107000, 0x42220004, 0x00000000,
> > + 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > + 0x00060004, 0x20337004, 0x90800000, 0x10000000,
> > + 0x10000000};
>
> What is the license of this firmware? With this patch, it would be
> shipped with Linux kernel images and this question is then very
> important.
>
I am very well aware of that. The driver under GPL 2.0 so that simply
would also apply to this. This was written by me specifically for this
particular driver anyway and my company allows me to GPL this stuff.
> In addition to this, the direction the kernel has taken was to *remove*
> binary firmwares from its source code. I'm afraid adding this is a
> no-go.
>
Actually, I would like to argue against that here.
For a HW engineer, there really is no fundamental difference between
control register contents or an instruction word. They can both have
the exact same effects internal to the HW.
If I had disguised this as a handful of config reg writes writing
some #define'd magic values, probably no one would have even noticed.
This is not firmware for some general purpose CPU, it is firmware
for some very dedicated microengine thingy controlling some local
datapath. By that same definition, the tokens the driver generates
for processing could be considered "firmware" as well (as they are
used by the hardware in a very similar way) ...
> The proper solution I believe would be to support loading this "MiniFW",
> which (depending on the license) could be either distributed in the
> rootfs and loaded (like what's done currently), or through
> CONFIG_EXTRA_FIRMWARE.
>
That seems total overkill for just a handful of words though.
But I can do that if the community insists ...
> This should be discussed first before discussing the implementation of
> this particular patch.
>
> Thanks!
> Antoine
>
> --
> Antoine T?nart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
http://www.insidesecure.com
Hi Pascal,
On Wed, Jun 19, 2019 at 02:22:19PM +0000, Pascal Van Leeuwen wrote:
> > From: Antoine Tenart <[email protected]>
> > On Tue, Jun 18, 2019 at 07:56:23AM +0200, Pascal van Leeuwen wrote:
> > >
> > > /* Fallback to the old firmware location for the
> > > @@ -294,6 +291,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
> > >
> > > + dev_info(priv->dev, "EIP(1)97 HW init: burst size %d beats, using %d pipe(s) and %d
> > ring(s)",
> > > + 16, priv->config.pes, priv->config.rings);
> >
> > Adding custom messages in the kernel log has to be done carefully.
> > Although it's not considered stable it could be difficult to rework
> > later on. Also, if all driver were to print custom messages the log
> > would be very hard to read. But you can also argue that a single message
> > when probing a driver is also done in other drivers.
> >
> Hmm ... don't know what the rules for logging are exactly, but from my
> perspective, I'm dealing with a zillion different HW configurations so
> some feedback whether the driver detected the *correct* HW parameters -
> or actually, whether I stuffed the correct image into my FPGA :o) - is
> very convenient to have. And not just for my local development, but also
> to debug deployments in the field at customer sites.
I understand it can be convenient, it's just a matter of having a
logging message for you that will end up in many builds for many users.
They do not necessarily have the same needs. So it's a matter of
compromise, one or two messages at boot can be OK, more is likely to
become an issue.
> Also, looking at my log, there's other drivers that are similarly
> (or even more) verbose.
There are *always* other examples in the kernel :)
> If it's a really considered a problem, I could make it dev_dbg?
Sure. I would say adding one message at boot like this one seems OK. But
the others would need to be debugging messages.
> > For this one particularly, the probe could fail later on. So if we were
> > to add this output, it should be done at the very end of the probe.
> >
> I'm in doubt about this one. I understand that you want to reduce the
> logging in that case, but at the same time that message can convey
> information as to WHY the probing fails later on ...
If the drivers fails to probe, there will be other messages. In that
case is this one really needed? I'm not sure.
> i.e. if it detects, say, 4 pipes on a device that, in fact, only has
> 2, then that may be the very reason for the FW init to fail later on.
In case of failure you'll need anyway to debug and understand what's
going on. By adding new prints, or enabling debugging messages.
> > > -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,
> >
> > You could probably use the DEVICE_IS_PCI flag instead.
> >
> I'm not currently passing priv to that function, so I can't access
> priv->version directly. I could pass a priv pointer instead and use
> the flag, but passing a bool constant seemed to be more efficient?
That right, whatever you prefer then.
> > > +static int safexcel_probe_generic(void *pdev,
> > > + struct safexcel_crypto_priv *priv,
> > > + int is_pci_dev)
> > > {
> >
> > > + if (IS_ENABLED(CONFIG_PCI) && (priv->version & DEVICE_IS_PCI)) {
> >
> > DEVICE_IS_PCI should be set in ->flags, not ->version.
> >
> As far as I could tell from the existing code, "version" was
> a highlevel indication of the device *type* (EIP97/EIP197B/D)
> while flags was more about low-level *features*.
Using the same argument I would say the way the device is accessed is
very run-time dependent, and it has nothing to do with the version of
the h/w (the same engine could be accessed through different ways given
the platform).
Maybe the name 'feature' is too specific, I agree with you.
> So in my humble opinion, version was the correct location, it
> is just a confusing name. (i.e. you can have many *versions*
> of an EIP197B, for instance ...)
That would be an issue with the driver. We named the 'version' given the
knowledge we had of the h/w, it might not be specific enough. Or maybe
you can think of this as being a "family of engine versions". The idea
is the version is what the h/w is capable of, not how it's being
wired/accessed.
> > > + /*
> > > + * 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);
> >
> > You need a space before and after the | here.
> >
> Ok, will add (checkpatch did not complain though)
It may complain with --strict on this one. (Well, I'm sure many --strict
tests won't pass on the existing upstream code, no worries :)).
> > > + if (ret < 0) {
> > > + dev_err(dev, "Failed to allocate PCI MSI interrupts");
> >
> > Do not forget the \n at the end of the string.
> >
> Actually, I removed all "\n"'s from my messages as I
> understood they are not needed? If they are really needed,
> I can add them to all log strings again ... anyone else?
The simple answer is: everybody set the \n, so at least for consistency
we should have them.
> > > @@ -1189,13 +1249,12 @@ static int safexcel_remove(struct platform_device *pdev)
> > > .compatible = "inside-secure,safexcel-eip197d",
> > > .data = (void *)EIP197D,
> > > },
> > > + /* For backward compatibiliry and intended for generic use */
> > > {
> > > - /* Deprecated. Kept for backward compatibility. */
> > > .compatible = "inside-secure,safexcel-eip97",
> > > .data = (void *)EIP97IES,
> > > },
> > > {
> > > - /* Deprecated. Kept for backward compatibility. */
> > > .compatible = "inside-secure,safexcel-eip197",
> > > .data = (void *)EIP197B,
> > > },
> >
> > I'm not sure about this. The compatible should describe what the
> > hardware is, and the driver can then decide if it has special things to
> > do or not. It is not used to configure the driver to be used with a
> > generic use or not.
> >
> > Do you have a practical reason to do this?
>
> I have to admit I don't fully understand how these compatible
> strings work. All I wanted to achieve is provide some generic
> device tree entry to point to this driver, to be used for
> devices other than Marvell. No need to convey b/d that way
> (or even eip97/197, for that matter) as that can all be probed.
Compatibles are used in device trees, which intend to be a description
of the hardware (not the configuration of how the hardware should be
used). So we can't have a compatible being a restricted configuration
use of a given hardware. But I think here you're right, and there is
room for a more generic eip197 compatible: the b/d versions only have
few differences and are part of the same family, so we can have a very
specific compatible plus a "family" one. Something like:
compatible = "inside-secure,safexcel-eip197d", "inside-secure,safexcel-eip197";
This would need to be in a separated patch, and this should be
documented in:
Documentation/devicetree/bindings/crypto/inside-secure-safexcel.txt
> > > + /* enable the device */
> > > + rc = pcim_enable_device(pdev);
> > > + if (rc) {
> > > + dev_err(dev, "pci_enable_device() failed");
> >
> > Please use error messages describing the issue rather than printing the
> > function names. (There are other examples).
> >
> But ... that IS the issue, isn't it? Well, apart from the actual
> return code. What message text would you suggest?
Yes, it is the issue. We generally use a sentence instead of function
names (something alongside s/_/ / :)).
> > > + /* HW reset FPGA dev board */
> > > + // assert reset
> > > + writel(1, priv->base + XILINX_GPIO_BASE);
> > > + wmb(); /* maintain strict ordering for accesses here */
> > > + // deassert reset
> > > + writel(0, priv->base + XILINX_GPIO_BASE);
> > > + wmb(); /* maintain strict ordering for accesses here */
> >
> > It seems the driver here access to a GPIO controller, to assert and
> > de-assert reset, which is outside the crypto engine i/o range. If true,
> > this is not acceptable in the upstream kernel: those GPIO or reset
> > should be accessed through the dedicated in-kernel API and be
> > implemented in separate drivers (maybe a reset driver here).
> >
> Hmmm ... this is some *local* GPIO controller *inside*
> the FPGA device (i.e. doesn't even leave the silicon die).
> It's inside the range of the (single!) PCIE device, it's not
> a seperate device and thus cannot be managed as such ...
>
> Effectively, it's just an MMIO mapped register no different
> from any other slave accessible register.
> If I had given it a less explicit name, nobody would've cared.
OK, that makes sense. If the register accessed is within the register
bank of the crypto engine you can keep this.
> > > +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,
> > > +};
> >
> > More generally, you should protect all the PCI specific functions and
> > definitions between #ifdef.
> >
> I asked the mailing list and the answer was I should NOT use #ifdef,
> but instead use IS_ENABLED to *only* remove relevant function bodies.
> Which is exactly what I did (or tried to do, anyway).
My bad, I realise there's a mistake in my comment. That should have
been: you should protect all the PCI specific functions and definitions
with #if IS_ENABLED(...). When part of a function should be excluded
you can use if(IS_ENABLED(...)), but if the entire function can be left
out, #if is the way to go.
Thanks!
Antoine
--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Pascal,
On Wed, Jun 19, 2019 at 02:37:44PM +0000, Pascal Van Leeuwen wrote:
> > From: Antoine Tenart <[email protected]>
> > On Tue, Jun 18, 2019 at 07:56:24AM +0200, Pascal van Leeuwen wrote:
>
> > In addition to this, the direction the kernel has taken was to *remove*
> > binary firmwares from its source code. I'm afraid adding this is a
> > no-go.
>
> For a HW engineer, there really is no fundamental difference between
> control register contents or an instruction word. They can both have
> the exact same effects internal to the HW.
> If I had disguised this as a handful of config reg writes writing
> some #define'd magic values, probably no one would have even noticed.
I do not fully agree. If this is comparable to configuring h/w
registers, then you could probably have defines explaining why each bit
is set and what it's doing. Which would be fine.
> By that same definition, the tokens the driver generates for
> processing could be considered "firmware" as well (as they are used by
> the hardware in a very similar way) ...
Right. The main difference here is we do have a clear definition of what
the tokens are doing. Thanks to your explanation, if this firmware is
really looking like the token we're using, the words have a defined
structure and the magic values could be generated with proper defines
and macros. And I think it's the main issue here: it's not acceptable to
have an array of magic values. If you can give a meaning to those bits,
I see no reason why it couldn't be added to the driver.
(And I'm all for what you're trying to achieve here :)).
> > The proper solution I believe would be to support loading this "MiniFW",
> > which (depending on the license) could be either distributed in the
> > rootfs and loaded (like what's done currently), or through
> > CONFIG_EXTRA_FIRMWARE.
> >
> That seems total overkill for just a handful of words though.
Given your explanation, I agree. (If those bits can have meaning).
Thanks!
Antoine
--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
> -----Original Message-----
> From: Antoine Tenart <[email protected]>
> Sent: Thursday, June 20, 2019 3:06 PM
> To: Pascal Van Leeuwen <[email protected]>
> Cc: Antoine Tenart <[email protected]>; Pascal van Leeuwen <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/3] crypto: inside-secure - add support for PCI based FPGA development board
>
> Hi Pascal,
>
> On Wed, Jun 19, 2019 at 02:22:19PM +0000, Pascal Van Leeuwen wrote:
> > > From: Antoine Tenart <[email protected]>
> > > On Tue, Jun 18, 2019 at 07:56:23AM +0200, Pascal van Leeuwen wrote:
> > > >
> > > > /* Fallback to the old firmware location for the
> > > > @@ -294,6 +291,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
> > > >
> > > > + dev_info(priv->dev, "EIP(1)97 HW init: burst size %d beats, using %d pipe(s) and %d
> > > ring(s)",
> > > > + 16, priv->config.pes, priv->config.rings);
> > >
> > > Adding custom messages in the kernel log has to be done carefully.
> > > Although it's not considered stable it could be difficult to rework
> > > later on. Also, if all driver were to print custom messages the log
> > > would be very hard to read. But you can also argue that a single message
> > > when probing a driver is also done in other drivers.
> > >
> > Hmm ... don't know what the rules for logging are exactly, but from my
> > perspective, I'm dealing with a zillion different HW configurations so
> > some feedback whether the driver detected the *correct* HW parameters -
> > or actually, whether I stuffed the correct image into my FPGA :o) - is
> > very convenient to have. And not just for my local development, but also
> > to debug deployments in the field at customer sites.
>
> I understand it can be convenient, it's just a matter of having a
> logging message for you that will end up in many builds for many users.
> They do not necessarily have the same needs. So it's a matter of
> compromise, one or two messages at boot can be OK, more is likely to
> become an issue.
>
OK, got it. So I have to stuff all my logging into one or two very long lines :-P
(just kidding)
> > Also, looking at my log, there's other drivers that are similarly
> > (or even more) verbose.
>
> There are *always* other examples in the kernel :)
>
> > If it's a really considered a problem, I could make it dev_dbg?
>
> Sure. I would say adding one message at boot like this one seems OK. But
> the others would need to be debugging messages.
>
OK, then that's what I'll really do.
> > > For this one particularly, the probe could fail later on. So if we were
> > > to add this output, it should be done at the very end of the probe.
> > >
> > I'm in doubt about this one. I understand that you want to reduce the
> > logging in that case, but at the same time that message can convey
> > information as to WHY the probing fails later on ...
>
> If the drivers fails to probe, there will be other messages. In that
> case is this one really needed? I'm not sure.
>
> > i.e. if it detects, say, 4 pipes on a device that, in fact, only has
> > 2, then that may be the very reason for the FW init to fail later on.
>
> In case of failure you'll need anyway to debug and understand what's
> going on. By adding new prints, or enabling debugging messages.
>
If it fails for me locally, I can do that. If it somehow fails "in the field",
I think most people won't be able to recompile their own Linux
kernel with debug messages let alone add their own debug messages.
Anyway, I'll just make everything dev_dbg to avoid further discussion.
> > > > -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,
> > >
> > > You could probably use the DEVICE_IS_PCI flag instead.
> > >
> > I'm not currently passing priv to that function, so I can't access
> > priv->version directly. I could pass a priv pointer instead and use
> > the flag, but passing a bool constant seemed to be more efficient?
>
> That right, whatever you prefer then.
>
Ok, then I'll leave it as it is.
> > > > +static int safexcel_probe_generic(void *pdev,
> > > > + struct safexcel_crypto_priv *priv,
> > > > + int is_pci_dev)
> > > > {
> > >
> > > > + if (IS_ENABLED(CONFIG_PCI) && (priv->version & DEVICE_IS_PCI)) {
> > >
> > > DEVICE_IS_PCI should be set in ->flags, not ->version.
> > >
> > As far as I could tell from the existing code, "version" was
> > a highlevel indication of the device *type* (EIP97/EIP197B/D)
> > while flags was more about low-level *features*.
>
> Using the same argument I would say the way the device is accessed is
> very run-time dependent, and it has nothing to do with the version of
> the h/w (the same engine could be accessed through different ways given
> the platform).
>
> Maybe the name 'feature' is too specific, I agree with you.
>
> > So in my humble opinion, version was the correct location, it
> > is just a confusing name. (i.e. you can have many *versions*
> > of an EIP197B, for instance ...)
>
> That would be an issue with the driver. We named the 'version' given the
> knowledge we had of the h/w, it might not be specific enough. Or maybe
> you can think of this as being a "family of engine versions". The idea
> is the version is what the h/w is capable of, not how it's being
> wired/accessed.
>
Well ... I want to avoid the whole discussion about the naming of the
variable (which can be trivially changed) and what the intention may
have been, if you allow me.
Fact is ... this variable is what receives .data / .driver_data from the
OF or PCI match table. So it is a means of conveying a value that is
specific to the table entry that was matched. No more, no less.
In "your" device tree case you want to distinguish between
Armada 39x, Armada 7K/8K and Armada 9K. In "my" PCI case I
want to potentially distinguish multiple FPGA boards/images.
It wouldn't make much sense to me to do the vendor/subvendor/
device/subdevice decoding all over again in my probe routine.
So what exactly is so very wrong with the way I'm doing this?
> > > > + /*
> > > > + * 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);
> > >
> > > You need a space before and after the | here.
> > >
> > Ok, will add (checkpatch did not complain though)
>
> It may complain with --strict on this one. (Well, I'm sure many --strict
> tests won't pass on the existing upstream code, no worries :)).
>
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "Failed to allocate PCI MSI interrupts");
> > >
> > > Do not forget the \n at the end of the string.
> > >
> > Actually, I removed all "\n"'s from my messages as I
> > understood they are not needed? If they are really needed,
> > I can add them to all log strings again ... anyone else?
>
> The simple answer is: everybody set the \n, so at least for consistency
> we should have them.
>
I did a bit of side-investigation to conclude the same. So I already added them.
> > > > @@ -1189,13 +1249,12 @@ static int safexcel_remove(struct platform_device *pdev)
> > > > .compatible = "inside-secure,safexcel-eip197d",
> > > > .data = (void *)EIP197D,
> > > > },
> > > > + /* For backward compatibiliry and intended for generic use */
> > > > {
> > > > - /* Deprecated. Kept for backward compatibility. */
> > > > .compatible = "inside-secure,safexcel-eip97",
> > > > .data = (void *)EIP97IES,
> > > > },
> > > > {
> > > > - /* Deprecated. Kept for backward compatibility. */
> > > > .compatible = "inside-secure,safexcel-eip197",
> > > > .data = (void *)EIP197B,
> > > > },
> > >
> > > I'm not sure about this. The compatible should describe what the
> > > hardware is, and the driver can then decide if it has special things to
> > > do or not. It is not used to configure the driver to be used with a
> > > generic use or not.
> > >
> > > Do you have a practical reason to do this?
> >
> > I have to admit I don't fully understand how these compatible
> > strings work. All I wanted to achieve is provide some generic
> > device tree entry to point to this driver, to be used for
> > devices other than Marvell. No need to convey b/d that way
> > (or even eip97/197, for that matter) as that can all be probed.
>
> Compatibles are used in device trees, which intend to be a description
> of the hardware (not the configuration of how the hardware should be
> used). So we can't have a compatible being a restricted configuration
> use of a given hardware. But I think here you're right, and there is
> room for a more generic eip197 compatible: the b/d versions only have
> few differences and are part of the same family, so we can have a very
> specific compatible plus a "family" one. Something like:
>
> compatible = "inside-secure,safexcel-eip197d", "inside-secure,safexcel-eip197";
>
> This would need to be in a separated patch, and this should be
> documented in:
> Documentation/devicetree/bindings/crypto/inside-secure-safexcel.txt
>
Ok, then I'll leave that part untouched for now.
(I only changed the comments anyway ...)
> > > > + /* enable the device */
> > > > + rc = pcim_enable_device(pdev);
> > > > + if (rc) {
> > > > + dev_err(dev, "pci_enable_device() failed");
> > >
> > > Please use error messages describing the issue rather than printing the
> > > function names. (There are other examples).
> > >
> > But ... that IS the issue, isn't it? Well, apart from the actual
> > return code. What message text would you suggest?
>
> Yes, it is the issue. We generally use a sentence instead of function
> names (something alongside s/_/ / :)).
>
Fine, will do.
> > > > + /* HW reset FPGA dev board */
> > > > + // assert reset
> > > > + writel(1, priv->base + XILINX_GPIO_BASE);
> > > > + wmb(); /* maintain strict ordering for accesses here */
> > > > + // deassert reset
> > > > + writel(0, priv->base + XILINX_GPIO_BASE);
> > > > + wmb(); /* maintain strict ordering for accesses here */
> > >
> > > It seems the driver here access to a GPIO controller, to assert and
> > > de-assert reset, which is outside the crypto engine i/o range. If true,
> > > this is not acceptable in the upstream kernel: those GPIO or reset
> > > should be accessed through the dedicated in-kernel API and be
> > > implemented in separate drivers (maybe a reset driver here).
> > >
> > Hmmm ... this is some *local* GPIO controller *inside*
> > the FPGA device (i.e. doesn't even leave the silicon die).
> > It's inside the range of the (single!) PCIE device, it's not
> > a seperate device and thus cannot be managed as such ...
> >
> > Effectively, it's just an MMIO mapped register no different
> > from any other slave accessible register.
> > If I had given it a less explicit name, nobody would've cared.
>
> OK, that makes sense. If the register accessed is within the register
> bank of the crypto engine you can keep this.
>
> > > > +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,
> > > > +};
> > >
> > > More generally, you should protect all the PCI specific functions and
> > > definitions between #ifdef.
> > >
> > I asked the mailing list and the answer was I should NOT use #ifdef,
> > but instead use IS_ENABLED to *only* remove relevant function bodies.
> > Which is exactly what I did (or tried to do, anyway).
>
> My bad, I realise there's a mistake in my comment. That should have
> been: you should protect all the PCI specific functions and definitions
> with #if IS_ENABLED(...). When part of a function should be excluded
> you can use if(IS_ENABLED(...)), but if the entire function can be left
> out, #if is the way to go.
>
Ok #if instead of if or #ifdef,that makes sense.
So can I just put all the PCI stuff into one big #if then?
Thanks,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
http://www.insidesecure.com
> -----Original Message-----
> From: Antoine Tenart <[email protected]>
> Sent: Thursday, June 20, 2019 3:15 PM
> To: Pascal Van Leeuwen <[email protected]>
> Cc: Antoine Tenart <[email protected]>; Pascal van Leeuwen <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 3/3] crypto: inside-secure - add support for using the EIP197 without firmware images
>
> Hi Pascal,
>
> On Wed, Jun 19, 2019 at 02:37:44PM +0000, Pascal Van Leeuwen wrote:
> > > From: Antoine Tenart <[email protected]>
> > > On Tue, Jun 18, 2019 at 07:56:24AM +0200, Pascal van Leeuwen wrote:
> >
> > > In addition to this, the direction the kernel has taken was to *remove*
> > > binary firmwares from its source code. I'm afraid adding this is a
> > > no-go.
> >
> > For a HW engineer, there really is no fundamental difference between
> > control register contents or an instruction word. They can both have
> > the exact same effects internal to the HW.
> > If I had disguised this as a handful of config reg writes writing
> > some #define'd magic values, probably no one would have even noticed.
>
> I do not fully agree. If this is comparable to configuring h/w
> registers, then you could probably have defines explaining why each bit
> is set and what it's doing. Which would be fine.
>
Strictly speaking, we (and probably most other HW vendors as well) don't do
that for every register bit either, not even in the official Programmer Manual.
Some bits are just "you don't need to know, just write this" :-)
But I get your point.
> > By that same definition, the tokens the driver generates for
> > processing could be considered "firmware" as well (as they are used by
> > the hardware in a very similar way) ...
>
> Right. The main difference here is we do have a clear definition of what
> the tokens are doing. Thanks to your explanation, if this firmware is
> really looking like the token we're using, the words have a defined
> structure and the magic values could be generated with proper defines
> and macros. And I think it's the main issue here: it's not acceptable to
> have an array of magic values. If you can give a meaning to those bits,
> I see no reason why it couldn't be added to the driver.
>
> (And I'm all for what you're trying to achieve here :)).
>
Now we're reaching a tricky subject. Because I think if some people here
find out those token bits are explicitly documented in the driver, they
will not be so happy ... (don't worry, I won't wake any sleeping dogs :-)
We provide this information to our customers under NDA, but it's
obviously quite sensitive information as it reveals a lot about the
inner workings of our HW design.
The encoding of the microengine control words is considered even
more sensitive, so we don't even provide that under NDA.
Adding that to the driver will probably get me in trouble.
So maybe putting these images in /lib/firmware is unavoidable, but
I'd really like to hear some more opinions on that subject.
> > > The proper solution I believe would be to support loading this "MiniFW",
> > > which (depending on the license) could be either distributed in the
> > > rootfs and loaded (like what's done currently), or through
> > > CONFIG_EXTRA_FIRMWARE.
> > >
> > That seems total overkill for just a handful of words though.
>
> Given your explanation, I agree. (If those bits can have meaning).
>
> Thanks!
> Antoine
>
> --
> Antoine T?nart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Thanks,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
http://www.insidesecure.com
Hi Pascal,
On Thu, Jun 20, 2019 at 02:47:30PM +0000, Pascal Van Leeuwen wrote:
> > From: Antoine Tenart <[email protected]>
> > On Wed, Jun 19, 2019 at 02:22:19PM +0000, Pascal Van Leeuwen wrote:
> > > > From: Antoine Tenart <[email protected]>
> > > > On Tue, Jun 18, 2019 at 07:56:23AM +0200, Pascal van Leeuwen wrote:
> > > > >
> > > > > /* Fallback to the old firmware location for the
> > > > > @@ -294,6 +291,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
> > > > >
> > > > > + dev_info(priv->dev, "EIP(1)97 HW init: burst size %d beats, using %d pipe(s) and %d
> > > > ring(s)",
> > > > > + 16, priv->config.pes, priv->config.rings);
> > > >
> > > > Adding custom messages in the kernel log has to be done carefully.
> > > > Although it's not considered stable it could be difficult to rework
> > > > later on. Also, if all driver were to print custom messages the log
> > > > would be very hard to read. But you can also argue that a single message
> > > > when probing a driver is also done in other drivers.
> > > >
> > > Hmm ... don't know what the rules for logging are exactly, but from my
> > > perspective, I'm dealing with a zillion different HW configurations so
> > > some feedback whether the driver detected the *correct* HW parameters -
> > > or actually, whether I stuffed the correct image into my FPGA :o) - is
> > > very convenient to have. And not just for my local development, but also
> > > to debug deployments in the field at customer sites.
> >
> > I understand it can be convenient, it's just a matter of having a
> > logging message for you that will end up in many builds for many users.
> > They do not necessarily have the same needs. So it's a matter of
> > compromise, one or two messages at boot can be OK, more is likely to
> > become an issue.
> >
> OK, got it. So I have to stuff all my logging into one or two very long lines :-P
> (just kidding)
Hehe :-)
> > > > For this one particularly, the probe could fail later on. So if we were
> > > > to add this output, it should be done at the very end of the probe.
> > > >
> > > I'm in doubt about this one. I understand that you want to reduce the
> > > logging in that case, but at the same time that message can convey
> > > information as to WHY the probing fails later on ...
> >
> > If the drivers fails to probe, there will be other messages. In that
> > case is this one really needed? I'm not sure.
> >
> > > i.e. if it detects, say, 4 pipes on a device that, in fact, only has
> > > 2, then that may be the very reason for the FW init to fail later on.
> >
> > In case of failure you'll need anyway to debug and understand what's
> > going on. By adding new prints, or enabling debugging messages.
> >
> If it fails for me locally, I can do that. If it somehow fails "in the field",
> I think most people won't be able to recompile their own Linux
> kernel with debug messages let alone add their own debug messages.
>
> Anyway, I'll just make everything dev_dbg to avoid further discussion.
There's always the 'loglevel' command-line parameter, but yes, that
probably do not cover all cases.
> > > So in my humble opinion, version was the correct location, it
> > > is just a confusing name. (i.e. you can have many *versions*
> > > of an EIP197B, for instance ...)
> >
> > That would be an issue with the driver. We named the 'version' given the
> > knowledge we had of the h/w, it might not be specific enough. Or maybe
> > you can think of this as being a "family of engine versions". The idea
> > is the version is what the h/w is capable of, not how it's being
> > wired/accessed.
>
> Well ... I want to avoid the whole discussion about the naming of the
> variable (which can be trivially changed) and what the intention may
> have been, if you allow me.
>
> Fact is ... this variable is what receives .data / .driver_data from the
> OF or PCI match table. So it is a means of conveying a value that is
> specific to the table entry that was matched. No more, no less.
> In "your" device tree case you want to distinguish between
> Armada 39x, Armada 7K/8K and Armada 9K. In "my" PCI case I
> want to potentially distinguish multiple FPGA boards/images.
>
> It wouldn't make much sense to me to do the vendor/subvendor/
> device/subdevice decoding all over again in my probe routine.
> So what exactly is so very wrong with the way I'm doing this?
I think what is an issue for me here is the re-use of a variable
intended to only control the version of the engine. And the way this
engine is probed/accessed has nothing to do with this.
One solution, that I think would work for both of us, would be to still
keep this information in .data (as you did) but to organise it within a
struct so that the version information is split from the way the device
is accessed. Would that work for you?
('version' can probably be a value and not a bitfield then).
I'm sorry if the discussion about this point seems disproportionate
compared to technical aspect, but I would like to avoid possible
maintenance issues in the future with conditions looking like:
if (version == EIP197)
Which could easily be merged in a big patch but would break the
existing, given on what h/w the submitter tested the changes.
> > > > > @@ -1189,13 +1249,12 @@ static int safexcel_remove(struct platform_device *pdev)
> > > > > .compatible = "inside-secure,safexcel-eip197d",
> > > > > .data = (void *)EIP197D,
> > > > > },
> > > > > + /* For backward compatibiliry and intended for generic use */
> > > > > {
> > > > > - /* Deprecated. Kept for backward compatibility. */
> > > > > .compatible = "inside-secure,safexcel-eip97",
> > > > > .data = (void *)EIP97IES,
> > > > > },
> > > > > {
> > > > > - /* Deprecated. Kept for backward compatibility. */
> > > > > .compatible = "inside-secure,safexcel-eip197",
> > > > > .data = (void *)EIP197B,
> > > > > },
> > > >
> > > > I'm not sure about this. The compatible should describe what the
> > > > hardware is, and the driver can then decide if it has special things to
> > > > do or not. It is not used to configure the driver to be used with a
> > > > generic use or not.
> > > >
> > > > Do you have a practical reason to do this?
> > >
> > > I have to admit I don't fully understand how these compatible
> > > strings work. All I wanted to achieve is provide some generic
> > > device tree entry to point to this driver, to be used for
> > > devices other than Marvell. No need to convey b/d that way
> > > (or even eip97/197, for that matter) as that can all be probed.
> >
> > Compatibles are used in device trees, which intend to be a description
> > of the hardware (not the configuration of how the hardware should be
> > used). So we can't have a compatible being a restricted configuration
> > use of a given hardware. But I think here you're right, and there is
> > room for a more generic eip197 compatible: the b/d versions only have
> > few differences and are part of the same family, so we can have a very
> > specific compatible plus a "family" one. Something like:
> >
> > compatible = "inside-secure,safexcel-eip197d", "inside-secure,safexcel-eip197";
> >
> > This would need to be in a separated patch, and this should be
> > documented in:
> > Documentation/devicetree/bindings/crypto/inside-secure-safexcel.txt
> >
> Ok, then I'll leave that part untouched for now.
> (I only changed the comments anyway ...)
Feel free to send a patch later on :) (Even if it's only about the
comment, it is important as well).
> > > > > +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,
> > > > > +};
> > > >
> > > > More generally, you should protect all the PCI specific functions and
> > > > definitions between #ifdef.
> > > >
> > > I asked the mailing list and the answer was I should NOT use #ifdef,
> > > but instead use IS_ENABLED to *only* remove relevant function bodies.
> > > Which is exactly what I did (or tried to do, anyway).
> >
> > My bad, I realise there's a mistake in my comment. That should have
> > been: you should protect all the PCI specific functions and definitions
> > with #if IS_ENABLED(...). When part of a function should be excluded
> > you can use if(IS_ENABLED(...)), but if the entire function can be left
> > out, #if is the way to go.
> >
> Ok #if instead of if or #ifdef,that makes sense.
> So can I just put all the PCI stuff into one big #if then?
Right.
You may also want to check for for helpers only defined if CONFIG_OF
is selected, as the driver could be compiled for a kernel with only
CONFIG_PCI enabled.
Thanks!
Antoine
--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Pascal,
On Thu, Jun 20, 2019 at 02:59:20PM +0000, Pascal Van Leeuwen wrote:
> > From: Antoine Tenart <[email protected]>
> > On Wed, Jun 19, 2019 at 02:37:44PM +0000, Pascal Van Leeuwen wrote:
> > > > From: Antoine Tenart <[email protected]>
> > > > On Tue, Jun 18, 2019 at 07:56:24AM +0200, Pascal van Leeuwen wrote:
> > >
> > > > In addition to this, the direction the kernel has taken was to *remove*
> > > > binary firmwares from its source code. I'm afraid adding this is a
> > > > no-go.
> > >
> > > For a HW engineer, there really is no fundamental difference between
> > > control register contents or an instruction word. They can both have
> > > the exact same effects internal to the HW.
> > > If I had disguised this as a handful of config reg writes writing
> > > some #define'd magic values, probably no one would have even noticed.
> >
> > I do not fully agree. If this is comparable to configuring h/w
> > registers, then you could probably have defines explaining why each bit
> > is set and what it's doing. Which would be fine.
> >
> Strictly speaking, we (and probably most other HW vendors as well) don't do
> that for every register bit either, not even in the official Programmer Manual.
> Some bits are just "you don't need to know, just write this" :-)
That's right... :-(
> > > By that same definition, the tokens the driver generates for
> > > processing could be considered "firmware" as well (as they are used by
> > > the hardware in a very similar way) ...
> >
> > Right. The main difference here is we do have a clear definition of what
> > the tokens are doing. Thanks to your explanation, if this firmware is
> > really looking like the token we're using, the words have a defined
> > structure and the magic values could be generated with proper defines
> > and macros. And I think it's the main issue here: it's not acceptable to
> > have an array of magic values. If you can give a meaning to those bits,
> > I see no reason why it couldn't be added to the driver.
> >
> > (And I'm all for what you're trying to achieve here :)).
> >
> Now we're reaching a tricky subject. Because I think if some people here
> find out those token bits are explicitly documented in the driver, they
> will not be so happy ... (don't worry, I won't wake any sleeping dogs :-)
> We provide this information to our customers under NDA, but it's
> obviously quite sensitive information as it reveals a lot about the
> inner workings of our HW design.
>
> The encoding of the microengine control words is considered even
> more sensitive, so we don't even provide that under NDA.
> Adding that to the driver will probably get me in trouble.
I fully understand this. This is not perfect, but at least it's the way
it is right now.
> So maybe putting these images in /lib/firmware is unavoidable, but
> I'd really like to hear some more opinions on that subject.
Yes, you either have to choice to put it in /lib/firmware (and in the
linux-firmwares project!) or to convince people to allow releasing this.
We can wait for others to hop in on the discussion, of course.
Thanks!
Antoine
--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Thu, Jun 20, 2019 at 05:42:59PM +0200, Antoine Tenart wrote:
>
> Yes, you either have to choice to put it in /lib/firmware (and in the
> linux-firmwares project!) or to convince people to allow releasing this.
I agree. We should not be adding firmware into the driver itself.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> -----Original Message-----
> From: Herbert Xu <[email protected]>
> Sent: Monday, June 24, 2019 4:20 PM
> To: Antoine Tenart <[email protected]>
> Cc: Pascal Van Leeuwen <[email protected]>; Pascal van Leeuwen <[email protected]>; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 3/3] crypto: inside-secure - add support for using the EIP197 without firmware images
>
> On Thu, Jun 20, 2019 at 05:42:59PM +0200, Antoine Tenart wrote:
> >
> > Yes, you either have to choice to put it in /lib/firmware (and in the
> > linux-firmwares project!) or to convince people to allow releasing this.
>
> I agree. We should not be adding firmware into the driver itself.
>
OK, so I guess the alternative would be to get the firmware binaries into the linux-firmwares
project? I can do that ...
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
http://www.insidesecure.com