[PATCH] drivers: tee: i2c trampoline driver
Jorge Ramirez-Ortiz, Foundries
jorge at foundries.io
Wed Jan 6 11:24:30 CET 2021
On 29/12/20, Simon Glass wrote:
> Hi Jorge,
>
> On Mon, 21 Dec 2020 at 11:15, Jorge Ramirez-Ortiz <jorge at 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 at 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
More information about the U-Boot
mailing list