[PATCH] drivers: tee: i2c trampoline driver

[PATCH] drivers: tee: i2c trampoline driver

[Jorge Ramirez-Ortiz]

This commit gives the secure world access to the I2C bus so it can
communicate with I2C slaves (tipically those would be secure elements
like the NXP SE050).

Tested on imx8mmevk.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
---
 drivers/tee/optee/Makefile               |  1 +
 drivers/tee/optee/i2c.c                  | 88 ++++++++++++++++++++++++
 drivers/tee/optee/optee_msg.h            | 22 ++++++
 drivers/tee/optee/optee_msg_supplicant.h |  5 ++
 drivers/tee/optee/optee_private.h        | 12 ++++
 drivers/tee/optee/supplicant.c           |  3 +
 6 files changed, 131 insertions(+)
 create mode 100644 drivers/tee/optee/i2c.c

diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
index 928d3f8002..068c6e7aa1 100644
--- a/drivers/tee/optee/Makefile
+++ b/drivers/tee/optee/Makefile
@@ -2,4 +2,5 @@
 
 obj-y += core.o
 obj-y += supplicant.o
+obj-$(CONFIG_DM_I2C) += i2c.o
 obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
new file mode 100644
index 0000000000..2ebbf1ff7c
--- /dev/null
+++ b/drivers/tee/optee/i2c.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: BSD-2-Clause
+/*
+ * Copyright (c) 2020 Foundries.io Ltd
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <i2c.h>
+#include <tee.h>
+#include "optee_msg.h"
+#include "optee_private.h"
+
+static struct {
+	struct udevice *dev;
+	int chip;
+	int bus;
+} xfer;
+
+void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
+				  struct optee_msg_arg *arg)
+{
+	const uint64_t attr[] = {
+		OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
+		OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
+		OPTEE_MSG_ATTR_TYPE_RMEM_INOUT,
+		OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
+	};
+	struct udevice *chip_dev = NULL;
+	struct tee_shm *shm = NULL;
+	uint8_t *buf = NULL;
+	size_t len = 0;
+	int chip = -1;
+	int bus = -1;
+	int ret = -1;
+
+	if (arg->num_params != ARRAY_SIZE(attr) ||
+	    arg->params[0].attr != attr[0] ||
+	    arg->params[1].attr != attr[1] ||
+	    arg->params[2].attr != attr[2] ||
+	    arg->params[3].attr != attr[3]) {
+		arg->ret = TEE_ERROR_BAD_PARAMETERS;
+		return;
+	}
+
+	len = arg->params[2].u.tmem.size;
+	shm = (struct tee_shm *)(unsigned long)arg->params[2].u.tmem.shm_ref;
+	buf = shm->addr;
+	if (!buf || !len)
+		goto bad;
+
+	bus = (int)arg->params[0].u.value.b;
+	chip = (int)arg->params[0].u.value.c;
+
+	if (!xfer.dev || xfer.chip != chip || xfer.bus != bus) {
+		if (i2c_get_chip_for_busnum(bus, chip, 0, &chip_dev))
+			goto bad;
+
+		xfer.dev = chip_dev;
+		xfer.chip = chip;
+		xfer.bus = bus;
+	}
+
+	if (arg->params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT)
+		if (i2c_set_chip_flags(xfer.dev, DM_I2C_CHIP_10BIT))
+			goto bad;
+
+	switch (arg->params[0].u.value.a) {
+	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+		ret = dm_i2c_read(xfer.dev, 0, buf, len);
+		break;
+	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+		ret = dm_i2c_write(xfer.dev, 0, buf, len);
+		break;
+	default:
+		goto bad;
+	}
+
+	if (ret) {
+		arg->ret = TEE_ERROR_COMMUNICATION;
+	} else {
+		arg->params[3].u.value.a = len;
+		arg->ret = TEE_SUCCESS;
+	}
+
+	return;
+bad:
+	arg->ret = TEE_ERROR_BAD_PARAMETERS;
+}
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 24c60960fc..7cedb59a82 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -422,4 +422,26 @@ struct optee_msg_arg {
  */
 #define OPTEE_MSG_RPC_CMD_SHM_FREE	7
 
+/*
+ * Access a device on an i2c bus
+ *
+ * [in]  param[0].u.value.a		mode: RD(0), WR(1)
+ * [in]  param[0].u.value.b		i2c adapter
+ * [in]  param[0].u.value.c		i2c chip
+ *
+ * [in]  param[1].u.value.a		i2c control flags
+ * [in]  param[1].u.value.b		i2c retry (optional)
+ *
+ * [in/out] memref[2]			buffer to exchange the transfer data
+ *					with the secure world
+ *
+ * [out]  param[3].u.value.a		bytes transferred by the driver
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+/* I2C master transfer modes */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+/* I2C master control flags */
+#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT  BIT(0)
+
 #endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/optee_msg_supplicant.h b/drivers/tee/optee/optee_msg_supplicant.h
index a0fb8063c8..963cfd4782 100644
--- a/drivers/tee/optee/optee_msg_supplicant.h
+++ b/drivers/tee/optee/optee_msg_supplicant.h
@@ -147,6 +147,11 @@
 #define OPTEE_MSG_RPC_CMD_SHM_ALLOC	6
 #define OPTEE_MSG_RPC_CMD_SHM_FREE	7
 
+/*
+ * I2C bus access
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+
 /*
  * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer
  */
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 9442d1c176..d7ab1f593f 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -60,6 +60,18 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev)
 }
 #endif
 
+#ifdef CONFIG_DM_I2C
+void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
+				  struct optee_msg_arg *arg);
+#else
+void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
+				  struct optee_msg_arg *arg)
+{
+	debug("OPTEE_MSG_RPC_CMD_I2C_TRANSFER not implemented\n");
+	arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
+}
+#endif
+
 void *optee_alloc_and_init_page_list(void *buf, ulong len, u64 *phys_buf_ptr);
 
 #endif /* __OPTEE_PRIVATE_H */
diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
index ae042b9a20..f7738983cd 100644
--- a/drivers/tee/optee/supplicant.c
+++ b/drivers/tee/optee/supplicant.c
@@ -89,6 +89,9 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm *shm_arg,
 	case OPTEE_MSG_RPC_CMD_RPMB:
 		optee_suppl_cmd_rpmb(dev, arg);
 		break;
+	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
+		optee_suppl_cmd_i2c_transfer(dev, arg);
+		break;
 	default:
 		arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
 	}
-- 
2.17.1

[PATCH] drivers: tee: i2c trampoline driver

[Jens Wiklander]

Hi Jorge,

On Mon, Dec 21, 2020 at 07:15:40PM +0100, Jorge Ramirez-Ortiz wrote:
> This commit gives the secure world access to the I2C bus so it can
> communicate with I2C slaves (tipically those would be secure elements
> like the NXP SE050).
> 
> Tested on imx8mmevk.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  drivers/tee/optee/Makefile               |  1 +
>  drivers/tee/optee/i2c.c                  | 88 ++++++++++++++++++++++++
>  drivers/tee/optee/optee_msg.h            | 22 ++++++
>  drivers/tee/optee/optee_msg_supplicant.h |  5 ++
>  drivers/tee/optee/optee_private.h        | 12 ++++
>  drivers/tee/optee/supplicant.c           |  3 +
>  6 files changed, 131 insertions(+)
>  create mode 100644 drivers/tee/optee/i2c.c
> 
> diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> index 928d3f8002..068c6e7aa1 100644
> --- a/drivers/tee/optee/Makefile
> +++ b/drivers/tee/optee/Makefile
> @@ -2,4 +2,5 @@
>  
>  obj-y += core.o
>  obj-y += supplicant.o
> +obj-$(CONFIG_DM_I2C) += i2c.o
>  obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
> diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
> new file mode 100644
> index 0000000000..2ebbf1ff7c
> --- /dev/null
> +++ b/drivers/tee/optee/i2c.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +/*
> + * Copyright (c) 2020 Foundries.io Ltd
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <i2c.h>
> +#include <tee.h>
> +#include "optee_msg.h"
> +#include "optee_private.h"
> +
> +static struct {
> +	struct udevice *dev;
> +	int chip;
> +	int bus;
> +} xfer;
> +
> +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> +				  struct optee_msg_arg *arg)
> +{
> +	const uint64_t attr[] = {
A u8 instead of uint64_t would give the same result.

> +		OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> +		OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> +		OPTEE_MSG_ATTR_TYPE_RMEM_INOUT,
> +		OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
> +	};
> +	struct udevice *chip_dev = NULL;
> +	struct tee_shm *shm = NULL;
> +	uint8_t *buf = NULL;
> +	size_t len = 0;
> +	int chip = -1;
> +	int bus = -1;
> +	int ret = -1;
> +
> +	if (arg->num_params != ARRAY_SIZE(attr) ||
> +	    arg->params[0].attr != attr[0] ||
> +	    arg->params[1].attr != attr[1] ||
> +	    arg->params[2].attr != attr[2] ||
> +	    arg->params[3].attr != attr[3]) {
> +		arg->ret = TEE_ERROR_BAD_PARAMETERS;
> +		return;
> +	}
> +
> +	len = arg->params[2].u.tmem.size;
> +	shm = (struct tee_shm *)(unsigned long)arg->params[2].u.tmem.shm_ref;
Please replace tmem with rmem. The OPTEE_MSG_ATTR_TYPE_RMEM_INOUT above
indicates that we're dealing with a struct optee_msg_param_rmem.

> +	buf = shm->addr;
> +	if (!buf || !len)
> +		goto bad;
> +
> +	bus = (int)arg->params[0].u.value.b;
> +	chip = (int)arg->params[0].u.value.c;
> +
> +	if (!xfer.dev || xfer.chip != chip || xfer.bus != bus) {
> +		if (i2c_get_chip_for_busnum(bus, chip, 0, &chip_dev))
> +			goto bad;
> +
> +		xfer.dev = chip_dev;
> +		xfer.chip = chip;
> +		xfer.bus = bus;
Is this caching safe? No risk of using stale data?

Thanks,
Jens

> +	}
> +
> +	if (arg->params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT)
> +		if (i2c_set_chip_flags(xfer.dev, DM_I2C_CHIP_10BIT))
> +			goto bad;
> +
> +	switch (arg->params[0].u.value.a) {
> +	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> +		ret = dm_i2c_read(xfer.dev, 0, buf, len);
> +		break;
> +	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> +		ret = dm_i2c_write(xfer.dev, 0, buf, len);
> +		break;
> +	default:
> +		goto bad;
> +	}
> +
> +	if (ret) {
> +		arg->ret = TEE_ERROR_COMMUNICATION;
> +	} else {
> +		arg->params[3].u.value.a = len;
> +		arg->ret = TEE_SUCCESS;
> +	}
> +
> +	return;
> +bad:
> +	arg->ret = TEE_ERROR_BAD_PARAMETERS;
> +}
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index 24c60960fc..7cedb59a82 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -422,4 +422,26 @@ struct optee_msg_arg {
>   */
>  #define OPTEE_MSG_RPC_CMD_SHM_FREE	7
>  
> +/*
> + * Access a device on an i2c bus
> + *
> + * [in]  param[0].u.value.a		mode: RD(0), WR(1)
> + * [in]  param[0].u.value.b		i2c adapter
> + * [in]  param[0].u.value.c		i2c chip
> + *
> + * [in]  param[1].u.value.a		i2c control flags
> + * [in]  param[1].u.value.b		i2c retry (optional)
> + *
> + * [in/out] memref[2]			buffer to exchange the transfer data
> + *					with the secure world
> + *
> + * [out]  param[3].u.value.a		bytes transferred by the driver
> + */
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> +/* I2C master transfer modes */
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> +/* I2C master control flags */
> +#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT  BIT(0)
> +
>  #endif /* _OPTEE_MSG_H */
> diff --git a/drivers/tee/optee/optee_msg_supplicant.h b/drivers/tee/optee/optee_msg_supplicant.h
> index a0fb8063c8..963cfd4782 100644
> --- a/drivers/tee/optee/optee_msg_supplicant.h
> +++ b/drivers/tee/optee/optee_msg_supplicant.h
> @@ -147,6 +147,11 @@
>  #define OPTEE_MSG_RPC_CMD_SHM_ALLOC	6
>  #define OPTEE_MSG_RPC_CMD_SHM_FREE	7
>  
> +/*
> + * I2C bus access
> + */
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> +
>  /*
>   * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer
>   */
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 9442d1c176..d7ab1f593f 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -60,6 +60,18 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev)
>  }
>  #endif
>  
> +#ifdef CONFIG_DM_I2C
> +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> +				  struct optee_msg_arg *arg);
> +#else
> +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> +				  struct optee_msg_arg *arg)
> +{
> +	debug("OPTEE_MSG_RPC_CMD_I2C_TRANSFER not implemented\n");
> +	arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> +}
> +#endif
> +
>  void *optee_alloc_and_init_page_list(void *buf, ulong len, u64 *phys_buf_ptr);
>  
>  #endif /* __OPTEE_PRIVATE_H */
> diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
> index ae042b9a20..f7738983cd 100644
> --- a/drivers/tee/optee/supplicant.c
> +++ b/drivers/tee/optee/supplicant.c
> @@ -89,6 +89,9 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm *shm_arg,
>  	case OPTEE_MSG_RPC_CMD_RPMB:
>  		optee_suppl_cmd_rpmb(dev, arg);
>  		break;
> +	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
> +		optee_suppl_cmd_i2c_transfer(dev, arg);
> +		break;
>  	default:
>  		arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
>  	}
> -- 
> 2.17.1
> 

[PATCH] drivers: tee: i2c trampoline driver

[Jorge]

On 23/12/20, Jens Wiklander wrote:
> Hi Jorge,

hey

> 
> On Mon, Dec 21, 2020 at 07:15:40PM +0100, Jorge Ramirez-Ortiz wrote:
> > This commit gives the secure world access to the I2C bus so it can
> > communicate with I2C slaves (tipically those would be secure elements
> > like the NXP SE050).
> > 
> > Tested on imx8mmevk.
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  drivers/tee/optee/Makefile               |  1 +
> >  drivers/tee/optee/i2c.c                  | 88 ++++++++++++++++++++++++
> >  drivers/tee/optee/optee_msg.h            | 22 ++++++
> >  drivers/tee/optee/optee_msg_supplicant.h |  5 ++
> >  drivers/tee/optee/optee_private.h        | 12 ++++
> >  drivers/tee/optee/supplicant.c           |  3 +
> >  6 files changed, 131 insertions(+)
> >  create mode 100644 drivers/tee/optee/i2c.c
> > 
> > diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> > index 928d3f8002..068c6e7aa1 100644
> > --- a/drivers/tee/optee/Makefile
> > +++ b/drivers/tee/optee/Makefile
> > @@ -2,4 +2,5 @@
> >  
> >  obj-y += core.o
> >  obj-y += supplicant.o
> > +obj-$(CONFIG_DM_I2C) += i2c.o
> >  obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
> > diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
> > new file mode 100644
> > index 0000000000..2ebbf1ff7c
> > --- /dev/null
> > +++ b/drivers/tee/optee/i2c.c
> > @@ -0,0 +1,88 @@
> > +// SPDX-License-Identifier: BSD-2-Clause
> > +/*
> > + * Copyright (c) 2020 Foundries.io Ltd
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <i2c.h>
> > +#include <tee.h>
> > +#include "optee_msg.h"
> > +#include "optee_private.h"
> > +
> > +static struct {
> > +	struct udevice *dev;
> > +	int chip;
> > +	int bus;
> > +} xfer;
> > +
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +				  struct optee_msg_arg *arg)
> > +{
> > +	const uint64_t attr[] = {
> A u8 instead of uint64_t would give the same result.

ok

> 
> > +		OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > +		OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > +		OPTEE_MSG_ATTR_TYPE_RMEM_INOUT,
> > +		OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
> > +	};
> > +	struct udevice *chip_dev = NULL;
> > +	struct tee_shm *shm = NULL;
> > +	uint8_t *buf = NULL;
> > +	size_t len = 0;
> > +	int chip = -1;
> > +	int bus = -1;
> > +	int ret = -1;
> > +
> > +	if (arg->num_params != ARRAY_SIZE(attr) ||
> > +	    arg->params[0].attr != attr[0] ||
> > +	    arg->params[1].attr != attr[1] ||
> > +	    arg->params[2].attr != attr[2] ||
> > +	    arg->params[3].attr != attr[3]) {
> > +		arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > +		return;
> > +	}
> > +
> > +	len = arg->params[2].u.tmem.size;
> > +	shm = (struct tee_shm *)(unsigned long)arg->params[2].u.tmem.shm_ref;
> Please replace tmem with rmem. The OPTEE_MSG_ATTR_TYPE_RMEM_INOUT above
> indicates that we're dealing with a struct optee_msg_param_rmem.

sure, thanks!

> 
> > +	buf = shm->addr;
> > +	if (!buf || !len)
> > +		goto bad;
> > +
> > +	bus = (int)arg->params[0].u.value.b;
> > +	chip = (int)arg->params[0].u.value.c;
> > +
> > +	if (!xfer.dev || xfer.chip != chip || xfer.bus != bus) {
> > +		if (i2c_get_chip_for_busnum(bus, chip, 0, &chip_dev))
> > +			goto bad;
> > +
> > +		xfer.dev = chip_dev;
> > +		xfer.chip = chip;
> > +		xfer.bus = bus;
> Is this caching safe? No risk of using stale data?

um no I dont think so - I can't think of a scenario that could cause
that error, but maybe someone else can comment?

> 
> Thanks,
> Jens
> 
> > +	}
> > +
> > +	if (arg->params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT)
> > +		if (i2c_set_chip_flags(xfer.dev, DM_I2C_CHIP_10BIT))
> > +			goto bad;
> > +
> > +	switch (arg->params[0].u.value.a) {
> > +	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > +		ret = dm_i2c_read(xfer.dev, 0, buf, len);
> > +		break;
> > +	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > +		ret = dm_i2c_write(xfer.dev, 0, buf, len);
> > +		break;
> > +	default:
> > +		goto bad;
> > +	}
> > +
> > +	if (ret) {
> > +		arg->ret = TEE_ERROR_COMMUNICATION;
> > +	} else {
> > +		arg->params[3].u.value.a = len;
> > +		arg->ret = TEE_SUCCESS;
> > +	}
> > +
> > +	return;
> > +bad:
> > +	arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > +}
> > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > index 24c60960fc..7cedb59a82 100644
> > --- a/drivers/tee/optee/optee_msg.h
> > +++ b/drivers/tee/optee/optee_msg.h
> > @@ -422,4 +422,26 @@ struct optee_msg_arg {
> >   */
> >  #define OPTEE_MSG_RPC_CMD_SHM_FREE	7
> >  
> > +/*
> > + * Access a device on an i2c bus
> > + *
> > + * [in]  param[0].u.value.a		mode: RD(0), WR(1)
> > + * [in]  param[0].u.value.b		i2c adapter
> > + * [in]  param[0].u.value.c		i2c chip
> > + *
> > + * [in]  param[1].u.value.a		i2c control flags
> > + * [in]  param[1].u.value.b		i2c retry (optional)
> > + *
> > + * [in/out] memref[2]			buffer to exchange the transfer data
> > + *					with the secure world
> > + *
> > + * [out]  param[3].u.value.a		bytes transferred by the driver
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > +/* I2C master transfer modes */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > +/* I2C master control flags */
> > +#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT  BIT(0)
> > +
> >  #endif /* _OPTEE_MSG_H */
> > diff --git a/drivers/tee/optee/optee_msg_supplicant.h b/drivers/tee/optee/optee_msg_supplicant.h
> > index a0fb8063c8..963cfd4782 100644
> > --- a/drivers/tee/optee/optee_msg_supplicant.h
> > +++ b/drivers/tee/optee/optee_msg_supplicant.h
> > @@ -147,6 +147,11 @@
> >  #define OPTEE_MSG_RPC_CMD_SHM_ALLOC	6
> >  #define OPTEE_MSG_RPC_CMD_SHM_FREE	7
> >  
> > +/*
> > + * I2C bus access
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > +
> >  /*
> >   * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer
> >   */
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 9442d1c176..d7ab1f593f 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -60,6 +60,18 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev)
> >  }
> >  #endif
> >  
> > +#ifdef CONFIG_DM_I2C
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +				  struct optee_msg_arg *arg);
> > +#else

this function below should be made static btw (issue was flagged
internally during testing by a team member). Perhaps the same should
be done for RPMB?

> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +				  struct optee_msg_arg *arg)
> > +{
> > +	debug("OPTEE_MSG_RPC_CMD_I2C_TRANSFER not implemented\n");
> > +	arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> > +}
> > +#endif
> > +
> >  void *optee_alloc_and_init_page_list(void *buf, ulong len, u64 *phys_buf_ptr);
> >  
> >  #endif /* __OPTEE_PRIVATE_H */
> > diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
> > index ae042b9a20..f7738983cd 100644
> > --- a/drivers/tee/optee/supplicant.c
> > +++ b/drivers/tee/optee/supplicant.c
> > @@ -89,6 +89,9 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm *shm_arg,
> >  	case OPTEE_MSG_RPC_CMD_RPMB:
> >  		optee_suppl_cmd_rpmb(dev, arg);
> >  		break;
> > +	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
> > +		optee_suppl_cmd_i2c_transfer(dev, arg);
> > +		break;
> >  	default:
> >  		arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> >  	}
> > -- 
> > 2.17.1
> > 

[PATCH] drivers: tee: i2c trampoline driver

[Jens Wiklander]

On Sun, Dec 27, 2020 at 6:07 PM Jorge Ramirez-Ortiz, Foundries
<jorge@foundries.io> wrote:
>
> On 23/12/20, Jens Wiklander wrote:
> > Hi Jorge,
>
> hey
>
> >
> > On Mon, Dec 21, 2020 at 07:15:40PM +0100, Jorge Ramirez-Ortiz wrote:
> > > This commit gives the secure world access to the I2C bus so it can
> > > communicate with I2C slaves (tipically those would be secure elements
> > > like the NXP SE050).
> > >
> > > Tested on imx8mmevk.
> > >
> > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > > ---
> > >  drivers/tee/optee/Makefile               |  1 +
> > >  drivers/tee/optee/i2c.c                  | 88 ++++++++++++++++++++++++
> > >  drivers/tee/optee/optee_msg.h            | 22 ++++++
> > >  drivers/tee/optee/optee_msg_supplicant.h |  5 ++
> > >  drivers/tee/optee/optee_private.h        | 12 ++++
> > >  drivers/tee/optee/supplicant.c           |  3 +
> > >  6 files changed, 131 insertions(+)
> > >  create mode 100644 drivers/tee/optee/i2c.c
> > >
> > > diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> > > index 928d3f8002..068c6e7aa1 100644
> > > --- a/drivers/tee/optee/Makefile
> > > +++ b/drivers/tee/optee/Makefile
> > > @@ -2,4 +2,5 @@
> > >
> > >  obj-y += core.o
> > >  obj-y += supplicant.o
> > > +obj-$(CONFIG_DM_I2C) += i2c.o
> > >  obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
> > > diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
> > > new file mode 100644
> > > index 0000000000..2ebbf1ff7c
> > > --- /dev/null
> > > +++ b/drivers/tee/optee/i2c.c
> > > @@ -0,0 +1,88 @@
> > > +// SPDX-License-Identifier: BSD-2-Clause
> > > +/*
> > > + * Copyright (c) 2020 Foundries.io Ltd
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <dm.h>
> > > +#include <i2c.h>
> > > +#include <tee.h>
> > > +#include "optee_msg.h"
> > > +#include "optee_private.h"
> > > +
> > > +static struct {
> > > +   struct udevice *dev;
> > > +   int chip;
> > > +   int bus;
> > > +} xfer;
> > > +
> > > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > > +                             struct optee_msg_arg *arg)
> > > +{
> > > +   const uint64_t attr[] = {
> > A u8 instead of uint64_t would give the same result.
>
> ok
>
> >
> > > +           OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > > +           OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > > +           OPTEE_MSG_ATTR_TYPE_RMEM_INOUT,
> > > +           OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
> > > +   };
> > > +   struct udevice *chip_dev = NULL;
> > > +   struct tee_shm *shm = NULL;
> > > +   uint8_t *buf = NULL;
> > > +   size_t len = 0;
> > > +   int chip = -1;
> > > +   int bus = -1;
> > > +   int ret = -1;
> > > +
> > > +   if (arg->num_params != ARRAY_SIZE(attr) ||
> > > +       arg->params[0].attr != attr[0] ||
> > > +       arg->params[1].attr != attr[1] ||
> > > +       arg->params[2].attr != attr[2] ||
> > > +       arg->params[3].attr != attr[3]) {
> > > +           arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > > +           return;
> > > +   }
> > > +
> > > +   len = arg->params[2].u.tmem.size;
> > > +   shm = (struct tee_shm *)(unsigned long)arg->params[2].u.tmem.shm_ref;
> > Please replace tmem with rmem. The OPTEE_MSG_ATTR_TYPE_RMEM_INOUT above
> > indicates that we're dealing with a struct optee_msg_param_rmem.
>
> sure, thanks!
>
> >
> > > +   buf = shm->addr;
> > > +   if (!buf || !len)
> > > +           goto bad;
> > > +
> > > +   bus = (int)arg->params[0].u.value.b;
> > > +   chip = (int)arg->params[0].u.value.c;
> > > +
> > > +   if (!xfer.dev || xfer.chip != chip || xfer.bus != bus) {
> > > +           if (i2c_get_chip_for_busnum(bus, chip, 0, &chip_dev))
> > > +                   goto bad;
> > > +
> > > +           xfer.dev = chip_dev;
> > > +           xfer.chip = chip;
> > > +           xfer.bus = bus;
> > Is this caching safe? No risk of using stale data?
>
> um no I dont think so - I can't think of a scenario that could cause
> that error, but maybe someone else can comment?
>
> >
> > Thanks,
> > Jens
> >
> > > +   }
> > > +
> > > +   if (arg->params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT)
> > > +           if (i2c_set_chip_flags(xfer.dev, DM_I2C_CHIP_10BIT))
> > > +                   goto bad;
> > > +
> > > +   switch (arg->params[0].u.value.a) {
> > > +   case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > > +           ret = dm_i2c_read(xfer.dev, 0, buf, len);
> > > +           break;
> > > +   case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > > +           ret = dm_i2c_write(xfer.dev, 0, buf, len);
> > > +           break;
> > > +   default:
> > > +           goto bad;
> > > +   }
> > > +
> > > +   if (ret) {
> > > +           arg->ret = TEE_ERROR_COMMUNICATION;
> > > +   } else {
> > > +           arg->params[3].u.value.a = len;
> > > +           arg->ret = TEE_SUCCESS;
> > > +   }
> > > +
> > > +   return;
> > > +bad:
> > > +   arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > > +}
> > > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > > index 24c60960fc..7cedb59a82 100644
> > > --- a/drivers/tee/optee/optee_msg.h
> > > +++ b/drivers/tee/optee/optee_msg.h
> > > @@ -422,4 +422,26 @@ struct optee_msg_arg {
> > >   */
> > >  #define OPTEE_MSG_RPC_CMD_SHM_FREE 7
> > >
> > > +/*
> > > + * Access a device on an i2c bus
> > > + *
> > > + * [in]  param[0].u.value.a                mode: RD(0), WR(1)
> > > + * [in]  param[0].u.value.b                i2c adapter
> > > + * [in]  param[0].u.value.c                i2c chip
> > > + *
> > > + * [in]  param[1].u.value.a                i2c control flags
> > > + * [in]  param[1].u.value.b                i2c retry (optional)
> > > + *
> > > + * [in/out] memref[2]                      buffer to exchange the transfer data
> > > + *                                 with the secure world
> > > + *
> > > + * [out]  param[3].u.value.a               bytes transferred by the driver
> > > + */
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > > +/* I2C master transfer modes */
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > > +/* I2C master control flags */
> > > +#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT  BIT(0)
> > > +
> > >  #endif /* _OPTEE_MSG_H */
> > > diff --git a/drivers/tee/optee/optee_msg_supplicant.h b/drivers/tee/optee/optee_msg_supplicant.h
> > > index a0fb8063c8..963cfd4782 100644
> > > --- a/drivers/tee/optee/optee_msg_supplicant.h
> > > +++ b/drivers/tee/optee/optee_msg_supplicant.h
> > > @@ -147,6 +147,11 @@
> > >  #define OPTEE_MSG_RPC_CMD_SHM_ALLOC        6
> > >  #define OPTEE_MSG_RPC_CMD_SHM_FREE 7
> > >
> > > +/*
> > > + * I2C bus access
> > > + */
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > > +
> > >  /*
> > >   * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer
> > >   */
> > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > > index 9442d1c176..d7ab1f593f 100644
> > > --- a/drivers/tee/optee/optee_private.h
> > > +++ b/drivers/tee/optee/optee_private.h
> > > @@ -60,6 +60,18 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev)
> > >  }
> > >  #endif
> > >
> > > +#ifdef CONFIG_DM_I2C
> > > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > > +                             struct optee_msg_arg *arg);
> > > +#else
>
> this function below should be made static btw (issue was flagged
> internally during testing by a team member). Perhaps the same should
> be done for RPMB?

I believe the RPMB functions already are static inline.

Cheers,
Jens

>
> > > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > > +                             struct optee_msg_arg *arg)
> > > +{
> > > +   debug("OPTEE_MSG_RPC_CMD_I2C_TRANSFER not implemented\n");
> > > +   arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> > > +}
> > > +#endif
> > > +
> > >  void *optee_alloc_and_init_page_list(void *buf, ulong len, u64 *phys_buf_ptr);
> > >
> > >  #endif /* __OPTEE_PRIVATE_H */
> > > diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
> > > index ae042b9a20..f7738983cd 100644
> > > --- a/drivers/tee/optee/supplicant.c
> > > +++ b/drivers/tee/optee/supplicant.c
> > > @@ -89,6 +89,9 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm *shm_arg,
> > >     case OPTEE_MSG_RPC_CMD_RPMB:
> > >             optee_suppl_cmd_rpmb(dev, arg);
> > >             break;
> > > +   case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
> > > +           optee_suppl_cmd_i2c_transfer(dev, arg);
> > > +           break;
> > >     default:
> > >             arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> > >     }
> > > --
> > > 2.17.1
> > >

[PATCH] drivers: tee: i2c trampoline driver

[Simon Glass]

Hi Jorge,

On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> This commit gives the secure world access to the I2C bus so it can
> communicate with I2C slaves (tipically those would be secure elements

typically

> like the NXP SE050).
>
> Tested on imx8mmevk.

We don't seem to have any optee tests in U-Boot at present. I vaguely
recall they were coming at some point. I think we need:

- a sandbox fake drive for optee, that understands and responds to the
6 uclass calls at a basic level
- an update to get_invoke_func() that provides a sandbox function too

Then we should be able to run optee tests in CI.

It is not a lot of work, but I don't think we should add to optee
until this is resolved.

Regards,
Simon
[.]

[PATCH] drivers: tee: i2c trampoline driver

[Jorge]

On 28/12/20, Simon Glass wrote:
> Hi Jorge,
> 
> On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> >
> > This commit gives the secure world access to the I2C bus so it can
> > communicate with I2C slaves (tipically those would be secure elements
> 
> typically

ok

> 
> > like the NXP SE050).
> >
> > Tested on imx8mmevk.
> 
> We don't seem to have any optee tests in U-Boot at present. I vaguely
> recall they were coming at some point. I think we need:
> 
> - a sandbox fake drive for optee, that understands and responds to the
> 6 uclass calls at a basic level
> - an update to get_invoke_func() that provides a sandbox function too
> 
> Then we should be able to run optee tests in CI.
> 
> It is not a lot of work, but I don't think we should add to optee
> until this is resolved.

um, ok but shouldnt this infrastructure better rest on a maintainer's
roadmap rather than on an off-the-blue request? I mean, had I known I
could have done it in parallel but now I'll need to find the time to
do this.

also notice that Linux's equivalent patchset was merged back in the
summer (ie, this is not untested code).

https://lkml.org/lkml/2020/8/12/276

> 
> Regards,
> Simon
> [.]

[PATCH] drivers: tee: i2c trampoline driver

[Simon Glass]

Hi Jorge,

On Tue, 29 Dec 2020 at 01:30, Jorge Ramirez-Ortiz, Foundries
<jorge@foundries.io> wrote:
>
> On 28/12/20, Simon Glass wrote:
> > Hi Jorge,
> >
> > On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> > >
> > > This commit gives the secure world access to the I2C bus so it can
> > > communicate with I2C slaves (tipically those would be secure elements
> >
> > typically
>
> ok
>
> >
> > > like the NXP SE050).
> > >
> > > Tested on imx8mmevk.
> >
> > We don't seem to have any optee tests in U-Boot at present. I vaguely
> > recall they were coming at some point. I think we need:
> >
> > - a sandbox fake drive for optee, that understands and responds to the
> > 6 uclass calls at a basic level
> > - an update to get_invoke_func() that provides a sandbox function too
> >
> > Then we should be able to run optee tests in CI.
> >
> > It is not a lot of work, but I don't think we should add to optee
> > until this is resolved.
>
> um, ok but shouldnt this infrastructure better rest on a maintainer's
> roadmap rather than on an off-the-blue request? I mean, had I known I
> could have done it in parallel but now I'll need to find the time to
> do this.

We always need tests in U-Boot, so if you are not writing a test it
would be a good question to ask as to how you can do that. Actually
patman sometimes warns about that, but perhaps only in certain
situations.

Actually I see that there is a test - it is hidden under the generic
unit tests so I didn't see it. See dm/test/tee.c

I'll make some comments on the patch.

>
> also notice that Linux's equivalent patchset was merged back in the
> summer (ie, this is not untested code).
>
> https://lkml.org/lkml/2020/8/12/276

I don't see any tests in that patch though...are they somewhere else?
Or do you justmean people have been running similar code? If so,
that's fair enough but it doesn't really help us much. Lots of people
test code manually before submitting patches, at least for their use
case, but this is an open-source project. Over time people want to
change and expand the code, and it is very hard for them to do that if
there are no automated tests.

Regards,
Simon

[PATCH] drivers: tee: i2c trampoline driver

[Simon Glass]

Hi Jorge,

On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> This commit gives the secure world access to the I2C bus so it can
> communicate with I2C slaves (tipically those would be secure elements
> like the NXP SE050).
>

Since this code is ported from linux it might be worth adding a link
to the linux commit or patch.

> Tested on imx8mmevk.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  drivers/tee/optee/Makefile               |  1 +
>  drivers/tee/optee/i2c.c                  | 88 ++++++++++++++++++++++++
>  drivers/tee/optee/optee_msg.h            | 22 ++++++
>  drivers/tee/optee/optee_msg_supplicant.h |  5 ++
>  drivers/tee/optee/optee_private.h        | 12 ++++
>  drivers/tee/optee/supplicant.c           |  3 +
>  6 files changed, 131 insertions(+)
>  create mode 100644 drivers/tee/optee/i2c.c
>
> diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> index 928d3f8002..068c6e7aa1 100644
> --- a/drivers/tee/optee/Makefile
> +++ b/drivers/tee/optee/Makefile
> @@ -2,4 +2,5 @@
>
>  obj-y += core.o
>  obj-y += supplicant.o
> +obj-$(CONFIG_DM_I2C) += i2c.o
>  obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
> diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
> new file mode 100644
> index 0000000000..2ebbf1ff7c
> --- /dev/null
> +++ b/drivers/tee/optee/i2c.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +/*
> + * Copyright (c) 2020 Foundries.io Ltd
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <i2c.h>
> +#include <tee.h>
> +#include "optee_msg.h"
> +#include "optee_private.h"
> +
> +static struct {

comments on members, but see below

> +       struct udevice *dev;
> +       int chip;
> +       int bus;
> +} xfer;

How come this is not a local variable? Is it an optimisation? Does it
make any difference in execution time? If so I think it would be
better to drop it as state should be kept in driver rmodel. If you
really want it, then perhaps just keep the dev, since you can use:

dev_seq(dev_get_parent(dev) - to get the bus number the device is on

struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);

then use chip->chip_addr to get the chip address

then store 'dev' in priv data in your dev (I think this is struct
optee_private), the one passed to the function below:

> +
> +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> +                                 struct optee_msg_arg *arg)
> +{
> +       const uint64_t attr[] = {
> +               OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> +               OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> +               OPTEE_MSG_ATTR_TYPE_RMEM_INOUT,
> +               OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
> +       };
> +       struct udevice *chip_dev = NULL;
> +       struct tee_shm *shm = NULL;
> +       uint8_t *buf = NULL;

Shouldn't init vars that don't need to be

> +       size_t len = 0;
> +       int chip = -1;
> +       int bus = -1;
> +       int ret = -1;
> +
> +       if (arg->num_params != ARRAY_SIZE(attr) ||
> +           arg->params[0].attr != attr[0] ||
> +           arg->params[1].attr != attr[1] ||
> +           arg->params[2].attr != attr[2] ||
> +           arg->params[3].attr != attr[3]) {
> +               arg->ret = TEE_ERROR_BAD_PARAMETERS;
> +               return;
> +       }
> +
> +       len = arg->params[2].u.tmem.size;
> +       shm = (struct tee_shm *)(unsigned long)arg->params[2].u.tmem.shm_ref;
> +       buf = shm->addr;
> +       if (!buf || !len)
> +               goto bad;
> +
> +       bus = (int)arg->params[0].u.value.b;
> +       chip = (int)arg->params[0].u.value.c;
> +
> +       if (!xfer.dev || xfer.chip != chip || xfer.bus != bus) {
> +               if (i2c_get_chip_for_busnum(bus, chip, 0, &chip_dev))
> +                       goto bad;
> +
> +               xfer.dev = chip_dev;
> +               xfer.chip = chip;
> +               xfer.bus = bus;
> +       }
> +
> +       if (arg->params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT)
> +               if (i2c_set_chip_flags(xfer.dev, DM_I2C_CHIP_10BIT))
> +                       goto bad;

Is this flag defined in the devicetree? If so we could read it in
i2c_chip_ofdata_to_platdata() (which will be i2c_chip_of_to_plat()
when the next merge window opens - see upstream/next).

It just seems odd that optee is controlling this, since presumably
U-Boot knows about it?

> +
> +       switch (arg->params[0].u.value.a) {
> +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> +               ret = dm_i2c_read(xfer.dev, 0, buf, len);
> +               break;
> +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> +               ret = dm_i2c_write(xfer.dev, 0, buf, len);

This code should run on sandbox and you can use a suitable i2c
emulator (UCLASS_I2C_EMUL), only three at present) or write a new one.
Then your test can arrange for sandbox to send an RPC (e.g. by calling
a function directly in that driver to tell it to do that next time it
has a chance), and your test can check that the i2c read/write
happened.

> +               break;
> +       default:
> +               goto bad;
> +       }
> +
> +       if (ret) {
> +               arg->ret = TEE_ERROR_COMMUNICATION;
> +       } else {
> +               arg->params[3].u.value.a = len;
> +               arg->ret = TEE_SUCCESS;
> +       }
> +
> +       return;
> +bad:
> +       arg->ret = TEE_ERROR_BAD_PARAMETERS;
> +}
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index 24c60960fc..7cedb59a82 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -422,4 +422,26 @@ struct optee_msg_arg {
>   */
>  #define OPTEE_MSG_RPC_CMD_SHM_FREE     7
>
> +/*
> + * Access a device on an i2c bus
> + *
> + * [in]  param[0].u.value.a            mode: RD(0), WR(1)
> + * [in]  param[0].u.value.b            i2c adapter
> + * [in]  param[0].u.value.c            i2c chip
> + *
> + * [in]  param[1].u.value.a            i2c control flags
> + * [in]  param[1].u.value.b            i2c retry (optional)
> + *
> + * [in/out] memref[2]                  buffer to exchange the transfer data
> + *                                     with the secure world
> + *
> + * [out]  param[3].u.value.a           bytes transferred by the driver
> + */
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> +/* I2C master transfer modes */
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> +/* I2C master control flags */
> +#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT  BIT(0)

Jens, perhaps OPTEE_MSG_RPC_CMD_ could be renamed to OPTEE_RPC_ as
this is way too long.

> +
>  #endif /* _OPTEE_MSG_H */
> diff --git a/drivers/tee/optee/optee_msg_supplicant.h b/drivers/tee/optee/optee_msg_supplicant.h
> index a0fb8063c8..963cfd4782 100644
> --- a/drivers/tee/optee/optee_msg_supplicant.h
> +++ b/drivers/tee/optee/optee_msg_supplicant.h
> @@ -147,6 +147,11 @@
>  #define OPTEE_MSG_RPC_CMD_SHM_ALLOC    6
>  #define OPTEE_MSG_RPC_CMD_SHM_FREE     7
>
> +/*
> + * I2C bus access
> + */
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> +
>  /*
>   * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer
>   */
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 9442d1c176..d7ab1f593f 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -60,6 +60,18 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev)
>  }
>  #endif
>
> +#ifdef CONFIG_DM_I2C

We can probably assume DM_I2C is used for any recent boards, but I'd
prefer to avoid cluttering up the header file when DM_I2C should be
supported. See below.

> +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> +                                 struct optee_msg_arg *arg);

Function comment please

> +#else

Yuk, please don't do this..

> +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> +                                 struct optee_msg_arg *arg)
> +{
> +       debug("OPTEE_MSG_RPC_CMD_I2C_TRANSFER not implemented\n");
> +       arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> +}
> +#endif
> +
>  void *optee_alloc_and_init_page_list(void *buf, ulong len, u64 *phys_buf_ptr);
>
>  #endif /* __OPTEE_PRIVATE_H */
> diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
> index ae042b9a20..f7738983cd 100644
> --- a/drivers/tee/optee/supplicant.c
> +++ b/drivers/tee/optee/supplicant.c
> @@ -89,6 +89,9 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm *shm_arg,

It seems a comment in the header file was missed. Can you please add
it while you are here?

>         case OPTEE_MSG_RPC_CMD_RPMB:
>                 optee_suppl_cmd_rpmb(dev, arg);
>                 break;
> +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:

if (IS_ENABLED(DM_I2C))

> +               optee_suppl_cmd_i2c_transfer(dev, arg);

or if permitted, make TEE depend on DM_I2C

> +               break;
>         default:
>                 arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
>         }
> --
> 2.17.1
>

Regards,
Simon

[PATCH] drivers: tee: i2c trampoline driver

[Jens Wiklander]

Hi Simon,

On Tue, Dec 29, 2020 at 4:32 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Jorge,
>
> On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> >
> > This commit gives the secure world access to the I2C bus so it can
> > communicate with I2C slaves (tipically those would be secure elements
> > like the NXP SE050).
> >
>
> Since this code is ported from linux it might be worth adding a link
> to the linux commit or patch.
>
> > Tested on imx8mmevk.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  drivers/tee/optee/Makefile               |  1 +
> >  drivers/tee/optee/i2c.c                  | 88 ++++++++++++++++++++++++
> >  drivers/tee/optee/optee_msg.h            | 22 ++++++
> >  drivers/tee/optee/optee_msg_supplicant.h |  5 ++
> >  drivers/tee/optee/optee_private.h        | 12 ++++
> >  drivers/tee/optee/supplicant.c           |  3 +
> >  6 files changed, 131 insertions(+)
> >  create mode 100644 drivers/tee/optee/i2c.c
> >
> > diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> > index 928d3f8002..068c6e7aa1 100644
> > --- a/drivers/tee/optee/Makefile
> > +++ b/drivers/tee/optee/Makefile
> > @@ -2,4 +2,5 @@
> >
> >  obj-y += core.o
> >  obj-y += supplicant.o
> > +obj-$(CONFIG_DM_I2C) += i2c.o
> >  obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
> > diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
> > new file mode 100644
> > index 0000000000..2ebbf1ff7c
> > --- /dev/null
> > +++ b/drivers/tee/optee/i2c.c
> > @@ -0,0 +1,88 @@
> > +// SPDX-License-Identifier: BSD-2-Clause
> > +/*
> > + * Copyright (c) 2020 Foundries.io Ltd
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <i2c.h>
> > +#include <tee.h>
> > +#include "optee_msg.h"
> > +#include "optee_private.h"
> > +
> > +static struct {
>
> comments on members, but see below
>
> > +       struct udevice *dev;
> > +       int chip;
> > +       int bus;
> > +} xfer;
>
> How come this is not a local variable? Is it an optimisation? Does it
> make any difference in execution time? If so I think it would be
> better to drop it as state should be kept in driver rmodel. If you
> really want it, then perhaps just keep the dev, since you can use:
>
> dev_seq(dev_get_parent(dev) - to get the bus number the device is on
>
> struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
>
> then use chip->chip_addr to get the chip address
>
> then store 'dev' in priv data in your dev (I think this is struct
> optee_private), the one passed to the function below:
>
> > +
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +                                 struct optee_msg_arg *arg)
> > +{
> > +       const uint64_t attr[] = {
> > +               OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > +               OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > +               OPTEE_MSG_ATTR_TYPE_RMEM_INOUT,
> > +               OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
> > +       };
> > +       struct udevice *chip_dev = NULL;
> > +       struct tee_shm *shm = NULL;
> > +       uint8_t *buf = NULL;
>
> Shouldn't init vars that don't need to be
>
> > +       size_t len = 0;
> > +       int chip = -1;
> > +       int bus = -1;
> > +       int ret = -1;
> > +
> > +       if (arg->num_params != ARRAY_SIZE(attr) ||
> > +           arg->params[0].attr != attr[0] ||
> > +           arg->params[1].attr != attr[1] ||
> > +           arg->params[2].attr != attr[2] ||
> > +           arg->params[3].attr != attr[3]) {
> > +               arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > +               return;
> > +       }
> > +
> > +       len = arg->params[2].u.tmem.size;
> > +       shm = (struct tee_shm *)(unsigned long)arg->params[2].u.tmem.shm_ref;
> > +       buf = shm->addr;
> > +       if (!buf || !len)
> > +               goto bad;
> > +
> > +       bus = (int)arg->params[0].u.value.b;
> > +       chip = (int)arg->params[0].u.value.c;
> > +
> > +       if (!xfer.dev || xfer.chip != chip || xfer.bus != bus) {
> > +               if (i2c_get_chip_for_busnum(bus, chip, 0, &chip_dev))
> > +                       goto bad;
> > +
> > +               xfer.dev = chip_dev;
> > +               xfer.chip = chip;
> > +               xfer.bus = bus;
> > +       }
> > +
> > +       if (arg->params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT)
> > +               if (i2c_set_chip_flags(xfer.dev, DM_I2C_CHIP_10BIT))
> > +                       goto bad;
>
> Is this flag defined in the devicetree? If so we could read it in
> i2c_chip_ofdata_to_platdata() (which will be i2c_chip_of_to_plat()
> when the next merge window opens - see upstream/next).
>
> It just seems odd that optee is controlling this, since presumably
> U-Boot knows about it?
>
> > +
> > +       switch (arg->params[0].u.value.a) {
> > +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > +               ret = dm_i2c_read(xfer.dev, 0, buf, len);
> > +               break;
> > +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > +               ret = dm_i2c_write(xfer.dev, 0, buf, len);
>
> This code should run on sandbox and you can use a suitable i2c
> emulator (UCLASS_I2C_EMUL), only three at present) or write a new one.
> Then your test can arrange for sandbox to send an RPC (e.g. by calling
> a function directly in that driver to tell it to do that next time it
> has a chance), and your test can check that the i2c read/write
> happened.
>
> > +               break;
> > +       default:
> > +               goto bad;
> > +       }
> > +
> > +       if (ret) {
> > +               arg->ret = TEE_ERROR_COMMUNICATION;
> > +       } else {
> > +               arg->params[3].u.value.a = len;
> > +               arg->ret = TEE_SUCCESS;
> > +       }
> > +
> > +       return;
> > +bad:
> > +       arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > +}
> > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > index 24c60960fc..7cedb59a82 100644
> > --- a/drivers/tee/optee/optee_msg.h
> > +++ b/drivers/tee/optee/optee_msg.h
> > @@ -422,4 +422,26 @@ struct optee_msg_arg {
> >   */
> >  #define OPTEE_MSG_RPC_CMD_SHM_FREE     7
> >
> > +/*
> > + * Access a device on an i2c bus
> > + *
> > + * [in]  param[0].u.value.a            mode: RD(0), WR(1)
> > + * [in]  param[0].u.value.b            i2c adapter
> > + * [in]  param[0].u.value.c            i2c chip
> > + *
> > + * [in]  param[1].u.value.a            i2c control flags
> > + * [in]  param[1].u.value.b            i2c retry (optional)
> > + *
> > + * [in/out] memref[2]                  buffer to exchange the transfer data
> > + *                                     with the secure world
> > + *
> > + * [out]  param[3].u.value.a           bytes transferred by the driver
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > +/* I2C master transfer modes */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > +/* I2C master control flags */
> > +#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT  BIT(0)
>
> Jens, perhaps OPTEE_MSG_RPC_CMD_ could be renamed to OPTEE_RPC_ as
> this is way too long.

Sure, we're already doing that (more or less) in the optee_os git,
except for the I2C defines for some reason. We must have missed that
while reviewing.

Thanks,
Jens

>
> > +
> >  #endif /* _OPTEE_MSG_H */
> > diff --git a/drivers/tee/optee/optee_msg_supplicant.h b/drivers/tee/optee/optee_msg_supplicant.h
> > index a0fb8063c8..963cfd4782 100644
> > --- a/drivers/tee/optee/optee_msg_supplicant.h
> > +++ b/drivers/tee/optee/optee_msg_supplicant.h
> > @@ -147,6 +147,11 @@
> >  #define OPTEE_MSG_RPC_CMD_SHM_ALLOC    6
> >  #define OPTEE_MSG_RPC_CMD_SHM_FREE     7
> >
> > +/*
> > + * I2C bus access
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > +
> >  /*
> >   * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer
> >   */
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 9442d1c176..d7ab1f593f 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -60,6 +60,18 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev)
> >  }
> >  #endif
> >
> > +#ifdef CONFIG_DM_I2C
>
> We can probably assume DM_I2C is used for any recent boards, but I'd
> prefer to avoid cluttering up the header file when DM_I2C should be
> supported. See below.
>
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +                                 struct optee_msg_arg *arg);
>
> Function comment please
>
> > +#else
>
> Yuk, please don't do this..
>
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +                                 struct optee_msg_arg *arg)
> > +{
> > +       debug("OPTEE_MSG_RPC_CMD_I2C_TRANSFER not implemented\n");
> > +       arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> > +}
> > +#endif
> > +
> >  void *optee_alloc_and_init_page_list(void *buf, ulong len, u64 *phys_buf_ptr);
> >
> >  #endif /* __OPTEE_PRIVATE_H */
> > diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
> > index ae042b9a20..f7738983cd 100644
> > --- a/drivers/tee/optee/supplicant.c
> > +++ b/drivers/tee/optee/supplicant.c
> > @@ -89,6 +89,9 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm *shm_arg,
>
> It seems a comment in the header file was missed. Can you please add
> it while you are here?
>
> >         case OPTEE_MSG_RPC_CMD_RPMB:
> >                 optee_suppl_cmd_rpmb(dev, arg);
> >                 break;
> > +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
>
> if (IS_ENABLED(DM_I2C))
>
> > +               optee_suppl_cmd_i2c_transfer(dev, arg);
>
> or if permitted, make TEE depend on DM_I2C
>
> > +               break;
> >         default:
> >                 arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> >         }
> > --
> > 2.17.1
> >
>
> Regards,
> Simon

[PATCH] drivers: tee: i2c trampoline driver

[Jorge]

On 29/12/20, Simon Glass wrote:
> Hi Jorge,
> 
> On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> >
> > This commit gives the secure world access to the I2C bus so it can
> > communicate with I2C slaves (tipically those would be secure elements
> > like the NXP SE050).
> >
> 
> Since this code is ported from linux it might be worth adding a link
> to the linux commit or patch.

sure, will do that.

> 
> > Tested on imx8mmevk.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  drivers/tee/optee/Makefile               |  1 +
> >  drivers/tee/optee/i2c.c                  | 88 ++++++++++++++++++++++++
> >  drivers/tee/optee/optee_msg.h            | 22 ++++++
> >  drivers/tee/optee/optee_msg_supplicant.h |  5 ++
> >  drivers/tee/optee/optee_private.h        | 12 ++++
> >  drivers/tee/optee/supplicant.c           |  3 +
> >  6 files changed, 131 insertions(+)
> >  create mode 100644 drivers/tee/optee/i2c.c
> >
> > diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> > index 928d3f8002..068c6e7aa1 100644
> > --- a/drivers/tee/optee/Makefile
> > +++ b/drivers/tee/optee/Makefile
> > @@ -2,4 +2,5 @@
> >
> >  obj-y += core.o
> >  obj-y += supplicant.o
> > +obj-$(CONFIG_DM_I2C) += i2c.o
> >  obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
> > diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
> > new file mode 100644
> > index 0000000000..2ebbf1ff7c
> > --- /dev/null
> > +++ b/drivers/tee/optee/i2c.c
> > @@ -0,0 +1,88 @@
> > +// SPDX-License-Identifier: BSD-2-Clause
> > +/*
> > + * Copyright (c) 2020 Foundries.io Ltd
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <i2c.h>
> > +#include <tee.h>
> > +#include "optee_msg.h"
> > +#include "optee_private.h"
> > +
> > +static struct {
> 
> comments on members, but see below
> 
> > +       struct udevice *dev;
> > +       int chip;
> > +       int bus;
> > +} xfer;
> 
> How come this is not a local variable? Is it an optimisation? Does it
> make any difference in execution time? If so I think it would be
> better to drop it as state should be kept in driver rmodel. If you
> really want it, then perhaps just keep the dev, since you can use:
> 
> dev_seq(dev_get_parent(dev) - to get the bus number the device is on
> 
> struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
> 
> then use chip->chip_addr to get the chip address
> 
> then store 'dev' in priv data in your dev (I think this is struct
> optee_private), the one passed to the function below:

I will remove this optimization instead. it is really not necessary
for a few i2c transfers (I think it will obfuscate the code which I'd
rather keep this trampoline code as simple as possible)

> 
> > +
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +                                 struct optee_msg_arg *arg)
> > +{
> > +       const uint64_t attr[] = {
> > +               OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > +               OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > +               OPTEE_MSG_ATTR_TYPE_RMEM_INOUT,
> > +               OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
> > +       };
> > +       struct udevice *chip_dev = NULL;
> > +       struct tee_shm *shm = NULL;
> > +       uint8_t *buf = NULL;
> 
> Shouldn't init vars that don't need to be

ok

> 
> > +       size_t len = 0;
> > +       int chip = -1;
> > +       int bus = -1;
> > +       int ret = -1;
> > +
> > +       if (arg->num_params != ARRAY_SIZE(attr) ||
> > +           arg->params[0].attr != attr[0] ||
> > +           arg->params[1].attr != attr[1] ||
> > +           arg->params[2].attr != attr[2] ||
> > +           arg->params[3].attr != attr[3]) {
> > +               arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > +               return;
> > +       }
> > +
> > +       len = arg->params[2].u.tmem.size;
> > +       shm = (struct tee_shm *)(unsigned long)arg->params[2].u.tmem.shm_ref;
> > +       buf = shm->addr;
> > +       if (!buf || !len)
> > +               goto bad;
> > +
> > +       bus = (int)arg->params[0].u.value.b;
> > +       chip = (int)arg->params[0].u.value.c;
> > +
> > +       if (!xfer.dev || xfer.chip != chip || xfer.bus != bus) {
> > +               if (i2c_get_chip_for_busnum(bus, chip, 0, &chip_dev))
> > +                       goto bad;
> > +
> > +               xfer.dev = chip_dev;
> > +               xfer.chip = chip;
> > +               xfer.bus = bus;
> > +       }
> > +
> > +       if (arg->params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT)
> > +               if (i2c_set_chip_flags(xfer.dev, DM_I2C_CHIP_10BIT))
> > +                       goto bad;
> 
> Is this flag defined in the devicetree? If so we could read it in
> i2c_chip_ofdata_to_platdata() (which will be i2c_chip_of_to_plat()
> when the next merge window opens - see upstream/next).
> 
> It just seems odd that optee is controlling this, since presumably
> U-Boot knows about it?

U-boot should take care of having set this bit for the chip device.
so I'll modify the code to just check for the flag was set since
OP-TEE also knows about it. In case of mismatch, I'll report the error
from u-boot and abort the transfer.

We can then discuss how u-boot sets this value and follow up with the
necessary changes to the driver model if needed. Maybe the i2c
maintainers want to comment on this as well.

> 
> > +
> > +       switch (arg->params[0].u.value.a) {
> > +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > +               ret = dm_i2c_read(xfer.dev, 0, buf, len);
> > +               break;
> > +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > +               ret = dm_i2c_write(xfer.dev, 0, buf, len);
> 
> This code should run on sandbox and you can use a suitable i2c
> emulator (UCLASS_I2C_EMUL), only three at present) or write a new
> one.
> Then your test can arrange for sandbox to send an RPC (e.g. by calling
> a function directly in that driver to tell it to do that next time it
> has a chance), and your test can check that the i2c read/write
> happened.

does this mean you wont accept the commit without emulation code to go
with it? is this a new rule? just trying to understand here.

This code is already tested on imx8mm, imx6ull and imx6ulz with the
upstream/tip of OP-TEE

> 
> > +               break;
> > +       default:
> > +               goto bad;
> > +       }
> > +
> > +       if (ret) {
> > +               arg->ret = TEE_ERROR_COMMUNICATION;
> > +       } else {
> > +               arg->params[3].u.value.a = len;
> > +               arg->ret = TEE_SUCCESS;
> > +       }
> > +
> > +       return;
> > +bad:
> > +       arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > +}
> > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > index 24c60960fc..7cedb59a82 100644
> > --- a/drivers/tee/optee/optee_msg.h
> > +++ b/drivers/tee/optee/optee_msg.h
> > @@ -422,4 +422,26 @@ struct optee_msg_arg {
> >   */
> >  #define OPTEE_MSG_RPC_CMD_SHM_FREE     7
> >
> > +/*
> > + * Access a device on an i2c bus
> > + *
> > + * [in]  param[0].u.value.a            mode: RD(0), WR(1)
> > + * [in]  param[0].u.value.b            i2c adapter
> > + * [in]  param[0].u.value.c            i2c chip
> > + *
> > + * [in]  param[1].u.value.a            i2c control flags
> > + * [in]  param[1].u.value.b            i2c retry (optional)
> > + *
> > + * [in/out] memref[2]                  buffer to exchange the transfer data
> > + *                                     with the secure world
> > + *
> > + * [out]  param[3].u.value.a           bytes transferred by the driver
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > +/* I2C master transfer modes */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > +/* I2C master control flags */
> > +#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT  BIT(0)
> 
> Jens, perhaps OPTEE_MSG_RPC_CMD_ could be renamed to OPTEE_RPC_ as
> this is way too long.
> 
> > +
> >  #endif /* _OPTEE_MSG_H */
> > diff --git a/drivers/tee/optee/optee_msg_supplicant.h b/drivers/tee/optee/optee_msg_supplicant.h
> > index a0fb8063c8..963cfd4782 100644
> > --- a/drivers/tee/optee/optee_msg_supplicant.h
> > +++ b/drivers/tee/optee/optee_msg_supplicant.h
> > @@ -147,6 +147,11 @@
> >  #define OPTEE_MSG_RPC_CMD_SHM_ALLOC    6
> >  #define OPTEE_MSG_RPC_CMD_SHM_FREE     7
> >
> > +/*
> > + * I2C bus access
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > +
> >  /*
> >   * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer
> >   */
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 9442d1c176..d7ab1f593f 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -60,6 +60,18 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev)
> >  }
> >  #endif
> >
> > +#ifdef CONFIG_DM_I2C
> 
> We can probably assume DM_I2C is used for any recent boards, but I'd
> prefer to avoid cluttering up the header file when DM_I2C should be
> supported. See below.

I am sorry, I am not quite sure what you are asking. This just
restrict the changes to DM_I2C. what do you propose we do?

> 
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +                                 struct optee_msg_arg *arg);
> 
> Function comment please
> 
> > +#else
> 
> Yuk, please don't do this..

sorry dont do what? isnt it just the same logic/pattern to what is already in
the file? how should we handle this otherwise?

> 
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +                                 struct optee_msg_arg *arg)
> > +{
> > +       debug("OPTEE_MSG_RPC_CMD_I2C_TRANSFER not implemented\n");
> > +       arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> > +}
> > +#endif
> > +
> >  void *optee_alloc_and_init_page_list(void *buf, ulong len, u64 *phys_buf_ptr);
> >
> >  #endif /* __OPTEE_PRIVATE_H */
> > diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
> > index ae042b9a20..f7738983cd 100644
> > --- a/drivers/tee/optee/supplicant.c
> > +++ b/drivers/tee/optee/supplicant.c
> > @@ -89,6 +89,9 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm *shm_arg,
> 
> It seems a comment in the header file was missed. Can you please add
> it while you are here?

sure

> 
> >         case OPTEE_MSG_RPC_CMD_RPMB:
> >                 optee_suppl_cmd_rpmb(dev, arg);
> >                 break;
> > +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
> 
> if (IS_ENABLED(DM_I2C))
> 
> > +               optee_suppl_cmd_i2c_transfer(dev, arg);
> 
> or if permitted, make TEE depend on DM_I2C
> 
> > +               break;
> >         default:
> >                 arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> >         }
> > --
> > 2.17.1
> >
> 
> Regards,
> Simon

[PATCH] drivers: tee: i2c trampoline driver

[Jorge]

On 29/12/20, Simon Glass wrote:
> Hi Jorge,
> 
> On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> >
> > This commit gives the secure world access to the I2C bus so it can
> > communicate with I2C slaves (tipically those would be secure elements
> > like the NXP SE050).
> >
> 
> Since this code is ported from linux it might be worth adding a link
> to the linux commit or patch.
> 
> > Tested on imx8mmevk.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  drivers/tee/optee/Makefile               |  1 +
> >  drivers/tee/optee/i2c.c                  | 88 ++++++++++++++++++++++++
> >  drivers/tee/optee/optee_msg.h            | 22 ++++++
> >  drivers/tee/optee/optee_msg_supplicant.h |  5 ++
> >  drivers/tee/optee/optee_private.h        | 12 ++++
> >  drivers/tee/optee/supplicant.c           |  3 +
> >  6 files changed, 131 insertions(+)
> >  create mode 100644 drivers/tee/optee/i2c.c
> >
> > diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> > index 928d3f8002..068c6e7aa1 100644
> > --- a/drivers/tee/optee/Makefile
> > +++ b/drivers/tee/optee/Makefile
> > @@ -2,4 +2,5 @@
> >
> >  obj-y += core.o
> >  obj-y += supplicant.o
> > +obj-$(CONFIG_DM_I2C) += i2c.o
> >  obj-$(CONFIG_SUPPORT_EMMC_RPMB) += rpmb.o
> > diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
> > new file mode 100644
> > index 0000000000..2ebbf1ff7c
> > --- /dev/null
> > +++ b/drivers/tee/optee/i2c.c
> > @@ -0,0 +1,88 @@
> > +// SPDX-License-Identifier: BSD-2-Clause
> > +/*
> > + * Copyright (c) 2020 Foundries.io Ltd
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <i2c.h>
> > +#include <tee.h>
> > +#include "optee_msg.h"
> > +#include "optee_private.h"
> > +
> > +static struct {
> 
> comments on members, but see below
> 
> > +       struct udevice *dev;
> > +       int chip;
> > +       int bus;
> > +} xfer;
> 
> How come this is not a local variable? Is it an optimisation? Does it
> make any difference in execution time? If so I think it would be
> better to drop it as state should be kept in driver rmodel. If you
> really want it, then perhaps just keep the dev, since you can use:
> 
> dev_seq(dev_get_parent(dev) - to get the bus number the device is on
> 
> struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
> 
> then use chip->chip_addr to get the chip address
> 
> then store 'dev' in priv data in your dev (I think this is struct
> optee_private), the one passed to the function below:
> 
> > +
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +                                 struct optee_msg_arg *arg)
> > +{
> > +       const uint64_t attr[] = {
> > +               OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > +               OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > +               OPTEE_MSG_ATTR_TYPE_RMEM_INOUT,
> > +               OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
> > +       };
> > +       struct udevice *chip_dev = NULL;
> > +       struct tee_shm *shm = NULL;
> > +       uint8_t *buf = NULL;
> 
> Shouldn't init vars that don't need to be
> 
> > +       size_t len = 0;
> > +       int chip = -1;
> > +       int bus = -1;
> > +       int ret = -1;
> > +
> > +       if (arg->num_params != ARRAY_SIZE(attr) ||
> > +           arg->params[0].attr != attr[0] ||
> > +           arg->params[1].attr != attr[1] ||
> > +           arg->params[2].attr != attr[2] ||
> > +           arg->params[3].attr != attr[3]) {
> > +               arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > +               return;
> > +       }
> > +
> > +       len = arg->params[2].u.tmem.size;
> > +       shm = (struct tee_shm *)(unsigned long)arg->params[2].u.tmem.shm_ref;
> > +       buf = shm->addr;
> > +       if (!buf || !len)
> > +               goto bad;
> > +
> > +       bus = (int)arg->params[0].u.value.b;
> > +       chip = (int)arg->params[0].u.value.c;
> > +
> > +       if (!xfer.dev || xfer.chip != chip || xfer.bus != bus) {
> > +               if (i2c_get_chip_for_busnum(bus, chip, 0, &chip_dev))
> > +                       goto bad;
> > +
> > +               xfer.dev = chip_dev;
> > +               xfer.chip = chip;
> > +               xfer.bus = bus;
> > +       }
> > +
> > +       if (arg->params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT)
> > +               if (i2c_set_chip_flags(xfer.dev, DM_I2C_CHIP_10BIT))
> > +                       goto bad;
> 
> Is this flag defined in the devicetree? If so we could read it in
> i2c_chip_ofdata_to_platdata() (which will be i2c_chip_of_to_plat()
> when the next merge window opens - see upstream/next).
> 
> It just seems odd that optee is controlling this, since presumably
> U-Boot knows about it?
> 
> > +
> > +       switch (arg->params[0].u.value.a) {
> > +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > +               ret = dm_i2c_read(xfer.dev, 0, buf, len);
> > +               break;
> > +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > +               ret = dm_i2c_write(xfer.dev, 0, buf, len);
> 
> This code should run on sandbox and you can use a suitable i2c
> emulator (UCLASS_I2C_EMUL), only three at present) or write a new one.
> Then your test can arrange for sandbox to send an RPC (e.g. by calling
> a function directly in that driver to tell it to do that next time it
> has a chance), and your test can check that the i2c read/write
> happened.
> 
> > +               break;
> > +       default:
> > +               goto bad;
> > +       }
> > +
> > +       if (ret) {
> > +               arg->ret = TEE_ERROR_COMMUNICATION;
> > +       } else {
> > +               arg->params[3].u.value.a = len;
> > +               arg->ret = TEE_SUCCESS;
> > +       }
> > +
> > +       return;
> > +bad:
> > +       arg->ret = TEE_ERROR_BAD_PARAMETERS;
> > +}
> > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > index 24c60960fc..7cedb59a82 100644
> > --- a/drivers/tee/optee/optee_msg.h
> > +++ b/drivers/tee/optee/optee_msg.h
> > @@ -422,4 +422,26 @@ struct optee_msg_arg {
> >   */
> >  #define OPTEE_MSG_RPC_CMD_SHM_FREE     7
> >
> > +/*
> > + * Access a device on an i2c bus
> > + *
> > + * [in]  param[0].u.value.a            mode: RD(0), WR(1)
> > + * [in]  param[0].u.value.b            i2c adapter
> > + * [in]  param[0].u.value.c            i2c chip
> > + *
> > + * [in]  param[1].u.value.a            i2c control flags
> > + * [in]  param[1].u.value.b            i2c retry (optional)
> > + *
> > + * [in/out] memref[2]                  buffer to exchange the transfer data
> > + *                                     with the secure world
> > + *
> > + * [out]  param[3].u.value.a           bytes transferred by the driver
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > +/* I2C master transfer modes */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > +/* I2C master control flags */
> > +#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT  BIT(0)
> 
> Jens, perhaps OPTEE_MSG_RPC_CMD_ could be renamed to OPTEE_RPC_ as
> this is way too long.
> 
> > +
> >  #endif /* _OPTEE_MSG_H */
> > diff --git a/drivers/tee/optee/optee_msg_supplicant.h b/drivers/tee/optee/optee_msg_supplicant.h
> > index a0fb8063c8..963cfd4782 100644
> > --- a/drivers/tee/optee/optee_msg_supplicant.h
> > +++ b/drivers/tee/optee/optee_msg_supplicant.h
> > @@ -147,6 +147,11 @@
> >  #define OPTEE_MSG_RPC_CMD_SHM_ALLOC    6
> >  #define OPTEE_MSG_RPC_CMD_SHM_FREE     7
> >
> > +/*
> > + * I2C bus access
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > +
> >  /*
> >   * Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer
> >   */
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 9442d1c176..d7ab1f593f 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -60,6 +60,18 @@ static inline void optee_suppl_rpmb_release(struct udevice *dev)
> >  }
> >  #endif
> >
> > +#ifdef CONFIG_DM_I2C
> 
> We can probably assume DM_I2C is used for any recent boards, but I'd
> prefer to avoid cluttering up the header file when DM_I2C should be
> supported. See below.
> 
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +                                 struct optee_msg_arg *arg);
> 
> Function comment please
> 
> > +#else
> 
> Yuk, please don't do this..
> 
> > +void optee_suppl_cmd_i2c_transfer(struct udevice *dev,
> > +                                 struct optee_msg_arg *arg)
> > +{
> > +       debug("OPTEE_MSG_RPC_CMD_I2C_TRANSFER not implemented\n");
> > +       arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> > +}
> > +#endif
> > +
> >  void *optee_alloc_and_init_page_list(void *buf, ulong len, u64 *phys_buf_ptr);
> >
> >  #endif /* __OPTEE_PRIVATE_H */
> > diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
> > index ae042b9a20..f7738983cd 100644
> > --- a/drivers/tee/optee/supplicant.c
> > +++ b/drivers/tee/optee/supplicant.c
> > @@ -89,6 +89,9 @@ void optee_suppl_cmd(struct udevice *dev, struct tee_shm *shm_arg,
> 
> It seems a comment in the header file was missed. Can you please add
> it while you are here?
> 
> >         case OPTEE_MSG_RPC_CMD_RPMB:
> >                 optee_suppl_cmd_rpmb(dev, arg);
> >                 break;
> > +       case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
>

Didnt notice this recommendation below earlier.

do we really want to diverge from what is already in the driver?
shouldnt we better report and error if DM_I2C is not enabled as done
for RPMB?

thanks for all the comments.

> if (IS_ENABLED(DM_I2C))
> 
> > +               optee_suppl_cmd_i2c_transfer(dev, arg);
> 
> or if permitted, make TEE depend on DM_I2C
> 
> > +               break;
> >         default:
> >                 arg->ret = TEE_ERROR_NOT_IMPLEMENTED;
> >         }
> > --
> > 2.17.1
> >
> 
> Regards,
> Simon

[PATCH] drivers: tee: i2c trampoline driver

[Igor Opaniuk]

Hi Simon,

On Tue, Dec 29, 2020 at 5:34 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Jorge,
>
> On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> >
> > This commit gives the secure world access to the I2C bus so it can
> > communicate with I2C slaves (tipically those would be secure elements
>
> typically
>
> > like the NXP SE050).
> >
> > Tested on imx8mmevk.
>
> We don't seem to have any optee tests in U-Boot at present. I vaguely
> recall they were coming at some point. I think we need:
>
> - a sandbox fake drive for optee, that understands and responds to the
> 6 uclass calls at a basic level
It already exists here drivers/tee/sandbox.c. Basically what can be
additionally done is extending
existing optee sandbox driver to support RPCs (check
drivers/tee/optee/core.c, do_call_with_arg() for details)
for triggering tee supplicant. Presumably that wasn't initially done
as frankly it didn't make any sense because
of the absence of RPMB emulation in sandbox.

There are also AVB tests, that under the hood use OP-TEE API and
sandbox driver for
persistent storage read/write tests in test/py/tests/test_android/test_avb.py


> - an update to get_invoke_func() that provides a sandbox function too
>
> Then we should be able to run optee tests in CI.
>
> It is not a lot of work, but I don't think we should add to optee
> until this is resolved.
>
> Regards,
> Simon
> [.]

-- 
Best regards - Freundliche Gr?sse - Meilleures salutations

Igor Opaniuk
Embedded Software Engineer
T:  +380 938364067
E: igor.opaniuk at foundries.io
W: www.foundries.io

[PATCH] drivers: tee: i2c trampoline driver

[Jorge]

On 29/12/20, Simon Glass wrote:
> Hi Jorge,
> 
> On Tue, 29 Dec 2020 at 01:30, Jorge Ramirez-Ortiz, Foundries
> <jorge@foundries.io> wrote:
> >
> > On 28/12/20, Simon Glass wrote:
> > > Hi Jorge,
> > >
> > > On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> > > >
> > > > This commit gives the secure world access to the I2C bus so it can
> > > > communicate with I2C slaves (tipically those would be secure elements
> > >
> > > typically
> >
> > ok
> >
> > >
> > > > like the NXP SE050).
> > > >
> > > > Tested on imx8mmevk.
> > >
> > > We don't seem to have any optee tests in U-Boot at present. I vaguely
> > > recall they were coming at some point. I think we need:
> > >
> > > - a sandbox fake drive for optee, that understands and responds to the
> > > 6 uclass calls at a basic level
> > > - an update to get_invoke_func() that provides a sandbox function too
> > >
> > > Then we should be able to run optee tests in CI.
> > >
> > > It is not a lot of work, but I don't think we should add to optee
> > > until this is resolved.
> >
> > um, ok but shouldnt this infrastructure better rest on a maintainer's
> > roadmap rather than on an off-the-blue request? I mean, had I known I
> > could have done it in parallel but now I'll need to find the time to
> > do this.
> 
> We always need tests in U-Boot, so if you are not writing a test it
> would be a good question to ask as to how you can do that. Actually
> patman sometimes warns about that, but perhaps only in certain
> situations.
> 
> Actually I see that there is a test - it is hidden under the generic
> unit tests so I didn't see it. See dm/test/tee.c
> 
> I'll make some comments on the patch.
> 
> >
> > also notice that Linux's equivalent patchset was merged back in the
> > summer (ie, this is not untested code).
> >
> > https://lkml.org/lkml/2020/8/12/276
> 
> I don't see any tests in that patch though...are they somewhere
else?

hey

no you are right, I didnt post any tests to linux. And the more I
think about it the less convinced I am that we needed one.

This commit is nothing more than a RPC TEE service that shares a
buffer with U-boot and then sends requests to an I2C chip.

> Or do you justmean people have been running similar code?

I am not sure many people is using it yet other than a number of our
customers at Foundries.io across different projects. But the number of
users is at a steady growth (everybody seems to need this
functionality).

In our use case this trampoline code is used to access the NXP SE050
from OP-TEE over I2C via:
https://github.com/foundriesio/plug-and-trust

(from that link there is access to a lot of information if you are
interested)

But of course, it can be used by OP-TEE to access (or even just probe)
any chip.

> If so,
> that's fair enough but it doesn't really help us much. Lots of people
> test code manually before submitting patches, at least for their use
> case, but this is an open-source project. Over time people want to
> change and expand the code, and it is very hard for them to do that if
> there are no automated tests.

Right. And I dont disagree (everything should be testable)
jfyi I maintain a couple of platforms here so I am familiar with this
project (been using it on/off for a couple of decades already...umm
time flies, seems like yesterday).

But I dont think we need a test that verifies this service since
this is just an amalgamation of two other functions that should be
tested somewhere else (ie TEE RPC and I2C transfers).

IMO, if they dont exist already, u-boot would benefit from:
- a test that verifies TEE RPC
- a test that verifies TEE buffer sharing with U-BOOT
- a test that verifies I2C reading/writing

But not so much from
- a test that verifies TEE I2C trampoline service

I am going to repost the patch addressing some of your concerns (I
found some other issues) and if you still think that having a test
will be convenient sure we can go ahead and work on it.

> 
> Regards,
> Simon

[PATCH] drivers: tee: i2c trampoline driver

[Simon Glass]

Hi Jorge,

On Wed, 6 Jan 2021 at 10:23, Jorge Ramirez-Ortiz, Foundries
<jorge@foundries.io> wrote:
>
> On 29/12/20, Simon Glass wrote:
> > Hi Jorge,
> >
> > On Tue, 29 Dec 2020 at 01:30, Jorge Ramirez-Ortiz, Foundries
> > <jorge@foundries.io> wrote:
> > >
> > > On 28/12/20, Simon Glass wrote:
> > > > Hi Jorge,
> > > >
> > > > On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> > > > >
> > > > > This commit gives the secure world access to the I2C bus so it can
> > > > > communicate with I2C slaves (tipically those would be secure elements
> > > >
> > > > typically
> > >
> > > ok
> > >
> > > >
> > > > > like the NXP SE050).
> > > > >
> > > > > Tested on imx8mmevk.
> > > >
> > > > We don't seem to have any optee tests in U-Boot at present. I vaguely
> > > > recall they were coming at some point. I think we need:
> > > >
> > > > - a sandbox fake drive for optee, that understands and responds to the
> > > > 6 uclass calls at a basic level
> > > > - an update to get_invoke_func() that provides a sandbox function too
> > > >
> > > > Then we should be able to run optee tests in CI.
> > > >
> > > > It is not a lot of work, but I don't think we should add to optee
> > > > until this is resolved.
> > >
> > > um, ok but shouldnt this infrastructure better rest on a maintainer's
> > > roadmap rather than on an off-the-blue request? I mean, had I known I
> > > could have done it in parallel but now I'll need to find the time to
> > > do this.
> >
> > We always need tests in U-Boot, so if you are not writing a test it
> > would be a good question to ask as to how you can do that. Actually
> > patman sometimes warns about that, but perhaps only in certain
> > situations.
> >
> > Actually I see that there is a test - it is hidden under the generic
> > unit tests so I didn't see it. See dm/test/tee.c
> >
> > I'll make some comments on the patch.
> >
> > >
> > > also notice that Linux's equivalent patchset was merged back in the
> > > summer (ie, this is not untested code).
> > >
> > > https://lkml.org/lkml/2020/8/12/276
> >
> > I don't see any tests in that patch though...are they somewhere
> else?
>
> hey
>
> no you are right, I didnt post any tests to linux. And the more I
> think about it the less convinced I am that we needed one.
>
> This commit is nothing more than a RPC TEE service that shares a
> buffer with U-boot and then sends requests to an I2C chip.
>
> > Or do you justmean people have been running similar code?
>
> I am not sure many people is using it yet other than a number of our
> customers at Foundries.io across different projects. But the number of
> users is at a steady growth (everybody seems to need this
> functionality).
>
> In our use case this trampoline code is used to access the NXP SE050
> from OP-TEE over I2C via:
> https://github.com/foundriesio/plug-and-trust
>
> (from that link there is access to a lot of information if you are
> interested)
>
> But of course, it can be used by OP-TEE to access (or even just probe)
> any chip.
>
> > If so,
> > that's fair enough but it doesn't really help us much. Lots of people
> > test code manually before submitting patches, at least for their use
> > case, but this is an open-source project. Over time people want to
> > change and expand the code, and it is very hard for them to do that if
> > there are no automated tests.
>
> Right. And I dont disagree (everything should be testable)
> jfyi I maintain a couple of platforms here so I am familiar with this
> project (been using it on/off for a couple of decades already...umm
> time flies, seems like yesterday).
>
> But I dont think we need a test that verifies this service since
> this is just an amalgamation of two other functions that should be
> tested somewhere else (ie TEE RPC and I2C transfers).
>
> IMO, if they dont exist already, u-boot would benefit from:
> - a test that verifies TEE RPC

Do we have that? If not, we need it.

> - a test that verifies TEE buffer sharing with U-BOOT

I'm not sure what that is, but if there is code in U-Boot it should be tested.

> - a test that verifies I2C reading/writing

This is test/dm/i2c.c
>
> But not so much from
> - a test that verifies TEE I2C trampoline service

I suppose with the above this is trivial to write? Often tests are
just a few lines of code but they provide coverage for the feature.

>
> I am going to repost the patch addressing some of your concerns (I
> found some other issues) and if you still think that having a test
> will be convenient sure we can go ahead and work on it.

OK ta.

Regards,
Simon