Messages in this thread Patch in this message | | | Date | Fri, 22 Apr 2022 13:56:55 +0100 | From | Cristian Marussi <> | Subject | Re: [PATCH v2] firmware: arm_scmi: support optee shared memory in optee transport |
| |
On Thu, Apr 21, 2022 at 09:49:35AM +0200, Etienne Carriere wrote: > Adds support for tee shared memory in optee scmi transport. When using > tee shared memory, scmi optee transport manages SCMI messages using > msg protocol (from msg.c) in shared memory, whereas smt (from shmem.c) > protocol is used with static IOMEM shared buffers. > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> >
Hi Etienne,
the bot complaints on linking failure are due to a missing dependency:
diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig index 7794bd41eaa0..1e7b7fec97d9 100644 --- a/drivers/firmware/arm_scmi/Kconfig +++ b/drivers/firmware/arm_scmi/Kconfig @@ -59,6 +59,7 @@ config ARM_SCMI_TRANSPORT_OPTEE depends on OPTEE=y || OPTEE=ARM_SCMI_PROTOCOL select ARM_SCMI_HAVE_TRANSPORT select ARM_SCMI_HAVE_SHMEM + select ARM_SCMI_HAVE_MSG default y help This enables the OP-TEE service based transport for SCMI. needed when you compile without any other transport using MSGs.
One remark down below.
> drivers/firmware/arm_scmi/optee.c | 128 ++++++++++++++++++++++++------ > 1 file changed, 104 insertions(+), 24 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c > index 8302a2b4aeeb..6676b17d2587 100644 > --- a/drivers/firmware/arm_scmi/optee.c > +++ b/drivers/firmware/arm_scmi/optee.c > @@ -64,6 +64,22 @@ enum scmi_optee_pta_cmd { > * [in] value[0].b: Requested capabilities mask (enum pta_scmi_caps) > */ > PTA_SCMI_CMD_GET_CHANNEL = 3, > + > + /* > + * PTA_SCMI_CMD_PROCESS_MSG_CHANNEL - Process SCMI message in a MSG > + * buffer pointed by memref parameters > + * > + * [in] value[0].a: Channel handle > + * [in] memref[1]: Message buffer (MSG and SCMI payload) > + * [out] memref[2]: Response buffer (MSG and SCMI payload) > + * > + * Shared memories used for SCMI message/response are MSG buffers > + * referenced by param[1] and param[2]. MSG transport protocol > + * uses a 32bit header to carry SCMI meta-data (protocol ID and > + * protocol message ID) followed by the effective SCMI message > + * payload. > + */ > + PTA_SCMI_CMD_PROCESS_MSG_CHANNEL = 4, > }; > > /* > @@ -72,9 +88,17 @@ enum scmi_optee_pta_cmd { > * PTA_SCMI_CAPS_SMT_HEADER > * When set, OP-TEE supports command using SMT header protocol (SCMI shmem) in > * shared memory buffers to carry SCMI protocol synchronisation information. > + * > + * PTA_SCMI_CAPS_MSG_HEADER > + * When set, OP-TEE supports command using MSG header protocol in an OP-TEE > + * shared memory to carry SCMI protocol synchronisation information and SCMI > + * message payload. > */ > #define PTA_SCMI_CAPS_NONE 0 > #define PTA_SCMI_CAPS_SMT_HEADER BIT(0) > +#define PTA_SCMI_CAPS_MSG_HEADER BIT(1) > +#define PTA_SCMI_CAPS_MASK (PTA_SCMI_CAPS_SMT_HEADER | \ > + PTA_SCMI_CAPS_MSG_HEADER) > > /** > * struct scmi_optee_channel - Description of an OP-TEE SCMI channel > @@ -85,7 +109,8 @@ enum scmi_optee_pta_cmd { > * @mu: Mutex protection on channel access > * @cinfo: SCMI channel information > * @shmem: Virtual base address of the shared memory > - * @tee_shm: Reference to TEE shared memory or NULL if using static shmem > + * @req: Shared memory protocol handle for SCMI request and synchronous response > + * @tee_shm: TEE shared memory handle @req or NULL if using IOMEM shmem > * @link: Reference in agent's channel list > */ > struct scmi_optee_channel { > @@ -94,7 +119,10 @@ struct scmi_optee_channel { > u32 caps; > struct mutex mu; > struct scmi_chan_info *cinfo; > - struct scmi_shared_mem __iomem *shmem; > + union { > + struct scmi_shared_mem __iomem *shmem; > + struct scmi_msg_payld *msg; > + } req; > struct tee_shm *tee_shm; > struct list_head link; > }; > @@ -178,8 +206,8 @@ static int get_capabilities(struct scmi_optee_agent *agent) > > caps = param[0].u.value.a; > > - if (!(caps & PTA_SCMI_CAPS_SMT_HEADER)) { > - dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT\n"); > + if (!(caps & (PTA_SCMI_CAPS_SMT_HEADER | PTA_SCMI_CAPS_MSG_HEADER))) { > + dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT and MSG\n"); > return -EOPNOTSUPP; > } > > @@ -193,9 +221,14 @@ static int get_channel(struct scmi_optee_channel *channel) > struct device *dev = scmi_optee_private->dev; > struct tee_ioctl_invoke_arg arg = { }; > struct tee_param param[1] = { }; > - unsigned int caps = PTA_SCMI_CAPS_SMT_HEADER; > + unsigned int caps = 0; > int ret; > > + if (channel->tee_shm) > + caps = PTA_SCMI_CAPS_MSG_HEADER; > + else > + caps = PTA_SCMI_CAPS_SMT_HEADER; > + > arg.func = PTA_SCMI_CMD_GET_CHANNEL; > arg.session = channel->tee_session; > arg.num_params = 1; > @@ -249,6 +282,37 @@ static int invoke_process_smt_channel(struct scmi_optee_channel *channel) > return 0; > } >
Looking at the invoke_process_smt_channel() code, I see that it still refers to ->tee_shm in an if condition when you send a PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE but that channel->tee_shm is always NULL when calling invoke_process_smt_channel() since tee_shm is only allocated by non-SMT transport via setup_dynamic_shmem(), so when this if-branch inside invoke_process_smt_channel() is supposed to be used ? or is it a leftover ? (or I am missing something :D)
> +static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t msg_size) > +{ > + struct tee_ioctl_invoke_arg arg = { > + .func = PTA_SCMI_CMD_PROCESS_MSG_CHANNEL, > + .session = channel->tee_session, > + .num_params = 3, > + }; > + struct tee_param param[3] = { }; > + int ret; > + > + param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > + param[0].u.value.a = channel->channel_id; > + > + param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > + param[1].u.memref.shm = channel->tee_shm; > + param[1].u.memref.size = msg_size; > + > + param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT; > + param[2].u.memref.shm = channel->tee_shm; > + param[2].u.memref.size = SCMI_OPTEE_MAX_MSG_SIZE; > + > + ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param); > + if (ret < 0 || arg.ret) { > + dev_err(scmi_optee_private->dev, "Can't invoke channel %u: %d / %#x\n", > + channel->channel_id, ret, arg.ret); > + return -EIO; > + } > + > + return 0; > +} > +
Thanks, Cristian
| |