[PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Etienne Carriere]

Introduce compatible "linaro,scmi-optee" for SCMI transport channel
based on an OP-TEE service invocation. The compatible mandates a
channel ID defined with property "linaro,optee-channel-id".

Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since v6:
 - Remove maxItems from linaro,optee-channel-id description

No change since v5

Changes since v4:
 - Fix sram node name in DTS example: s/-shm-/-sram-/

Changes since v3:
 - Add description for linaro,optee-channel-id in patternProperties
   specifying protocol can optionaly define a dedicated channel id.
 - Fix DTS example (duplicated phandles issue, subnodes ordering)
 - Fix typo in DTS example and description comments.

Changes since v2:
 - Define mandatory property linaro,optee-channel-id
 - Rebased on yaml description file

Changes since v1:
 - Removed modification regarding mboxes property description.
---
 .../bindings/firmware/arm,scmi.yaml           | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5c4c6782e052..eae15df36eef 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -38,6 +38,9 @@ properties:
                      The virtio transport only supports a single device.
         items:
           - const: arm,scmi-virtio
+      - description: SCMI compliant firmware with OP-TEE transport
+        items:
+          - const: linaro,scmi-optee
 
   interrupts:
     description:
@@ -83,6 +86,11 @@ properties:
     description:
       SMC id required when using smc or hvc transports
 
+  linaro,optee-channel-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Channel specifier required when using OP-TEE transport.
+
   protocol@11:
     type: object
     properties:
@@ -195,6 +203,12 @@ patternProperties:
         minItems: 1
         maxItems: 2
 
+      linaro,optee-channel-id:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:
+          Channel specifier required when using OP-TEE transport and
+          protocol has a dedicated communication channel.
+
     required:
       - reg
 
@@ -226,6 +240,16 @@ else:
       - arm,smc-id
       - shmem
 
+  else:
+    if:
+      properties:
+        compatible:
+          contains:
+            const: linaro,scmi-optee
+    then:
+      required:
+        - linaro,optee-channel-id
+
 examples:
   - |
     firmware {
@@ -340,7 +364,48 @@ examples:
                 reg = <0x11>;
                 #power-domain-cells = <1>;
             };
+        };
+    };
+
+  - |
+    firmware {
+        scmi {
+            compatible = "linaro,scmi-optee";
+            linaro,optee-channel-id = <0>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            scmi_dvfs1: protocol@13 {
+                reg = <0x13>;
+                linaro,optee-channel-id = <1>;
+                shmem = <&cpu_optee_lpri0>;
+                #clock-cells = <1>;
+            };
+
+            scmi_clk0: protocol@14 {
+                reg = <0x14>;
+                #clock-cells = <1>;
+            };
+        };
+    };
 
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        sram@51000000 {
+            compatible = "mmio-sram";
+            reg = <0x0 0x51000000 0x0 0x10000>;
+
+            #address-cells = <1>;
+            #size-cells = <1>;
+            ranges = <0 0x0 0x51000000 0x10000>;
+
+            cpu_optee_lpri0: optee-sram-section@0 {
+                compatible = "arm,scmi-shmem";
+                reg = <0x0 0x80>;
+            };
         };
     };
 
-- 
2.17.1

[PATCH v8 2/2] firmware: arm_scmi: Add optee transport

[Etienne Carriere]

Add a new transport channel to the SCMI firmware interface driver for
SCMI message exchange based on optee transport channel. The optee
transport is realized by connecting and invoking OP-TEE SCMI service
interface PTA.

Optee transport support (CONFIG_ARM_SCMI_TRANSPORT_OPTEE) is default
enabled when optee driver (CONFIG_OPTEE) is enabled. Effective optee
transport is setup upon OP-TEE SCMI service discovery at optee
device initialization. For this SCMI UUID is registered to the optee
bus for probing. This is done from the link_supplier operator of the
SCMI optee transport.

The optee transport can use a statically defined shared memory in
which case SCMI device tree node defines it using an "arm,scmi-shmem"
compatible phandle through property shmem. Alternatively, optee transport
allocates the shared memory buffer from the optee driver when no shmem
property is defined.

The protocol used to exchange SCMI message over that shared memory is
negotiated between optee transport driver and the OP-TEE service through
capabilities exchange.

OP-TEE SCMI service is integrated in OP-TEE since its release tag 3.13.0.
The service interface is published in [1].

Link: [1] https://github.com/OP-TEE/optee_os/blob/3.13.0/lib/libutee/include/pta_scmi_client.h
Cc: Cristian Marussi <cristian.marussi@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since v7:
 - Fix transport exit to not unregister scmi optee driver from tee bus
   if the transport was not initialized (hence registered to tee bus).

Changes since v6:
 - Fixed at last s/CFG_OPTEE/CONFIG_OPTEE/ in commit log.

Changes since v5:
 - scmi_optee_link_supplier() doesn't test scmi_optee_private->tee_ctx.
 - Free allocated shared memory when scmi_optee_chan_setup() fails.
 - Close session to TEE SCMI service when SCMI channel is freed.
 - Use SCMI_OPTEE_MAX_MSG_SIZE in SCMI transport descriptor.

Changes since v4:
 - Fix commit log that was not updated to v4 changes.
 - Operator scmi_optee_chan_setup() don't need the defer probe
   operation, it's already done from scmi_optee_link_supplier().

Changes since v3:
 - Fix use of configuration switches when CONFIG_OPTEE and
   CONFIG_ARM_SCMI_PROTOCOL are enabled/modules/disabled.
   Mimics scmi virtio integration.
 - Implement link_supplier operator for the scmi_optee transport
   to possibly defer probing when optee bus has not yet enumerated
   the SCMI OP-TEE service. The function ensures scmi_optee registers
   to optee bus enumeration when probe is deferred.
 - Add memory barriers to protect global optee service reference
   when it's updated at transport initialization and removal.
 - Replace enum pta_scmi_caps with macro definitions as enumerated
   types do not really match bit flags definitions. The capabilities
   data is now of type u32.
 - Use scmi_optee_ prefix for scmi transport operator handles
   and few other resources.
 - Fix typo: s/optee_smci_pta_cmd/optee_scmi_pta_cmd/
 - Remove useless DRIVER_NAME.
 - Minor reordering in struct optee_channel.
 - Removed some useless empty lines.

Changes since v2:
- Rebase on for-next/scmi, based on Linux v5.15-rc1.
- Implement support for dynamic and static shared memory.
- Factorize some functions and simplify transport exit sequence.
- Rename driver source file from optee_service.c to optee.c.

No change since v1
---
 drivers/firmware/arm_scmi/Kconfig  |  12 +
 drivers/firmware/arm_scmi/Makefile |   1 +
 drivers/firmware/arm_scmi/common.h |   3 +
 drivers/firmware/arm_scmi/driver.c |   3 +
 drivers/firmware/arm_scmi/optee.c  | 581 +++++++++++++++++++++++++++++
 5 files changed, 600 insertions(+)
 create mode 100644 drivers/firmware/arm_scmi/optee.c

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index 3d7081e84853..30746350349c 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -77,6 +77,18 @@ config ARM_SCMI_TRANSPORT_VIRTIO
 	  If you want the ARM SCMI PROTOCOL stack to include support for a
 	  transport based on VirtIO, answer Y.
 
+config ARM_SCMI_TRANSPORT_OPTEE
+	bool "SCMI transport based on OP-TEE service"
+	depends on OPTEE=y || OPTEE=ARM_SCMI_PROTOCOL
+	select ARM_SCMI_HAVE_TRANSPORT
+	select ARM_SCMI_HAVE_SHMEM
+	default y
+	help
+	  This enables the OP-TEE service based transport for SCMI.
+
+	  If you want the ARM SCMI PROTOCOL stack to include support for a
+	  transport based on OP-TEE SCMI service, answer Y.
+
 endif #ARM_SCMI_PROTOCOL
 
 config ARM_SCMI_POWER_DOMAIN
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 1dcf123d64ab..ef66ec8ca917 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -6,6 +6,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
 scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
 scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
 scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
+scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
 scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o
 scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
 		    $(scmi-transport-y)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index dea1bfbe1052..6438b5248c24 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -421,6 +421,9 @@ extern const struct scmi_desc scmi_smc_desc;
 #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
 extern const struct scmi_desc scmi_virtio_desc;
 #endif
+#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE
+extern const struct scmi_desc scmi_optee_desc;
+#endif
 
 void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv);
 void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index b406b3f78f46..e3f87e0c4936 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1999,6 +1999,9 @@ static const struct of_device_id scmi_of_match[] = {
 #endif
 #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
 	{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
+#endif
+#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE
+	{ .compatible = "linaro,scmi-optee", .data = &scmi_optee_desc },
 #endif
 	{ /* Sentinel */ },
 };
diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
new file mode 100644
index 000000000000..d9819b0197ec
--- /dev/null
+++ b/drivers/firmware/arm_scmi/optee.c
@@ -0,0 +1,581 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019-2021 Linaro Ltd.
+ */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/tee_drv.h>
+#include <linux/uuid.h>
+#include <uapi/linux/tee.h>
+
+#include "common.h"
+
+#define SCMI_OPTEE_MAX_MSG_SIZE		128
+
+enum scmi_optee_pta_cmd {
+	/*
+	 * PTA_SCMI_CMD_CAPABILITIES - Get channel capabilities
+	 *
+	 * [out]    value[0].a: Capability bit mask (enum pta_scmi_caps)
+	 * [out]    value[0].b: Extended capabilities or 0
+	 */
+	PTA_SCMI_CMD_CAPABILITIES = 0,
+
+	/*
+	 * PTA_SCMI_CMD_PROCESS_SMT_CHANNEL - Process SCMI message in SMT buffer
+	 *
+	 * [in]     value[0].a: Channel handle
+	 *
+	 * Shared memory used for SCMI message/response exhange is expected
+	 * already identified and bound to channel handle in both SCMI agent
+	 * and SCMI server (OP-TEE) parts.
+	 * The memory uses SMT header to carry SCMI meta-data (protocol ID and
+	 * protocol message ID).
+	 */
+	PTA_SCMI_CMD_PROCESS_SMT_CHANNEL = 1,
+
+	/*
+	 * PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE - Process SMT/SCMI message
+	 *
+	 * [in]     value[0].a: Channel handle
+	 * [in/out] memref[1]: Message/response buffer (SMT and SCMI payload)
+	 *
+	 * Shared memory used for SCMI message/response is a SMT buffer
+	 * referenced by param[1]. It shall be 128 bytes large to fit response
+	 * payload whatever message playload size.
+	 * The memory uses SMT header to carry SCMI meta-data (protocol ID and
+	 * protocol message ID).
+	 */
+	PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE = 2,
+
+	/*
+	 * PTA_SCMI_CMD_GET_CHANNEL - Get channel handle
+	 *
+	 * SCMI shm information are 0 if agent expects to use OP-TEE regular SHM
+	 *
+	 * [in]     value[0].a: Channel identifier
+	 * [out]    value[0].a: Returned channel handle
+	 * [in]     value[0].b: Requested capabilities mask (enum pta_scmi_caps)
+	 */
+	PTA_SCMI_CMD_GET_CHANNEL = 3,
+};
+
+/*
+ * OP-TEE SCMI service capabilities bit flags (32bit)
+ *
+ * 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.
+ */
+#define PTA_SCMI_CAPS_NONE		0
+#define PTA_SCMI_CAPS_SMT_HEADER	BIT(0)
+
+/**
+ * struct scmi_optee_channel - Description of an OP-TEE SCMI channel
+ *
+ * @channel_id: OP-TEE channel ID used for this transport
+ * @tee_session: TEE session identifier
+ * @caps: OP-TEE SCMI channel capabilities
+ * @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
+ * @link: Reference in agent's channel list
+ */
+struct scmi_optee_channel {
+	u32 channel_id;
+	u32 tee_session;
+	u32 caps;
+	struct mutex mu;
+	struct scmi_chan_info *cinfo;
+	struct scmi_shared_mem __iomem *shmem;
+	struct tee_shm *tee_shm;
+	struct list_head link;
+};
+
+/**
+ * struct scmi_optee_agent - OP-TEE transport private data
+ *
+ * @dev: Device used for communication with TEE
+ * @tee_ctx: TEE context used for communication
+ * @caps: Supported channel capabilities
+ * @mu: Mutex for protection of @channel_list
+ * @channel_list: List of all created channels for the agent
+ */
+struct scmi_optee_agent {
+	struct device *dev;
+	struct tee_context *tee_ctx;
+	u32 caps;
+	struct mutex mu;
+	struct list_head channel_list;
+};
+
+/* There can be only 1 SCMI service in OP-TEE we connect to */
+static struct scmi_optee_agent *scmi_optee_private;
+
+/* Forward reference to scmi_optee transport initialization */
+static int scmi_optee_init(void);
+
+/* Open a session toward SCMI OP-TEE service with REE_KERNEL identity */
+static int open_session(struct scmi_optee_agent *agent, u32 *tee_session)
+{
+	struct device *dev = agent->dev;
+	struct tee_client_device *scmi_pta = to_tee_client_device(dev);
+	struct tee_ioctl_open_session_arg arg = { };
+	int ret;
+
+	memcpy(arg.uuid, scmi_pta->id.uuid.b, TEE_IOCTL_UUID_LEN);
+	arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+
+	ret = tee_client_open_session(agent->tee_ctx, &arg, NULL);
+	if (ret < 0 || arg.ret) {
+		dev_err(dev, "Can't open tee session: %d / %#x\n", ret, arg.ret);
+		return -EOPNOTSUPP;
+	}
+
+	*tee_session = arg.session;
+
+	return 0;
+}
+
+static void close_session(struct scmi_optee_agent *agent, u32 tee_session)
+{
+	tee_client_close_session(agent->tee_ctx, tee_session);
+}
+
+static int get_capabilities(struct scmi_optee_agent *agent)
+{
+	struct tee_ioctl_invoke_arg arg = { };
+	struct tee_param param[1] = { };
+	u32 caps;
+	u32 tee_session;
+	int ret;
+
+	ret = open_session(agent, &tee_session);
+	if (ret)
+		return ret;
+
+	arg.func = PTA_SCMI_CMD_CAPABILITIES;
+	arg.session = tee_session;
+	arg.num_params = 1;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+
+	ret = tee_client_invoke_func(agent->tee_ctx, &arg, param);
+
+	close_session(agent, tee_session);
+
+	if (ret < 0 || arg.ret) {
+		dev_err(agent->dev, "Can't get capabilities: %d / %#x\n", ret, arg.ret);
+		return -EOPNOTSUPP;
+	}
+
+	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");
+		return -EOPNOTSUPP;
+	}
+
+	agent->caps = caps;
+
+	return 0;
+}
+
+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;
+	int ret;
+
+	arg.func = PTA_SCMI_CMD_GET_CHANNEL;
+	arg.session = channel->tee_session;
+	arg.num_params = 1;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT;
+	param[0].u.value.a = channel->channel_id;
+	param[0].u.value.b = caps;
+
+	ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param);
+
+	if (ret || arg.ret) {
+		dev_err(dev, "Can't get channel with caps %#x: %d / %#x\n", caps, ret, arg.ret);
+		return -EOPNOTSUPP;
+	}
+
+	/* From now on use channel identifer provided by OP-TEE SCMI service */
+	channel->channel_id = param[0].u.value.a;
+	channel->caps = caps;
+
+	return 0;
+}
+
+static int invoke_process_smt_channel(struct scmi_optee_channel *channel)
+{
+	struct tee_ioctl_invoke_arg arg = { };
+	struct tee_param param[2] = { };
+	int ret;
+
+	arg.session = channel->tee_session;
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+	param[0].u.value.a = channel->channel_id;
+
+	if (channel->tee_shm) {
+		param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
+		param[1].u.memref.shm = channel->tee_shm;
+		param[1].u.memref.size = SCMI_OPTEE_MAX_MSG_SIZE;
+		arg.num_params = 2;
+		arg.func = PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE;
+	} else {
+		arg.num_params = 1;
+		arg.func = PTA_SCMI_CMD_PROCESS_SMT_CHANNEL;
+	}
+
+	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;
+}
+
+static int scmi_optee_link_supplier(struct device *dev)
+{
+	if (!scmi_optee_private) {
+		if (scmi_optee_init())
+			dev_dbg(dev, "Optee bus not yet ready\n");
+
+		/* Wait for optee bus */
+		return -EPROBE_DEFER;
+	}
+
+	if (!device_link_add(dev, scmi_optee_private->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
+		dev_err(dev, "Adding link to supplier optee device failed\n");
+		return -ECANCELED;
+	}
+
+	return 0;
+}
+
+static bool scmi_optee_chan_available(struct device *dev, int idx)
+{
+	u32 channel_id;
+
+	return !of_property_read_u32_index(dev->of_node, "linaro,optee-channel-id",
+					   idx, &channel_id);
+}
+
+static void scmi_optee_clear_channel(struct scmi_chan_info *cinfo)
+{
+	struct scmi_optee_channel *channel = cinfo->transport_info;
+
+	shmem_clear_channel(channel->shmem);
+}
+
+static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
+{
+	const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
+
+	channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
+	if (IS_ERR(channel->tee_shm)) {
+		dev_err(channel->cinfo->dev, "shmem allocation failed\n");
+		return -ENOMEM;
+	}
+
+	channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
+	memset(channel->shmem, 0, msg_size);
+	shmem_clear_channel(channel->shmem);
+
+	return 0;
+}
+
+static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
+			      struct scmi_optee_channel *channel)
+{
+	struct device_node *np;
+	resource_size_t size;
+	struct resource res;
+	int ret;
+
+	np = of_parse_phandle(cinfo->dev->of_node, "shmem", 0);
+	if (!of_device_is_compatible(np, "arm,scmi-shmem")) {
+		ret = -ENXIO;
+		goto out;
+	}
+
+	ret = of_address_to_resource(np, 0, &res);
+	if (ret) {
+		dev_err(dev, "Failed to get SCMI Tx shared memory\n");
+		goto out;
+	}
+
+	size = resource_size(&res);
+
+	channel->shmem = devm_ioremap(dev, res.start, size);
+	if (!channel->shmem) {
+		dev_err(dev, "Failed to ioremap SCMI Tx shared memory\n");
+		ret = -EADDRNOTAVAIL;
+		goto out;
+	}
+
+	ret = 0;
+
+out:
+	of_node_put(np);
+
+	return ret;
+}
+
+static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
+		       struct scmi_optee_channel *channel)
+{
+	if (of_find_property(cinfo->dev->of_node, "shmem", NULL))
+		return setup_static_shmem(dev, cinfo, channel);
+	else
+		return setup_dynamic_shmem(dev, channel);
+}
+
+static int scmi_optee_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, bool tx)
+{
+	struct scmi_optee_channel *channel;
+	uint32_t channel_id;
+	int ret;
+
+	if (!tx)
+		return -ENODEV;
+
+	channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
+	if (!channel)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_index(cinfo->dev->of_node, "linaro,optee-channel-id",
+					 0, &channel_id);
+	if (ret)
+		return ret;
+
+	cinfo->transport_info = channel;
+	channel->cinfo = cinfo;
+	channel->channel_id = channel_id;
+	mutex_init(&channel->mu);
+
+	ret = setup_shmem(dev, cinfo, channel);
+	if (ret)
+		return ret;
+
+	ret = open_session(scmi_optee_private, &channel->tee_session);
+	if (ret)
+		goto err_free_shm;
+
+	ret = get_channel(channel);
+	if (ret)
+		goto err_close_sess;
+
+	mutex_lock(&scmi_optee_private->mu);
+	list_add(&channel->link, &scmi_optee_private->channel_list);
+	mutex_unlock(&scmi_optee_private->mu);
+
+	return 0;
+
+err_close_sess:
+	close_session(scmi_optee_private, channel->tee_session);
+err_free_shm:
+	if (channel->tee_shm)
+		tee_shm_free(channel->tee_shm);
+
+	return ret;
+}
+
+static int scmi_optee_chan_free(int id, void *p, void *data)
+{
+	struct scmi_chan_info *cinfo = p;
+	struct scmi_optee_channel *channel = cinfo->transport_info;
+
+	mutex_lock(&scmi_optee_private->mu);
+	list_del(&channel->link);
+	mutex_unlock(&scmi_optee_private->mu);
+
+	close_session(scmi_optee_private, channel->tee_session);
+
+	if (channel->tee_shm) {
+		tee_shm_free(channel->tee_shm);
+		channel->tee_shm = NULL;
+	}
+
+	cinfo->transport_info = NULL;
+	channel->cinfo = NULL;
+
+	scmi_free_channel(cinfo, data, id);
+
+	return 0;
+}
+
+static struct scmi_shared_mem *get_channel_shm(struct scmi_optee_channel *chan,
+					       struct scmi_xfer *xfer)
+{
+	if (!chan)
+		return NULL;
+
+	return chan->shmem;
+}
+
+
+static int scmi_optee_send_message(struct scmi_chan_info *cinfo,
+				   struct scmi_xfer *xfer)
+{
+	struct scmi_optee_channel *channel = cinfo->transport_info;
+	struct scmi_shared_mem *shmem = get_channel_shm(channel, xfer);
+	int ret;
+
+	mutex_lock(&channel->mu);
+	shmem_tx_prepare(shmem, xfer);
+
+	ret = invoke_process_smt_channel(channel);
+
+	scmi_rx_callback(cinfo, shmem_read_header(shmem), NULL);
+	mutex_unlock(&channel->mu);
+
+	return ret;
+}
+
+static void scmi_optee_fetch_response(struct scmi_chan_info *cinfo,
+				      struct scmi_xfer *xfer)
+{
+	struct scmi_optee_channel *channel = cinfo->transport_info;
+	struct scmi_shared_mem *shmem = get_channel_shm(channel, xfer);
+
+	shmem_fetch_response(shmem, xfer);
+}
+
+static bool scmi_optee_poll_done(struct scmi_chan_info *cinfo,
+				 struct scmi_xfer *xfer)
+{
+	struct scmi_optee_channel *channel = cinfo->transport_info;
+	struct scmi_shared_mem *shmem = get_channel_shm(channel, xfer);
+
+	return shmem_poll_done(shmem, xfer);
+}
+
+static struct scmi_transport_ops scmi_optee_ops = {
+	.link_supplier = scmi_optee_link_supplier,
+	.chan_available = scmi_optee_chan_available,
+	.chan_setup = scmi_optee_chan_setup,
+	.chan_free = scmi_optee_chan_free,
+	.send_message = scmi_optee_send_message,
+	.fetch_response = scmi_optee_fetch_response,
+	.clear_channel = scmi_optee_clear_channel,
+	.poll_done = scmi_optee_poll_done,
+};
+
+static int scmi_optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	return ver->impl_id == TEE_IMPL_ID_OPTEE;
+}
+
+static int scmi_optee_service_probe(struct device *dev)
+{
+	struct scmi_optee_agent *agent;
+	struct tee_context *tee_ctx;
+	int ret;
+
+	/* Only one SCMI OP-TEE device allowed */
+	if (scmi_optee_private) {
+		dev_err(dev, "An SCMI OP-TEE device was already initialized: only one allowed\n");
+		return -EBUSY;
+	}
+
+	tee_ctx = tee_client_open_context(NULL, scmi_optee_ctx_match, NULL, NULL);
+	if (IS_ERR(tee_ctx))
+		return -ENODEV;
+
+	agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
+	if (!agent) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	agent->dev = dev;
+	agent->tee_ctx = tee_ctx;
+	INIT_LIST_HEAD(&agent->channel_list);
+
+	ret = get_capabilities(agent);
+	if (ret)
+		goto err;
+
+	/* Ensure agent resources are all visible before scmi_optee_private is */
+	smp_mb();
+	scmi_optee_private = agent;
+
+	return 0;
+
+err:
+	tee_client_close_context(tee_ctx);
+
+	return ret;
+}
+
+static int scmi_optee_service_remove(struct device *dev)
+{
+	struct scmi_optee_agent *agent = scmi_optee_private;
+
+	if (!scmi_optee_private)
+		return -EINVAL;
+
+	if (!list_empty(&scmi_optee_private->channel_list))
+		return -EBUSY;
+
+	/* Ensure cleared reference is visible before resources are released */
+	smp_store_mb(scmi_optee_private, NULL);
+
+	tee_client_close_context(agent->tee_ctx);
+
+	return 0;
+}
+
+static const struct tee_client_device_id scmi_optee_service_id[] = {
+	{
+		UUID_INIT(0xa8cfe406, 0xd4f5, 0x4a2e,
+			  0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99)
+	},
+	{ }
+};
+
+MODULE_DEVICE_TABLE(tee, scmi_optee_service_id);
+
+static struct tee_client_driver scmi_optee_driver = {
+	.id_table	= scmi_optee_service_id,
+	.driver		= {
+		.name = "scmi-optee",
+		.bus = &tee_bus_type,
+		.probe = scmi_optee_service_probe,
+		.remove = scmi_optee_service_remove,
+	},
+};
+
+static int scmi_optee_init(void)
+{
+	return driver_register(&scmi_optee_driver.driver);
+}
+
+static void scmi_optee_exit(void)
+{
+	if (scmi_optee_private)
+		driver_unregister(&scmi_optee_driver.driver);
+}
+
+const struct scmi_desc scmi_optee_desc = {
+	.transport_exit = scmi_optee_exit,
+	.ops = &scmi_optee_ops,
+	.max_rx_timeout_ms = 30,
+	.max_msg = 20,
+	.max_msg_size = SCMI_OPTEE_MAX_MSG_SIZE,
+};
-- 
2.17.1

Re: [PATCH v8 2/2] firmware: arm_scmi: Add optee transport

[Cristian Marussi]

On Thu, Oct 28, 2021 at 04:00:09PM +0200, Etienne Carriere wrote:
> Add a new transport channel to the SCMI firmware interface driver for
> SCMI message exchange based on optee transport channel. The optee
> transport is realized by connecting and invoking OP-TEE SCMI service
> interface PTA.
> 

Hi Etienne,

retested a bit...just in case :P

LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Cristian Marussi]

On Thu, Oct 28, 2021 at 04:00:08PM +0200, Etienne Carriere wrote:
> Introduce compatible "linaro,scmi-optee" for SCMI transport channel
> based on an OP-TEE service invocation. The compatible mandates a
> channel ID defined with property "linaro,optee-channel-id".
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Rob Herring]

On Thu, 28 Oct 2021 16:00:08 +0200, Etienne Carriere wrote:
> Introduce compatible "linaro,scmi-optee" for SCMI transport channel
> based on an OP-TEE service invocation. The compatible mandates a
> channel ID defined with property "linaro,optee-channel-id".
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v6:
>  - Remove maxItems from linaro,optee-channel-id description
> 
> No change since v5
> 
> Changes since v4:
>  - Fix sram node name in DTS example: s/-shm-/-sram-/
> 
> Changes since v3:
>  - Add description for linaro,optee-channel-id in patternProperties
>    specifying protocol can optionaly define a dedicated channel id.
>  - Fix DTS example (duplicated phandles issue, subnodes ordering)
>  - Fix typo in DTS example and description comments.
> 
> Changes since v2:
>  - Define mandatory property linaro,optee-channel-id
>  - Rebased on yaml description file
> 
> Changes since v1:
>  - Removed modification regarding mboxes property description.
> ---
>  .../bindings/firmware/arm,scmi.yaml           | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Sudeep Holla]

On Thu, 28 Oct 2021 16:00:08 +0200, Etienne Carriere wrote:
> Introduce compatible "linaro,scmi-optee" for SCMI transport channel
> based on an OP-TEE service invocation. The compatible mandates a
> channel ID defined with property "linaro,optee-channel-id".
> 
> 


Applied to sudeep.holla/linux (for-next/scmi), thanks!




[1/2] dt-bindings: arm: Add OP-TEE transport for SCMI
      (no commit info)
[2/2] firmware: arm_scmi: Add optee transport
      (no commit info)

--

Regards,
Sudeep

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Sudeep Holla]

On Thu, 28 Oct 2021 16:00:08 +0200, Etienne Carriere wrote:
> Introduce compatible "linaro,scmi-optee" for SCMI transport channel
> based on an OP-TEE service invocation. The compatible mandates a
> channel ID defined with property "linaro,optee-channel-id".
>
>

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/2] dt-bindings: arm: Add OP-TEE transport for SCMI
      https://git.kernel.org/sudeep.holla/c/b7d2cf7c81
[2/2] firmware: arm_scmi: Add optee transport
      https://git.kernel.org/sudeep.holla/c/5f90f189a0

Sorry for the earlier incomplete message.

--
Regards,
Sudeep

Re: [PATCH v8 2/2] firmware: arm_scmi: Add optee transport

[Sudeep Holla]

On Thu, Oct 28, 2021 at 04:00:09PM +0200, Etienne Carriere wrote:
> Add a new transport channel to the SCMI firmware interface driver for
> SCMI message exchange based on optee transport channel. The optee
> transport is realized by connecting and invoking OP-TEE SCMI service
> interface PTA.
> 
> Optee transport support (CONFIG_ARM_SCMI_TRANSPORT_OPTEE) is default
> enabled when optee driver (CONFIG_OPTEE) is enabled. Effective optee
> transport is setup upon OP-TEE SCMI service discovery at optee
> device initialization. For this SCMI UUID is registered to the optee
> bus for probing. This is done from the link_supplier operator of the
> SCMI optee transport.
> 
> The optee transport can use a statically defined shared memory in
> which case SCMI device tree node defines it using an "arm,scmi-shmem"
> compatible phandle through property shmem. Alternatively, optee transport
> allocates the shared memory buffer from the optee driver when no shmem
> property is defined.
> 
> The protocol used to exchange SCMI message over that shared memory is
> negotiated between optee transport driver and the OP-TEE service through
> capabilities exchange.
> 
> OP-TEE SCMI service is integrated in OP-TEE since its release tag 3.13.0.
> The service interface is published in [1].
> 
> Link: [1] https://github.com/OP-TEE/optee_os/blob/3.13.0/lib/libutee/include/pta_scmi_client.h
> Cc: Cristian Marussi <cristian.marussi@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v7:
>  - Fix transport exit to not unregister scmi optee driver from tee bus
>    if the transport was not initialized (hence registered to tee bus).
> 
> Changes since v6:
>  - Fixed at last s/CFG_OPTEE/CONFIG_OPTEE/ in commit log.
> 
> Changes since v5:
>  - scmi_optee_link_supplier() doesn't test scmi_optee_private->tee_ctx.
>  - Free allocated shared memory when scmi_optee_chan_setup() fails.
>  - Close session to TEE SCMI service when SCMI channel is freed.
>  - Use SCMI_OPTEE_MAX_MSG_SIZE in SCMI transport descriptor.
> 
> Changes since v4:
>  - Fix commit log that was not updated to v4 changes.
>  - Operator scmi_optee_chan_setup() don't need the defer probe
>    operation, it's already done from scmi_optee_link_supplier().
> 
> Changes since v3:
>  - Fix use of configuration switches when CONFIG_OPTEE and
>    CONFIG_ARM_SCMI_PROTOCOL are enabled/modules/disabled.
>    Mimics scmi virtio integration.
>  - Implement link_supplier operator for the scmi_optee transport
>    to possibly defer probing when optee bus has not yet enumerated
>    the SCMI OP-TEE service. The function ensures scmi_optee registers
>    to optee bus enumeration when probe is deferred.
>  - Add memory barriers to protect global optee service reference
>    when it's updated at transport initialization and removal.
>  - Replace enum pta_scmi_caps with macro definitions as enumerated
>    types do not really match bit flags definitions. The capabilities
>    data is now of type u32.
>  - Use scmi_optee_ prefix for scmi transport operator handles
>    and few other resources.
>  - Fix typo: s/optee_smci_pta_cmd/optee_scmi_pta_cmd/
>  - Remove useless DRIVER_NAME.
>  - Minor reordering in struct optee_channel.
>  - Removed some useless empty lines.
> 
> Changes since v2:
> - Rebase on for-next/scmi, based on Linux v5.15-rc1.
> - Implement support for dynamic and static shared memory.
> - Factorize some functions and simplify transport exit sequence.
> - Rename driver source file from optee_service.c to optee.c.
> 
> No change since v1
> ---
>  drivers/firmware/arm_scmi/Kconfig  |  12 +
>  drivers/firmware/arm_scmi/Makefile |   1 +
>  drivers/firmware/arm_scmi/common.h |   3 +
>  drivers/firmware/arm_scmi/driver.c |   3 +
>  drivers/firmware/arm_scmi/optee.c  | 581 +++++++++++++++++++++++++++++
>  5 files changed, 600 insertions(+)
>  create mode 100644 drivers/firmware/arm_scmi/optee.c
> 

[...]

> +static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
> +{
> +	const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
> +
> +	channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
> +	if (IS_ERR(channel->tee_shm)) {
> +		dev_err(channel->cinfo->dev, "shmem allocation failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
> +	memset(channel->shmem, 0, msg_size);
> +	shmem_clear_channel(channel->shmem);
> +
> +	return 0;
> +}

I was holding on applying this patch as I reviewed this partially when I was
on vacation and couldn't remember the comment I had until I just replied now
with applied message 😄.

Anyways, is this dynamic_shmem tested on a hardware ?
shmem_* apis are based on __iomem and IIUC tee_shm_alloc_kernel_buf returns
normal memory, so you can't use those shmem_* apis as is. Please drop the
whole dynamic_shmem and send me a patch for v5.17 as we may need more changes
to support that.

I remember Broadcom or someone else wanted this with normal memory, we can
add that support correctly at once involving them too. For now, please drop
that or I can post a patch to do that if you agree with my arguments.

Sorry for completely forgetting about this.
--
Regards,
Sudeep

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Ahmad Fatoum]

Hello Etienne,

On 28.10.21 16:00, Etienne Carriere wrote:
> Introduce compatible "linaro,scmi-optee" for SCMI transport channel
> based on an OP-TEE service invocation. The compatible mandates a
> channel ID defined with property "linaro,optee-channel-id".

I just found this thread via the compatible in the STM32MP131 patch set:
https://lore.kernel.org/all/20220225133137.813919-1-gabriel.fernandez@foss.st.com/

Linux doesn't care whether PSCI is provided by TF-A, OP-TEE or something
else, so there is just the arm,psci* compatible.

What's different about SCMI that this is not possible? Why couldn't the
existing binding and driver be used to communicate with OP-TEE as secure
monitor as well?

Cheers,
Ahmad

> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v6:
>  - Remove maxItems from linaro,optee-channel-id description
> 
> No change since v5
> 
> Changes since v4:
>  - Fix sram node name in DTS example: s/-shm-/-sram-/
> 
> Changes since v3:
>  - Add description for linaro,optee-channel-id in patternProperties
>    specifying protocol can optionaly define a dedicated channel id.
>  - Fix DTS example (duplicated phandles issue, subnodes ordering)
>  - Fix typo in DTS example and description comments.
> 
> Changes since v2:
>  - Define mandatory property linaro,optee-channel-id
>  - Rebased on yaml description file
> 
> Changes since v1:
>  - Removed modification regarding mboxes property description.
> ---
>  .../bindings/firmware/arm,scmi.yaml           | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5c4c6782e052..eae15df36eef 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -38,6 +38,9 @@ properties:
>                       The virtio transport only supports a single device.
>          items:
>            - const: arm,scmi-virtio
> +      - description: SCMI compliant firmware with OP-TEE transport
> +        items:
> +          - const: linaro,scmi-optee
>  
>    interrupts:
>      description:
> @@ -83,6 +86,11 @@ properties:
>      description:
>        SMC id required when using smc or hvc transports
>  
> +  linaro,optee-channel-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Channel specifier required when using OP-TEE transport.
> +
>    protocol@11:
>      type: object
>      properties:
> @@ -195,6 +203,12 @@ patternProperties:
>          minItems: 1
>          maxItems: 2
>  
> +      linaro,optee-channel-id:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          Channel specifier required when using OP-TEE transport and
> +          protocol has a dedicated communication channel.
> +
>      required:
>        - reg
>  
> @@ -226,6 +240,16 @@ else:
>        - arm,smc-id
>        - shmem
>  
> +  else:
> +    if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: linaro,scmi-optee
> +    then:
> +      required:
> +        - linaro,optee-channel-id
> +
>  examples:
>    - |
>      firmware {
> @@ -340,7 +364,48 @@ examples:
>                  reg = <0x11>;
>                  #power-domain-cells = <1>;
>              };
> +        };
> +    };
> +
> +  - |
> +    firmware {
> +        scmi {
> +            compatible = "linaro,scmi-optee";
> +            linaro,optee-channel-id = <0>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            scmi_dvfs1: protocol@13 {
> +                reg = <0x13>;
> +                linaro,optee-channel-id = <1>;
> +                shmem = <&cpu_optee_lpri0>;
> +                #clock-cells = <1>;
> +            };
> +
> +            scmi_clk0: protocol@14 {
> +                reg = <0x14>;
> +                #clock-cells = <1>;
> +            };
> +        };
> +    };
>  
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        sram@51000000 {
> +            compatible = "mmio-sram";
> +            reg = <0x0 0x51000000 0x0 0x10000>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <1>;
> +            ranges = <0 0x0 0x51000000 0x10000>;
> +
> +            cpu_optee_lpri0: optee-sram-section@0 {
> +                compatible = "arm,scmi-shmem";
> +                reg = <0x0 0x80>;
> +            };
>          };
>      };
>  


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Etienne Carriere]

Hello Ahmad,

On Mon, 28 Feb 2022 at 17:01, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Etienne,
>
> On 28.10.21 16:00, Etienne Carriere wrote:
> > Introduce compatible "linaro,scmi-optee" for SCMI transport channel
> > based on an OP-TEE service invocation. The compatible mandates a
> > channel ID defined with property "linaro,optee-channel-id".
>
> I just found this thread via the compatible in the STM32MP131 patch set:
> https://lore.kernel.org/all/20220225133137.813919-1-gabriel.fernandez@foss.st.com/
>
> Linux doesn't care whether PSCI is provided by TF-A, OP-TEE or something
> else, so there is just the arm,psci* compatible.
>
> What's different about SCMI that this is not possible? Why couldn't the
> existing binding and driver be used to communicate with OP-TEE as secure
> monitor as well?

Compatible "linaro,scmi-optee" denote a alternate SCMI transport to
those already in v5.16.
"arm,psci" defines a mailbox + shmem interface.
"arm,psci
 it uses an optee service as in service


>
> Cheers,
> Ahmad
>
> >
> > Cc: devicetree@vger.kernel.org
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since v6:
> >  - Remove maxItems from linaro,optee-channel-id description
> >
> > No change since v5
> >
> > Changes since v4:
> >  - Fix sram node name in DTS example: s/-shm-/-sram-/
> >
> > Changes since v3:
> >  - Add description for linaro,optee-channel-id in patternProperties
> >    specifying protocol can optionaly define a dedicated channel id.
> >  - Fix DTS example (duplicated phandles issue, subnodes ordering)
> >  - Fix typo in DTS example and description comments.
> >
> > Changes since v2:
> >  - Define mandatory property linaro,optee-channel-id
> >  - Rebased on yaml description file
> >
> > Changes since v1:
> >  - Removed modification regarding mboxes property description.
> > ---
> >  .../bindings/firmware/arm,scmi.yaml           | 65 +++++++++++++++++++
> >  1 file changed, 65 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 5c4c6782e052..eae15df36eef 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -38,6 +38,9 @@ properties:
> >                       The virtio transport only supports a single device.
> >          items:
> >            - const: arm,scmi-virtio
> > +      - description: SCMI compliant firmware with OP-TEE transport
> > +        items:
> > +          - const: linaro,scmi-optee
> >
> >    interrupts:
> >      description:
> > @@ -83,6 +86,11 @@ properties:
> >      description:
> >        SMC id required when using smc or hvc transports
> >
> > +  linaro,optee-channel-id:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Channel specifier required when using OP-TEE transport.
> > +
> >    protocol@11:
> >      type: object
> >      properties:
> > @@ -195,6 +203,12 @@ patternProperties:
> >          minItems: 1
> >          maxItems: 2
> >
> > +      linaro,optee-channel-id:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description:
> > +          Channel specifier required when using OP-TEE transport and
> > +          protocol has a dedicated communication channel.
> > +
> >      required:
> >        - reg
> >
> > @@ -226,6 +240,16 @@ else:
> >        - arm,smc-id
> >        - shmem
> >
> > +  else:
> > +    if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: linaro,scmi-optee
> > +    then:
> > +      required:
> > +        - linaro,optee-channel-id
> > +
> >  examples:
> >    - |
> >      firmware {
> > @@ -340,7 +364,48 @@ examples:
> >                  reg = <0x11>;
> >                  #power-domain-cells = <1>;
> >              };
> > +        };
> > +    };
> > +
> > +  - |
> > +    firmware {
> > +        scmi {
> > +            compatible = "linaro,scmi-optee";
> > +            linaro,optee-channel-id = <0>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            scmi_dvfs1: protocol@13 {
> > +                reg = <0x13>;
> > +                linaro,optee-channel-id = <1>;
> > +                shmem = <&cpu_optee_lpri0>;
> > +                #clock-cells = <1>;
> > +            };
> > +
> > +            scmi_clk0: protocol@14 {
> > +                reg = <0x14>;
> > +                #clock-cells = <1>;
> > +            };
> > +        };
> > +    };
> >
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        sram@51000000 {
> > +            compatible = "mmio-sram";
> > +            reg = <0x0 0x51000000 0x0 0x10000>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <1>;
> > +            ranges = <0 0x0 0x51000000 0x10000>;
> > +
> > +            cpu_optee_lpri0: optee-sram-section@0 {
> > +                compatible = "arm,scmi-shmem";
> > +                reg = <0x0 0x80>;
> > +            };
> >          };
> >      };
> >
>
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Etienne Carriere]

Hi sorry,

I sent the mail while i was still typing it...
Here is with the full answer.


On Tue, 1 Mar 2022 at 15:05, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Ahmad,
>
> On Mon, 28 Feb 2022 at 17:01, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >
> > Hello Etienne,
> >
> > On 28.10.21 16:00, Etienne Carriere wrote:
> > > Introduce compatible "linaro,scmi-optee" for SCMI transport channel
> > > based on an OP-TEE service invocation. The compatible mandates a
> > > channel ID defined with property "linaro,optee-channel-id".
> >
> > I just found this thread via the compatible in the STM32MP131 patch set:
> > https://lore.kernel.org/all/20220225133137.813919-1-gabriel.fernandez@foss.st.com/
> >
> > Linux doesn't care whether PSCI is provided by TF-A, OP-TEE or something
> > else, so there is just the arm,psci* compatible.
> >
> > What's different about SCMI that this is not possible? Why couldn't the
> > existing binding and driver be used to communicate with OP-TEE as secure
> > monitor as well?
>
> Compatible "linaro,scmi-optee" denote a alternate SCMI transport to
> those already in v5.16.

It is names scmi-optee because the interface exposed to access SCMI services is
based on TEE's interface (UUID to open a session with and invoke commands).

The compatible is described in the Linux Documentation but not yet
merged in the linux-next.
It can be found in the tree of arm_scmi driver maintainers:
https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-linux-next

This commit:
https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/commit/?h=for-linux-next&id=b7d2cf7c817b86e705b97f72c6be192a6760a14f

Br,
Etienne

>
>
> >
> > Cheers,
> > Ahmad
> >
> > >
> > > Cc: devicetree@vger.kernel.org
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > > Changes since v6:
> > >  - Remove maxItems from linaro,optee-channel-id description
> > >
> > > No change since v5
> > >
> > > Changes since v4:
> > >  - Fix sram node name in DTS example: s/-shm-/-sram-/
> > >
> > > Changes since v3:
> > >  - Add description for linaro,optee-channel-id in patternProperties
> > >    specifying protocol can optionaly define a dedicated channel id.
> > >  - Fix DTS example (duplicated phandles issue, subnodes ordering)
> > >  - Fix typo in DTS example and description comments.
> > >
> > > Changes since v2:
> > >  - Define mandatory property linaro,optee-channel-id
> > >  - Rebased on yaml description file
> > >
> > > Changes since v1:
> > >  - Removed modification regarding mboxes property description.
> > > ---
> > >  .../bindings/firmware/arm,scmi.yaml           | 65 +++++++++++++++++++
> > >  1 file changed, 65 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > index 5c4c6782e052..eae15df36eef 100644
> > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > @@ -38,6 +38,9 @@ properties:
> > >                       The virtio transport only supports a single device.
> > >          items:
> > >            - const: arm,scmi-virtio
> > > +      - description: SCMI compliant firmware with OP-TEE transport
> > > +        items:
> > > +          - const: linaro,scmi-optee
> > >
> > >    interrupts:
> > >      description:
> > > @@ -83,6 +86,11 @@ properties:
> > >      description:
> > >        SMC id required when using smc or hvc transports
> > >
> > > +  linaro,optee-channel-id:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    description:
> > > +      Channel specifier required when using OP-TEE transport.
> > > +
> > >    protocol@11:
> > >      type: object
> > >      properties:
> > > @@ -195,6 +203,12 @@ patternProperties:
> > >          minItems: 1
> > >          maxItems: 2
> > >
> > > +      linaro,optee-channel-id:
> > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > +        description:
> > > +          Channel specifier required when using OP-TEE transport and
> > > +          protocol has a dedicated communication channel.
> > > +
> > >      required:
> > >        - reg
> > >
> > > @@ -226,6 +240,16 @@ else:
> > >        - arm,smc-id
> > >        - shmem
> > >
> > > +  else:
> > > +    if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: linaro,scmi-optee
> > > +    then:
> > > +      required:
> > > +        - linaro,optee-channel-id
> > > +
> > >  examples:
> > >    - |
> > >      firmware {
> > > @@ -340,7 +364,48 @@ examples:
> > >                  reg = <0x11>;
> > >                  #power-domain-cells = <1>;
> > >              };
> > > +        };
> > > +    };
> > > +
> > > +  - |
> > > +    firmware {
> > > +        scmi {
> > > +            compatible = "linaro,scmi-optee";
> > > +            linaro,optee-channel-id = <0>;
> > > +
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +
> > > +            scmi_dvfs1: protocol@13 {
> > > +                reg = <0x13>;
> > > +                linaro,optee-channel-id = <1>;
> > > +                shmem = <&cpu_optee_lpri0>;
> > > +                #clock-cells = <1>;
> > > +            };
> > > +
> > > +            scmi_clk0: protocol@14 {
> > > +                reg = <0x14>;
> > > +                #clock-cells = <1>;
> > > +            };
> > > +        };
> > > +    };
> > >
> > > +    soc {
> > > +        #address-cells = <2>;
> > > +        #size-cells = <2>;
> > > +
> > > +        sram@51000000 {
> > > +            compatible = "mmio-sram";
> > > +            reg = <0x0 0x51000000 0x0 0x10000>;
> > > +
> > > +            #address-cells = <1>;
> > > +            #size-cells = <1>;
> > > +            ranges = <0 0x0 0x51000000 0x10000>;
> > > +
> > > +            cpu_optee_lpri0: optee-sram-section@0 {
> > > +                compatible = "arm,scmi-shmem";
> > > +                reg = <0x0 0x80>;
> > > +            };
> > >          };
> > >      };
> > >
> >
> >
> > --
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Sudeep Holla]

Hi Ahmad,

On Mon, Feb 28, 2022 at 05:01:39PM +0100, Ahmad Fatoum wrote:
> Hello Etienne,
> 
> On 28.10.21 16:00, Etienne Carriere wrote:
> > Introduce compatible "linaro,scmi-optee" for SCMI transport channel
> > based on an OP-TEE service invocation. The compatible mandates a
> > channel ID defined with property "linaro,optee-channel-id".
>

Not sure if Etienne's reply addressed your queries/concerns correctly.
I thought I will add my view anyways.

> I just found this thread via the compatible in the STM32MP131 patch set:
> https://lore.kernel.org/all/20220225133137.813919-1-gabriel.fernandez@foss.st.com/
> 
> Linux doesn't care whether PSCI is provided by TF-A, OP-TEE or something
> else, so there is just the arm,psci* compatible.
>

Correct, the interface to the kernel is fixed and hence we must be able
to manage with the standard and fixed sole set of bindings for the same.

> What's different about SCMI that this is not possible? Why couldn't the
> existing binding and driver be used to communicate with OP-TEE as secure
> monitor as well?
>

However with SCMI, the spec concentrates and standardises all the aspects
of the protocol used for the communication while it allows the transport
used for such a communication to be implementation specific. It does
address some standard transports like mailbox and PCC(ACPI). However,
because of the flexibility and also depending on the hardware(or VM),
different transports have been added to the list. SMC/HVC was the one,
followed by the virtio and OPTEE. While I agree SMC/HVC and OPTEE seem
to have lot of common and may have avoided separate bindings.

However the FIDs for SMC/HVC is vendor defined(the spec doesn't cover this
and hence we utilised/exploited DT). Some vendors wanted interrupt support
too which got added. OPTEE eliminates the need for FID and can also provide
dynamic shared memory info. In short, it does differ in a way that the driver
needs to understand the difference and act differently with each of the
unique transports defined in the binding.

Hope that explains and addresses your concern.

-- 
Regards,
Sudeep

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Ahmad Fatoum]

Hello Sudeep,

On 01.03.22 16:12, Sudeep Holla wrote:
> 
> Hi Ahmad,
> 
> On Mon, Feb 28, 2022 at 05:01:39PM +0100, Ahmad Fatoum wrote:
>> Hello Etienne,
>>
>> On 28.10.21 16:00, Etienne Carriere wrote:
>>> Introduce compatible "linaro,scmi-optee" for SCMI transport channel
>>> based on an OP-TEE service invocation. The compatible mandates a
>>> channel ID defined with property "linaro,optee-channel-id".
>>
> 
> Not sure if Etienne's reply addressed your queries/concerns correctly.
> I thought I will add my view anyways.
> 
>> I just found this thread via the compatible in the STM32MP131 patch set:
>> https://lore.kernel.org/all/20220225133137.813919-1-gabriel.fernandez@foss.st.com/
>>
>> Linux doesn't care whether PSCI is provided by TF-A, OP-TEE or something
>> else, so there is just the arm,psci* compatible.
>>
> 
> Correct, the interface to the kernel is fixed and hence we must be able
> to manage with the standard and fixed sole set of bindings for the same.
> 
>> What's different about SCMI that this is not possible? Why couldn't the
>> existing binding and driver be used to communicate with OP-TEE as secure
>> monitor as well?
>>
> 
> However with SCMI, the spec concentrates and standardises all the aspects
> of the protocol used for the communication while it allows the transport
> used for such a communication to be implementation specific. It does
> address some standard transports like mailbox and PCC(ACPI). However,
> because of the flexibility and also depending on the hardware(or VM),
> different transports have been added to the list. SMC/HVC was the one,
> followed by the virtio and OPTEE. While I agree SMC/HVC and OPTEE seem
> to have lot of common and may have avoided separate bindings.
> 
> However the FIDs for SMC/HVC is vendor defined(the spec doesn't cover this
> and hence we utilised/exploited DT). Some vendors wanted interrupt support
> too which got added. OPTEE eliminates the need for FID and can also provide
> dynamic shared memory info. In short, it does differ in a way that the driver
> needs to understand the difference and act differently with each of the
> unique transports defined in the binding.
> 
> Hope that explains and addresses your concern.

Thanks for the elaborate answer. I see now why it's beneficial to have
an OP-TEE transport in general. I don't yet see the benefit to use it
in the STM32MP13x instead of SMCs like with STM32MP15x, but that a discussion
that I need to have in the aforementioned thread.

Thanks again!
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Ahmad Fatoum]

Hell Etienne,

On 01.03.22 15:11, Etienne Carriere wrote:
> Hi sorry,
> 
> I sent the mail while i was still typing it...
> Here is with the full answer.

Thanks for taking the time.

> On Tue, 1 Mar 2022 at 15:05, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
>>
>> Hello Ahmad,
>>
>> On Mon, 28 Feb 2022 at 17:01, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>>
>>> Hello Etienne,
>>>
>>> On 28.10.21 16:00, Etienne Carriere wrote:
>>>> Introduce compatible "linaro,scmi-optee" for SCMI transport channel
>>>> based on an OP-TEE service invocation. The compatible mandates a
>>>> channel ID defined with property "linaro,optee-channel-id".
>>>
>>> I just found this thread via the compatible in the STM32MP131 patch set:
>>> https://lore.kernel.org/all/20220225133137.813919-1-gabriel.fernandez@foss.st.com/
>>>
>>> Linux doesn't care whether PSCI is provided by TF-A, OP-TEE or something
>>> else, so there is just the arm,psci* compatible.
>>>
>>> What's different about SCMI that this is not possible? Why couldn't the
>>> existing binding and driver be used to communicate with OP-TEE as secure
>>> monitor as well?
>>
>> Compatible "linaro,scmi-optee" denote a alternate SCMI transport to
>> those already in v5.16.
> 
> It is names scmi-optee because the interface exposed to access SCMI services is
> based on TEE's interface (UUID to open a session with and invoke commands).

I gathered as much, but I didn't understand why it had to be an extra transport
when SCMI over SMC already exists. Sudeep cleared this point up for me.

Cheers,
Ahmad

> 
> The compatible is described in the Linux Documentation but not yet
> merged in the linux-next.
> It can be found in the tree of arm_scmi driver maintainers:
> https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-linux-next
> 
> This commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/commit/?h=for-linux-next&id=b7d2cf7c817b86e705b97f72c6be192a6760a14f
> 
> Br,
> Etienne
> 
>>
>>
>>>
>>> Cheers,
>>> Ahmad
>>>
>>>>
>>>> Cc: devicetree@vger.kernel.org
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
>>>> ---
>>>> Changes since v6:
>>>>  - Remove maxItems from linaro,optee-channel-id description
>>>>
>>>> No change since v5
>>>>
>>>> Changes since v4:
>>>>  - Fix sram node name in DTS example: s/-shm-/-sram-/
>>>>
>>>> Changes since v3:
>>>>  - Add description for linaro,optee-channel-id in patternProperties
>>>>    specifying protocol can optionaly define a dedicated channel id.
>>>>  - Fix DTS example (duplicated phandles issue, subnodes ordering)
>>>>  - Fix typo in DTS example and description comments.
>>>>
>>>> Changes since v2:
>>>>  - Define mandatory property linaro,optee-channel-id
>>>>  - Rebased on yaml description file
>>>>
>>>> Changes since v1:
>>>>  - Removed modification regarding mboxes property description.
>>>> ---
>>>>  .../bindings/firmware/arm,scmi.yaml           | 65 +++++++++++++++++++
>>>>  1 file changed, 65 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>>> index 5c4c6782e052..eae15df36eef 100644
>>>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>>> @@ -38,6 +38,9 @@ properties:
>>>>                       The virtio transport only supports a single device.
>>>>          items:
>>>>            - const: arm,scmi-virtio
>>>> +      - description: SCMI compliant firmware with OP-TEE transport
>>>> +        items:
>>>> +          - const: linaro,scmi-optee
>>>>
>>>>    interrupts:
>>>>      description:
>>>> @@ -83,6 +86,11 @@ properties:
>>>>      description:
>>>>        SMC id required when using smc or hvc transports
>>>>
>>>> +  linaro,optee-channel-id:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      Channel specifier required when using OP-TEE transport.
>>>> +
>>>>    protocol@11:
>>>>      type: object
>>>>      properties:
>>>> @@ -195,6 +203,12 @@ patternProperties:
>>>>          minItems: 1
>>>>          maxItems: 2
>>>>
>>>> +      linaro,optee-channel-id:
>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>>> +        description:
>>>> +          Channel specifier required when using OP-TEE transport and
>>>> +          protocol has a dedicated communication channel.
>>>> +
>>>>      required:
>>>>        - reg
>>>>
>>>> @@ -226,6 +240,16 @@ else:
>>>>        - arm,smc-id
>>>>        - shmem
>>>>
>>>> +  else:
>>>> +    if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            const: linaro,scmi-optee
>>>> +    then:
>>>> +      required:
>>>> +        - linaro,optee-channel-id
>>>> +
>>>>  examples:
>>>>    - |
>>>>      firmware {
>>>> @@ -340,7 +364,48 @@ examples:
>>>>                  reg = <0x11>;
>>>>                  #power-domain-cells = <1>;
>>>>              };
>>>> +        };
>>>> +    };
>>>> +
>>>> +  - |
>>>> +    firmware {
>>>> +        scmi {
>>>> +            compatible = "linaro,scmi-optee";
>>>> +            linaro,optee-channel-id = <0>;
>>>> +
>>>> +            #address-cells = <1>;
>>>> +            #size-cells = <0>;
>>>> +
>>>> +            scmi_dvfs1: protocol@13 {
>>>> +                reg = <0x13>;
>>>> +                linaro,optee-channel-id = <1>;
>>>> +                shmem = <&cpu_optee_lpri0>;
>>>> +                #clock-cells = <1>;
>>>> +            };
>>>> +
>>>> +            scmi_clk0: protocol@14 {
>>>> +                reg = <0x14>;
>>>> +                #clock-cells = <1>;
>>>> +            };
>>>> +        };
>>>> +    };
>>>>
>>>> +    soc {
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +
>>>> +        sram@51000000 {
>>>> +            compatible = "mmio-sram";
>>>> +            reg = <0x0 0x51000000 0x0 0x10000>;
>>>> +
>>>> +            #address-cells = <1>;
>>>> +            #size-cells = <1>;
>>>> +            ranges = <0 0x0 0x51000000 0x10000>;
>>>> +
>>>> +            cpu_optee_lpri0: optee-sram-section@0 {
>>>> +                compatible = "arm,scmi-shmem";
>>>> +                reg = <0x0 0x80>;
>>>> +            };
>>>>          };
>>>>      };
>>>>
>>>
>>>
>>> --
>>> Pengutronix e.K.                           |                             |
>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Etienne Carriere]

Hello Ahmad,

On Tue, 8 Mar 2022 at 10:51, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Sudeep,
>
> On 01.03.22 16:12, Sudeep Holla wrote:
> >
> > Hi Ahmad,
> >
> > On Mon, Feb 28, 2022 at 05:01:39PM +0100, Ahmad Fatoum wrote:
> >> Hello Etienne,
> >>
> >> On 28.10.21 16:00, Etienne Carriere wrote:
> >>> Introduce compatible "linaro,scmi-optee" for SCMI transport channel
> >>> based on an OP-TEE service invocation. The compatible mandates a
> >>> channel ID defined with property "linaro,optee-channel-id".
> >>
> >
> > Not sure if Etienne's reply addressed your queries/concerns correctly.
> > I thought I will add my view anyways.
> >
> >> I just found this thread via the compatible in the STM32MP131 patch set:
> >> https://lore.kernel.org/all/20220225133137.813919-1-gabriel.fernandez@foss.st.com/
> >>
> >> Linux doesn't care whether PSCI is provided by TF-A, OP-TEE or something
> >> else, so there is just the arm,psci* compatible.
> >>
> >
> > Correct, the interface to the kernel is fixed and hence we must be able
> > to manage with the standard and fixed sole set of bindings for the same.
> >
> >> What's different about SCMI that this is not possible? Why couldn't the
> >> existing binding and driver be used to communicate with OP-TEE as secure
> >> monitor as well?
> >>
> >
> > However with SCMI, the spec concentrates and standardises all the aspects
> > of the protocol used for the communication while it allows the transport
> > used for such a communication to be implementation specific. It does
> > address some standard transports like mailbox and PCC(ACPI). However,
> > because of the flexibility and also depending on the hardware(or VM),
> > different transports have been added to the list. SMC/HVC was the one,
> > followed by the virtio and OPTEE. While I agree SMC/HVC and OPTEE seem
> > to have lot of common and may have avoided separate bindings.
> >
> > However the FIDs for SMC/HVC is vendor defined(the spec doesn't cover this
> > and hence we utilised/exploited DT). Some vendors wanted interrupt support
> > too which got added. OPTEE eliminates the need for FID and can also provide
> > dynamic shared memory info. In short, it does differ in a way that the driver
> > needs to understand the difference and act differently with each of the
> > unique transports defined in the binding.
> >
> > Hope that explains and addresses your concern.
>
> Thanks for the elaborate answer. I see now why it's beneficial to have
> an OP-TEE transport in general. I don't yet see the benefit to use it
> in the STM32MP13x instead of SMCs like with STM32MP15x, but that a discussion
> that I need to have in the aforementioned thread.

Some SCMI operations in OP-TEE need to execute in a threaded context
(preemptible, ...).
There is no SMC function ID defined for an SCMI thread entry in
OP-TEE. We rather use standard invocation of a TEE service: opening a
session and invoking commands.
Invoked commands are executed in an OP-TEE native threaded context.
The service accessed is referred to as the OP-TEE SCMI PTA.

As for STM32MP15x, one willing to extend resources assigned to secure
world may also need to move mp15 SCMI from SMC transport to optee
transport.

Regards,
Etienne

>
> Thanks again!
> Ahmad
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v8 1/2] dt-bindings: arm: Add OP-TEE transport for SCMI

[Ahmad Fatoum]

Hello Etienne,

On 08.03.22 11:18, Etienne Carriere wrote:
> Hello Ahmad,
> 
> On Tue, 8 Mar 2022 at 10:51, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>
>> Hello Sudeep,
>>
>> On 01.03.22 16:12, Sudeep Holla wrote:
>>>
>>> Hi Ahmad,
>>>
>>> On Mon, Feb 28, 2022 at 05:01:39PM +0100, Ahmad Fatoum wrote:
>>>> Hello Etienne,
>>>>
>>>> On 28.10.21 16:00, Etienne Carriere wrote:
>>>>> Introduce compatible "linaro,scmi-optee" for SCMI transport channel
>>>>> based on an OP-TEE service invocation. The compatible mandates a
>>>>> channel ID defined with property "linaro,optee-channel-id".
>>>>
>>>
>>> Not sure if Etienne's reply addressed your queries/concerns correctly.
>>> I thought I will add my view anyways.
>>>
>>>> I just found this thread via the compatible in the STM32MP131 patch set:
>>>> https://lore.kernel.org/all/20220225133137.813919-1-gabriel.fernandez@foss.st.com/
>>>>
>>>> Linux doesn't care whether PSCI is provided by TF-A, OP-TEE or something
>>>> else, so there is just the arm,psci* compatible.
>>>>
>>>
>>> Correct, the interface to the kernel is fixed and hence we must be able
>>> to manage with the standard and fixed sole set of bindings for the same.
>>>
>>>> What's different about SCMI that this is not possible? Why couldn't the
>>>> existing binding and driver be used to communicate with OP-TEE as secure
>>>> monitor as well?
>>>>
>>>
>>> However with SCMI, the spec concentrates and standardises all the aspects
>>> of the protocol used for the communication while it allows the transport
>>> used for such a communication to be implementation specific. It does
>>> address some standard transports like mailbox and PCC(ACPI). However,
>>> because of the flexibility and also depending on the hardware(or VM),
>>> different transports have been added to the list. SMC/HVC was the one,
>>> followed by the virtio and OPTEE. While I agree SMC/HVC and OPTEE seem
>>> to have lot of common and may have avoided separate bindings.
>>>
>>> However the FIDs for SMC/HVC is vendor defined(the spec doesn't cover this
>>> and hence we utilised/exploited DT). Some vendors wanted interrupt support
>>> too which got added. OPTEE eliminates the need for FID and can also provide
>>> dynamic shared memory info. In short, it does differ in a way that the driver
>>> needs to understand the difference and act differently with each of the
>>> unique transports defined in the binding.
>>>
>>> Hope that explains and addresses your concern.
>>
>> Thanks for the elaborate answer. I see now why it's beneficial to have
>> an OP-TEE transport in general. I don't yet see the benefit to use it
>> in the STM32MP13x instead of SMCs like with STM32MP15x, but that a discussion
>> that I need to have in the aforementioned thread.
> 
> Some SCMI operations in OP-TEE need to execute in a threaded context
> (preemptible, ...).
> There is no SMC function ID defined for an SCMI thread entry in
> OP-TEE. We rather use standard invocation of a TEE service: opening a
> session and invoking commands.
> Invoked commands are executed in an OP-TEE native threaded context.
> The service accessed is referred to as the OP-TEE SCMI PTA.
> 
> As for STM32MP15x, one willing to extend resources assigned to secure
> world may also need to move mp15 SCMI from SMC transport to optee
> transport.

Yes. Makes sense.

Thanks again for explaining,
Ahmad

> 
> Regards,
> Etienne
> 
>>
>> Thanks again!
>> Ahmad
>>
>> --
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
etienn

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |