From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jorge Ramirez-Ortiz, Foundries Date: Wed, 6 Jan 2021 11:48:24 +0100 Subject: [PATCH] drivers: tee: i2c trampoline driver In-Reply-To: References: <20201221181540.17949-1-jorge@foundries.io> Message-ID: <20210106104824.GB15131@trex> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 29/12/20, Simon Glass wrote: > Hi Jorge, > > On Mon, 21 Dec 2020 at 11:15, 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). > > > > 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 > > --- > > 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 > > +#include > > +#include > > +#include > > +#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