[PATCH 0/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[PATCH 0/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Patrick Delaunay]

This serie update the NVMEM BSEC driver to be compatible with STM32MP13x
SoC and the trusted application STM32MP BSEC in OP-TEE

This serie solve issue in initial support of STM32MP131
(using BSEC STM32MP15 compatible) and so it break the DTS compatible.

I create this serie for more efficient review.

The 2 first patches of this series are re-sent of patches already sent

1- "dt-bindings: nvmem: add new stm32mp13 compatible for stm32-romem"
    https://lore.kernel.org/all/20221014172324.1.Ifc1812116ff63f5501f3edd155d3cf5c0ecc846c@changeid/
    https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=685403

2- "ARM: dts: stm32mp13: fix compatible for BSEC"
    https://lore.kernel.org/all/20221017134437.1.I167a5efc1f8777cce14518c6fa38400ac684de3e@changeid/
    https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=685815

This DTS break is acceptable as
- the STM32MP13x SoC is not yet available outside STMicroelectronics
  (not official)
- the same patch is already integrated or modifications are in progress in
  the other users (arm-trusted-firmware/TF-A, OP-TEE and U-Boot) of
  stm32mp131 device tree.

It is the good time to correct this issue before the real availability of
the SoC and before full support of SoC in Linux kernel.

This last patch on NVMEM STM32 ROMEM driver in depend on the preliminary
patch for the driver:

  "nvmem: stm32: move STM32MP15_BSEC_NUM_LOWER in config"
  https://lore.kernel.org/all/20221017174953.v2.1.I95e71328dd654723bd4c57206bd008ff81c726bb@changeid/

present in the serie

  "nvmem: stm32: several minor improvements"
  https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=685886

Regards

Patrick

Changes in v1:
- update commit message to indicate DTS break reason.

Patrick Delaunay (3):
  dt-bindings: nvmem: add new stm32mp13 compatible for stm32-romem
  ARM: dts: stm32mp13: fix compatible for BSEC
  nvmem: stm32: add OP-TEE support for STM32MP13x

 .../bindings/nvmem/st,stm32-romem.yaml        |   1 +
 arch/arm/boot/dts/stm32mp131.dtsi             |   2 +-
 drivers/nvmem/stm32-romem.c                   | 450 +++++++++++++++++-
 3 files changed, 448 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/3] dt-bindings: nvmem: add new stm32mp13 compatible for stm32-romem

[Patrick Delaunay]

Add a new compatible for stm32mp13 support.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
This patch is already sent separately in:
https://lore.kernel.org/all/20221014172324.1.Ifc1812116ff63f5501f3edd155d3cf5c0ecc846c@changeid/
https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=685403

I create a serie for more efficient review.

Patrick.

 Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml b/Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml
index 448a2678dc62..16f4cad2fa55 100644
--- a/Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml
@@ -22,6 +22,7 @@ properties:
   compatible:
     enum:
       - st,stm32f4-otp
+      - st,stm32mp13-bsec
       - st,stm32mp15-bsec
 
   reg:
-- 
2.25.1

Re: [PATCH 1/3] dt-bindings: nvmem: add new stm32mp13 compatible for stm32-romem

[Fabrice Gasnier]

On 10/28/22 16:52, Patrick Delaunay wrote:
> Add a new compatible for stm32mp13 support.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Hi Patrick,

You can add my:
Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thanks,
Fabrice

> ---
> This patch is already sent separately in:
> https://lore.kernel.org/all/20221014172324.1.Ifc1812116ff63f5501f3edd155d3cf5c0ecc846c@changeid/
> https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=685403
> 
> I create a serie for more efficient review.
> 
> Patrick.
> 
>  Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml b/Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml
> index 448a2678dc62..16f4cad2fa55 100644
> --- a/Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/st,stm32-romem.yaml
> @@ -22,6 +22,7 @@ properties:
>    compatible:
>      enum:
>        - st,stm32f4-otp
> +      - st,stm32mp13-bsec
>        - st,stm32mp15-bsec
>  
>    reg:

[PATCH 2/3] ARM: dts: stm32mp13: fix compatible for BSEC

[Patrick Delaunay]

Use the correct compatible for stm32mp13 support.

The BSEC driver for STM32MP15x is not compatible with STM32MP13x. For
example the proprietary's smc STM32_SMC_BSEC is not supported in
STM32MP13x OP-TEE, it is replaced by SM32MP BSEC Pseudo Trusted
Application in OP-TEE to access to the secured IP BSEC on STM32MP13X SoC.

The correct compatible is already used in U-Boot and in upstream is in
progress for OP-TEE device tree.

As the SoC STM32MP13X is not yet official and it is not available
outside STMicroelectronics, it is the good time to break the DTS
compatibility and to correct the error done in the introduction of
STM32MP131.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
This patch is already sent separately in:
https://lore.kernel.org/all/20221017134437.1.I167a5efc1f8777cce14518c6fa38400ac684de3e@changeid/
https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=685815

I create a serie for more efficient review.

Patrick.

Changes in v1:
- update commit message to indicate DTS break reason.

 arch/arm/boot/dts/stm32mp131.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stm32mp131.dtsi b/arch/arm/boot/dts/stm32mp131.dtsi
index dd35a607073d..89a027f894f9 100644
--- a/arch/arm/boot/dts/stm32mp131.dtsi
+++ b/arch/arm/boot/dts/stm32mp131.dtsi
@@ -398,7 +398,7 @@ rtc: rtc@5c004000 {
 		};
 
 		bsec: efuse@5c005000 {
-			compatible = "st,stm32mp15-bsec";
+			compatible = "st,stm32mp13-bsec";
 			reg = <0x5c005000 0x400>;
 			#address-cells = <1>;
 			#size-cells = <1>;
-- 
2.25.1

[PATCH 3/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Patrick Delaunay]

For boot with OP-TEE on STM32MP13, the communication with the secure
world no more use STMicroelectronics SMC but communication with the
BSEC TA, for data access (read/write) or lock operation:
- all the request are sent to OP-TEE trusted application,
- for upper OTP with ECC protection and with word programming only
  each OTP are permanently locked when programmed to avoid ECC error
  on the second write operation

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

 drivers/nvmem/stm32-romem.c | 450 +++++++++++++++++++++++++++++++++++-
 1 file changed, 446 insertions(+), 4 deletions(-)

diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c
index 6de565639d5f..dfdedbcca9b9 100644
--- a/drivers/nvmem/stm32-romem.c
+++ b/drivers/nvmem/stm32-romem.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/nvmem-provider.h>
 #include <linux/of_device.h>
+#include <linux/tee_drv.h>
 
 /* BSEC secure service access from non-secure */
 #define STM32_SMC_BSEC			0x82001003
@@ -25,14 +26,22 @@
 struct stm32_romem_cfg {
 	int size;
 	u8 lower;
+	bool ta;
 };
 
 struct stm32_romem_priv {
 	void __iomem *base;
 	struct nvmem_config cfg;
 	u8 lower;
+	struct device *ta;
 };
 
+struct device *stm32_bsec_pta_find(struct device *dev);
+static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
+			       size_t bytes);
+static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
+				size_t bytes);
+
 static int stm32_romem_read(void *context, unsigned int offset, void *buf,
 			    size_t bytes)
 {
@@ -173,15 +182,25 @@ static int stm32_romem_probe(struct platform_device *pdev)
 	} else {
 		priv->cfg.size = cfg->size;
 		priv->lower = cfg->lower;
-		priv->cfg.reg_read = stm32_bsec_read;
-		priv->cfg.reg_write = stm32_bsec_write;
+		if (cfg->ta) {
+			priv->ta = stm32_bsec_pta_find(dev);
+			/* wait for OP-TEE client driver to be up and ready */
+			if (!priv->ta)
+				return -EPROBE_DEFER;
+
+			priv->cfg.reg_read = stm32_bsec_pta_read;
+			priv->cfg.reg_write = stm32_bsec_pta_write;
+		} else {
+			priv->cfg.reg_read = stm32_bsec_read;
+			priv->cfg.reg_write = stm32_bsec_write;
+		}
 	}
 
 	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &priv->cfg));
 }
 
 /*
- * STM32MP15 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
+ * STM32MP15/13 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
  * => 96 x 32-bits data words
  * - Lower: 1K bits, 2:1 redundancy, incremental bit programming
  *   => 32 (x 32-bits) lower shadow registers = words 0 to 31
@@ -191,6 +210,13 @@ static int stm32_romem_probe(struct platform_device *pdev)
 static const struct stm32_romem_cfg stm32mp15_bsec_cfg = {
 	.size = 384,
 	.lower = 32,
+	.ta = false,
+};
+
+static const struct stm32_romem_cfg stm32mp13_bsec_cfg = {
+	.size = 384,
+	.lower = 32,
+	.ta = true,
 };
 
 static const struct of_device_id stm32_romem_of_match[] = {
@@ -198,6 +224,8 @@ static const struct of_device_id stm32_romem_of_match[] = {
 		.compatible = "st,stm32mp15-bsec",
 		.data = (void *)&stm32mp15_bsec_cfg,
 	}, {
+		.compatible = "st,stm32mp13-bsec",
+		.data = (void *)&stm32mp13_bsec_cfg,
 	},
 };
 MODULE_DEVICE_TABLE(of, stm32_romem_of_match);
@@ -209,7 +237,421 @@ static struct platform_driver stm32_romem_driver = {
 		.of_match_table = of_match_ptr(stm32_romem_of_match),
 	},
 };
-module_platform_driver(stm32_romem_driver);
+
+#if IS_ENABLED(CONFIG_OPTEE)
+/*************************************************************************
+ * BSEC PTA : OP-TEE client driver to pseudo trusted application
+ *************************************************************************/
+
+/*
+ * Read OTP memory
+ *
+ * [in]		value[0].a		OTP start offset in byte
+ * [in]		value[0].b		Access type (0 : shadow,
+ *					1 : fuse, 2 : lock)
+ * [out]	memref[1].buffer	Output buffer to store read values
+ * [out]	memref[1].size		Size of OTP to be read
+ *
+ * Return codes:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define PTA_BSEC_READ_MEM		0x0 /* Read OTP */
+
+/*
+ * Write OTP memory
+ *
+ * [in]		value[0].a		OTP start offset in byte
+ * [in]		value[0].b		Access type (0 : shadow,
+ *					1 : fuse, 2 : lock)
+ * [in]		memref[1].buffer	Input buffer to read values
+ * [in]		memref[1].size		Size of OTP to be written
+ *
+ * Return codes:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define PTA_BSEC_WRITE_MEM		0x1	/* Write OTP */
+
+/* value of PTA_BSEC access type = value[in] b */
+#define SHADOW_ACCESS	0
+#define FUSE_ACCESS	1
+#define LOCK_ACCESS	2
+
+/* Bitfield definition for LOCK status */
+#define LOCK_PERM			BIT(30)
+
+/**
+ * struct stm32_bsec_pta_priv - OP-TEE BSEC TA private data
+ * @ctx:		OP-TEE context handler.
+ * @session_id:		TA session identifier.
+ */
+struct stm32_bsec_pta_priv {
+	struct tee_context *ctx;
+	u32 session_id;
+};
+
+/*
+ * Check whether this driver supports the BSEC TA in the TEE instance
+ * represented by the params (ver/data) to this function.
+ */
+static int stm32_bsec_pta_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	/*
+	 * Currently this driver only supports GP compliant, OP-TEE based TA
+	 */
+	if ((ver->impl_id == TEE_IMPL_ID_OPTEE) &&
+		(ver->gen_caps & TEE_GEN_CAP_GP))
+		return 1;
+	else
+		return 0;
+}
+
+/**
+ * stm32_bsec_pta_probe() - initialize the PTA BSEC
+ * @dev: the platform_device description.
+ *
+ * Return:
+ *	On success, 0. On failure, -errno.
+ */
+static int stm32_bsec_pta_probe(struct device *dev)
+{
+	int rc;
+	struct tee_ioctl_open_session_arg sess_arg;
+	struct tee_client_device *tee_device = to_tee_client_device(dev);
+	struct stm32_bsec_pta_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/* Open context with TEE driver */
+	priv->ctx = tee_client_open_context(NULL, stm32_bsec_pta_match, NULL, NULL);
+	if (IS_ERR(priv->ctx)) {
+		if (PTR_ERR(priv->ctx) == -ENOENT)
+			return -EPROBE_DEFER;
+		dev_err(dev, "%s: tee_client_open_context failed\n", __func__);
+		return PTR_ERR(priv->ctx);
+	}
+
+	/* Open a session with BSEC TA */
+	memset(&sess_arg, 0, sizeof(sess_arg));
+	export_uuid(sess_arg.uuid, &tee_device->id.uuid);
+	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+	sess_arg.num_params = 0;
+
+	rc = tee_client_open_session(priv->ctx, &sess_arg, NULL);
+	if ((rc < 0) || (sess_arg.ret != 0)) {
+		dev_err(dev, "%s: tee_client_open_session failed, err=%x\n",
+			__func__, sess_arg.ret);
+		rc = -EINVAL;
+		goto out_tee_session;
+	}
+	priv->session_id = sess_arg.session;
+	dev_set_drvdata(dev, priv);
+
+	return 0;
+
+out_tee_session:
+	tee_client_close_context(priv->ctx);
+	priv->ctx = NULL;
+
+	return rc;
+}
+
+/**
+ * stm32_bsec_pta_remove() - remove the BSEC TEE device
+ * @dev: the platform_device description.
+ *
+ * Return:
+ *	0 always.
+ */
+static int stm32_bsec_pta_remove(struct device *dev)
+{
+	struct stm32_bsec_pta_priv *priv = dev_get_drvdata(dev);
+
+	if (!IS_ERR_OR_NULL(priv->ctx)) {
+		tee_client_close_session(priv->ctx, priv->session_id);
+		tee_client_close_context(priv->ctx);
+	}
+
+	return 0;
+}
+
+/**
+ * stm32_bsec_pta_read() - nvmem read access using PTA client driver
+ * @context: nvmem context => romem privdate data
+ * @offset: nvmem offset
+ * @buf: buffer to fill with nvem values
+ * @bytes: number of bytes to read
+ *
+ * Return:
+ *	On success, 0. On failure, -errno.
+ */
+static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
+			       size_t bytes)
+{
+	struct stm32_romem_priv *romem_priv = context;
+	struct device *dev;
+	struct stm32_bsec_pta_priv *priv;
+	struct tee_shm *shm;
+	struct tee_ioctl_invoke_arg arg;
+	struct tee_param param[2];
+	u8 *shm_buf;
+	u32 start, num_bytes;
+	int ret;
+
+	dev = romem_priv->ta;
+	if (!dev) {
+		pr_err("TA_BSEC invoke with driver\n");
+		return -ENXIO;
+	}
+
+	priv = dev_get_drvdata(dev);
+
+	memset(&arg, 0, sizeof(arg));
+	memset(&param, 0, sizeof(param));
+
+	arg.func = PTA_BSEC_READ_MEM;
+	arg.session = priv->session_id;
+	arg.num_params = 2;
+
+	/* align access on 32bits */
+	start = ALIGN_DOWN(offset, 4);
+	num_bytes = round_up(offset + bytes - start, 4);
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+	param[0].u.value.a = start;
+	param[0].u.value.b = SHADOW_ACCESS;
+
+	shm = tee_shm_alloc_kernel_buf(priv->ctx, num_bytes);
+	if (IS_ERR(shm))
+		return PTR_ERR(shm);
+
+	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[1].u.memref.shm = shm;
+	param[1].u.memref.size = num_bytes;
+
+	ret = tee_client_invoke_func(priv->ctx, &arg, param);
+	if (ret < 0 || arg.ret != 0) {
+		dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
+			arg.ret, ret);
+		if (!ret)
+			ret = -EIO;
+	}
+	if (!ret) {
+		shm_buf = tee_shm_get_va(shm, 0);
+		if (IS_ERR(shm_buf)) {
+			dev_err(dev, "tee_shm_get_va failed for transmit\n");
+			ret = PTR_ERR(shm_buf);
+		} else {
+			ret = 0;
+			/* read data from 32 bits aligned buffer */
+			memcpy(buf, &shm_buf[offset % 4], bytes);
+		}
+	}
+
+	tee_shm_free(shm);
+
+	return ret;
+}
+
+/**
+ * stm32_bsec_pta_write() - nvmem write access using PTA client driver
+ * @context: nvmem context => romem privdate data
+ * @offset: nvmem offset
+ * @buf: buffer with nvem values
+ * @bytes: number of bytes to write
+ *
+ * Return:
+ *	On success, 0. On failure, -errno.
+ */
+static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
+				size_t bytes)
+{
+	struct stm32_romem_priv *romem_priv = context;
+	struct device *dev;
+	struct stm32_bsec_pta_priv *priv;
+	struct tee_shm *shm;
+	struct tee_ioctl_invoke_arg arg;
+	struct tee_param param[2];
+	u8 *shm_buf;
+	int ret;
+
+	dev = romem_priv->ta;
+	if (!dev) {
+		pr_err("TA_BSEC invoke with driver\n");
+		return -ENXIO;
+	}
+
+	/* Allow only writing complete 32-bits aligned words */
+	if ((bytes % 4) || (offset % 4))
+		return -EINVAL;
+
+	priv = dev_get_drvdata(dev);
+
+	memset(&arg, 0, sizeof(arg));
+	memset(&param, 0, sizeof(param));
+
+	arg.func = PTA_BSEC_WRITE_MEM;
+	arg.session = priv->session_id;
+	arg.num_params = 2;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+	param[0].u.value.a = offset;
+	param[0].u.value.b = FUSE_ACCESS;
+
+	shm = tee_shm_alloc_kernel_buf(priv->ctx, bytes);
+	if (IS_ERR(shm))
+		return PTR_ERR(shm);
+
+	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+	param[1].u.memref.shm = shm;
+	param[1].u.memref.size = bytes;
+
+	shm_buf = tee_shm_get_va(shm, 0);
+	if (IS_ERR(shm_buf)) {
+		dev_err(dev, "tee_shm_get_va failed for transmit\n");
+		return PTR_ERR(shm_buf);
+	}
+
+	memcpy(shm_buf, buf, bytes);
+
+	ret = tee_client_invoke_func(priv->ctx, &arg, param);
+	if (ret < 0 || arg.ret != 0) {
+		dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
+			arg.ret, ret);
+		if (!ret)
+			ret = -EIO;
+	}
+	dev_dbg(dev, "Write OTPs %d to %d, ret=%d\n",
+		offset / 4, (offset + bytes) / 4, ret);
+
+	/* Lock the upper OTPs with ECC protection, word programming only */
+	if (!ret && ((offset + bytes) >= (romem_priv->lower * 4))) {
+		u32 start, nb_lock;
+		u32 *lock = (u32 *)shm_buf;
+		int i;
+
+		/*
+		 * don't lock the lower OTPs, no ECC protection and incremental
+		 * bit programming, a second write is allowed
+		 */
+		start = max_t(u32, offset, romem_priv->lower * 4);
+		nb_lock = (offset + bytes - start) / 4;
+
+		param[0].u.value.a = start;
+		param[0].u.value.b = LOCK_ACCESS;
+		param[1].u.memref.size = nb_lock * 4;
+
+		for (i = 0; i < nb_lock; i++)
+			lock[i] = LOCK_PERM;
+
+		ret = tee_client_invoke_func(priv->ctx, &arg, param);
+		if (ret < 0 || arg.ret != 0) {
+			dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
+				arg.ret, ret);
+			if (!ret)
+				ret = -EIO;
+		}
+		dev_dbg(dev, "Lock upper OTPs %d to %d, ret=%d\n",
+			start / 4, start / 4 + nb_lock, ret);
+	}
+
+	tee_shm_free(shm);
+
+	return ret;
+}
+
+static const struct tee_client_device_id stm32_bsec_id_table[] = {
+	{
+		UUID_INIT(0x94cf71ad, 0x80e6, 0x40b5,
+			  0xa7, 0xc6, 0x3d, 0xc5, 0x01, 0xeb, 0x28, 0x03)
+	},
+	{ }
+};
+
+MODULE_DEVICE_TABLE(tee, stm32_bsec_id_table);
+
+static struct tee_client_driver stm32_bsec_pta_driver = {
+	.id_table	= stm32_bsec_id_table,
+	.driver		= {
+		.name = "stm32-bsec-pta",
+		.bus = &tee_bus_type,
+		.probe = stm32_bsec_pta_probe,
+		.remove = stm32_bsec_pta_remove,
+	},
+};
+
+static void stm32_bsec_put_device(void *data)
+{
+	put_device(data);
+}
+
+struct device *stm32_bsec_pta_find(struct device *dev)
+{
+	struct device *pta_dev;
+
+	pta_dev = driver_find_next_device(&stm32_bsec_pta_driver.driver, NULL);
+
+	if (pta_dev && devm_add_action_or_reset(dev, stm32_bsec_put_device, pta_dev)) {
+		dev_err(dev, "unable to register cleanup action\n");
+
+		return NULL;
+	}
+
+	return pta_dev;
+}
+
+#else
+static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
+			       size_t bytes)
+{
+	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
+
+	return -ENXIO;
+}
+
+static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
+				size_t bytes)
+{
+	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
+
+	return -ENXIO;
+}
+
+struct device *stm32_bsec_pta_find(struct device *dev)
+{
+	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
+
+	return NULL;
+}
+#endif
+
+static int __init stm32_romem_init(void)
+{
+	int rc;
+
+	rc = platform_driver_register(&stm32_romem_driver);
+	if (rc)
+		return rc;
+
+#if IS_ENABLED(CONFIG_OPTEE)
+	rc = driver_register(&stm32_bsec_pta_driver.driver);
+#endif
+
+	return rc;
+}
+
+static void __exit stm32_romem_exit(void)
+{
+	platform_driver_unregister(&stm32_romem_driver);
+#if IS_ENABLED(CONFIG_OPTEE)
+	driver_unregister(&stm32_bsec_pta_driver.driver);
+#endif
+}
+
+module_init(stm32_romem_init);
+module_exit(stm32_romem_exit);
 
 MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
 MODULE_DESCRIPTION("STMicroelectronics STM32 RO-MEM");
-- 
2.25.1

Re: [PATCH 3/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Srinivas Kandagatla]

On 28/10/2022 15:52, Patrick Delaunay wrote:
> For boot with OP-TEE on STM32MP13, the communication with the secure
> world no more use STMicroelectronics SMC but communication with the
> BSEC TA, for data access (read/write) or lock operation:
> - all the request are sent to OP-TEE trusted application,
> - for upper OTP with ECC protection and with word programming only
>    each OTP are permanently locked when programmed to avoid ECC error
>    on the second write operation
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---

For some reason I pushed this patch without a full review, This is now 
reverted from nvmem-next.

Why not add TEE client based new driver instead of ifdefing around this 
driver? Also I see there is not much common across both drivers anyway.


> 
>   drivers/nvmem/stm32-romem.c | 450 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 446 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c
> index 6de565639d5f..dfdedbcca9b9 100644
> --- a/drivers/nvmem/stm32-romem.c
> +++ b/drivers/nvmem/stm32-romem.c
> @@ -11,6 +11,7 @@
>   #include <linux/module.h>
>   #include <linux/nvmem-provider.h>
>   #include <linux/of_device.h>
> +#include <linux/tee_drv.h>
>   
>   /* BSEC secure service access from non-secure */
>   #define STM32_SMC_BSEC			0x82001003
> @@ -25,14 +26,22 @@
>   struct stm32_romem_cfg {
>   	int size;
>   	u8 lower;
> +	bool ta;
>   };
>   
>   struct stm32_romem_priv {
>   	void __iomem *base;
>   	struct nvmem_config cfg;
>   	u8 lower;
> +	struct device *ta;
>   };
>   
> +struct device *stm32_bsec_pta_find(struct device *dev);
> +static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
> +			       size_t bytes);
> +static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
> +				size_t bytes);
> +
>   static int stm32_romem_read(void *context, unsigned int offset, void *buf,
>   			    size_t bytes)
>   {
> @@ -173,15 +182,25 @@ static int stm32_romem_probe(struct platform_device *pdev)
>   	} else {
>   		priv->cfg.size = cfg->size;
>   		priv->lower = cfg->lower;
> -		priv->cfg.reg_read = stm32_bsec_read;
> -		priv->cfg.reg_write = stm32_bsec_write;
> +		if (cfg->ta) {
> +			priv->ta = stm32_bsec_pta_find(dev);
> +			/* wait for OP-TEE client driver to be up and ready */
> +			if (!priv->ta)
> +				return -EPROBE_DEFER;
> +
> +			priv->cfg.reg_read = stm32_bsec_pta_read;
> +			priv->cfg.reg_write = stm32_bsec_pta_write;
> +		} else {
> +			priv->cfg.reg_read = stm32_bsec_read;
> +			priv->cfg.reg_write = stm32_bsec_write;
> +		}
>   	}
>   
>   	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &priv->cfg));
>   }
>   
>   /*
> - * STM32MP15 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
> + * STM32MP15/13 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
>    * => 96 x 32-bits data words
>    * - Lower: 1K bits, 2:1 redundancy, incremental bit programming
>    *   => 32 (x 32-bits) lower shadow registers = words 0 to 31
> @@ -191,6 +210,13 @@ static int stm32_romem_probe(struct platform_device *pdev)
>   static const struct stm32_romem_cfg stm32mp15_bsec_cfg = {
>   	.size = 384,
>   	.lower = 32,
> +	.ta = false,
> +};
> +
> +static const struct stm32_romem_cfg stm32mp13_bsec_cfg = {
> +	.size = 384,
> +	.lower = 32,
> +	.ta = true,
>   };
>   
>   static const struct of_device_id stm32_romem_of_match[] = {
> @@ -198,6 +224,8 @@ static const struct of_device_id stm32_romem_of_match[] = {
>   		.compatible = "st,stm32mp15-bsec",
>   		.data = (void *)&stm32mp15_bsec_cfg,
>   	}, {
> +		.compatible = "st,stm32mp13-bsec",
> +		.data = (void *)&stm32mp13_bsec_cfg,

missing sentinel, which caused a regression in next.


--srini
>   	},
>   };
>   MODULE_DEVICE_TABLE(of, stm32_romem_of_match);
> @@ -209,7 +237,421 @@ static struct platform_driver stm32_romem_driver = {
>   		.of_match_table = of_match_ptr(stm32_romem_of_match),
>   	},
>   };
> -module_platform_driver(stm32_romem_driver);
> +
> +#if IS_ENABLED(CONFIG_OPTEE)
> +/*************************************************************************
> + * BSEC PTA : OP-TEE client driver to pseudo trusted application
> + *************************************************************************/
> +
> +/*
> + * Read OTP memory
> + *
> + * [in]		value[0].a		OTP start offset in byte
> + * [in]		value[0].b		Access type (0 : shadow,
> + *					1 : fuse, 2 : lock)
> + * [out]	memref[1].buffer	Output buffer to store read values
> + * [out]	memref[1].size		Size of OTP to be read
> + *
> + * Return codes:
> + * TEE_SUCCESS - Invoke command success
> + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
> + */
> +#define PTA_BSEC_READ_MEM		0x0 /* Read OTP */
> +
> +/*
> + * Write OTP memory
> + *
> + * [in]		value[0].a		OTP start offset in byte
> + * [in]		value[0].b		Access type (0 : shadow,
> + *					1 : fuse, 2 : lock)
> + * [in]		memref[1].buffer	Input buffer to read values
> + * [in]		memref[1].size		Size of OTP to be written
> + *
> + * Return codes:
> + * TEE_SUCCESS - Invoke command success
> + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
> + */
> +#define PTA_BSEC_WRITE_MEM		0x1	/* Write OTP */
> +
> +/* value of PTA_BSEC access type = value[in] b */
> +#define SHADOW_ACCESS	0
> +#define FUSE_ACCESS	1
> +#define LOCK_ACCESS	2
> +
> +/* Bitfield definition for LOCK status */
> +#define LOCK_PERM			BIT(30)
> +
> +/**
> + * struct stm32_bsec_pta_priv - OP-TEE BSEC TA private data
> + * @ctx:		OP-TEE context handler.
> + * @session_id:		TA session identifier.
> + */
> +struct stm32_bsec_pta_priv {
> +	struct tee_context *ctx;
> +	u32 session_id;
> +};
> +
> +/*
> + * Check whether this driver supports the BSEC TA in the TEE instance
> + * represented by the params (ver/data) to this function.
> + */
> +static int stm32_bsec_pta_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> +	/*
> +	 * Currently this driver only supports GP compliant, OP-TEE based TA
> +	 */
> +	if ((ver->impl_id == TEE_IMPL_ID_OPTEE) &&
> +		(ver->gen_caps & TEE_GEN_CAP_GP))
> +		return 1;
> +	else
> +		return 0;
> +}
> +
> +/**
> + * stm32_bsec_pta_probe() - initialize the PTA BSEC
> + * @dev: the platform_device description.
> + *
> + * Return:
> + *	On success, 0. On failure, -errno.
> + */
> +static int stm32_bsec_pta_probe(struct device *dev)
> +{
> +	int rc;
> +	struct tee_ioctl_open_session_arg sess_arg;
> +	struct tee_client_device *tee_device = to_tee_client_device(dev);
> +	struct stm32_bsec_pta_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	/* Open context with TEE driver */
> +	priv->ctx = tee_client_open_context(NULL, stm32_bsec_pta_match, NULL, NULL);
> +	if (IS_ERR(priv->ctx)) {
> +		if (PTR_ERR(priv->ctx) == -ENOENT)
> +			return -EPROBE_DEFER;
> +		dev_err(dev, "%s: tee_client_open_context failed\n", __func__);
> +		return PTR_ERR(priv->ctx);
> +	}
> +
> +	/* Open a session with BSEC TA */
> +	memset(&sess_arg, 0, sizeof(sess_arg));
> +	export_uuid(sess_arg.uuid, &tee_device->id.uuid);
> +	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> +	sess_arg.num_params = 0;
> +
> +	rc = tee_client_open_session(priv->ctx, &sess_arg, NULL);
> +	if ((rc < 0) || (sess_arg.ret != 0)) {
> +		dev_err(dev, "%s: tee_client_open_session failed, err=%x\n",
> +			__func__, sess_arg.ret);
> +		rc = -EINVAL;
> +		goto out_tee_session;
> +	}
> +	priv->session_id = sess_arg.session;
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +
> +out_tee_session:
> +	tee_client_close_context(priv->ctx);
> +	priv->ctx = NULL;
> +
> +	return rc;
> +}
> +
> +/**
> + * stm32_bsec_pta_remove() - remove the BSEC TEE device
> + * @dev: the platform_device description.
> + *
> + * Return:
> + *	0 always.
> + */
> +static int stm32_bsec_pta_remove(struct device *dev)
> +{
> +	struct stm32_bsec_pta_priv *priv = dev_get_drvdata(dev);
> +
> +	if (!IS_ERR_OR_NULL(priv->ctx)) {
> +		tee_client_close_session(priv->ctx, priv->session_id);
> +		tee_client_close_context(priv->ctx);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * stm32_bsec_pta_read() - nvmem read access using PTA client driver
> + * @context: nvmem context => romem privdate data
> + * @offset: nvmem offset
> + * @buf: buffer to fill with nvem values
> + * @bytes: number of bytes to read
> + *
> + * Return:
> + *	On success, 0. On failure, -errno.
> + */
> +static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
> +			       size_t bytes)
> +{
> +	struct stm32_romem_priv *romem_priv = context;
> +	struct device *dev;
> +	struct stm32_bsec_pta_priv *priv;
> +	struct tee_shm *shm;
> +	struct tee_ioctl_invoke_arg arg;
> +	struct tee_param param[2];
> +	u8 *shm_buf;
> +	u32 start, num_bytes;
> +	int ret;
> +
> +	dev = romem_priv->ta;
> +	if (!dev) {
> +		pr_err("TA_BSEC invoke with driver\n");
> +		return -ENXIO;
> +	}
> +
> +	priv = dev_get_drvdata(dev);
> +
> +	memset(&arg, 0, sizeof(arg));
> +	memset(&param, 0, sizeof(param));
> +
> +	arg.func = PTA_BSEC_READ_MEM;
> +	arg.session = priv->session_id;
> +	arg.num_params = 2;
> +
> +	/* align access on 32bits */
> +	start = ALIGN_DOWN(offset, 4);
> +	num_bytes = round_up(offset + bytes - start, 4);
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +	param[0].u.value.a = start;
> +	param[0].u.value.b = SHADOW_ACCESS;
> +
> +	shm = tee_shm_alloc_kernel_buf(priv->ctx, num_bytes);
> +	if (IS_ERR(shm))
> +		return PTR_ERR(shm);
> +
> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> +	param[1].u.memref.shm = shm;
> +	param[1].u.memref.size = num_bytes;
> +
> +	ret = tee_client_invoke_func(priv->ctx, &arg, param);
> +	if (ret < 0 || arg.ret != 0) {
> +		dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
> +			arg.ret, ret);
> +		if (!ret)
> +			ret = -EIO;
> +	}
> +	if (!ret) {
> +		shm_buf = tee_shm_get_va(shm, 0);
> +		if (IS_ERR(shm_buf)) {
> +			dev_err(dev, "tee_shm_get_va failed for transmit\n");
> +			ret = PTR_ERR(shm_buf);
> +		} else {
> +			ret = 0;
> +			/* read data from 32 bits aligned buffer */
> +			memcpy(buf, &shm_buf[offset % 4], bytes);
> +		}
> +	}
> +
> +	tee_shm_free(shm);
> +
> +	return ret;
> +}
> +
> +/**
> + * stm32_bsec_pta_write() - nvmem write access using PTA client driver
> + * @context: nvmem context => romem privdate data
> + * @offset: nvmem offset
> + * @buf: buffer with nvem values
> + * @bytes: number of bytes to write
> + *
> + * Return:
> + *	On success, 0. On failure, -errno.
> + */
> +static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
> +				size_t bytes)
> +{
> +	struct stm32_romem_priv *romem_priv = context;
> +	struct device *dev;
> +	struct stm32_bsec_pta_priv *priv;
> +	struct tee_shm *shm;
> +	struct tee_ioctl_invoke_arg arg;
> +	struct tee_param param[2];
> +	u8 *shm_buf;
> +	int ret;
> +
> +	dev = romem_priv->ta;
> +	if (!dev) {
> +		pr_err("TA_BSEC invoke with driver\n");
> +		return -ENXIO;
> +	}
> +
> +	/* Allow only writing complete 32-bits aligned words */
> +	if ((bytes % 4) || (offset % 4))
> +		return -EINVAL;
> +
> +	priv = dev_get_drvdata(dev);
> +
> +	memset(&arg, 0, sizeof(arg));
> +	memset(&param, 0, sizeof(param));
> +
> +	arg.func = PTA_BSEC_WRITE_MEM;
> +	arg.session = priv->session_id;
> +	arg.num_params = 2;
> +
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +	param[0].u.value.a = offset;
> +	param[0].u.value.b = FUSE_ACCESS;
> +
> +	shm = tee_shm_alloc_kernel_buf(priv->ctx, bytes);
> +	if (IS_ERR(shm))
> +		return PTR_ERR(shm);
> +
> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> +	param[1].u.memref.shm = shm;
> +	param[1].u.memref.size = bytes;
> +
> +	shm_buf = tee_shm_get_va(shm, 0);
> +	if (IS_ERR(shm_buf)) {
> +		dev_err(dev, "tee_shm_get_va failed for transmit\n");
> +		return PTR_ERR(shm_buf);
> +	}
> +
> +	memcpy(shm_buf, buf, bytes);
> +
> +	ret = tee_client_invoke_func(priv->ctx, &arg, param);
> +	if (ret < 0 || arg.ret != 0) {
> +		dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
> +			arg.ret, ret);
> +		if (!ret)
> +			ret = -EIO;
> +	}
> +	dev_dbg(dev, "Write OTPs %d to %d, ret=%d\n",
> +		offset / 4, (offset + bytes) / 4, ret);
> +
> +	/* Lock the upper OTPs with ECC protection, word programming only */
> +	if (!ret && ((offset + bytes) >= (romem_priv->lower * 4))) {
> +		u32 start, nb_lock;
> +		u32 *lock = (u32 *)shm_buf;
> +		int i;
> +
> +		/*
> +		 * don't lock the lower OTPs, no ECC protection and incremental
> +		 * bit programming, a second write is allowed
> +		 */
> +		start = max_t(u32, offset, romem_priv->lower * 4);
> +		nb_lock = (offset + bytes - start) / 4;
> +
> +		param[0].u.value.a = start;
> +		param[0].u.value.b = LOCK_ACCESS;
> +		param[1].u.memref.size = nb_lock * 4;
> +
> +		for (i = 0; i < nb_lock; i++)
> +			lock[i] = LOCK_PERM;
> +
> +		ret = tee_client_invoke_func(priv->ctx, &arg, param);
> +		if (ret < 0 || arg.ret != 0) {
> +			dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
> +				arg.ret, ret);
> +			if (!ret)
> +				ret = -EIO;
> +		}
> +		dev_dbg(dev, "Lock upper OTPs %d to %d, ret=%d\n",
> +			start / 4, start / 4 + nb_lock, ret);
> +	}
> +
> +	tee_shm_free(shm);
> +
> +	return ret;
> +}
> +
> +static const struct tee_client_device_id stm32_bsec_id_table[] = {
> +	{
> +		UUID_INIT(0x94cf71ad, 0x80e6, 0x40b5,
> +			  0xa7, 0xc6, 0x3d, 0xc5, 0x01, 0xeb, 0x28, 0x03)
> +	},
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(tee, stm32_bsec_id_table);
> +
> +static struct tee_client_driver stm32_bsec_pta_driver = {
> +	.id_table	= stm32_bsec_id_table,
> +	.driver		= {
> +		.name = "stm32-bsec-pta",
> +		.bus = &tee_bus_type,
> +		.probe = stm32_bsec_pta_probe,
> +		.remove = stm32_bsec_pta_remove,
> +	},
> +};
> +
> +static void stm32_bsec_put_device(void *data)
> +{
> +	put_device(data);
> +}
> +
> +struct device *stm32_bsec_pta_find(struct device *dev)
> +{
> +	struct device *pta_dev;
> +
> +	pta_dev = driver_find_next_device(&stm32_bsec_pta_driver.driver, NULL);
> +
> +	if (pta_dev && devm_add_action_or_reset(dev, stm32_bsec_put_device, pta_dev)) {
> +		dev_err(dev, "unable to register cleanup action\n");
> +
> +		return NULL;
> +	}
> +
> +	return pta_dev;
> +}
> +
> +#else
> +static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
> +			       size_t bytes)
> +{
> +	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
> +
> +	return -ENXIO;
> +}
> +
> +static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
> +				size_t bytes)
> +{
> +	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
> +
> +	return -ENXIO;
> +}
> +
> +struct device *stm32_bsec_pta_find(struct device *dev)
> +{
> +	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
> +
> +	return NULL;
> +}
> +#endif
> +
> +static int __init stm32_romem_init(void)
> +{
> +	int rc;
> +
> +	rc = platform_driver_register(&stm32_romem_driver);
> +	if (rc)
> +		return rc;
> +
> +#if IS_ENABLED(CONFIG_OPTEE)
> +	rc = driver_register(&stm32_bsec_pta_driver.driver);
> +#endif
> +
> +	return rc;
> +}
> +
> +static void __exit stm32_romem_exit(void)
> +{
> +	platform_driver_unregister(&stm32_romem_driver);
> +#if IS_ENABLED(CONFIG_OPTEE)
> +	driver_unregister(&stm32_bsec_pta_driver.driver);
> +#endif
> +}
> +
> +module_init(stm32_romem_init);
> +module_exit(stm32_romem_exit);
>   
>   MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
>   MODULE_DESCRIPTION("STMicroelectronics STM32 RO-MEM");

Re: [PATCH 3/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Patrick DELAUNAY]

Hi,

On 11/1/22 08:26, Srinivas Kandagatla wrote:
>
>
> On 28/10/2022 15:52, Patrick Delaunay wrote:
>> For boot with OP-TEE on STM32MP13, the communication with the secure
>> world no more use STMicroelectronics SMC but communication with the
>> BSEC TA, for data access (read/write) or lock operation:
>> - all the request are sent to OP-TEE trusted application,
>> - for upper OTP with ECC protection and with word programming only
>>    each OTP are permanently locked when programmed to avoid ECC error
>>    on the second write operation
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> ---
>
> For some reason I pushed this patch without a full review, This is now 
> reverted from nvmem-next.


Ok


>
> Why not add TEE client based new driver instead of ifdefing around 
> this driver? Also I see there is not much common across both drivers 
> anyway.


I hesitate between the 2 solutions. I choose this update to handle the 
STM32MP15 support with OP-TEE.

For backward compatibility reason the same driver STM32 ROMEM associated 
to compatible "st,stm32mp15-bsec" should be kept.

- the lower OTP can directly accessible by Linux (the IP is not secured) 
=> boot with SPL

- the upper OTP and the write operation are requested by 
STMicroelectronics SMCs

    => boot with TF-A SPMIN and old OP-TEE (before migration to STM32 
BSEC PTA)


But in the future OP-TEE the access to OTP should be also done with 
STM32 BSEC PTA...


I can manage this compatibility by detection in STM32 romem driver if 
the booth access are managed in the same driver.

This patch can be added in the serie to understood the detection mechanism.


>
>
>>
>>   drivers/nvmem/stm32-romem.c | 450 +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 446 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c
>> index 6de565639d5f..dfdedbcca9b9 100644
>> --- a/drivers/nvmem/stm32-romem.c
>> +++ b/drivers/nvmem/stm32-romem.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/module.h>
>>   #include <linux/nvmem-provider.h>
>>   #include <linux/of_device.h>
>> +#include <linux/tee_drv.h>
>>     /* BSEC secure service access from non-secure */
>>   #define STM32_SMC_BSEC            0x82001003
>> @@ -25,14 +26,22 @@
>>   struct stm32_romem_cfg {
>>       int size;
>>       u8 lower;
>> +    bool ta;
>>   };
>>     struct stm32_romem_priv {
>>       void __iomem *base;
>>       struct nvmem_config cfg;
>>       u8 lower;
>> +    struct device *ta;
>>   };
>>   +struct device *stm32_bsec_pta_find(struct device *dev);
>> +static int stm32_bsec_pta_read(void *context, unsigned int offset, 
>> void *buf,
>> +                   size_t bytes);
>> +static int stm32_bsec_pta_write(void *context, unsigned int offset, 
>> void *buf,
>> +                size_t bytes);
>> +
>>   static int stm32_romem_read(void *context, unsigned int offset, 
>> void *buf,
>>                   size_t bytes)
>>   {
>> @@ -173,15 +182,25 @@ static int stm32_romem_probe(struct 
>> platform_device *pdev)
>>       } else {
>>           priv->cfg.size = cfg->size;
>>           priv->lower = cfg->lower;
>> -        priv->cfg.reg_read = stm32_bsec_read;
>> -        priv->cfg.reg_write = stm32_bsec_write;
>> +        if (cfg->ta) {
>> +            priv->ta = stm32_bsec_pta_find(dev);
>> +            /* wait for OP-TEE client driver to be up and ready */
>> +            if (!priv->ta)
>> +                return -EPROBE_DEFER;
>> +
>> +            priv->cfg.reg_read = stm32_bsec_pta_read;
>> +            priv->cfg.reg_write = stm32_bsec_pta_write;
>> +        } else {
>> +            priv->cfg.reg_read = stm32_bsec_read;
>> +            priv->cfg.reg_write = stm32_bsec_write;
>> +        }
>>       }
>>         return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &priv->cfg));
>>   }
>>     /*
>> - * STM32MP15 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
>> + * STM32MP15/13 BSEC OTP regions: 4096 OTP bits (with 3072 effective 
>> bits)
>>    * => 96 x 32-bits data words
>>    * - Lower: 1K bits, 2:1 redundancy, incremental bit programming
>>    *   => 32 (x 32-bits) lower shadow registers = words 0 to 31
>> @@ -191,6 +210,13 @@ static int stm32_romem_probe(struct 
>> platform_device *pdev)
>>   static const struct stm32_romem_cfg stm32mp15_bsec_cfg = {
>>       .size = 384,
>>       .lower = 32,
>> +    .ta = false,
>> +};
>> +
>> +static const struct stm32_romem_cfg stm32mp13_bsec_cfg = {
>> +    .size = 384,
>> +    .lower = 32,
>> +    .ta = true,
>>   };
>>     static const struct of_device_id stm32_romem_of_match[] = {
>> @@ -198,6 +224,8 @@ static const struct of_device_id 
>> stm32_romem_of_match[] = {
>>           .compatible = "st,stm32mp15-bsec",
>>           .data = (void *)&stm32mp15_bsec_cfg,
>>       }, {
>> +        .compatible = "st,stm32mp13-bsec",
>> +        .data = (void *)&stm32mp13_bsec_cfg,
>
> missing sentinel, which caused a regression in next.


Ok, sorry for my error in the rebase conflict.


Patrick


>
>
> --srini
>>       },
>>   };
>>   MODULE_DEVICE_TABLE(of, stm32_romem_of_match);
>> @@ -209,7 +237,421 @@ static struct platform_driver 
>> stm32_romem_driver = {
>>           .of_match_table = of_match_ptr(stm32_romem_of_match),
>>       },
>>   };
>> -module_platform_driver(stm32_romem_driver);
>> +
>> +#if IS_ENABLED(CONFIG_OPTEE)
>>
....


>> +
>> +module_init(stm32_romem_init);
>> +module_exit(stm32_romem_exit);
>>     MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
>>   MODULE_DESCRIPTION("STMicroelectronics STM32 RO-MEM");

Re: [PATCH 3/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Srinivas Kandagatla]

On 02/11/2022 10:59, Patrick DELAUNAY wrote:
> Hi,
> 
> On 11/1/22 08:26, Srinivas Kandagatla wrote:
>>
>>
>> On 28/10/2022 15:52, Patrick Delaunay wrote:
>>> For boot with OP-TEE on STM32MP13, the communication with the secure
>>> world no more use STMicroelectronics SMC but communication with the
>>> BSEC TA, for data access (read/write) or lock operation:
>>> - all the request are sent to OP-TEE trusted application,
>>> - for upper OTP with ECC protection and with word programming only
>>>    each OTP are permanently locked when programmed to avoid ECC error
>>>    on the second write operation
>>>
>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>> ---
>>
>> For some reason I pushed this patch without a full review, This is now 
>> reverted from nvmem-next.
> 
> 
> Ok
> 
> 
>>
>> Why not add TEE client based new driver instead of ifdefing around 
>> this driver? Also I see there is not much common across both drivers 
>> anyway.
> 
> 
> I hesitate between the 2 solutions. I choose this update to handle the 
> STM32MP15 support with OP-TEE.

How are you to handing this?

> 
> For backward compatibility reason the same driver STM32 ROMEM associated 
> to compatible "st,stm32mp15-bsec" should be kept.
> 
> - the lower OTP can directly accessible by Linux (the IP is not secured) 
> => boot with SPL

Can we determine this at runtime?

> 
> - the upper OTP and the write operation are requested by 
> STMicroelectronics SMCs
> 
>     => boot with TF-A SPMIN and old OP-TEE (before migration to STM32 
> BSEC PTA)
> 
> 
> But in the future OP-TEE the access to OTP should be also done with 
> STM32 BSEC PTA...

Given that we have only one compatible for these two type of 
combinations how are you planning to deal with both the cases and still 
be backward compatible?

--srini
> 
> 
> I can manage this compatibility by detection in STM32 romem driver if 
> the booth access are managed in the same driver.



> 
> This patch can be added in the serie to understood the detection mechanism.
> 
> 
>>
>>
>>>
>>>   drivers/nvmem/stm32-romem.c | 450 +++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 446 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c
>>> index 6de565639d5f..dfdedbcca9b9 100644
>>> --- a/drivers/nvmem/stm32-romem.c
>>> +++ b/drivers/nvmem/stm32-romem.c
>>> @@ -11,6 +11,7 @@
>>>   #include <linux/module.h>
>>>   #include <linux/nvmem-provider.h>
>>>   #include <linux/of_device.h>
>>> +#include <linux/tee_drv.h>
>>>     /* BSEC secure service access from non-secure */
>>>   #define STM32_SMC_BSEC            0x82001003
>>> @@ -25,14 +26,22 @@
>>>   struct stm32_romem_cfg {
>>>       int size;
>>>       u8 lower;
>>> +    bool ta;
>>>   };
>>>     struct stm32_romem_priv {
>>>       void __iomem *base;
>>>       struct nvmem_config cfg;
>>>       u8 lower;
>>> +    struct device *ta;
>>>   };
>>>   +struct device *stm32_bsec_pta_find(struct device *dev);
>>> +static int stm32_bsec_pta_read(void *context, unsigned int offset, 
>>> void *buf,
>>> +                   size_t bytes);
>>> +static int stm32_bsec_pta_write(void *context, unsigned int offset, 
>>> void *buf,
>>> +                size_t bytes);
>>> +
>>>   static int stm32_romem_read(void *context, unsigned int offset, 
>>> void *buf,
>>>                   size_t bytes)
>>>   {
>>> @@ -173,15 +182,25 @@ static int stm32_romem_probe(struct 
>>> platform_device *pdev)
>>>       } else {
>>>           priv->cfg.size = cfg->size;
>>>           priv->lower = cfg->lower;
>>> -        priv->cfg.reg_read = stm32_bsec_read;
>>> -        priv->cfg.reg_write = stm32_bsec_write;
>>> +        if (cfg->ta) {
>>> +            priv->ta = stm32_bsec_pta_find(dev);
>>> +            /* wait for OP-TEE client driver to be up and ready */
>>> +            if (!priv->ta)
>>> +                return -EPROBE_DEFER;
>>> +
>>> +            priv->cfg.reg_read = stm32_bsec_pta_read;
>>> +            priv->cfg.reg_write = stm32_bsec_pta_write;
>>> +        } else {
>>> +            priv->cfg.reg_read = stm32_bsec_read;
>>> +            priv->cfg.reg_write = stm32_bsec_write;
>>> +        }
>>>       }
>>>         return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &priv->cfg));
>>>   }
>>>     /*
>>> - * STM32MP15 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
>>> + * STM32MP15/13 BSEC OTP regions: 4096 OTP bits (with 3072 effective 
>>> bits)
>>>    * => 96 x 32-bits data words
>>>    * - Lower: 1K bits, 2:1 redundancy, incremental bit programming
>>>    *   => 32 (x 32-bits) lower shadow registers = words 0 to 31
>>> @@ -191,6 +210,13 @@ static int stm32_romem_probe(struct 
>>> platform_device *pdev)
>>>   static const struct stm32_romem_cfg stm32mp15_bsec_cfg = {
>>>       .size = 384,
>>>       .lower = 32,
>>> +    .ta = false,
>>> +};
>>> +
>>> +static const struct stm32_romem_cfg stm32mp13_bsec_cfg = {
>>> +    .size = 384,
>>> +    .lower = 32,
>>> +    .ta = true,
>>>   };
>>>     static const struct of_device_id stm32_romem_of_match[] = {
>>> @@ -198,6 +224,8 @@ static const struct of_device_id 
>>> stm32_romem_of_match[] = {
>>>           .compatible = "st,stm32mp15-bsec",
>>>           .data = (void *)&stm32mp15_bsec_cfg,
>>>       }, {
>>> +        .compatible = "st,stm32mp13-bsec",
>>> +        .data = (void *)&stm32mp13_bsec_cfg,
>>
>> missing sentinel, which caused a regression in next.
> 
> 
> Ok, sorry for my error in the rebase conflict.
> 
> 
> Patrick
> 
> 
>>
>>
>> --srini
>>>       },
>>>   };
>>>   MODULE_DEVICE_TABLE(of, stm32_romem_of_match);
>>> @@ -209,7 +237,421 @@ static struct platform_driver 
>>> stm32_romem_driver = {
>>>           .of_match_table = of_match_ptr(stm32_romem_of_match),
>>>       },
>>>   };
>>> -module_platform_driver(stm32_romem_driver);
>>> +
>>> +#if IS_ENABLED(CONFIG_OPTEE)
>>>
> ....
> 
> 
>>> +
>>> +module_init(stm32_romem_init);
>>> +module_exit(stm32_romem_exit);
>>>     MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
>>>   MODULE_DESCRIPTION("STMicroelectronics STM32 RO-MEM");

Re: [PATCH 3/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Patrick DELAUNAY]

Hi,

On 11/8/22 11:03, Srinivas Kandagatla wrote:
>
>
> On 02/11/2022 10:59, Patrick DELAUNAY wrote:
>> Hi,
>>
>> On 11/1/22 08:26, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 28/10/2022 15:52, Patrick Delaunay wrote:
>>>> For boot with OP-TEE on STM32MP13, the communication with the secure
>>>> world no more use STMicroelectronics SMC but communication with the
>>>> BSEC TA, for data access (read/write) or lock operation:
>>>> - all the request are sent to OP-TEE trusted application,
>>>> - for upper OTP with ECC protection and with word programming only
>>>>    each OTP are permanently locked when programmed to avoid ECC error
>>>>    on the second write operation
>>>>
>>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>> ---
>>>
>>> For some reason I pushed this patch without a full review, This is 
>>> now reverted from nvmem-next.
>>
>>
>> Ok
>>
>>
>>>
>>> Why not add TEE client based new driver instead of ifdefing around 
>>> this driver? Also I see there is not much common across both drivers 
>>> anyway.
>>
>>
>> I hesitate between the 2 solutions. I choose this update to handle 
>> the STM32MP15 support with OP-TEE.
>
> How are you to handing this?
>
>>
>> For backward compatibility reason the same driver STM32 ROMEM 
>> associated to compatible "st,stm32mp15-bsec" should be kept.
>>
>> - the lower OTP can directly accessible by Linux (the IP is not 
>> secured) => boot with SPL
>
> Can we determine this at runtime?


Not directly with IP register, but we detect the OP-TEE support at runtime.


>
>>
>> - the upper OTP and the write operation are requested by 
>> STMicroelectronics SMCs
>>
>>     => boot with TF-A SPMIN and old OP-TEE (before migration to STM32 
>> BSEC PTA)
>>
>>
>> But in the future OP-TEE the access to OTP should be also done with 
>> STM32 BSEC PTA...
>
> Given that we have only one compatible for these two type of 
> combinations how are you planning to deal with both the cases and 
> still be backward compatible?


yes, as it is the SOC dtsi, I don't want change the IP compatible

but I will handle in this driver probe to be backward compatible: 
detection of OP-TEE presence and SMC/PTA support.


only the STM32MP13 SoC only use the PTA, as we have no backward 
compatibility issue.


I will include this STM32MP15 patch in the serie for V2, it should be 
mode clear for the review.


>
> --srini
>>
>>
>> I can manage this compatibility by detection in STM32 romem driver if 
>> the booth access are managed in the same driver.
>
>
>
>>
>> This patch can be added in the serie to understood the detection 
>> mechanism.
>>

Regards

Patrick

Re: [PATCH 3/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Srinivas Kandagatla]

On 09/11/2022 17:35, Patrick DELAUNAY wrote:
>>>
>>>
>>>>
>>>> Why not add TEE client based new driver instead of ifdefing around 
>>>> this driver? Also I see there is not much common across both drivers 
>>>> anyway.
>>>
>>>
>>> I hesitate between the 2 solutions. I choose this update to handle 
>>> the STM32MP15 support with OP-TEE.
>>
>> How are you to handing this?
>>
>>>
>>> For backward compatibility reason the same driver STM32 ROMEM 
>>> associated to compatible "st,stm32mp15-bsec" should be kept.
>>>
>>> - the lower OTP can directly accessible by Linux (the IP is not 
>>> secured) => boot with SPL
>>
>> Can we determine this at runtime?
> 
> 
> Not directly with IP register, but we detect the OP-TEE support at runtime.
> 
You mean using CONFIG_OP_TEE ?


> 
>>
>>>
>>> - the upper OTP and the write operation are requested by 
>>> STMicroelectronics SMCs
>>>
>>>     => boot with TF-A SPMIN and old OP-TEE (before migration to STM32 
>>> BSEC PTA)
>>>
>>>
>>> But in the future OP-TEE the access to OTP should be also done with 
>>> STM32 BSEC PTA...
>>
>> Given that we have only one compatible for these two type of 
>> combinations how are you planning to deal with both the cases and 
>> still be backward compatible?

Re: [Linux-stm32] [PATCH 3/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Fabrice Gasnier]

On 10/28/22 16:52, Patrick Delaunay wrote:
> For boot with OP-TEE on STM32MP13, the communication with the secure
> world no more use STMicroelectronics SMC but communication with the
> BSEC TA, for data access (read/write) or lock operation:
> - all the request are sent to OP-TEE trusted application,
> - for upper OTP with ECC protection and with word programming only
>   each OTP are permanently locked when programmed to avoid ECC error
>   on the second write operation
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
> 
>  drivers/nvmem/stm32-romem.c | 450 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 446 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c
> index 6de565639d5f..dfdedbcca9b9 100644
> --- a/drivers/nvmem/stm32-romem.c
> +++ b/drivers/nvmem/stm32-romem.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/nvmem-provider.h>
>  #include <linux/of_device.h>
> +#include <linux/tee_drv.h>
>  
>  /* BSEC secure service access from non-secure */
>  #define STM32_SMC_BSEC			0x82001003
> @@ -25,14 +26,22 @@
>  struct stm32_romem_cfg {
>  	int size;
>  	u8 lower;
> +	bool ta;
>  };
>  
>  struct stm32_romem_priv {
>  	void __iomem *base;
>  	struct nvmem_config cfg;
>  	u8 lower;
> +	struct device *ta;
>  };
>  
> +struct device *stm32_bsec_pta_find(struct device *dev);
> +static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
> +			       size_t bytes);
> +static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
> +				size_t bytes);
> +

Hi Patrick,

Please find some suggestions and few remarks here below.

Adding prototypes could be avoided, by moving these new routines:
- stm32_bsec_pta_find
- stm32_bsec_pta_read
- stm32_bsec_pta_write

>  static int stm32_romem_read(void *context, unsigned int offset, void *buf,
>  			    size_t bytes)
>  {
> @@ -173,15 +182,25 @@ static int stm32_romem_probe(struct platform_device *pdev)
>  	} else {
>  		priv->cfg.size = cfg->size;
>  		priv->lower = cfg->lower;
> -		priv->cfg.reg_read = stm32_bsec_read;
> -		priv->cfg.reg_write = stm32_bsec_write;
> +		if (cfg->ta) {
> +			priv->ta = stm32_bsec_pta_find(dev);
> +			/* wait for OP-TEE client driver to be up and ready */
> +			if (!priv->ta)
> +				return -EPROBE_DEFER;
> +
> +			priv->cfg.reg_read = stm32_bsec_pta_read;
> +			priv->cfg.reg_write = stm32_bsec_pta_write;
> +		} else {
> +			priv->cfg.reg_read = stm32_bsec_read;
> +			priv->cfg.reg_write = stm32_bsec_write;
> +		}
>  	}
>  
>  	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &priv->cfg));
>  }
>  
>  /*
> - * STM32MP15 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
> + * STM32MP15/13 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
>   * => 96 x 32-bits data words
>   * - Lower: 1K bits, 2:1 redundancy, incremental bit programming
>   *   => 32 (x 32-bits) lower shadow registers = words 0 to 31
> @@ -191,6 +210,13 @@ static int stm32_romem_probe(struct platform_device *pdev)
>  static const struct stm32_romem_cfg stm32mp15_bsec_cfg = {
>  	.size = 384,
>  	.lower = 32,
> +	.ta = false,
> +};
> +
> +static const struct stm32_romem_cfg stm32mp13_bsec_cfg = {
> +	.size = 384,
> +	.lower = 32,
> +	.ta = true,
>  };
>  
>  static const struct of_device_id stm32_romem_of_match[] = {
> @@ -198,6 +224,8 @@ static const struct of_device_id stm32_romem_of_match[] = {
>  		.compatible = "st,stm32mp15-bsec",
>  		.data = (void *)&stm32mp15_bsec_cfg,
>  	}, {
> +		.compatible = "st,stm32mp13-bsec",
> +		.data = (void *)&stm32mp13_bsec_cfg,
>  	},
>  };
>  MODULE_DEVICE_TABLE(of, stm32_romem_of_match);
> @@ -209,7 +237,421 @@ static struct platform_driver stm32_romem_driver = {
>  		.of_match_table = of_match_ptr(stm32_romem_of_match),
>  	},
>  };
> -module_platform_driver(stm32_romem_driver);
> +
> +#if IS_ENABLED(CONFIG_OPTEE)
> +/*************************************************************************
> + * BSEC PTA : OP-TEE client driver to pseudo trusted application
> + *************************************************************************/

Just suggesting: could improve readability on above comment ?
(mutli-line comment starting with an emtpy /*. But here there's a single
line.

> +
> +/*
> + * Read OTP memory
> + *
> + * [in]		value[0].a		OTP start offset in byte
> + * [in]		value[0].b		Access type (0 : shadow,
> + *					1 : fuse, 2 : lock)

Suggestion: fits into 100 chars line

> + * [out]	memref[1].buffer	Output buffer to store read values
> + * [out]	memref[1].size		Size of OTP to be read
> + *
> + * Return codes:
> + * TEE_SUCCESS - Invoke command success
> + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
> + */
> +#define PTA_BSEC_READ_MEM		0x0 /* Read OTP */
> +
> +/*
> + * Write OTP memory
> + *
> + * [in]		value[0].a		OTP start offset in byte
> + * [in]		value[0].b		Access type (0 : shadow,
> + *					1 : fuse, 2 : lock)

same here

> + * [in]		memref[1].buffer	Input buffer to read values
> + * [in]		memref[1].size		Size of OTP to be written
> + *
> + * Return codes:
> + * TEE_SUCCESS - Invoke command success
> + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
> + */
> +#define PTA_BSEC_WRITE_MEM		0x1	/* Write OTP */
> +
> +/* value of PTA_BSEC access type = value[in] b */
> +#define SHADOW_ACCESS	0
> +#define FUSE_ACCESS	1
> +#define LOCK_ACCESS	2
> +
> +/* Bitfield definition for LOCK status */
> +#define LOCK_PERM			BIT(30)
> +
> +/**
> + * struct stm32_bsec_pta_priv - OP-TEE BSEC TA private data
> + * @ctx:		OP-TEE context handler.
> + * @session_id:		TA session identifier.
> + */
> +struct stm32_bsec_pta_priv {
> +	struct tee_context *ctx;
> +	u32 session_id;
> +};
> +
> +/*
> + * Check whether this driver supports the BSEC TA in the TEE instance
> + * represented by the params (ver/data) to this function.
> + */
> +static int stm32_bsec_pta_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> +	/*
> +	 * Currently this driver only supports GP compliant, OP-TEE based TA
> +	 */

/* suggestion: single line comment */

> +	if ((ver->impl_id == TEE_IMPL_ID_OPTEE) &&
> +		(ver->gen_caps & TEE_GEN_CAP_GP))
> +		return 1;
> +	else
> +		return 0;
> +}
> +
> +/**
> + * stm32_bsec_pta_probe() - initialize the PTA BSEC
> + * @dev: the platform_device description.
> + *
> + * Return:
> + *	On success, 0. On failure, -errno.
> + */
> +static int stm32_bsec_pta_probe(struct device *dev)
> +{
> +	int rc;
> +	struct tee_ioctl_open_session_arg sess_arg;
> +	struct tee_client_device *tee_device = to_tee_client_device(dev);
> +	struct stm32_bsec_pta_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	/* Open context with TEE driver */
> +	priv->ctx = tee_client_open_context(NULL, stm32_bsec_pta_match, NULL, NULL);
> +	if (IS_ERR(priv->ctx)) {
> +		if (PTR_ERR(priv->ctx) == -ENOENT)
> +			return -EPROBE_DEFER;
> +		dev_err(dev, "%s: tee_client_open_context failed\n", __func__);
> +		return PTR_ERR(priv->ctx);
> +	}
> +
> +	/* Open a session with BSEC TA */
> +	memset(&sess_arg, 0, sizeof(sess_arg));
> +	export_uuid(sess_arg.uuid, &tee_device->id.uuid);
> +	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> +	sess_arg.num_params = 0;
> +
> +	rc = tee_client_open_session(priv->ctx, &sess_arg, NULL);
> +	if ((rc < 0) || (sess_arg.ret != 0)) {
> +		dev_err(dev, "%s: tee_client_open_session failed, err=%x\n",
> +			__func__, sess_arg.ret);
> +		rc = -EINVAL;
> +		goto out_tee_session;
> +	}
> +	priv->session_id = sess_arg.session;
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +
> +out_tee_session:
> +	tee_client_close_context(priv->ctx);
> +	priv->ctx = NULL;
> +
> +	return rc;
> +}
> +
> +/**
> + * stm32_bsec_pta_remove() - remove the BSEC TEE device
> + * @dev: the platform_device description.
> + *
> + * Return:
> + *	0 always.
> + */
> +static int stm32_bsec_pta_remove(struct device *dev)
> +{
> +	struct stm32_bsec_pta_priv *priv = dev_get_drvdata(dev);
> +
> +	if (!IS_ERR_OR_NULL(priv->ctx)) {
> +		tee_client_close_session(priv->ctx, priv->session_id);
> +		tee_client_close_context(priv->ctx);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * stm32_bsec_pta_read() - nvmem read access using PTA client driver
> + * @context: nvmem context => romem privdate data
> + * @offset: nvmem offset
> + * @buf: buffer to fill with nvem values
> + * @bytes: number of bytes to read
> + *
> + * Return:
> + *	On success, 0. On failure, -errno.
> + */
> +static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
> +			       size_t bytes)
> +{
> +	struct stm32_romem_priv *romem_priv = context;
> +	struct device *dev;
> +	struct stm32_bsec_pta_priv *priv;
> +	struct tee_shm *shm;
> +	struct tee_ioctl_invoke_arg arg;
> +	struct tee_param param[2];
> +	u8 *shm_buf;
> +	u32 start, num_bytes;
> +	int ret;
> +
> +	dev = romem_priv->ta;
> +	if (!dev) {
> +		pr_err("TA_BSEC invoke with driver\n");
> +		return -ENXIO;
> +	}
> +
> +	priv = dev_get_drvdata(dev);
> +
> +	memset(&arg, 0, sizeof(arg));
> +	memset(&param, 0, sizeof(param));
> +
> +	arg.func = PTA_BSEC_READ_MEM;
> +	arg.session = priv->session_id;
> +	arg.num_params = 2;
> +
> +	/* align access on 32bits */
> +	start = ALIGN_DOWN(offset, 4);
> +	num_bytes = round_up(offset + bytes - start, 4);
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +	param[0].u.value.a = start;
> +	param[0].u.value.b = SHADOW_ACCESS;
> +
> +	shm = tee_shm_alloc_kernel_buf(priv->ctx, num_bytes);
> +	if (IS_ERR(shm))
> +		return PTR_ERR(shm);
> +
> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> +	param[1].u.memref.shm = shm;
> +	param[1].u.memref.size = num_bytes;
> +
> +	ret = tee_client_invoke_func(priv->ctx, &arg, param);
> +	if (ret < 0 || arg.ret != 0) {
> +		dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
> +			arg.ret, ret);
> +		if (!ret)
> +			ret = -EIO;
> +	}
> +	if (!ret) {
> +		shm_buf = tee_shm_get_va(shm, 0);
> +		if (IS_ERR(shm_buf)) {
> +			dev_err(dev, "tee_shm_get_va failed for transmit\n");
> +			ret = PTR_ERR(shm_buf);
> +		} else {
> +			ret = 0;

ret = 0; maybe removed ? (ret is already 0 here)

> +			/* read data from 32 bits aligned buffer */
> +			memcpy(buf, &shm_buf[offset % 4], bytes);
> +		}
> +	}
> +
> +	tee_shm_free(shm);
> +
> +	return ret;
> +}
> +
> +/**
> + * stm32_bsec_pta_write() - nvmem write access using PTA client driver
> + * @context: nvmem context => romem privdate data
> + * @offset: nvmem offset
> + * @buf: buffer with nvem values
> + * @bytes: number of bytes to write
> + *
> + * Return:
> + *	On success, 0. On failure, -errno.
> + */
> +static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
> +				size_t bytes)
> +{
> +	struct stm32_romem_priv *romem_priv = context;
> +	struct device *dev;
> +	struct stm32_bsec_pta_priv *priv;
> +	struct tee_shm *shm;
> +	struct tee_ioctl_invoke_arg arg;
> +	struct tee_param param[2];
> +	u8 *shm_buf;
> +	int ret;
> +
> +	dev = romem_priv->ta;
> +	if (!dev) {
> +		pr_err("TA_BSEC invoke with driver\n");
> +		return -ENXIO;
> +	}
> +
> +	/* Allow only writing complete 32-bits aligned words */
> +	if ((bytes % 4) || (offset % 4))
> +		return -EINVAL;
> +
> +	priv = dev_get_drvdata(dev);
> +
> +	memset(&arg, 0, sizeof(arg));
> +	memset(&param, 0, sizeof(param));
> +
> +	arg.func = PTA_BSEC_WRITE_MEM;
> +	arg.session = priv->session_id;
> +	arg.num_params = 2;
> +
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +	param[0].u.value.a = offset;
> +	param[0].u.value.b = FUSE_ACCESS;
> +
> +	shm = tee_shm_alloc_kernel_buf(priv->ctx, bytes);
> +	if (IS_ERR(shm))
> +		return PTR_ERR(shm);
> +
> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> +	param[1].u.memref.shm = shm;
> +	param[1].u.memref.size = bytes;
> +
> +	shm_buf = tee_shm_get_va(shm, 0);
> +	if (IS_ERR(shm_buf)) {
> +		dev_err(dev, "tee_shm_get_va failed for transmit\n");

Need to add error handling: tee_shm_free(shm);

> +		return PTR_ERR(shm_buf);
> +	}
> +
> +	memcpy(shm_buf, buf, bytes);
> +
> +	ret = tee_client_invoke_func(priv->ctx, &arg, param);
> +	if (ret < 0 || arg.ret != 0) {
> +		dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
> +			arg.ret, ret);
> +		if (!ret)
> +			ret = -EIO;
> +	}
> +	dev_dbg(dev, "Write OTPs %d to %d, ret=%d\n",
> +		offset / 4, (offset + bytes) / 4, ret);
> +
> +	/* Lock the upper OTPs with ECC protection, word programming only */
> +	if (!ret && ((offset + bytes) >= (romem_priv->lower * 4))) {
> +		u32 start, nb_lock;
> +		u32 *lock = (u32 *)shm_buf;
> +		int i;
> +
> +		/*
> +		 * don't lock the lower OTPs, no ECC protection and incremental
> +		 * bit programming, a second write is allowed
> +		 */
> +		start = max_t(u32, offset, romem_priv->lower * 4);
> +		nb_lock = (offset + bytes - start) / 4;
> +
> +		param[0].u.value.a = start;
> +		param[0].u.value.b = LOCK_ACCESS;
> +		param[1].u.memref.size = nb_lock * 4;
> +
> +		for (i = 0; i < nb_lock; i++)
> +			lock[i] = LOCK_PERM;
> +
> +		ret = tee_client_invoke_func(priv->ctx, &arg, param);
> +		if (ret < 0 || arg.ret != 0) {
> +			dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
> +				arg.ret, ret);
> +			if (!ret)
> +				ret = -EIO;
> +		}
> +		dev_dbg(dev, "Lock upper OTPs %d to %d, ret=%d\n",
> +			start / 4, start / 4 + nb_lock, ret);
> +	}
> +
> +	tee_shm_free(shm);
> +
> +	return ret;
> +}
> +
> +static const struct tee_client_device_id stm32_bsec_id_table[] = {
> +	{
> +		UUID_INIT(0x94cf71ad, 0x80e6, 0x40b5,
> +			  0xa7, 0xc6, 0x3d, 0xc5, 0x01, 0xeb, 0x28, 0x03)
> +	},
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(tee, stm32_bsec_id_table);
> +
> +static struct tee_client_driver stm32_bsec_pta_driver = {
> +	.id_table	= stm32_bsec_id_table,
> +	.driver		= {
> +		.name = "stm32-bsec-pta",
> +		.bus = &tee_bus_type,
> +		.probe = stm32_bsec_pta_probe,
> +		.remove = stm32_bsec_pta_remove,
> +	},
> +};
> +
> +static void stm32_bsec_put_device(void *data)
> +{
> +	put_device(data);
> +}
> +
> +struct device *stm32_bsec_pta_find(struct device *dev)
> +{
> +	struct device *pta_dev;
> +
> +	pta_dev = driver_find_next_device(&stm32_bsec_pta_driver.driver, NULL);
> +
> +	if (pta_dev && devm_add_action_or_reset(dev, stm32_bsec_put_device, pta_dev)) {
> +		dev_err(dev, "unable to register cleanup action\n");

I suggest to improve the error message here, to make it more explicit,
and also propagate the error code to the caller.
Something like:
ret = devm_add_action_or_reset()
if (ret) {
    dev_err(dev, "devm_add_action_or_reset() failed (%d)\n", ret );
    return ret;
}
This may avoid turning a real error here, into a EPROBE_DEFER at the
caller side, e.g.:
	if (!priv->ta)
		return -EPROBE_DEFER;

Thanks & Best Regards,
Fabrice

> +
> +		return NULL;
> +	}
> +
> +	return pta_dev;
> +}
> +
> +#else
> +static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
> +			       size_t bytes)
> +{
> +	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
> +
> +	return -ENXIO;
> +}
> +
> +static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
> +				size_t bytes)
> +{
> +	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
> +
> +	return -ENXIO;
> +}
> +
> +struct device *stm32_bsec_pta_find(struct device *dev)
> +{
> +	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
> +
> +	return NULL;
> +}
> +#endif
> +
> +static int __init stm32_romem_init(void)
> +{
> +	int rc;
> +
> +	rc = platform_driver_register(&stm32_romem_driver);
> +	if (rc)
> +		return rc;
> +
> +#if IS_ENABLED(CONFIG_OPTEE)
> +	rc = driver_register(&stm32_bsec_pta_driver.driver);
> +#endif
> +
> +	return rc;
> +}
> +
> +static void __exit stm32_romem_exit(void)
> +{
> +	platform_driver_unregister(&stm32_romem_driver);
> +#if IS_ENABLED(CONFIG_OPTEE)
> +	driver_unregister(&stm32_bsec_pta_driver.driver);
> +#endif
> +}
> +
> +module_init(stm32_romem_init);
> +module_exit(stm32_romem_exit);
>  
>  MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
>  MODULE_DESCRIPTION("STMicroelectronics STM32 RO-MEM");

Re: [Linux-stm32] [PATCH 3/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Patrick DELAUNAY]

Hi Fabrice,


Thanks for the review,

I am preparing a V2 with your remarks.


Patrick


On 11/3/22 14:18, Fabrice Gasnier wrote:
> On 10/28/22 16:52, Patrick Delaunay wrote:
>> For boot with OP-TEE on STM32MP13, the communication with the secure
>> world no more use STMicroelectronics SMC but communication with the
>> BSEC TA, for data access (read/write) or lock operation:
>> - all the request are sent to OP-TEE trusted application,
>> - for upper OTP with ECC protection and with word programming only
>>    each OTP are permanently locked when programmed to avoid ECC error
>>    on the second write operation
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> ---
>>
>>   drivers/nvmem/stm32-romem.c | 450 +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 446 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c
>> index 6de565639d5f..dfdedbcca9b9 100644
>> --- a/drivers/nvmem/stm32-romem.c
>> +++ b/drivers/nvmem/stm32-romem.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/module.h>
>>   #include <linux/nvmem-provider.h>
>>   #include <linux/of_device.h>
>> +#include <linux/tee_drv.h>
>>   
>>   /* BSEC secure service access from non-secure */
>>   #define STM32_SMC_BSEC			0x82001003
>> @@ -25,14 +26,22 @@
>>   struct stm32_romem_cfg {
>>   	int size;
>>   	u8 lower;
>> +	bool ta;
>>   };
>>   
>>   struct stm32_romem_priv {
>>   	void __iomem *base;
>>   	struct nvmem_config cfg;
>>   	u8 lower;
>> +	struct device *ta;
>>   };
>>   
>> +struct device *stm32_bsec_pta_find(struct device *dev);
>> +static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
>> +			       size_t bytes);
>> +static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
>> +				size_t bytes);
>> +
> Hi Patrick,
>
> Please find some suggestions and few remarks here below.
>
> Adding prototypes could be avoided, by moving these new routines:
> - stm32_bsec_pta_find
> - stm32_bsec_pta_read
> - stm32_bsec_pta_write


ok


>
>>   static int stm32_romem_read(void *context, unsigned int offset, void *buf,
>>   			    size_t bytes)
>>   {
>> @@ -173,15 +182,25 @@ static int stm32_romem_probe(struct platform_device *pdev)
>>   	} else {
>>   		priv->cfg.size = cfg->size;
>>   		priv->lower = cfg->lower;
>> -		priv->cfg.reg_read = stm32_bsec_read;
>> -		priv->cfg.reg_write = stm32_bsec_write;
>> +		if (cfg->ta) {
>> +			priv->ta = stm32_bsec_pta_find(dev);
>> +			/* wait for OP-TEE client driver to be up and ready */
>> +			if (!priv->ta)
>> +				return -EPROBE_DEFER;
>> +
>> +			priv->cfg.reg_read = stm32_bsec_pta_read;
>> +			priv->cfg.reg_write = stm32_bsec_pta_write;
>> +		} else {
>> +			priv->cfg.reg_read = stm32_bsec_read;
>> +			priv->cfg.reg_write = stm32_bsec_write;
>> +		}
>>   	}
>>   
>>   	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &priv->cfg));
>>   }
>>   
>>   /*
>> - * STM32MP15 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
>> + * STM32MP15/13 BSEC OTP regions: 4096 OTP bits (with 3072 effective bits)
>>    * => 96 x 32-bits data words
>>    * - Lower: 1K bits, 2:1 redundancy, incremental bit programming
>>    *   => 32 (x 32-bits) lower shadow registers = words 0 to 31
>> @@ -191,6 +210,13 @@ static int stm32_romem_probe(struct platform_device *pdev)
>>   static const struct stm32_romem_cfg stm32mp15_bsec_cfg = {
>>   	.size = 384,
>>   	.lower = 32,
>> +	.ta = false,
>> +};
>> +
>> +static const struct stm32_romem_cfg stm32mp13_bsec_cfg = {
>> +	.size = 384,
>> +	.lower = 32,
>> +	.ta = true,
>>   };
>>   
>>   static const struct of_device_id stm32_romem_of_match[] = {
>> @@ -198,6 +224,8 @@ static const struct of_device_id stm32_romem_of_match[] = {
>>   		.compatible = "st,stm32mp15-bsec",
>>   		.data = (void *)&stm32mp15_bsec_cfg,
>>   	}, {
>> +		.compatible = "st,stm32mp13-bsec",
>> +		.data = (void *)&stm32mp13_bsec_cfg,
>>   	},
>>   };
>>   MODULE_DEVICE_TABLE(of, stm32_romem_of_match);
>> @@ -209,7 +237,421 @@ static struct platform_driver stm32_romem_driver = {
>>   		.of_match_table = of_match_ptr(stm32_romem_of_match),
>>   	},
>>   };
>> -module_platform_driver(stm32_romem_driver);
>> +
>> +#if IS_ENABLED(CONFIG_OPTEE)
>> +/*************************************************************************
>> + * BSEC PTA : OP-TEE client driver to pseudo trusted application
>> + *************************************************************************/
> Just suggesting: could improve readability on above comment ?
> (mutli-line comment starting with an emtpy /*. But here there's a single
> line.

I remove this comment (not needed).


>
>> +
>> +/*
>> + * Read OTP memory
>> + *
>> + * [in]		value[0].a		OTP start offset in byte
>> + * [in]		value[0].b		Access type (0 : shadow,
>> + *					1 : fuse, 2 : lock)
> Suggestion: fits into 100 chars line

done


>
>> + * [out]	memref[1].buffer	Output buffer to store read values
>> + * [out]	memref[1].size		Size of OTP to be read
>> + *
>> + * Return codes:
>> + * TEE_SUCCESS - Invoke command success
>> + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
>> + */
>> +#define PTA_BSEC_READ_MEM		0x0 /* Read OTP */
>> +
>> +/*
>> + * Write OTP memory
>> + *
>> + * [in]		value[0].a		OTP start offset in byte
>> + * [in]		value[0].b		Access type (0 : shadow,
>> + *					1 : fuse, 2 : lock)
> same here


done


>
>> + * [in]		memref[1].buffer	Input buffer to read values
>> + * [in]		memref[1].size		Size of OTP to be written
>> + *
>> + * Return codes:
>> + * TEE_SUCCESS - Invoke command success
>> + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
>> + */
>> +#define PTA_BSEC_WRITE_MEM		0x1	/* Write OTP */
>> +
>> +/* value of PTA_BSEC access type = value[in] b */
>> +#define SHADOW_ACCESS	0
>> +#define FUSE_ACCESS	1
>> +#define LOCK_ACCESS	2
>> +
>> +/* Bitfield definition for LOCK status */
>> +#define LOCK_PERM			BIT(30)
>> +
>> +/**
>> + * struct stm32_bsec_pta_priv - OP-TEE BSEC TA private data
>> + * @ctx:		OP-TEE context handler.
>> + * @session_id:		TA session identifier.
>> + */
>> +struct stm32_bsec_pta_priv {
>> +	struct tee_context *ctx;
>> +	u32 session_id;
>> +};
>> +
>> +/*
>> + * Check whether this driver supports the BSEC TA in the TEE instance
>> + * represented by the params (ver/data) to this function.
>> + */
>> +static int stm32_bsec_pta_match(struct tee_ioctl_version_data *ver, const void *data)
>> +{
>> +	/*
>> +	 * Currently this driver only supports GP compliant, OP-TEE based TA
>> +	 */
> /* suggestion: single line comment */


done


>
>> +	if ((ver->impl_id == TEE_IMPL_ID_OPTEE) &&
>> +		(ver->gen_caps & TEE_GEN_CAP_GP))
>> +		return 1;
>> +	else
>> +		return 0;
>> +}
>> +
>> +/**
>> + * stm32_bsec_pta_probe() - initialize the PTA BSEC
>> + * @dev: the platform_device description.
>> + *
>> + * Return:
>> + *	On success, 0. On failure, -errno.
>> + */
>> +static int stm32_bsec_pta_probe(struct device *dev)
>> +{
>> +	int rc;
>> +	struct tee_ioctl_open_session_arg sess_arg;
>> +	struct tee_client_device *tee_device = to_tee_client_device(dev);
>> +	struct stm32_bsec_pta_priv *priv;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	/* Open context with TEE driver */
>> +	priv->ctx = tee_client_open_context(NULL, stm32_bsec_pta_match, NULL, NULL);
>> +	if (IS_ERR(priv->ctx)) {
>> +		if (PTR_ERR(priv->ctx) == -ENOENT)
>> +			return -EPROBE_DEFER;
>> +		dev_err(dev, "%s: tee_client_open_context failed\n", __func__);
>> +		return PTR_ERR(priv->ctx);
>> +	}
>> +
>> +	/* Open a session with BSEC TA */
>> +	memset(&sess_arg, 0, sizeof(sess_arg));
>> +	export_uuid(sess_arg.uuid, &tee_device->id.uuid);
>> +	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
>> +	sess_arg.num_params = 0;
>> +
>> +	rc = tee_client_open_session(priv->ctx, &sess_arg, NULL);
>> +	if ((rc < 0) || (sess_arg.ret != 0)) {
>> +		dev_err(dev, "%s: tee_client_open_session failed, err=%x\n",
>> +			__func__, sess_arg.ret);
>> +		rc = -EINVAL;
>> +		goto out_tee_session;
>> +	}
>> +	priv->session_id = sess_arg.session;
>> +	dev_set_drvdata(dev, priv);
>> +
>> +	return 0;
>> +
>> +out_tee_session:
>> +	tee_client_close_context(priv->ctx);
>> +	priv->ctx = NULL;
>> +
>> +	return rc;
>> +}
>> +
>> +/**
>> + * stm32_bsec_pta_remove() - remove the BSEC TEE device
>> + * @dev: the platform_device description.
>> + *
>> + * Return:
>> + *	0 always.
>> + */
>> +static int stm32_bsec_pta_remove(struct device *dev)
>> +{
>> +	struct stm32_bsec_pta_priv *priv = dev_get_drvdata(dev);
>> +
>> +	if (!IS_ERR_OR_NULL(priv->ctx)) {
>> +		tee_client_close_session(priv->ctx, priv->session_id);
>> +		tee_client_close_context(priv->ctx);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * stm32_bsec_pta_read() - nvmem read access using PTA client driver
>> + * @context: nvmem context => romem privdate data
>> + * @offset: nvmem offset
>> + * @buf: buffer to fill with nvem values
>> + * @bytes: number of bytes to read
>> + *
>> + * Return:
>> + *	On success, 0. On failure, -errno.
>> + */
>> +static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
>> +			       size_t bytes)
>> +{
>> +	struct stm32_romem_priv *romem_priv = context;
>> +	struct device *dev;
>> +	struct stm32_bsec_pta_priv *priv;
>> +	struct tee_shm *shm;
>> +	struct tee_ioctl_invoke_arg arg;
>> +	struct tee_param param[2];
>> +	u8 *shm_buf;
>> +	u32 start, num_bytes;
>> +	int ret;
>> +
>> +	dev = romem_priv->ta;
>> +	if (!dev) {
>> +		pr_err("TA_BSEC invoke with driver\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	priv = dev_get_drvdata(dev);
>> +
>> +	memset(&arg, 0, sizeof(arg));
>> +	memset(&param, 0, sizeof(param));
>> +
>> +	arg.func = PTA_BSEC_READ_MEM;
>> +	arg.session = priv->session_id;
>> +	arg.num_params = 2;
>> +
>> +	/* align access on 32bits */
>> +	start = ALIGN_DOWN(offset, 4);
>> +	num_bytes = round_up(offset + bytes - start, 4);
>> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
>> +	param[0].u.value.a = start;
>> +	param[0].u.value.b = SHADOW_ACCESS;
>> +
>> +	shm = tee_shm_alloc_kernel_buf(priv->ctx, num_bytes);
>> +	if (IS_ERR(shm))
>> +		return PTR_ERR(shm);
>> +
>> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
>> +	param[1].u.memref.shm = shm;
>> +	param[1].u.memref.size = num_bytes;
>> +
>> +	ret = tee_client_invoke_func(priv->ctx, &arg, param);
>> +	if (ret < 0 || arg.ret != 0) {
>> +		dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
>> +			arg.ret, ret);
>> +		if (!ret)
>> +			ret = -EIO;
>> +	}
>> +	if (!ret) {
>> +		shm_buf = tee_shm_get_va(shm, 0);
>> +		if (IS_ERR(shm_buf)) {
>> +			dev_err(dev, "tee_shm_get_va failed for transmit\n");
>> +			ret = PTR_ERR(shm_buf);
>> +		} else {
>> +			ret = 0;
> ret = 0; maybe removed ? (ret is already 0 here)


yes => line removed


>
>> +			/* read data from 32 bits aligned buffer */
>> +			memcpy(buf, &shm_buf[offset % 4], bytes);
>> +		}
>> +	}
>> +
>> +	tee_shm_free(shm);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * stm32_bsec_pta_write() - nvmem write access using PTA client driver
>> + * @context: nvmem context => romem privdate data
>> + * @offset: nvmem offset
>> + * @buf: buffer with nvem values
>> + * @bytes: number of bytes to write
>> + *
>> + * Return:
>> + *	On success, 0. On failure, -errno.
>> + */
>> +static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
>> +				size_t bytes)
>> +{
>> +	struct stm32_romem_priv *romem_priv = context;
>> +	struct device *dev;
>> +	struct stm32_bsec_pta_priv *priv;
>> +	struct tee_shm *shm;
>> +	struct tee_ioctl_invoke_arg arg;
>> +	struct tee_param param[2];
>> +	u8 *shm_buf;
>> +	int ret;
>> +
>> +	dev = romem_priv->ta;
>> +	if (!dev) {
>> +		pr_err("TA_BSEC invoke with driver\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	/* Allow only writing complete 32-bits aligned words */
>> +	if ((bytes % 4) || (offset % 4))
>> +		return -EINVAL;
>> +
>> +	priv = dev_get_drvdata(dev);
>> +
>> +	memset(&arg, 0, sizeof(arg));
>> +	memset(&param, 0, sizeof(param));
>> +
>> +	arg.func = PTA_BSEC_WRITE_MEM;
>> +	arg.session = priv->session_id;
>> +	arg.num_params = 2;
>> +
>> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
>> +	param[0].u.value.a = offset;
>> +	param[0].u.value.b = FUSE_ACCESS;
>> +
>> +	shm = tee_shm_alloc_kernel_buf(priv->ctx, bytes);
>> +	if (IS_ERR(shm))
>> +		return PTR_ERR(shm);
>> +
>> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
>> +	param[1].u.memref.shm = shm;
>> +	param[1].u.memref.size = bytes;
>> +
>> +	shm_buf = tee_shm_get_va(shm, 0);
>> +	if (IS_ERR(shm_buf)) {
>> +		dev_err(dev, "tee_shm_get_va failed for transmit\n");
> Need to add error handling: tee_shm_free(shm);

done => line added


>
>> +		return PTR_ERR(shm_buf);
>> +	}
>> +
>> +	memcpy(shm_buf, buf, bytes);
>> +
>> +	ret = tee_client_invoke_func(priv->ctx, &arg, param);
>> +	if (ret < 0 || arg.ret != 0) {
>> +		dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
>> +			arg.ret, ret);
>> +		if (!ret)
>> +			ret = -EIO;
>> +	}
>> +	dev_dbg(dev, "Write OTPs %d to %d, ret=%d\n",
>> +		offset / 4, (offset + bytes) / 4, ret);
>> +
>> +	/* Lock the upper OTPs with ECC protection, word programming only */
>> +	if (!ret && ((offset + bytes) >= (romem_priv->lower * 4))) {
>> +		u32 start, nb_lock;
>> +		u32 *lock = (u32 *)shm_buf;
>> +		int i;
>> +
>> +		/*
>> +		 * don't lock the lower OTPs, no ECC protection and incremental
>> +		 * bit programming, a second write is allowed
>> +		 */
>> +		start = max_t(u32, offset, romem_priv->lower * 4);
>> +		nb_lock = (offset + bytes - start) / 4;
>> +
>> +		param[0].u.value.a = start;
>> +		param[0].u.value.b = LOCK_ACCESS;
>> +		param[1].u.memref.size = nb_lock * 4;
>> +
>> +		for (i = 0; i < nb_lock; i++)
>> +			lock[i] = LOCK_PERM;
>> +
>> +		ret = tee_client_invoke_func(priv->ctx, &arg, param);
>> +		if (ret < 0 || arg.ret != 0) {
>> +			dev_err(dev, "TA_BSEC invoke failed TEE err: %x, ret:%x\n",
>> +				arg.ret, ret);
>> +			if (!ret)
>> +				ret = -EIO;
>> +		}
>> +		dev_dbg(dev, "Lock upper OTPs %d to %d, ret=%d\n",
>> +			start / 4, start / 4 + nb_lock, ret);
>> +	}
>> +
>> +	tee_shm_free(shm);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct tee_client_device_id stm32_bsec_id_table[] = {
>> +	{
>> +		UUID_INIT(0x94cf71ad, 0x80e6, 0x40b5,
>> +			  0xa7, 0xc6, 0x3d, 0xc5, 0x01, 0xeb, 0x28, 0x03)
>> +	},
>> +	{ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(tee, stm32_bsec_id_table);
>> +
>> +static struct tee_client_driver stm32_bsec_pta_driver = {
>> +	.id_table	= stm32_bsec_id_table,
>> +	.driver		= {
>> +		.name = "stm32-bsec-pta",
>> +		.bus = &tee_bus_type,
>> +		.probe = stm32_bsec_pta_probe,
>> +		.remove = stm32_bsec_pta_remove,
>> +	},
>> +};
>> +
>> +static void stm32_bsec_put_device(void *data)
>> +{
>> +	put_device(data);
>> +}
>> +
>> +struct device *stm32_bsec_pta_find(struct device *dev)
>> +{
>> +	struct device *pta_dev;
>> +
>> +	pta_dev = driver_find_next_device(&stm32_bsec_pta_driver.driver, NULL);
>> +
>> +	if (pta_dev && devm_add_action_or_reset(dev, stm32_bsec_put_device, pta_dev)) {
>> +		dev_err(dev, "unable to register cleanup action\n");
> I suggest to improve the error message here, to make it more explicit,
> and also propagate the error code to the caller.
> Something like:
> ret = devm_add_action_or_reset()
> if (ret) {
>      dev_err(dev, "devm_add_action_or_reset() failed (%d)\n", ret );
>      return ret;
> }
> This may avoid turning a real error here, into a EPROBE_DEFER at the
> caller side, e.g.:
> 	if (!priv->ta)
> 		return -EPROBE_DEFER;


Yes: no need to probe defer if a error is detected:


staticstructdevice *stm32_bsec_pta_find(structdevice *dev)
{
structdevice *pta_dev;
intret;
pta_dev = driver_find_next_device(&stm32_bsec_pta_driver.driver, NULL);
if(pta_dev) {
ret = devm_add_action_or_reset(dev, stm32_bsec_put_device, pta_dev);
if(ret)
dev_err(dev, "devm_add_action_or_reset() failed (%d)\n", ret );
returnERR_PTR(ret);
}
returnpta_dev;
}
and for caller side:
priv->ta= stm32_bsec_pta_find(dev);
/* wait for OP-TEE client driver to be up and ready */
if(!priv->ta)
return-EPROBE_DEFER;
if(IS_ERR(priv->ta))
returnPTR_ERR(priv->ta);


>
> Thanks & Best Regards,
> Fabrice
>
>> +
>> +		return NULL;
>> +	}
>> +
>> +	return pta_dev;
>> +}
>> +
>> +#else
>> +static int stm32_bsec_pta_read(void *context, unsigned int offset, void *buf,
>> +			       size_t bytes)
>> +{
>> +	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
>> +
>> +	return -ENXIO;
>> +}
>> +
>> +static int stm32_bsec_pta_write(void *context, unsigned int offset, void *buf,
>> +				size_t bytes)
>> +{
>> +	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
>> +
>> +	return -ENXIO;
>> +}
>> +
>> +struct device *stm32_bsec_pta_find(struct device *dev)
>> +{
>> +	pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
>> +
>> +	return NULL;
>> +}
>> +#endif
>> +
>> +static int __init stm32_romem_init(void)
>> +{
>> +	int rc;
>> +
>> +	rc = platform_driver_register(&stm32_romem_driver);
>> +	if (rc)
>> +		return rc;
>> +
>> +#if IS_ENABLED(CONFIG_OPTEE)
>> +	rc = driver_register(&stm32_bsec_pta_driver.driver);
>> +#endif
>> +
>> +	return rc;
>> +}
>> +
>> +static void __exit stm32_romem_exit(void)
>> +{
>> +	platform_driver_unregister(&stm32_romem_driver);
>> +#if IS_ENABLED(CONFIG_OPTEE)
>> +	driver_unregister(&stm32_bsec_pta_driver.driver);
>> +#endif
>> +}
>> +
>> +module_init(stm32_romem_init);
>> +module_exit(stm32_romem_exit);
>>   
>>   MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
>>   MODULE_DESCRIPTION("STMicroelectronics STM32 RO-MEM");

Re: [PATCH 0/3] nvmem: stm32: add OP-TEE support for STM32MP13x

[Srinivas Kandagatla]

On 28/10/2022 15:52, Patrick Delaunay wrote:
> 
> This serie update the NVMEM BSEC driver to be compatible with STM32MP13x
> SoC and the trusted application STM32MP BSEC in OP-TEE
> 
> This serie solve issue in initial support of STM32MP131
> (using BSEC STM32MP15 compatible) and so it break the DTS compatible.
> 
> I create this serie for more efficient review.
> 
> The 2 first patches of this series are re-sent of patches already sent
> 
> 1- "dt-bindings: nvmem: add new stm32mp13 compatible for stm32-romem"
>      https://lore.kernel.org/all/20221014172324.1.Ifc1812116ff63f5501f3edd155d3cf5c0ecc846c@changeid/
>      https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=685403
> 
> 2- "ARM: dts: stm32mp13: fix compatible for BSEC"
>      https://lore.kernel.org/all/20221017134437.1.I167a5efc1f8777cce14518c6fa38400ac684de3e@changeid/
>      https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=685815
> 
> This DTS break is acceptable as
> - the STM32MP13x SoC is not yet available outside STMicroelectronics
>    (not official)
> - the same patch is already integrated or modifications are in progress in
>    the other users (arm-trusted-firmware/TF-A, OP-TEE and U-Boot) of
>    stm32mp131 device tree.
> 
> It is the good time to correct this issue before the real availability of
> the SoC and before full support of SoC in Linux kernel.
> 
> This last patch on NVMEM STM32 ROMEM driver in depend on the preliminary
> patch for the driver:
> 
>    "nvmem: stm32: move STM32MP15_BSEC_NUM_LOWER in config"
>    https://lore.kernel.org/all/20221017174953.v2.1.I95e71328dd654723bd4c57206bd008ff81c726bb@changeid/
> 
> present in the serie
> 
>    "nvmem: stm32: several minor improvements"
>    https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=685886
> 
> Regards
> 
> Patrick
> 
> Changes in v1:
> - update commit message to indicate DTS break reason.
> 
> Patrick Delaunay (3):
>    dt-bindings: nvmem: add new stm32mp13 compatible for stm32-romem
>    ARM: dts: stm32mp13: fix compatible for BSEC
>    nvmem: stm32: add OP-TEE support for STM32MP13x

Applied 1/3 and 3/3 thanks,

dts changes need to go via the dts maintainer..

--srini
> 
>   .../bindings/nvmem/st,stm32-romem.yaml        |   1 +
>   arch/arm/boot/dts/stm32mp131.dtsi             |   2 +-
>   drivers/nvmem/stm32-romem.c                   | 450 +++++++++++++++++-
>   3 files changed, 448 insertions(+), 5 deletions(-)
>