[PATCH v2 1/3] dt-bindings: arm: optee: add interrupt controller properties

[PATCH v2 1/3] dt-bindings: arm: optee: add interrupt controller properties

[Etienne Carriere]

Adds an optional interrupt controller property to optee firmware node
in the DT bindings. Optee driver may embeds an irqchip exposing
interrupts notified by the TEE world. Optee registers up to 1 interrupt
controller and identifies each line with a line number from 0 to
UINT16_MAX.

In the example, the platform SCMI device uses optee interrupt irq 5
as async signal to trigger processing of an async incoming SCMI message,
in the scope of a CPU DVFS control. A platform can have several SCMI
channels driven this way. Optee irqs also permits small embedded devices
to share e.g. a gpio expander, a group of wakeup sources, etc... between
OP-TEE world (for sensitive services) and Linux world (for non-sensitive
services). The physical controller is driven from the TEE which exposes
some controls to Linux kernel.

Cc: Jens Wiklander <jens.wiklander@linaro.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Sumit Garg <sumit.garg@linaro.org>

Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since v1:
- Added a description to #interrupt-cells property.
- Changed of example. Linux wakeup event was subject to discussion and
  i don't know much about input events in Linux. So move to SCMI.
  In the example, an SCMI server in OP-TEE world raises optee irq 5
  so that Linux scmi optee channel &scmi_cpu_dvfs pushed in the incoming
  SCMI message in the scmi device for liekly later processing in threaded
  context. The example includes all parties: optee, scmi, sram, gic.
- Obviously rephrased the commit message.
- Added Cc: tags
---
 .../arm/firmware/linaro,optee-tz.yaml         | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
index d4dc0749f9fd..9c00c27f8b2c 100644
--- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
+++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
@@ -40,6 +40,14 @@ properties:
       HVC #0, register assignments
       register assignments are specified in drivers/tee/optee/optee_smc.h
 
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 1
+    description: |
+      OP-TEE exposes irq for irp chip controllers from OP-TEE world. Each
+      irq is assigned a single line number identifier used as first argument.
+
 required:
   - compatible
   - method
@@ -64,3 +72,62 @@ examples:
             method = "hvc";
         };
     };
+
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    firmware  {
+        optee: optee {
+            compatible = "linaro,optee-tz";
+            method = "smc";
+            interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
+            interrupt-controller;
+            #interrupt-cells = <1>;
+        };
+
+        scmi {
+            compatible = "linaro,scmi-optee";
+            linaro,optee-channel-id = <0>;
+            interrupt-parent = <&gic>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            scmi_cpu_dvfs: protocol@13 {
+                reg = <0x13>;
+                linaro,optee-channel-id = <1>;
+                shmem = <&scmi_shm_tx>, <&scmi_shm_rx>;
+                interrupts-extended = <&optee 5>;
+                interrupt-names = "a2p";
+                #clock-cells = <1>;
+            };
+        };
+    };
+
+    gic: interrupt-controller@a0021000 {
+        compatible = "arm,cortex-a7-gic";
+        reg = <0xa0021000 0x1000>, <0xa0022000 0x2000>;
+        interrupt-controller;
+        #interrupt-cells = <3>;
+    };
+
+    soc {
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        sram@2ffff000 {
+            compatible = "mmio-sram";
+            reg = <0x2ffff000 0x1000>;
+            #address-cells = <1>;
+            #size-cells = <1>;
+            ranges = <0 0x2ffff000 0x1000>;
+
+            scmi_shm_tx: scmi-sram@0 {
+                compatible = "arm,scmi-shmem";
+                reg = <0 0x80>;
+            };
+
+            scmi_shm_rx: scmi-sram@100 {
+                compatible = "arm,scmi-shmem";
+                reg = <0x100 0x80>;
+            };
+        };
+    };
-- 
2.25.1

[PATCH v2 2/3] optee: multiplex tee interrupt events on optee async notif irq

[Etienne Carriere]

Implements an irqchip in optee driver to relay interrupts notified
from OP-TEE world to the Linux OS. Optee registers up to 1 interrupt
controller and identifies each line with a line number from 0 to
UINT16_MAX.

Existing optee async notif uses an irq for OP-TEE to signal Linux of a
pending event. The implementation binds each event (an async notif value)
to the awaking of a waiting thread or the processing of some TEE
background (bottom half) tasks to be scheduled. The interrupt
notification service added by this change allows TEE to relay interrupt
signals to Linux on secure interrupt occurrences where the end consumer
of the signal lives in normal world.

When optee driver initializes, it negotiates with the TEE whether
interrupt notification is supported or not. The feature is enabled
if both Linux kernel and OP-TEE support it.

OP-TEE defines 2 SMC function IDs for non-secure world to control
interrupt events.

SMC function OPTEE_SMC_GET_NOTIF_IT allows non-secure world to
retrieve pending interrupts by grapes up to 5 lines. The function also
reports whether there are pending async values targeting suspended
threaded sequences execution and whether TEE has background threaded
work to do (async notif value 0 was retrieved).

SMC function OPTEE_SMC_CONFIG_NOTIF_IT configures the insterrupt line
for masking and enabling state and wakeup source.

Cc: Jens Wiklander <jens.wiklander@linaro.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Sumit Garg <sumit.garg@linaro.org>

Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
Co-developed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since v1:
- Removed dependency on optee per-cpu irq notification.
- Change SMC function ID API to retrieves up to 5 optee irq events,
  the optee bottom half event and returns if other async notifications
  are pending, in a single invocation.
- Implement only mask/unmask irqchip handlers with a 2nd SMC function
  to mask/unmask a optee irq in OP-TEE world from an interrupt context.
- Added Cc: tags.
---
 drivers/tee/optee/optee_private.h |  10 ++
 drivers/tee/optee/optee_smc.h     |  88 ++++++++++++++++-
 drivers/tee/optee/smc_abi.c       | 158 ++++++++++++++++++++++++++++--
 3 files changed, 249 insertions(+), 7 deletions(-)

diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 04ae58892608..f467409e02e9 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -94,11 +94,21 @@ struct optee_supp {
 	struct completion reqs_c;
 };
 
+/*
+ * struct optee_smc - optee smc communication struct
+ * @invoke_fn		handler function to invoke secure monitor
+ * @memremaped_shm	virtual address of memory in shared memory pool
+ * @sec_caps:		secure world capabilities defined by
+ *			OPTEE_SMC_SEC_CAP_* in optee_smc.h
+ * @notif_irq		interrupt used as async notification by OP-TEE or 0
+ * @domain		interrupt domain registered by OP-TEE driver
+ */
 struct optee_smc {
 	optee_invoke_fn *invoke_fn;
 	void *memremaped_shm;
 	u32 sec_caps;
 	unsigned int notif_irq;
+	struct irq_domain *domain;
 };
 
 /**
diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
index 73b5e7760d10..df32ad18eddb 100644
--- a/drivers/tee/optee/optee_smc.h
+++ b/drivers/tee/optee/optee_smc.h
@@ -226,7 +226,8 @@ struct optee_smc_get_shm_config_result {
  * a3	Bit[7:0]: Number of parameters needed for RPC to be supplied
  *		  as the second MSG arg struct for
  *		  OPTEE_SMC_CALL_WITH_ARG
- *	Bit[31:8]: Reserved (MBZ)
+ *	Bit[23:8]: The maximum interrupt event notification number
+ *	Bit[31:24]: Reserved (MBZ)
  * a4-7	Preserved
  *
  * Error return register usage:
@@ -254,6 +255,11 @@ struct optee_smc_get_shm_config_result {
 #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF		BIT(5)
 /* Secure world supports pre-allocating RPC arg struct */
 #define OPTEE_SMC_SEC_CAP_RPC_ARG		BIT(6)
+/* Secure world supports interrupt events notification to normal world */
+#define OPTEE_SMC_SEC_CAP_IT_NOTIF		BIT(7)
+
+#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK	GENMASK(23, 8)
+#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT	8
 
 #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES	9
 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
@@ -426,6 +432,86 @@ struct optee_smc_disable_shm_cache_result {
 /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
 #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG	19
 
+/*
+ * Retrieve up to 5 pending interrupt event notified by OP-TEE world
+ * and whether threaded event are pending on OP-TEE async notif
+ * controller, all this since the last call of this function.
+ *
+ * OP-TEE keeps a record of all posted interrupt notification events.
+ * When the async notif interrupt is received by
+ * non-secure world, this function should be called until all pended
+ * interrupt events have been retrieved. When an interrupt event is
+ * retrieved it is cleared from the record in OP-TEE world.
+ *
+ * It is expected that this function is called from an interrupt handler
+ * in normal world.
+ *
+ * Call requests usage:
+ * a0	SMC Function ID, OPTEE_SMC_GET_NOTIF_IT
+ * a1-6	Not used
+ * a7	Hypervisor Client ID register
+ *
+ * Normal return register usage:
+ * a0	OPTEE_SMC_RETURN_OK
+ * a1	Bit[7:0]: Number of pending interrupt carried in a1..a5
+ *	Bit[8]: OPTEE_SMC_NOTIF_IT_PENDING if other interrupt(s) are pending
+ *	Bit[9]: OPTEE_SMC_NOTIF_ASYNC_PENDING if an threaded event is pending
+ *	Bit[10]: OPTEE_SMC_NOTIF_DO_BOTTOM_HALF if retrieved bottom half notif
+ *	Bit[15:11]: Reserved for furture use, MBZ
+ *	Bit[31:16]: Pending interrupt line value if a1 & 0xFF >= 1
+ * a2	Bit[15:0]:  Pending interrupt line value if a1 & 0xFF >= 2
+ *	Bit[31:16]: Pending interrupt line value if a1 & 0xFF >= 3
+ * a3	Bit[15:0]:  Pending interrupt line value if a1 & 0xFF >= 4
+ *	Bit[31:16]: Pending interrupt line value if a1 & 0xFF == 5
+ * a4-7	Preserved
+ *
+ * Not supported return register usage:
+ * a0	OPTEE_SMC_RETURN_ENOTAVAIL
+ * a1-7	Preserved
+ */
+#define OPTEE_SMC_NOTIF_IT_COUNT_MASK		GENMASK(7, 0)
+#define OPTEE_SMC_NOTIF_IT_PENDING		BIT(8)
+#define OPTEE_SMC_NOTIF_VALUE_PENDING		BIT(9)
+#define OPTEE_SMC_NOTIF_DO_BOTTOM_HALF		BIT(10)
+
+#define OPTEE_SMC_FUNCID_GET_NOTIF_IT		20
+#define OPTEE_SMC_GET_NOTIF_IT \
+	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_NOTIF_IT)
+
+/*
+ * Configure an interrupt notification event
+ *
+ * It is expected that this function is called from an interrupt handler
+ * in normal world.
+ *
+ * Call requests usage:
+ * a0	SMC Function ID, OPTEE_SMC_CONFIGURE_NOTIF_IT
+ * a1	Interrupt identifier value
+ * a2	Bit[0]: 1 to configure interrupt mask with a3[bit 0], else 0
+ *	Bit[31:1] Reserved for future use, MBZ
+ * a3	Bit[0]: 1 to mask the interrupt, 0 to unmask (applies if a2[bit 0]=1)
+ *	Bit[31:1] Reserved for future use, MBZ
+ * a4-6	Reserved for furture use, MBZ
+ * a7	Hypervisor Client ID register
+ *
+ * Normal return register usage:
+ * a0	OPTEE_SMC_RETURN_OK
+ * a1-7	Preserved
+ *
+ * Not supported return register usage:
+ * a0	OPTEE_SMC_RETURN_ENOTAVAIL
+ * a1-7	Preserved
+ *
+ * Invalid command with provided arguments return usage:
+ * a0	OPTEE_SMC_RETURN_EBADCMD
+ * a1-7	Preserved
+ */
+#define OPTEE_SMC_NOTIF_CONFIG_MASK		BIT(0)
+
+#define OPTEE_SMC_FUNCID_CONFIGURE_NOTIF_IT	21
+#define OPTEE_SMC_CONFIGURE_NOTIF_IT \
+	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_CONFIGURE_NOTIF_IT)
+
 /*
  * Resume from RPC (for example after processing a foreign interrupt)
  *
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index a1c1fa1a9c28..9f4fdd28f04a 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -977,6 +977,71 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
  * 5. Asynchronous notification
  */
 
+static void config_notif_it(optee_invoke_fn *invoke_fn, u32 it_value,
+			    u32 op_mask, u32 val_mask)
+{
+	struct arm_smccc_res res = { };
+
+	invoke_fn(OPTEE_SMC_CONFIGURE_NOTIF_IT, it_value, op_mask, val_mask,
+		  0, 0, 0, 0, &res);
+}
+
+static void optee_it_irq_mask(struct irq_data *d)
+{
+	struct optee *optee = d->domain->host_data;
+
+	config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK,
+			OPTEE_SMC_NOTIF_CONFIG_MASK);
+}
+
+static void optee_it_irq_unmask(struct irq_data *d)
+{
+	struct optee *optee = d->domain->host_data;
+
+	config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK, 0);
+}
+
+static struct irq_chip optee_it_irq_chip = {
+	.name = "optee-it",
+	.irq_mask = optee_it_irq_mask,
+	.irq_unmask = optee_it_irq_unmask,
+};
+
+static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
+			  unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec *fwspec = data;
+	irq_hw_number_t hwirq;
+
+	hwirq = fwspec->param[0];
+
+	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &optee_it_irq_chip, d->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops optee_it_irq_domain_ops = {
+	.alloc = optee_it_alloc,
+	.free = irq_domain_free_irqs_common,
+};
+
+static int optee_irq_domain_init(struct platform_device *pdev,
+				 struct optee *optee, u_int max_it)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	optee->smc.domain = irq_domain_add_linear(np, max_it,
+						  &optee_it_irq_domain_ops,
+						  optee);
+	if (!optee->smc.domain) {
+		dev_err(dev, "Unable to add irq domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
 static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
 				 bool *value_pending)
 {
@@ -991,6 +1056,60 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
 	return res.a1;
 }
 
+static void forward_irq(struct optee *optee, unsigned int id)
+{
+	if (generic_handle_domain_irq(optee->smc.domain, id)) {
+		pr_err("No consumer for optee irq %u, it will be masked\n", id);
+		config_notif_it(optee->smc.invoke_fn, id,
+				OPTEE_SMC_NOTIF_CONFIG_MASK,
+				OPTEE_SMC_NOTIF_CONFIG_MASK);
+	}
+}
+
+static void retrieve_pending_irqs(struct optee *optee, bool *async_value_pending,
+				  bool *do_bottom_half)
+
+{
+	struct arm_smccc_res res;
+	bool it_pending;
+	ssize_t cnt;
+	const unsigned int lsb_mask = GENMASK(15, 0);
+	const unsigned int msb_shift = 16;
+
+	*async_value_pending = false;
+	*do_bottom_half = false;
+
+	do {
+		optee->smc.invoke_fn(OPTEE_SMC_GET_NOTIF_IT, 0, 0, 0, 0, 0, 0, 0, &res);
+
+		if (res.a0)
+			return;
+
+		if (res.a1 & OPTEE_SMC_NOTIF_DO_BOTTOM_HALF)
+			*do_bottom_half = true;
+
+		it_pending = res.a1 & OPTEE_SMC_NOTIF_IT_PENDING;
+		cnt = res.a1 & OPTEE_SMC_NOTIF_IT_COUNT_MASK;
+		if (cnt > 5 || (!cnt && it_pending)) {
+			WARN_ONCE(0, "Unexpected interrupt notif count %zi\n", cnt);
+			break;
+		}
+
+		if (--cnt >= 0)
+			forward_irq(optee, res.a1 >> msb_shift);
+		if (--cnt >= 0)
+			forward_irq(optee, res.a2 & lsb_mask);
+		if (--cnt >= 0)
+			forward_irq(optee, res.a2 >> msb_shift);
+		if (--cnt >= 0)
+			forward_irq(optee, res.a3 & lsb_mask);
+		if (--cnt >= 0)
+			forward_irq(optee, res.a3 >> msb_shift);
+	} while (it_pending);
+
+	*async_value_pending = res.a1 & OPTEE_SMC_NOTIF_VALUE_PENDING;
+}
+
 static irqreturn_t notif_irq_handler(int irq, void *dev_id)
 {
 	struct optee *optee = dev_id;
@@ -999,9 +1118,14 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
 	bool value_pending;
 	u32 value;
 
-	do {
-		value = get_async_notif_value(optee->smc.invoke_fn,
-					      &value_valid, &value_pending);
+	if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
+		retrieve_pending_irqs(optee, &value_pending, &do_bottom_half);
+	else
+		value_pending = true;
+
+	while (value_pending) {
+		value = get_async_notif_value(optee->smc.invoke_fn, &value_valid,
+					      &value_pending);
 		if (!value_valid)
 			break;
 
@@ -1009,10 +1133,11 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
 			do_bottom_half = true;
 		else
 			optee_notif_send(optee, value);
-	} while (value_pending);
+	};
 
 	if (do_bottom_half)
 		return IRQ_WAKE_THREAD;
+
 	return IRQ_HANDLED;
 }
 
@@ -1048,6 +1173,9 @@ static void optee_smc_notif_uninit_irq(struct optee *optee)
 			free_irq(optee->smc.notif_irq, optee);
 			irq_dispose_mapping(optee->smc.notif_irq);
 		}
+
+		if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
+			irq_domain_remove(optee->smc.domain);
 	}
 }
 
@@ -1187,13 +1315,14 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
 
 static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
 					    u32 *sec_caps, u32 *max_notif_value,
+					    u32 *max_notif_it,
 					    unsigned int *rpc_param_count)
 {
 	union {
 		struct arm_smccc_res smccc;
 		struct optee_smc_exchange_capabilities_result result;
 	} res;
-	u32 a1 = 0;
+	u32 a1 = OPTEE_SMC_SEC_CAP_IT_NOTIF;
 
 	/*
 	 * TODO This isn't enough to tell if it's UP system (from kernel
@@ -1219,6 +1348,12 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
 	else
 		*rpc_param_count = 0;
 
+	if (*sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
+		*max_notif_it = (res.result.data & OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK) >>
+				OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT;
+	else
+		*max_notif_it = 0;
+
 	return true;
 }
 
@@ -1364,6 +1499,7 @@ static int optee_probe(struct platform_device *pdev)
 	struct tee_device *teedev;
 	struct tee_context *ctx;
 	u32 max_notif_value;
+	u32 max_notif_it;
 	u32 arg_cache_flags;
 	u32 sec_caps;
 	int rc;
@@ -1385,7 +1521,7 @@ static int optee_probe(struct platform_device *pdev)
 	}
 
 	if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
-					     &max_notif_value,
+					     &max_notif_value, &max_notif_it,
 					     &rpc_param_count)) {
 		pr_warn("capabilities mismatch\n");
 		return -EINVAL;
@@ -1506,6 +1642,16 @@ static int optee_probe(struct platform_device *pdev)
 			irq_dispose_mapping(irq);
 			goto err_notif_uninit;
 		}
+
+		if (sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF) {
+			rc = optee_irq_domain_init(pdev, optee, max_notif_it);
+			if (rc) {
+				free_irq(optee->smc.notif_irq, optee);
+				irq_dispose_mapping(irq);
+				goto err_notif_uninit;
+			}
+		}
+
 		enable_async_notif(optee->smc.invoke_fn);
 		pr_info("Asynchronous notifications enabled\n");
 	}
-- 
2.25.1

Re: [PATCH v2 2/3] optee: multiplex tee interrupt events on optee async notif irq

[Sumit Garg]

On Tue, 24 Jan 2023 at 16:26, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Implements an irqchip in optee driver to relay interrupts notified
> from OP-TEE world to the Linux OS. Optee registers up to 1 interrupt
> controller and identifies each line with a line number from 0 to
> UINT16_MAX.
>
> Existing optee async notif uses an irq for OP-TEE to signal Linux of a
> pending event. The implementation binds each event (an async notif value)
> to the awaking of a waiting thread or the processing of some TEE
> background (bottom half) tasks to be scheduled. The interrupt
> notification service added by this change allows TEE to relay interrupt
> signals to Linux on secure interrupt occurrences where the end consumer
> of the signal lives in normal world.

Why do we need OP-TEE as a mediator when the end consumer of an
interrupt is Linux? Is it some kind of hardware limitation that we
can't granularly assign interrupts among Linux and OP-TEE?

-Sumit

>
> When optee driver initializes, it negotiates with the TEE whether
> interrupt notification is supported or not. The feature is enabled
> if both Linux kernel and OP-TEE support it.
>
> OP-TEE defines 2 SMC function IDs for non-secure world to control
> interrupt events.
>
> SMC function OPTEE_SMC_GET_NOTIF_IT allows non-secure world to
> retrieve pending interrupts by grapes up to 5 lines. The function also
> reports whether there are pending async values targeting suspended
> threaded sequences execution and whether TEE has background threaded
> work to do (async notif value 0 was retrieved).
>
> SMC function OPTEE_SMC_CONFIG_NOTIF_IT configures the insterrupt line
> for masking and enabling state and wakeup source.
>
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Sumit Garg <sumit.garg@linaro.org>
>
> Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> Co-developed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v1:
> - Removed dependency on optee per-cpu irq notification.
> - Change SMC function ID API to retrieves up to 5 optee irq events,
>   the optee bottom half event and returns if other async notifications
>   are pending, in a single invocation.
> - Implement only mask/unmask irqchip handlers with a 2nd SMC function
>   to mask/unmask a optee irq in OP-TEE world from an interrupt context.
> - Added Cc: tags.
> ---
>  drivers/tee/optee/optee_private.h |  10 ++
>  drivers/tee/optee/optee_smc.h     |  88 ++++++++++++++++-
>  drivers/tee/optee/smc_abi.c       | 158 ++++++++++++++++++++++++++++--
>  3 files changed, 249 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 04ae58892608..f467409e02e9 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -94,11 +94,21 @@ struct optee_supp {
>         struct completion reqs_c;
>  };
>
> +/*
> + * struct optee_smc - optee smc communication struct
> + * @invoke_fn          handler function to invoke secure monitor
> + * @memremaped_shm     virtual address of memory in shared memory pool
> + * @sec_caps:          secure world capabilities defined by
> + *                     OPTEE_SMC_SEC_CAP_* in optee_smc.h
> + * @notif_irq          interrupt used as async notification by OP-TEE or 0
> + * @domain             interrupt domain registered by OP-TEE driver
> + */
>  struct optee_smc {
>         optee_invoke_fn *invoke_fn;
>         void *memremaped_shm;
>         u32 sec_caps;
>         unsigned int notif_irq;
> +       struct irq_domain *domain;
>  };
>
>  /**
> diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> index 73b5e7760d10..df32ad18eddb 100644
> --- a/drivers/tee/optee/optee_smc.h
> +++ b/drivers/tee/optee/optee_smc.h
> @@ -226,7 +226,8 @@ struct optee_smc_get_shm_config_result {
>   * a3  Bit[7:0]: Number of parameters needed for RPC to be supplied
>   *               as the second MSG arg struct for
>   *               OPTEE_SMC_CALL_WITH_ARG
> - *     Bit[31:8]: Reserved (MBZ)
> + *     Bit[23:8]: The maximum interrupt event notification number
> + *     Bit[31:24]: Reserved (MBZ)
>   * a4-7        Preserved
>   *
>   * Error return register usage:
> @@ -254,6 +255,11 @@ struct optee_smc_get_shm_config_result {
>  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
>  /* Secure world supports pre-allocating RPC arg struct */
>  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> +/* Secure world supports interrupt events notification to normal world */
> +#define OPTEE_SMC_SEC_CAP_IT_NOTIF             BIT(7)
> +
> +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK    GENMASK(23, 8)
> +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT   8
>
>  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
>  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> @@ -426,6 +432,86 @@ struct optee_smc_disable_shm_cache_result {
>  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
>  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
>
> +/*
> + * Retrieve up to 5 pending interrupt event notified by OP-TEE world
> + * and whether threaded event are pending on OP-TEE async notif
> + * controller, all this since the last call of this function.
> + *
> + * OP-TEE keeps a record of all posted interrupt notification events.
> + * When the async notif interrupt is received by
> + * non-secure world, this function should be called until all pended
> + * interrupt events have been retrieved. When an interrupt event is
> + * retrieved it is cleared from the record in OP-TEE world.
> + *
> + * It is expected that this function is called from an interrupt handler
> + * in normal world.
> + *
> + * Call requests usage:
> + * a0  SMC Function ID, OPTEE_SMC_GET_NOTIF_IT
> + * a1-6        Not used
> + * a7  Hypervisor Client ID register
> + *
> + * Normal return register usage:
> + * a0  OPTEE_SMC_RETURN_OK
> + * a1  Bit[7:0]: Number of pending interrupt carried in a1..a5
> + *     Bit[8]: OPTEE_SMC_NOTIF_IT_PENDING if other interrupt(s) are pending
> + *     Bit[9]: OPTEE_SMC_NOTIF_ASYNC_PENDING if an threaded event is pending
> + *     Bit[10]: OPTEE_SMC_NOTIF_DO_BOTTOM_HALF if retrieved bottom half notif
> + *     Bit[15:11]: Reserved for furture use, MBZ
> + *     Bit[31:16]: Pending interrupt line value if a1 & 0xFF >= 1
> + * a2  Bit[15:0]:  Pending interrupt line value if a1 & 0xFF >= 2
> + *     Bit[31:16]: Pending interrupt line value if a1 & 0xFF >= 3
> + * a3  Bit[15:0]:  Pending interrupt line value if a1 & 0xFF >= 4
> + *     Bit[31:16]: Pending interrupt line value if a1 & 0xFF == 5
> + * a4-7        Preserved
> + *
> + * Not supported return register usage:
> + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> + * a1-7        Preserved
> + */
> +#define OPTEE_SMC_NOTIF_IT_COUNT_MASK          GENMASK(7, 0)
> +#define OPTEE_SMC_NOTIF_IT_PENDING             BIT(8)
> +#define OPTEE_SMC_NOTIF_VALUE_PENDING          BIT(9)
> +#define OPTEE_SMC_NOTIF_DO_BOTTOM_HALF         BIT(10)
> +
> +#define OPTEE_SMC_FUNCID_GET_NOTIF_IT          20
> +#define OPTEE_SMC_GET_NOTIF_IT \
> +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_NOTIF_IT)
> +
> +/*
> + * Configure an interrupt notification event
> + *
> + * It is expected that this function is called from an interrupt handler
> + * in normal world.
> + *
> + * Call requests usage:
> + * a0  SMC Function ID, OPTEE_SMC_CONFIGURE_NOTIF_IT
> + * a1  Interrupt identifier value
> + * a2  Bit[0]: 1 to configure interrupt mask with a3[bit 0], else 0
> + *     Bit[31:1] Reserved for future use, MBZ
> + * a3  Bit[0]: 1 to mask the interrupt, 0 to unmask (applies if a2[bit 0]=1)
> + *     Bit[31:1] Reserved for future use, MBZ
> + * a4-6        Reserved for furture use, MBZ
> + * a7  Hypervisor Client ID register
> + *
> + * Normal return register usage:
> + * a0  OPTEE_SMC_RETURN_OK
> + * a1-7        Preserved
> + *
> + * Not supported return register usage:
> + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> + * a1-7        Preserved
> + *
> + * Invalid command with provided arguments return usage:
> + * a0  OPTEE_SMC_RETURN_EBADCMD
> + * a1-7        Preserved
> + */
> +#define OPTEE_SMC_NOTIF_CONFIG_MASK            BIT(0)
> +
> +#define OPTEE_SMC_FUNCID_CONFIGURE_NOTIF_IT    21
> +#define OPTEE_SMC_CONFIGURE_NOTIF_IT \
> +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_CONFIGURE_NOTIF_IT)
> +
>  /*
>   * Resume from RPC (for example after processing a foreign interrupt)
>   *
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..9f4fdd28f04a 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -977,6 +977,71 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
>   * 5. Asynchronous notification
>   */
>
> +static void config_notif_it(optee_invoke_fn *invoke_fn, u32 it_value,
> +                           u32 op_mask, u32 val_mask)
> +{
> +       struct arm_smccc_res res = { };
> +
> +       invoke_fn(OPTEE_SMC_CONFIGURE_NOTIF_IT, it_value, op_mask, val_mask,
> +                 0, 0, 0, 0, &res);
> +}
> +
> +static void optee_it_irq_mask(struct irq_data *d)
> +{
> +       struct optee *optee = d->domain->host_data;
> +
> +       config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK,
> +                       OPTEE_SMC_NOTIF_CONFIG_MASK);
> +}
> +
> +static void optee_it_irq_unmask(struct irq_data *d)
> +{
> +       struct optee *optee = d->domain->host_data;
> +
> +       config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK, 0);
> +}
> +
> +static struct irq_chip optee_it_irq_chip = {
> +       .name = "optee-it",
> +       .irq_mask = optee_it_irq_mask,
> +       .irq_unmask = optee_it_irq_unmask,
> +};
> +
> +static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
> +                         unsigned int nr_irqs, void *data)
> +{
> +       struct irq_fwspec *fwspec = data;
> +       irq_hw_number_t hwirq;
> +
> +       hwirq = fwspec->param[0];
> +
> +       irq_domain_set_hwirq_and_chip(d, virq, hwirq, &optee_it_irq_chip, d->host_data);
> +
> +       return 0;
> +}
> +
> +static const struct irq_domain_ops optee_it_irq_domain_ops = {
> +       .alloc = optee_it_alloc,
> +       .free = irq_domain_free_irqs_common,
> +};
> +
> +static int optee_irq_domain_init(struct platform_device *pdev,
> +                                struct optee *optee, u_int max_it)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +
> +       optee->smc.domain = irq_domain_add_linear(np, max_it,
> +                                                 &optee_it_irq_domain_ops,
> +                                                 optee);
> +       if (!optee->smc.domain) {
> +               dev_err(dev, "Unable to add irq domain\n");
> +               return -ENOMEM;
> +       }
> +
> +       return 0;
> +}
> +
>  static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
>                                  bool *value_pending)
>  {
> @@ -991,6 +1056,60 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
>         return res.a1;
>  }
>
> +static void forward_irq(struct optee *optee, unsigned int id)
> +{
> +       if (generic_handle_domain_irq(optee->smc.domain, id)) {
> +               pr_err("No consumer for optee irq %u, it will be masked\n", id);
> +               config_notif_it(optee->smc.invoke_fn, id,
> +                               OPTEE_SMC_NOTIF_CONFIG_MASK,
> +                               OPTEE_SMC_NOTIF_CONFIG_MASK);
> +       }
> +}
> +
> +static void retrieve_pending_irqs(struct optee *optee, bool *async_value_pending,
> +                                 bool *do_bottom_half)
> +
> +{
> +       struct arm_smccc_res res;
> +       bool it_pending;
> +       ssize_t cnt;
> +       const unsigned int lsb_mask = GENMASK(15, 0);
> +       const unsigned int msb_shift = 16;
> +
> +       *async_value_pending = false;
> +       *do_bottom_half = false;
> +
> +       do {
> +               optee->smc.invoke_fn(OPTEE_SMC_GET_NOTIF_IT, 0, 0, 0, 0, 0, 0, 0, &res);
> +
> +               if (res.a0)
> +                       return;
> +
> +               if (res.a1 & OPTEE_SMC_NOTIF_DO_BOTTOM_HALF)
> +                       *do_bottom_half = true;
> +
> +               it_pending = res.a1 & OPTEE_SMC_NOTIF_IT_PENDING;
> +               cnt = res.a1 & OPTEE_SMC_NOTIF_IT_COUNT_MASK;
> +               if (cnt > 5 || (!cnt && it_pending)) {
> +                       WARN_ONCE(0, "Unexpected interrupt notif count %zi\n", cnt);
> +                       break;
> +               }
> +
> +               if (--cnt >= 0)
> +                       forward_irq(optee, res.a1 >> msb_shift);
> +               if (--cnt >= 0)
> +                       forward_irq(optee, res.a2 & lsb_mask);
> +               if (--cnt >= 0)
> +                       forward_irq(optee, res.a2 >> msb_shift);
> +               if (--cnt >= 0)
> +                       forward_irq(optee, res.a3 & lsb_mask);
> +               if (--cnt >= 0)
> +                       forward_irq(optee, res.a3 >> msb_shift);
> +       } while (it_pending);
> +
> +       *async_value_pending = res.a1 & OPTEE_SMC_NOTIF_VALUE_PENDING;
> +}
> +
>  static irqreturn_t notif_irq_handler(int irq, void *dev_id)
>  {
>         struct optee *optee = dev_id;
> @@ -999,9 +1118,14 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
>         bool value_pending;
>         u32 value;
>
> -       do {
> -               value = get_async_notif_value(optee->smc.invoke_fn,
> -                                             &value_valid, &value_pending);
> +       if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
> +               retrieve_pending_irqs(optee, &value_pending, &do_bottom_half);
> +       else
> +               value_pending = true;
> +
> +       while (value_pending) {
> +               value = get_async_notif_value(optee->smc.invoke_fn, &value_valid,
> +                                             &value_pending);
>                 if (!value_valid)
>                         break;
>
> @@ -1009,10 +1133,11 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
>                         do_bottom_half = true;
>                 else
>                         optee_notif_send(optee, value);
> -       } while (value_pending);
> +       };
>
>         if (do_bottom_half)
>                 return IRQ_WAKE_THREAD;
> +
>         return IRQ_HANDLED;
>  }
>
> @@ -1048,6 +1173,9 @@ static void optee_smc_notif_uninit_irq(struct optee *optee)
>                         free_irq(optee->smc.notif_irq, optee);
>                         irq_dispose_mapping(optee->smc.notif_irq);
>                 }
> +
> +               if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
> +                       irq_domain_remove(optee->smc.domain);
>         }
>  }
>
> @@ -1187,13 +1315,14 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
>
>  static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
>                                             u32 *sec_caps, u32 *max_notif_value,
> +                                           u32 *max_notif_it,
>                                             unsigned int *rpc_param_count)
>  {
>         union {
>                 struct arm_smccc_res smccc;
>                 struct optee_smc_exchange_capabilities_result result;
>         } res;
> -       u32 a1 = 0;
> +       u32 a1 = OPTEE_SMC_SEC_CAP_IT_NOTIF;
>
>         /*
>          * TODO This isn't enough to tell if it's UP system (from kernel
> @@ -1219,6 +1348,12 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
>         else
>                 *rpc_param_count = 0;
>
> +       if (*sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
> +               *max_notif_it = (res.result.data & OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK) >>
> +                               OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT;
> +       else
> +               *max_notif_it = 0;
> +
>         return true;
>  }
>
> @@ -1364,6 +1499,7 @@ static int optee_probe(struct platform_device *pdev)
>         struct tee_device *teedev;
>         struct tee_context *ctx;
>         u32 max_notif_value;
> +       u32 max_notif_it;
>         u32 arg_cache_flags;
>         u32 sec_caps;
>         int rc;
> @@ -1385,7 +1521,7 @@ static int optee_probe(struct platform_device *pdev)
>         }
>
>         if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> -                                            &max_notif_value,
> +                                            &max_notif_value, &max_notif_it,
>                                              &rpc_param_count)) {
>                 pr_warn("capabilities mismatch\n");
>                 return -EINVAL;
> @@ -1506,6 +1642,16 @@ static int optee_probe(struct platform_device *pdev)
>                         irq_dispose_mapping(irq);
>                         goto err_notif_uninit;
>                 }
> +
> +               if (sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF) {
> +                       rc = optee_irq_domain_init(pdev, optee, max_notif_it);
> +                       if (rc) {
> +                               free_irq(optee->smc.notif_irq, optee);
> +                               irq_dispose_mapping(irq);
> +                               goto err_notif_uninit;
> +                       }
> +               }
> +
>                 enable_async_notif(optee->smc.invoke_fn);
>                 pr_info("Asynchronous notifications enabled\n");
>         }
> --
> 2.25.1
>

Re: [PATCH v2 2/3] optee: multiplex tee interrupt events on optee async notif irq

[Etienne Carriere]

Hi Sumit,

On Thu, 2 Feb 2023 at 12:15, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Tue, 24 Jan 2023 at 16:26, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Implements an irqchip in optee driver to relay interrupts notified
> > from OP-TEE world to the Linux OS. Optee registers up to 1 interrupt
> > controller and identifies each line with a line number from 0 to
> > UINT16_MAX.
> >
> > Existing optee async notif uses an irq for OP-TEE to signal Linux of a
> > pending event. The implementation binds each event (an async notif value)
> > to the awaking of a waiting thread or the processing of some TEE
> > background (bottom half) tasks to be scheduled. The interrupt
> > notification service added by this change allows TEE to relay interrupt
> > signals to Linux on secure interrupt occurrences where the end consumer
> > of the signal lives in normal world.
>
> Why do we need OP-TEE as a mediator when the end consumer of an
> interrupt is Linux? Is it some kind of hardware limitation that we
> can't granularly assign interrupts among Linux and OP-TEE?

I face 2 cases.
A case where wakeup interrupts each have a GIC line (that can be
assigned to Linux) in the system but the wakeup configuration of each
interrupt must be done from secure world. So set_wake() operation must
go through OP-TEE. Exposing these interrupts as optee irq's allows
consumers to manage these interrupts using standard Linux kernel irq
APIs.

There is also the case of interrupt expanders i've mentioned  in the
commit message of PATCH v2 1/3. As for example, considering the
interrupts raised from a system PMIC device that is accessed through
an I2C bus or similar. When the PMIC bus is assigned to OP-TEE world,
these accesses are to be done from OP-TEE world. Exposing PMIC
interrupts as optee irq's for that platform allows to use standard
Linux irq's support so consumers simply get a regular irq resource,
with well defined interface to invoke secure world for the interrupt
management.

Br,
etienne

>
> -Sumit
>
> >
> > When optee driver initializes, it negotiates with the TEE whether
> > interrupt notification is supported or not. The feature is enabled
> > if both Linux kernel and OP-TEE support it.
> >
> > OP-TEE defines 2 SMC function IDs for non-secure world to control
> > interrupt events.
> >
> > SMC function OPTEE_SMC_GET_NOTIF_IT allows non-secure world to
> > retrieve pending interrupts by grapes up to 5 lines. The function also
> > reports whether there are pending async values targeting suspended
> > threaded sequences execution and whether TEE has background threaded
> > work to do (async notif value 0 was retrieved).
> >
> > SMC function OPTEE_SMC_CONFIG_NOTIF_IT configures the insterrupt line
> > for masking and enabling state and wakeup source.
> >
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Sumit Garg <sumit.garg@linaro.org>
> >
> > Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
> > Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> > Co-developed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since v1:
> > - Removed dependency on optee per-cpu irq notification.
> > - Change SMC function ID API to retrieves up to 5 optee irq events,
> >   the optee bottom half event and returns if other async notifications
> >   are pending, in a single invocation.
> > - Implement only mask/unmask irqchip handlers with a 2nd SMC function
> >   to mask/unmask a optee irq in OP-TEE world from an interrupt context.
> > - Added Cc: tags.
> > ---
> >  drivers/tee/optee/optee_private.h |  10 ++
> >  drivers/tee/optee/optee_smc.h     |  88 ++++++++++++++++-
> >  drivers/tee/optee/smc_abi.c       | 158 ++++++++++++++++++++++++++++--
> >  3 files changed, 249 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 04ae58892608..f467409e02e9 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -94,11 +94,21 @@ struct optee_supp {
> >         struct completion reqs_c;
> >  };
> >
> > +/*
> > + * struct optee_smc - optee smc communication struct
> > + * @invoke_fn          handler function to invoke secure monitor
> > + * @memremaped_shm     virtual address of memory in shared memory pool
> > + * @sec_caps:          secure world capabilities defined by
> > + *                     OPTEE_SMC_SEC_CAP_* in optee_smc.h
> > + * @notif_irq          interrupt used as async notification by OP-TEE or 0
> > + * @domain             interrupt domain registered by OP-TEE driver
> > + */
> >  struct optee_smc {
> >         optee_invoke_fn *invoke_fn;
> >         void *memremaped_shm;
> >         u32 sec_caps;
> >         unsigned int notif_irq;
> > +       struct irq_domain *domain;
> >  };
> >
> >  /**
> > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > index 73b5e7760d10..df32ad18eddb 100644
> > --- a/drivers/tee/optee/optee_smc.h
> > +++ b/drivers/tee/optee/optee_smc.h
> > @@ -226,7 +226,8 @@ struct optee_smc_get_shm_config_result {
> >   * a3  Bit[7:0]: Number of parameters needed for RPC to be supplied
> >   *               as the second MSG arg struct for
> >   *               OPTEE_SMC_CALL_WITH_ARG
> > - *     Bit[31:8]: Reserved (MBZ)
> > + *     Bit[23:8]: The maximum interrupt event notification number
> > + *     Bit[31:24]: Reserved (MBZ)
> >   * a4-7        Preserved
> >   *
> >   * Error return register usage:
> > @@ -254,6 +255,11 @@ struct optee_smc_get_shm_config_result {
> >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> >  /* Secure world supports pre-allocating RPC arg struct */
> >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > +/* Secure world supports interrupt events notification to normal world */
> > +#define OPTEE_SMC_SEC_CAP_IT_NOTIF             BIT(7)
> > +
> > +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK    GENMASK(23, 8)
> > +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT   8
> >
> >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > @@ -426,6 +432,86 @@ struct optee_smc_disable_shm_cache_result {
> >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> >
> > +/*
> > + * Retrieve up to 5 pending interrupt event notified by OP-TEE world
> > + * and whether threaded event are pending on OP-TEE async notif
> > + * controller, all this since the last call of this function.
> > + *
> > + * OP-TEE keeps a record of all posted interrupt notification events.
> > + * When the async notif interrupt is received by
> > + * non-secure world, this function should be called until all pended
> > + * interrupt events have been retrieved. When an interrupt event is
> > + * retrieved it is cleared from the record in OP-TEE world.
> > + *
> > + * It is expected that this function is called from an interrupt handler
> > + * in normal world.
> > + *
> > + * Call requests usage:
> > + * a0  SMC Function ID, OPTEE_SMC_GET_NOTIF_IT
> > + * a1-6        Not used
> > + * a7  Hypervisor Client ID register
> > + *
> > + * Normal return register usage:
> > + * a0  OPTEE_SMC_RETURN_OK
> > + * a1  Bit[7:0]: Number of pending interrupt carried in a1..a5
> > + *     Bit[8]: OPTEE_SMC_NOTIF_IT_PENDING if other interrupt(s) are pending
> > + *     Bit[9]: OPTEE_SMC_NOTIF_ASYNC_PENDING if an threaded event is pending
> > + *     Bit[10]: OPTEE_SMC_NOTIF_DO_BOTTOM_HALF if retrieved bottom half notif
> > + *     Bit[15:11]: Reserved for furture use, MBZ
> > + *     Bit[31:16]: Pending interrupt line value if a1 & 0xFF >= 1
> > + * a2  Bit[15:0]:  Pending interrupt line value if a1 & 0xFF >= 2
> > + *     Bit[31:16]: Pending interrupt line value if a1 & 0xFF >= 3
> > + * a3  Bit[15:0]:  Pending interrupt line value if a1 & 0xFF >= 4
> > + *     Bit[31:16]: Pending interrupt line value if a1 & 0xFF == 5
> > + * a4-7        Preserved
> > + *
> > + * Not supported return register usage:
> > + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> > + * a1-7        Preserved
> > + */
> > +#define OPTEE_SMC_NOTIF_IT_COUNT_MASK          GENMASK(7, 0)
> > +#define OPTEE_SMC_NOTIF_IT_PENDING             BIT(8)
> > +#define OPTEE_SMC_NOTIF_VALUE_PENDING          BIT(9)
> > +#define OPTEE_SMC_NOTIF_DO_BOTTOM_HALF         BIT(10)
> > +
> > +#define OPTEE_SMC_FUNCID_GET_NOTIF_IT          20
> > +#define OPTEE_SMC_GET_NOTIF_IT \
> > +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_NOTIF_IT)
> > +
> > +/*
> > + * Configure an interrupt notification event
> > + *
> > + * It is expected that this function is called from an interrupt handler
> > + * in normal world.
> > + *
> > + * Call requests usage:
> > + * a0  SMC Function ID, OPTEE_SMC_CONFIGURE_NOTIF_IT
> > + * a1  Interrupt identifier value
> > + * a2  Bit[0]: 1 to configure interrupt mask with a3[bit 0], else 0
> > + *     Bit[31:1] Reserved for future use, MBZ
> > + * a3  Bit[0]: 1 to mask the interrupt, 0 to unmask (applies if a2[bit 0]=1)
> > + *     Bit[31:1] Reserved for future use, MBZ
> > + * a4-6        Reserved for furture use, MBZ
> > + * a7  Hypervisor Client ID register
> > + *
> > + * Normal return register usage:
> > + * a0  OPTEE_SMC_RETURN_OK
> > + * a1-7        Preserved
> > + *
> > + * Not supported return register usage:
> > + * a0  OPTEE_SMC_RETURN_ENOTAVAIL
> > + * a1-7        Preserved
> > + *
> > + * Invalid command with provided arguments return usage:
> > + * a0  OPTEE_SMC_RETURN_EBADCMD
> > + * a1-7        Preserved
> > + */
> > +#define OPTEE_SMC_NOTIF_CONFIG_MASK            BIT(0)
> > +
> > +#define OPTEE_SMC_FUNCID_CONFIGURE_NOTIF_IT    21
> > +#define OPTEE_SMC_CONFIGURE_NOTIF_IT \
> > +       OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_CONFIGURE_NOTIF_IT)
> > +
> >  /*
> >   * Resume from RPC (for example after processing a foreign interrupt)
> >   *
> > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > index a1c1fa1a9c28..9f4fdd28f04a 100644
> > --- a/drivers/tee/optee/smc_abi.c
> > +++ b/drivers/tee/optee/smc_abi.c
> > @@ -977,6 +977,71 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
> >   * 5. Asynchronous notification
> >   */
> >
> > +static void config_notif_it(optee_invoke_fn *invoke_fn, u32 it_value,
> > +                           u32 op_mask, u32 val_mask)
> > +{
> > +       struct arm_smccc_res res = { };
> > +
> > +       invoke_fn(OPTEE_SMC_CONFIGURE_NOTIF_IT, it_value, op_mask, val_mask,
> > +                 0, 0, 0, 0, &res);
> > +}
> > +
> > +static void optee_it_irq_mask(struct irq_data *d)
> > +{
> > +       struct optee *optee = d->domain->host_data;
> > +
> > +       config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK,
> > +                       OPTEE_SMC_NOTIF_CONFIG_MASK);
> > +}
> > +
> > +static void optee_it_irq_unmask(struct irq_data *d)
> > +{
> > +       struct optee *optee = d->domain->host_data;
> > +
> > +       config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK, 0);
> > +}
> > +
> > +static struct irq_chip optee_it_irq_chip = {
> > +       .name = "optee-it",
> > +       .irq_mask = optee_it_irq_mask,
> > +       .irq_unmask = optee_it_irq_unmask,
> > +};
> > +
> > +static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
> > +                         unsigned int nr_irqs, void *data)
> > +{
> > +       struct irq_fwspec *fwspec = data;
> > +       irq_hw_number_t hwirq;
> > +
> > +       hwirq = fwspec->param[0];
> > +
> > +       irq_domain_set_hwirq_and_chip(d, virq, hwirq, &optee_it_irq_chip, d->host_data);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct irq_domain_ops optee_it_irq_domain_ops = {
> > +       .alloc = optee_it_alloc,
> > +       .free = irq_domain_free_irqs_common,
> > +};
> > +
> > +static int optee_irq_domain_init(struct platform_device *pdev,
> > +                                struct optee *optee, u_int max_it)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *np = dev->of_node;
> > +
> > +       optee->smc.domain = irq_domain_add_linear(np, max_it,
> > +                                                 &optee_it_irq_domain_ops,
> > +                                                 optee);
> > +       if (!optee->smc.domain) {
> > +               dev_err(dev, "Unable to add irq domain\n");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
> >                                  bool *value_pending)
> >  {
> > @@ -991,6 +1056,60 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
> >         return res.a1;
> >  }
> >
> > +static void forward_irq(struct optee *optee, unsigned int id)
> > +{
> > +       if (generic_handle_domain_irq(optee->smc.domain, id)) {
> > +               pr_err("No consumer for optee irq %u, it will be masked\n", id);
> > +               config_notif_it(optee->smc.invoke_fn, id,
> > +                               OPTEE_SMC_NOTIF_CONFIG_MASK,
> > +                               OPTEE_SMC_NOTIF_CONFIG_MASK);
> > +       }
> > +}
> > +
> > +static void retrieve_pending_irqs(struct optee *optee, bool *async_value_pending,
> > +                                 bool *do_bottom_half)
> > +
> > +{
> > +       struct arm_smccc_res res;
> > +       bool it_pending;
> > +       ssize_t cnt;
> > +       const unsigned int lsb_mask = GENMASK(15, 0);
> > +       const unsigned int msb_shift = 16;
> > +
> > +       *async_value_pending = false;
> > +       *do_bottom_half = false;
> > +
> > +       do {
> > +               optee->smc.invoke_fn(OPTEE_SMC_GET_NOTIF_IT, 0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > +               if (res.a0)
> > +                       return;
> > +
> > +               if (res.a1 & OPTEE_SMC_NOTIF_DO_BOTTOM_HALF)
> > +                       *do_bottom_half = true;
> > +
> > +               it_pending = res.a1 & OPTEE_SMC_NOTIF_IT_PENDING;
> > +               cnt = res.a1 & OPTEE_SMC_NOTIF_IT_COUNT_MASK;
> > +               if (cnt > 5 || (!cnt && it_pending)) {
> > +                       WARN_ONCE(0, "Unexpected interrupt notif count %zi\n", cnt);
> > +                       break;
> > +               }
> > +
> > +               if (--cnt >= 0)
> > +                       forward_irq(optee, res.a1 >> msb_shift);
> > +               if (--cnt >= 0)
> > +                       forward_irq(optee, res.a2 & lsb_mask);
> > +               if (--cnt >= 0)
> > +                       forward_irq(optee, res.a2 >> msb_shift);
> > +               if (--cnt >= 0)
> > +                       forward_irq(optee, res.a3 & lsb_mask);
> > +               if (--cnt >= 0)
> > +                       forward_irq(optee, res.a3 >> msb_shift);
> > +       } while (it_pending);
> > +
> > +       *async_value_pending = res.a1 & OPTEE_SMC_NOTIF_VALUE_PENDING;
> > +}
> > +
> >  static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> >  {
> >         struct optee *optee = dev_id;
> > @@ -999,9 +1118,14 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> >         bool value_pending;
> >         u32 value;
> >
> > -       do {
> > -               value = get_async_notif_value(optee->smc.invoke_fn,
> > -                                             &value_valid, &value_pending);
> > +       if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
> > +               retrieve_pending_irqs(optee, &value_pending, &do_bottom_half);
> > +       else
> > +               value_pending = true;
> > +
> > +       while (value_pending) {
> > +               value = get_async_notif_value(optee->smc.invoke_fn, &value_valid,
> > +                                             &value_pending);
> >                 if (!value_valid)
> >                         break;
> >
> > @@ -1009,10 +1133,11 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> >                         do_bottom_half = true;
> >                 else
> >                         optee_notif_send(optee, value);
> > -       } while (value_pending);
> > +       };
> >
> >         if (do_bottom_half)
> >                 return IRQ_WAKE_THREAD;
> > +
> >         return IRQ_HANDLED;
> >  }
> >
> > @@ -1048,6 +1173,9 @@ static void optee_smc_notif_uninit_irq(struct optee *optee)
> >                         free_irq(optee->smc.notif_irq, optee);
> >                         irq_dispose_mapping(optee->smc.notif_irq);
> >                 }
> > +
> > +               if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
> > +                       irq_domain_remove(optee->smc.domain);
> >         }
> >  }
> >
> > @@ -1187,13 +1315,14 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
> >
> >  static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> >                                             u32 *sec_caps, u32 *max_notif_value,
> > +                                           u32 *max_notif_it,
> >                                             unsigned int *rpc_param_count)
> >  {
> >         union {
> >                 struct arm_smccc_res smccc;
> >                 struct optee_smc_exchange_capabilities_result result;
> >         } res;
> > -       u32 a1 = 0;
> > +       u32 a1 = OPTEE_SMC_SEC_CAP_IT_NOTIF;
> >
> >         /*
> >          * TODO This isn't enough to tell if it's UP system (from kernel
> > @@ -1219,6 +1348,12 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> >         else
> >                 *rpc_param_count = 0;
> >
> > +       if (*sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
> > +               *max_notif_it = (res.result.data & OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK) >>
> > +                               OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT;
> > +       else
> > +               *max_notif_it = 0;
> > +
> >         return true;
> >  }
> >
> > @@ -1364,6 +1499,7 @@ static int optee_probe(struct platform_device *pdev)
> >         struct tee_device *teedev;
> >         struct tee_context *ctx;
> >         u32 max_notif_value;
> > +       u32 max_notif_it;
> >         u32 arg_cache_flags;
> >         u32 sec_caps;
> >         int rc;
> > @@ -1385,7 +1521,7 @@ static int optee_probe(struct platform_device *pdev)
> >         }
> >
> >         if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> > -                                            &max_notif_value,
> > +                                            &max_notif_value, &max_notif_it,
> >                                              &rpc_param_count)) {
> >                 pr_warn("capabilities mismatch\n");
> >                 return -EINVAL;
> > @@ -1506,6 +1642,16 @@ static int optee_probe(struct platform_device *pdev)
> >                         irq_dispose_mapping(irq);
> >                         goto err_notif_uninit;
> >                 }
> > +
> > +               if (sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF) {
> > +                       rc = optee_irq_domain_init(pdev, optee, max_notif_it);
> > +                       if (rc) {
> > +                               free_irq(optee->smc.notif_irq, optee);
> > +                               irq_dispose_mapping(irq);
> > +                               goto err_notif_uninit;
> > +                       }
> > +               }
> > +
> >                 enable_async_notif(optee->smc.invoke_fn);
> >                 pr_info("Asynchronous notifications enabled\n");
> >         }
> > --
> > 2.25.1
> >

Re: [PATCH v2 2/3] optee: multiplex tee interrupt events on optee async notif irq

[Jens Wiklander]

Hi Etienne,

On Tue, Jan 24, 2023 at 11:56:42AM +0100, Etienne Carriere wrote:
> Implements an irqchip in optee driver to relay interrupts notified
> from OP-TEE world to the Linux OS. Optee registers up to 1 interrupt
> controller and identifies each line with a line number from 0 to
> UINT16_MAX.
> 
> Existing optee async notif uses an irq for OP-TEE to signal Linux of a
> pending event. The implementation binds each event (an async notif value)
> to the awaking of a waiting thread or the processing of some TEE
> background (bottom half) tasks to be scheduled. The interrupt
> notification service added by this change allows TEE to relay interrupt
> signals to Linux on secure interrupt occurrences where the end consumer
> of the signal lives in normal world.
> 
> When optee driver initializes, it negotiates with the TEE whether
> interrupt notification is supported or not. The feature is enabled
> if both Linux kernel and OP-TEE support it.
> 
> OP-TEE defines 2 SMC function IDs for non-secure world to control
> interrupt events.
> 
> SMC function OPTEE_SMC_GET_NOTIF_IT allows non-secure world to
> retrieve pending interrupts by grapes up to 5 lines. The function also
> reports whether there are pending async values targeting suspended
> threaded sequences execution and whether TEE has background threaded
> work to do (async notif value 0 was retrieved).
> 
> SMC function OPTEE_SMC_CONFIG_NOTIF_IT configures the insterrupt line
> for masking and enabling state and wakeup source.
> 
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> 
> Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> Co-developed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v1:
> - Removed dependency on optee per-cpu irq notification.
> - Change SMC function ID API to retrieves up to 5 optee irq events,
>   the optee bottom half event and returns if other async notifications
>   are pending, in a single invocation.
> - Implement only mask/unmask irqchip handlers with a 2nd SMC function
>   to mask/unmask a optee irq in OP-TEE world from an interrupt context.
> - Added Cc: tags.
> ---
>  drivers/tee/optee/optee_private.h |  10 ++
>  drivers/tee/optee/optee_smc.h     |  88 ++++++++++++++++-
>  drivers/tee/optee/smc_abi.c       | 158 ++++++++++++++++++++++++++++--
>  3 files changed, 249 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 04ae58892608..f467409e02e9 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -94,11 +94,21 @@ struct optee_supp {
>  	struct completion reqs_c;
>  };
>  
> +/*
> + * struct optee_smc - optee smc communication struct
> + * @invoke_fn		handler function to invoke secure monitor
> + * @memremaped_shm	virtual address of memory in shared memory pool
> + * @sec_caps:		secure world capabilities defined by
> + *			OPTEE_SMC_SEC_CAP_* in optee_smc.h
> + * @notif_irq		interrupt used as async notification by OP-TEE or 0
> + * @domain		interrupt domain registered by OP-TEE driver
> + */
>  struct optee_smc {
>  	optee_invoke_fn *invoke_fn;
>  	void *memremaped_shm;
>  	u32 sec_caps;
>  	unsigned int notif_irq;
> +	struct irq_domain *domain;
>  };
>  
>  /**
> diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> index 73b5e7760d10..df32ad18eddb 100644
> --- a/drivers/tee/optee/optee_smc.h
> +++ b/drivers/tee/optee/optee_smc.h
> @@ -226,7 +226,8 @@ struct optee_smc_get_shm_config_result {
>   * a3	Bit[7:0]: Number of parameters needed for RPC to be supplied
>   *		  as the second MSG arg struct for
>   *		  OPTEE_SMC_CALL_WITH_ARG
> - *	Bit[31:8]: Reserved (MBZ)
> + *	Bit[23:8]: The maximum interrupt event notification number
> + *	Bit[31:24]: Reserved (MBZ)
>   * a4-7	Preserved
>   *
>   * Error return register usage:
> @@ -254,6 +255,11 @@ struct optee_smc_get_shm_config_result {
>  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF		BIT(5)
>  /* Secure world supports pre-allocating RPC arg struct */
>  #define OPTEE_SMC_SEC_CAP_RPC_ARG		BIT(6)
> +/* Secure world supports interrupt events notification to normal world */
> +#define OPTEE_SMC_SEC_CAP_IT_NOTIF		BIT(7)
> +
> +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK	GENMASK(23, 8)
> +#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT	8
>  
>  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES	9
>  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> @@ -426,6 +432,86 @@ struct optee_smc_disable_shm_cache_result {
>  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
>  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG	19
>  
> +/*
> + * Retrieve up to 5 pending interrupt event notified by OP-TEE world
> + * and whether threaded event are pending on OP-TEE async notif
> + * controller, all this since the last call of this function.
> + *
> + * OP-TEE keeps a record of all posted interrupt notification events.
> + * When the async notif interrupt is received by
> + * non-secure world, this function should be called until all pended
> + * interrupt events have been retrieved. When an interrupt event is
> + * retrieved it is cleared from the record in OP-TEE world.
> + *
> + * It is expected that this function is called from an interrupt handler
> + * in normal world.
> + *
> + * Call requests usage:
> + * a0	SMC Function ID, OPTEE_SMC_GET_NOTIF_IT
> + * a1-6	Not used
> + * a7	Hypervisor Client ID register
> + *
> + * Normal return register usage:
> + * a0	OPTEE_SMC_RETURN_OK
> + * a1	Bit[7:0]: Number of pending interrupt carried in a1..a5
> + *	Bit[8]: OPTEE_SMC_NOTIF_IT_PENDING if other interrupt(s) are pending
> + *	Bit[9]: OPTEE_SMC_NOTIF_ASYNC_PENDING if an threaded event is pending
> + *	Bit[10]: OPTEE_SMC_NOTIF_DO_BOTTOM_HALF if retrieved bottom half notif
> + *	Bit[15:11]: Reserved for furture use, MBZ
> + *	Bit[31:16]: Pending interrupt line value if a1 & 0xFF >= 1
> + * a2	Bit[15:0]:  Pending interrupt line value if a1 & 0xFF >= 2
> + *	Bit[31:16]: Pending interrupt line value if a1 & 0xFF >= 3
> + * a3	Bit[15:0]:  Pending interrupt line value if a1 & 0xFF >= 4
> + *	Bit[31:16]: Pending interrupt line value if a1 & 0xFF == 5
> + * a4-7	Preserved
> + *
> + * Not supported return register usage:
> + * a0	OPTEE_SMC_RETURN_ENOTAVAIL
> + * a1-7	Preserved
> + */
> +#define OPTEE_SMC_NOTIF_IT_COUNT_MASK		GENMASK(7, 0)
> +#define OPTEE_SMC_NOTIF_IT_PENDING		BIT(8)
> +#define OPTEE_SMC_NOTIF_VALUE_PENDING		BIT(9)
> +#define OPTEE_SMC_NOTIF_DO_BOTTOM_HALF		BIT(10)
> +
> +#define OPTEE_SMC_FUNCID_GET_NOTIF_IT		20
> +#define OPTEE_SMC_GET_NOTIF_IT \
> +	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_NOTIF_IT)
> +
> +/*
> + * Configure an interrupt notification event
> + *
> + * It is expected that this function is called from an interrupt handler
> + * in normal world.
> + *
> + * Call requests usage:
> + * a0	SMC Function ID, OPTEE_SMC_CONFIGURE_NOTIF_IT
> + * a1	Interrupt identifier value
> + * a2	Bit[0]: 1 to configure interrupt mask with a3[bit 0], else 0
> + *	Bit[31:1] Reserved for future use, MBZ
> + * a3	Bit[0]: 1 to mask the interrupt, 0 to unmask (applies if a2[bit 0]=1)
> + *	Bit[31:1] Reserved for future use, MBZ
> + * a4-6	Reserved for furture use, MBZ
> + * a7	Hypervisor Client ID register
> + *
> + * Normal return register usage:
> + * a0	OPTEE_SMC_RETURN_OK
> + * a1-7	Preserved
> + *
> + * Not supported return register usage:
> + * a0	OPTEE_SMC_RETURN_ENOTAVAIL
> + * a1-7	Preserved
> + *
> + * Invalid command with provided arguments return usage:
> + * a0	OPTEE_SMC_RETURN_EBADCMD
> + * a1-7	Preserved
> + */
> +#define OPTEE_SMC_NOTIF_CONFIG_MASK		BIT(0)
> +
> +#define OPTEE_SMC_FUNCID_CONFIGURE_NOTIF_IT	21
> +#define OPTEE_SMC_CONFIGURE_NOTIF_IT \
> +	OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_CONFIGURE_NOTIF_IT)
> +

I assume you're going to update the changes to this file this with the
latest changes in https://github.com/OP-TEE/optee_os/pull/5793

>  /*
>   * Resume from RPC (for example after processing a foreign interrupt)
>   *
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..9f4fdd28f04a 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -977,6 +977,71 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
>   * 5. Asynchronous notification
>   */
>  
> +static void config_notif_it(optee_invoke_fn *invoke_fn, u32 it_value,
> +			    u32 op_mask, u32 val_mask)

"it" is a bit short as an abbreviation for interrupt, I'd prefer "itr"
or such. Then there's the question is it an interrupt value, identifier,
line, or number? It would help if we could use a consistant terminology
in this file at least.

> +{
> +	struct arm_smccc_res res = { };
> +
> +	invoke_fn(OPTEE_SMC_CONFIGURE_NOTIF_IT, it_value, op_mask, val_mask,
> +		  0, 0, 0, 0, &res);
> +}
> +
> +static void optee_it_irq_mask(struct irq_data *d)
> +{
> +	struct optee *optee = d->domain->host_data;
> +
> +	config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK,

Please try to keep the lines below 80 columns. Same for the other
functions in this patch.

> +			OPTEE_SMC_NOTIF_CONFIG_MASK);
> +}
> +
> +static void optee_it_irq_unmask(struct irq_data *d)
> +{
> +	struct optee *optee = d->domain->host_data;
> +
> +	config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK, 0);
> +}
> +
> +static struct irq_chip optee_it_irq_chip = {
> +	.name = "optee-it",
> +	.irq_mask = optee_it_irq_mask,
> +	.irq_unmask = optee_it_irq_unmask,
> +};
> +
> +static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
> +			  unsigned int nr_irqs, void *data)
> +{
> +	struct irq_fwspec *fwspec = data;
> +	irq_hw_number_t hwirq;
> +
> +	hwirq = fwspec->param[0];
> +
> +	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &optee_it_irq_chip, d->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops optee_it_irq_domain_ops = {
> +	.alloc = optee_it_alloc,
> +	.free = irq_domain_free_irqs_common,
> +};
> +
> +static int optee_irq_domain_init(struct platform_device *pdev,
> +				 struct optee *optee, u_int max_it)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +
> +	optee->smc.domain = irq_domain_add_linear(np, max_it,
> +						  &optee_it_irq_domain_ops,
> +						  optee);
> +	if (!optee->smc.domain) {
> +		dev_err(dev, "Unable to add irq domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
>  static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
>  				 bool *value_pending)
>  {
> @@ -991,6 +1056,60 @@ static u32 get_async_notif_value(optee_invoke_fn *invoke_fn, bool *value_valid,
>  	return res.a1;
>  }
>  
> +static void forward_irq(struct optee *optee, unsigned int id)
> +{
> +	if (generic_handle_domain_irq(optee->smc.domain, id)) {
> +		pr_err("No consumer for optee irq %u, it will be masked\n", id);
> +		config_notif_it(optee->smc.invoke_fn, id,
> +				OPTEE_SMC_NOTIF_CONFIG_MASK,
> +				OPTEE_SMC_NOTIF_CONFIG_MASK);
> +	}
> +}
> +
> +static void retrieve_pending_irqs(struct optee *optee, bool *async_value_pending,
> +				  bool *do_bottom_half)
> +
> +{
> +	struct arm_smccc_res res;
> +	bool it_pending;
> +	ssize_t cnt;
> +	const unsigned int lsb_mask = GENMASK(15, 0);
> +	const unsigned int msb_shift = 16;
> +
> +	*async_value_pending = false;
> +	*do_bottom_half = false;
> +
> +	do {
> +		optee->smc.invoke_fn(OPTEE_SMC_GET_NOTIF_IT, 0, 0, 0, 0, 0, 0, 0, &res);
> +
> +		if (res.a0)
> +			return;
> +
> +		if (res.a1 & OPTEE_SMC_NOTIF_DO_BOTTOM_HALF)
> +			*do_bottom_half = true;
> +
> +		it_pending = res.a1 & OPTEE_SMC_NOTIF_IT_PENDING;
> +		cnt = res.a1 & OPTEE_SMC_NOTIF_IT_COUNT_MASK;
> +		if (cnt > 5 || (!cnt && it_pending)) {
> +			WARN_ONCE(0, "Unexpected interrupt notif count %zi\n", cnt);
> +			break;
> +		}
> +
> +		if (--cnt >= 0)
> +			forward_irq(optee, res.a1 >> msb_shift);
> +		if (--cnt >= 0)
> +			forward_irq(optee, res.a2 & lsb_mask);
> +		if (--cnt >= 0)
> +			forward_irq(optee, res.a2 >> msb_shift);
> +		if (--cnt >= 0)
> +			forward_irq(optee, res.a3 & lsb_mask);
> +		if (--cnt >= 0)
> +			forward_irq(optee, res.a3 >> msb_shift);

I believe this should be easier to read:
                if (cnt >= 1)
                        forward_irq(optee, res.a1 >> msb_shift);
                if (cnt >= 2)
                        forward_irq(optee, res.a2 & lsb_mask);
                if (cnt >= 3)
                        forward_irq(optee, res.a2 >> msb_shift);
                if (cnt >= 4)
                        forward_irq(optee, res.a3 & lsb_mask);
                if (cnt >= 5)
                        forward_irq(optee, res.a3 >> msb_shift);

> +	} while (it_pending);
> +
> +	*async_value_pending = res.a1 & OPTEE_SMC_NOTIF_VALUE_PENDING;
> +}
> +
>  static irqreturn_t notif_irq_handler(int irq, void *dev_id)
>  {
>  	struct optee *optee = dev_id;
> @@ -999,9 +1118,14 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
>  	bool value_pending;
>  	u32 value;
>  
> -	do {
> -		value = get_async_notif_value(optee->smc.invoke_fn,
> -					      &value_valid, &value_pending);
> +	if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
> +		retrieve_pending_irqs(optee, &value_pending, &do_bottom_half);
> +	else
> +		value_pending = true;
> +
> +	while (value_pending) {
> +		value = get_async_notif_value(optee->smc.invoke_fn, &value_valid,
> +					      &value_pending);
>  		if (!value_valid)
>  			break;
>  
> @@ -1009,10 +1133,11 @@ static irqreturn_t notif_irq_handler(int irq, void *dev_id)
>  			do_bottom_half = true;
>  		else
>  			optee_notif_send(optee, value);
> -	} while (value_pending);
> +	};
>  
>  	if (do_bottom_half)
>  		return IRQ_WAKE_THREAD;
> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -1048,6 +1173,9 @@ static void optee_smc_notif_uninit_irq(struct optee *optee)
>  			free_irq(optee->smc.notif_irq, optee);
>  			irq_dispose_mapping(optee->smc.notif_irq);
>  		}
> +
> +		if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
> +			irq_domain_remove(optee->smc.domain);
>  	}
>  }
>  
> @@ -1187,13 +1315,14 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn)
>  
>  static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
>  					    u32 *sec_caps, u32 *max_notif_value,
> +					    u32 *max_notif_it,
>  					    unsigned int *rpc_param_count)
>  {
>  	union {
>  		struct arm_smccc_res smccc;
>  		struct optee_smc_exchange_capabilities_result result;
>  	} res;
> -	u32 a1 = 0;
> +	u32 a1 = OPTEE_SMC_SEC_CAP_IT_NOTIF;
>  
>  	/*
>  	 * TODO This isn't enough to tell if it's UP system (from kernel
> @@ -1219,6 +1348,12 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
>  	else
>  		*rpc_param_count = 0;
>  
> +	if (*sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF)
> +		*max_notif_it = (res.result.data & OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_MASK) >>
> +				OPTEE_SMC_SEC_CAP_MAX_NOTIF_IT_SHIFT;
> +	else
> +		*max_notif_it = 0;
> +
>  	return true;
>  }
>  
> @@ -1364,6 +1499,7 @@ static int optee_probe(struct platform_device *pdev)
>  	struct tee_device *teedev;
>  	struct tee_context *ctx;
>  	u32 max_notif_value;
> +	u32 max_notif_it;
>  	u32 arg_cache_flags;
>  	u32 sec_caps;
>  	int rc;
> @@ -1385,7 +1521,7 @@ static int optee_probe(struct platform_device *pdev)
>  	}
>  
>  	if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> -					     &max_notif_value,
> +					     &max_notif_value, &max_notif_it,
>  					     &rpc_param_count)) {
>  		pr_warn("capabilities mismatch\n");
>  		return -EINVAL;
> @@ -1506,6 +1642,16 @@ static int optee_probe(struct platform_device *pdev)
>  			irq_dispose_mapping(irq);
>  			goto err_notif_uninit;
>  		}
> +
> +		if (sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF) {
> +			rc = optee_irq_domain_init(pdev, optee, max_notif_it);

It looks like there's no cleanup for this in the error path, if the call
to optee_enumerate_devices() below would fail.

Thanks,
Jens

> +			if (rc) {
> +				free_irq(optee->smc.notif_irq, optee);
> +				irq_dispose_mapping(irq);
> +				goto err_notif_uninit;
> +			}
> +		}
> +
>  		enable_async_notif(optee->smc.invoke_fn);
>  		pr_info("Asynchronous notifications enabled\n");
>  	}
> -- 
> 2.25.1
> 

[PATCH v2 3/3] optee: add enable/disable/set_wake handlers to optee irqs

[Etienne Carriere]

Implements OP-TEE's It Notif PTA API to deactivate and activate an
interrupt notifier and to configure the wakeup capability
of that interrupt. These controls are useful for efficient power
management of the device when an interrupt controller resources is
hosted in OP-TEE world.

When OP-TEE does not implement the It Notif PTA, the related handlers
simply return with success. If OP-TEE exposes the PTA services, they
are invoked on enable, disable and set_wake irqchip operations.

Cc: Jens Wiklander <jens.wiklander@linaro.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Sumit Garg <sumit.garg@linaro.org>

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since v1:
- Patch added in v2 series for power-up/down and wakeup configuration
  of the irq chip.
---
 drivers/tee/optee/optee_private.h |   2 +
 drivers/tee/optee/smc_abi.c       | 157 ++++++++++++++++++++++++++++++
 2 files changed, 159 insertions(+)

diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index f467409e02e9..257bb505a1fb 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -166,6 +166,7 @@ struct optee_ops {
  * @scan_bus_done	flag if device registation was already done.
  * @scan_bus_wq		workqueue to scan optee bus and register optee drivers
  * @scan_bus_work	workq to scan optee bus and register optee drivers
+ * @notif_it_pta_ctx    TEE context for invoking interrupt conif services
  */
 struct optee {
 	struct tee_device *supp_teedev;
@@ -185,6 +186,7 @@ struct optee {
 	bool   scan_bus_done;
 	struct workqueue_struct *scan_bus_wq;
 	struct work_struct scan_bus_work;
+	struct tee_context *notif_it_pta_ctx;
 };
 
 struct optee_session {
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index 9f4fdd28f04a..95adf8c93c98 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -52,6 +52,43 @@
  */
 #define OPTEE_MIN_STATIC_POOL_ALIGN    9 /* 512 bytes aligned */
 
+/*
+ * Interrupt notification can be configured using Notif-IT PTA services.
+ * Below are the PTA UUID and its API commands.
+ */
+#define PTA_IT_NOTIF_UUID \
+	UUID_INIT(0x4461e5c7, 0xb523, 0x4b73, 0xac, 0xed, 0x75, 0xad, \
+		  0x2b, 0x9b, 0x59, 0xa1)
+
+/*
+ * PTA_IT_NOTIF_ACTIVATE_DETECTION - Enable a interrupt notification
+ *
+ * [in]  params[0].value.a     Interrupt ID
+ */
+#define PTA_IT_NOTIF_ACTIVATE_DETECTION		0
+
+/*
+ * PTA_IT_NOTIF_DEACTIVATE_DETECTION - Disable a interrupt notification
+ *
+ * [in]  params[0].value.a     Interrupt ID
+ */
+#define PTA_IT_NOTIF_DEACTIVATE_DETECTION	1
+
+/*
+ * PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE - Enable an interrupt wakeup source
+ *
+ * [in]  params[0].value.a     Interrupt ID
+ */
+#define PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE	2
+
+/*
+ * PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE - Disable an interrupt wakeup source
+ *
+ * [in]  params[0].value.a     Interrupt ID
+ */
+#define PTA_IT_NOTIF_DISABLE_WAKEUP_SOURCE	3
+
+
 /*
  * 1. Convert between struct tee_param and struct optee_msg_param
  *
@@ -977,6 +1014,92 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
  * 5. Asynchronous notification
  */
 
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	return ver->impl_id == TEE_IMPL_ID_OPTEE;
+}
+
+static void init_optee_pta_context(struct optee *optee)
+{
+	struct tee_context *ctx = NULL;
+	const uuid_t pta_uuid = PTA_IT_NOTIF_UUID;
+	struct tee_ioctl_open_session_arg sess_arg;
+	int ret;
+
+	ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);
+	if (IS_ERR(ctx))
+		return;
+
+	memset(&sess_arg, 0, sizeof(sess_arg));
+	export_uuid(sess_arg.uuid, &pta_uuid);
+	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+
+	ret = tee_client_open_session(ctx, &sess_arg, NULL);
+	if ((ret < 0) || (sess_arg.ret != 0)) {
+		pr_err("Can't open IT_NOTIF PTA session: %#x\n", sess_arg.ret);
+		tee_client_close_context(ctx);
+		return;
+	}
+
+	tee_client_close_session(ctx, sess_arg.session);
+
+	optee->notif_it_pta_ctx = ctx;
+}
+
+static void release_optee_pta_context(struct optee *optee)
+{
+	if (optee->notif_it_pta_ctx) {
+		tee_client_close_context(optee->notif_it_pta_ctx);
+		optee->notif_it_pta_ctx = NULL;
+	}
+}
+
+static int invoke_optee_pta(struct optee *optee, unsigned int command,
+			    unsigned int irq_id)
+{
+	const uuid_t pta_uuid = PTA_IT_NOTIF_UUID;
+	struct tee_ioctl_open_session_arg sess_arg;
+	struct tee_ioctl_invoke_arg inv_arg;
+	struct tee_param param[1];
+	int ret;
+
+	if (!optee->notif_it_pta_ctx)
+		return -ENOENT;
+
+	memset(&sess_arg, 0, sizeof(sess_arg));
+	export_uuid(sess_arg.uuid, &pta_uuid);
+	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+
+	ret = tee_client_open_session(optee->notif_it_pta_ctx, &sess_arg, NULL);
+	if ((ret < 0) || (sess_arg.ret != 0)) {
+		pr_err("tee_client_open_session failed, err: %#x\n", sess_arg.ret);
+		if (!ret)
+			ret = -EINVAL;
+		return ret;
+	}
+
+	memset(&inv_arg, 0, sizeof(inv_arg));
+	inv_arg.session = sess_arg.session;
+	inv_arg.func = command;
+	inv_arg.num_params = 1;
+
+	memset(&param, 0, sizeof(param));
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+	param[0].u.value.a = irq_id;
+
+	ret = tee_client_invoke_func(optee->notif_it_pta_ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		pr_err("tee_client_invoke_func failed, ret: %d, err: %#x\n",
+		       ret, inv_arg.ret);
+		if (!ret)
+			ret = -EINVAL;
+	}
+
+	tee_client_close_session(optee->notif_it_pta_ctx, sess_arg.session);
+
+	return ret;
+}
+
 static void config_notif_it(optee_invoke_fn *invoke_fn, u32 it_value,
 			    u32 op_mask, u32 val_mask)
 {
@@ -1001,10 +1124,40 @@ static void optee_it_irq_unmask(struct irq_data *d)
 	config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK, 0);
 }
 
+static void optee_it_irq_disable(struct irq_data *d)
+{
+	struct optee *optee = d->domain->host_data;
+
+	(void)invoke_optee_pta(optee, PTA_IT_NOTIF_DEACTIVATE_DETECTION, d->hwirq);
+}
+
+static void optee_it_irq_enable(struct irq_data *d)
+{
+	struct optee *optee = d->domain->host_data;
+
+	(void)invoke_optee_pta(optee, PTA_IT_NOTIF_ACTIVATE_DETECTION, d->hwirq);
+}
+
+static int optee_it_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct optee *optee = d->domain->host_data;
+	u32 command;
+
+	if (on)
+		command = PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE;
+	else
+		command = PTA_IT_NOTIF_DISABLE_WAKEUP_SOURCE;
+
+	return invoke_optee_pta(optee, command, d->hwirq);
+}
+
 static struct irq_chip optee_it_irq_chip = {
 	.name = "optee-it",
 	.irq_mask = optee_it_irq_mask,
 	.irq_unmask = optee_it_irq_unmask,
+	.irq_disable = optee_it_irq_disable,
+	.irq_enable = optee_it_irq_enable,
+	.irq_set_wake = optee_it_irq_set_wake,
 };
 
 static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
@@ -1463,6 +1616,7 @@ static int optee_smc_remove(struct platform_device *pdev)
 		optee_disable_shm_cache(optee);
 
 	optee_smc_notif_uninit_irq(optee);
+	release_optee_pta_context(optee);
 
 	optee_remove_common(optee);
 
@@ -1650,6 +1804,8 @@ static int optee_probe(struct platform_device *pdev)
 				irq_dispose_mapping(irq);
 				goto err_notif_uninit;
 			}
+
+			init_optee_pta_context(optee);
 		}
 
 		enable_async_notif(optee->smc.invoke_fn);
@@ -1687,6 +1843,7 @@ static int optee_probe(struct platform_device *pdev)
 		optee_disable_shm_cache(optee);
 	optee_smc_notif_uninit_irq(optee);
 	optee_unregister_devices();
+	release_optee_pta_context(optee);
 err_notif_uninit:
 	optee_notif_uninit(optee);
 err_close_ctx:
-- 
2.25.1

Re: [PATCH v2 3/3] optee: add enable/disable/set_wake handlers to optee irqs

[Jens Wiklander]

On Tue, Jan 24, 2023 at 11:56:43AM +0100, Etienne Carriere wrote:
> Implements OP-TEE's It Notif PTA API to deactivate and activate an
> interrupt notifier and to configure the wakeup capability
> of that interrupt. These controls are useful for efficient power
> management of the device when an interrupt controller resources is
> hosted in OP-TEE world.
> 
> When OP-TEE does not implement the It Notif PTA, the related handlers
> simply return with success. If OP-TEE exposes the PTA services, they
> are invoked on enable, disable and set_wake irqchip operations.
> 
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> 
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v1:
> - Patch added in v2 series for power-up/down and wakeup configuration
>   of the irq chip.
> ---
>  drivers/tee/optee/optee_private.h |   2 +
>  drivers/tee/optee/smc_abi.c       | 157 ++++++++++++++++++++++++++++++
>  2 files changed, 159 insertions(+)
> 
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index f467409e02e9..257bb505a1fb 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -166,6 +166,7 @@ struct optee_ops {
>   * @scan_bus_done	flag if device registation was already done.
>   * @scan_bus_wq		workqueue to scan optee bus and register optee drivers
>   * @scan_bus_work	workq to scan optee bus and register optee drivers
> + * @notif_it_pta_ctx    TEE context for invoking interrupt conif services

We already have "ctx" above, why do we need another TEE context?

Thanks,
Jens

>   */
>  struct optee {
>  	struct tee_device *supp_teedev;
> @@ -185,6 +186,7 @@ struct optee {
>  	bool   scan_bus_done;
>  	struct workqueue_struct *scan_bus_wq;
>  	struct work_struct scan_bus_work;
> +	struct tee_context *notif_it_pta_ctx;
>  };
>  
>  struct optee_session {
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index 9f4fdd28f04a..95adf8c93c98 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -52,6 +52,43 @@
>   */
>  #define OPTEE_MIN_STATIC_POOL_ALIGN    9 /* 512 bytes aligned */
>  
> +/*
> + * Interrupt notification can be configured using Notif-IT PTA services.
> + * Below are the PTA UUID and its API commands.
> + */
> +#define PTA_IT_NOTIF_UUID \
> +	UUID_INIT(0x4461e5c7, 0xb523, 0x4b73, 0xac, 0xed, 0x75, 0xad, \
> +		  0x2b, 0x9b, 0x59, 0xa1)
> +
> +/*
> + * PTA_IT_NOTIF_ACTIVATE_DETECTION - Enable a interrupt notification
> + *
> + * [in]  params[0].value.a     Interrupt ID
> + */
> +#define PTA_IT_NOTIF_ACTIVATE_DETECTION		0
> +
> +/*
> + * PTA_IT_NOTIF_DEACTIVATE_DETECTION - Disable a interrupt notification
> + *
> + * [in]  params[0].value.a     Interrupt ID
> + */
> +#define PTA_IT_NOTIF_DEACTIVATE_DETECTION	1
> +
> +/*
> + * PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE - Enable an interrupt wakeup source
> + *
> + * [in]  params[0].value.a     Interrupt ID
> + */
> +#define PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE	2
> +
> +/*
> + * PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE - Disable an interrupt wakeup source
> + *
> + * [in]  params[0].value.a     Interrupt ID
> + */
> +#define PTA_IT_NOTIF_DISABLE_WAKEUP_SOURCE	3
> +
> +
>  /*
>   * 1. Convert between struct tee_param and struct optee_msg_param
>   *
> @@ -977,6 +1014,92 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
>   * 5. Asynchronous notification
>   */
>  
> +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> +	return ver->impl_id == TEE_IMPL_ID_OPTEE;
> +}
> +
> +static void init_optee_pta_context(struct optee *optee)
> +{
> +	struct tee_context *ctx = NULL;
> +	const uuid_t pta_uuid = PTA_IT_NOTIF_UUID;
> +	struct tee_ioctl_open_session_arg sess_arg;
> +	int ret;
> +
> +	ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);
> +	if (IS_ERR(ctx))
> +		return;
> +
> +	memset(&sess_arg, 0, sizeof(sess_arg));
> +	export_uuid(sess_arg.uuid, &pta_uuid);
> +	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> +
> +	ret = tee_client_open_session(ctx, &sess_arg, NULL);
> +	if ((ret < 0) || (sess_arg.ret != 0)) {
> +		pr_err("Can't open IT_NOTIF PTA session: %#x\n", sess_arg.ret);
> +		tee_client_close_context(ctx);
> +		return;
> +	}
> +
> +	tee_client_close_session(ctx, sess_arg.session);
> +
> +	optee->notif_it_pta_ctx = ctx;
> +}
> +
> +static void release_optee_pta_context(struct optee *optee)
> +{
> +	if (optee->notif_it_pta_ctx) {
> +		tee_client_close_context(optee->notif_it_pta_ctx);
> +		optee->notif_it_pta_ctx = NULL;
> +	}
> +}
> +
> +static int invoke_optee_pta(struct optee *optee, unsigned int command,
> +			    unsigned int irq_id)
> +{
> +	const uuid_t pta_uuid = PTA_IT_NOTIF_UUID;
> +	struct tee_ioctl_open_session_arg sess_arg;
> +	struct tee_ioctl_invoke_arg inv_arg;
> +	struct tee_param param[1];
> +	int ret;
> +
> +	if (!optee->notif_it_pta_ctx)
> +		return -ENOENT;
> +
> +	memset(&sess_arg, 0, sizeof(sess_arg));
> +	export_uuid(sess_arg.uuid, &pta_uuid);
> +	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> +
> +	ret = tee_client_open_session(optee->notif_it_pta_ctx, &sess_arg, NULL);
> +	if ((ret < 0) || (sess_arg.ret != 0)) {
> +		pr_err("tee_client_open_session failed, err: %#x\n", sess_arg.ret);
> +		if (!ret)
> +			ret = -EINVAL;
> +		return ret;
> +	}
> +
> +	memset(&inv_arg, 0, sizeof(inv_arg));
> +	inv_arg.session = sess_arg.session;
> +	inv_arg.func = command;
> +	inv_arg.num_params = 1;
> +
> +	memset(&param, 0, sizeof(param));
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +	param[0].u.value.a = irq_id;
> +
> +	ret = tee_client_invoke_func(optee->notif_it_pta_ctx, &inv_arg, param);
> +	if ((ret < 0) || (inv_arg.ret != 0)) {
> +		pr_err("tee_client_invoke_func failed, ret: %d, err: %#x\n",
> +		       ret, inv_arg.ret);
> +		if (!ret)
> +			ret = -EINVAL;
> +	}
> +
> +	tee_client_close_session(optee->notif_it_pta_ctx, sess_arg.session);
> +
> +	return ret;
> +}
> +
>  static void config_notif_it(optee_invoke_fn *invoke_fn, u32 it_value,
>  			    u32 op_mask, u32 val_mask)
>  {
> @@ -1001,10 +1124,40 @@ static void optee_it_irq_unmask(struct irq_data *d)
>  	config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK, 0);
>  }
>  
> +static void optee_it_irq_disable(struct irq_data *d)
> +{
> +	struct optee *optee = d->domain->host_data;
> +
> +	(void)invoke_optee_pta(optee, PTA_IT_NOTIF_DEACTIVATE_DETECTION, d->hwirq);
> +}
> +
> +static void optee_it_irq_enable(struct irq_data *d)
> +{
> +	struct optee *optee = d->domain->host_data;
> +
> +	(void)invoke_optee_pta(optee, PTA_IT_NOTIF_ACTIVATE_DETECTION, d->hwirq);
> +}
> +
> +static int optee_it_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct optee *optee = d->domain->host_data;
> +	u32 command;
> +
> +	if (on)
> +		command = PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE;
> +	else
> +		command = PTA_IT_NOTIF_DISABLE_WAKEUP_SOURCE;
> +
> +	return invoke_optee_pta(optee, command, d->hwirq);
> +}
> +
>  static struct irq_chip optee_it_irq_chip = {
>  	.name = "optee-it",
>  	.irq_mask = optee_it_irq_mask,
>  	.irq_unmask = optee_it_irq_unmask,
> +	.irq_disable = optee_it_irq_disable,
> +	.irq_enable = optee_it_irq_enable,
> +	.irq_set_wake = optee_it_irq_set_wake,
>  };
>  
>  static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
> @@ -1463,6 +1616,7 @@ static int optee_smc_remove(struct platform_device *pdev)
>  		optee_disable_shm_cache(optee);
>  
>  	optee_smc_notif_uninit_irq(optee);
> +	release_optee_pta_context(optee);
>  
>  	optee_remove_common(optee);
>  
> @@ -1650,6 +1804,8 @@ static int optee_probe(struct platform_device *pdev)
>  				irq_dispose_mapping(irq);
>  				goto err_notif_uninit;
>  			}
> +
> +			init_optee_pta_context(optee);
>  		}
>  
>  		enable_async_notif(optee->smc.invoke_fn);
> @@ -1687,6 +1843,7 @@ static int optee_probe(struct platform_device *pdev)
>  		optee_disable_shm_cache(optee);
>  	optee_smc_notif_uninit_irq(optee);
>  	optee_unregister_devices();
> +	release_optee_pta_context(optee);
>  err_notif_uninit:
>  	optee_notif_uninit(optee);
>  err_close_ctx:
> -- 
> 2.25.1
> 

Re: [PATCH v2 3/3] optee: add enable/disable/set_wake handlers to optee irqs

[Etienne Carriere]

On Fri, 3 Feb 2023 at 11:39, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Jan 24, 2023 at 11:56:43AM +0100, Etienne Carriere wrote:
> > Implements OP-TEE's It Notif PTA API to deactivate and activate an
> > interrupt notifier and to configure the wakeup capability
> > of that interrupt. These controls are useful for efficient power
> > management of the device when an interrupt controller resources is
> > hosted in OP-TEE world.
> >
> > When OP-TEE does not implement the It Notif PTA, the related handlers
> > simply return with success. If OP-TEE exposes the PTA services, they
> > are invoked on enable, disable and set_wake irqchip operations.
> >
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Sumit Garg <sumit.garg@linaro.org>
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since v1:
> > - Patch added in v2 series for power-up/down and wakeup configuration
> >   of the irq chip.
> > ---
> >  drivers/tee/optee/optee_private.h |   2 +
> >  drivers/tee/optee/smc_abi.c       | 157 ++++++++++++++++++++++++++++++
> >  2 files changed, 159 insertions(+)
> >
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index f467409e02e9..257bb505a1fb 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -166,6 +166,7 @@ struct optee_ops {
> >   * @scan_bus_done    flag if device registation was already done.
> >   * @scan_bus_wq              workqueue to scan optee bus and register optee drivers
> >   * @scan_bus_work    workq to scan optee bus and register optee drivers
> > + * @notif_it_pta_ctx    TEE context for invoking interrupt conif services
>
> We already have "ctx" above, why do we need another TEE context?
>

True, not needed. I'll remove it.

Thanks,
Etienne

> Thanks,
> Jens
>
> >   */
> >  struct optee {
> >       struct tee_device *supp_teedev;
> > @@ -185,6 +186,7 @@ struct optee {
> >       bool   scan_bus_done;
> >       struct workqueue_struct *scan_bus_wq;
> >       struct work_struct scan_bus_work;
> > +     struct tee_context *notif_it_pta_ctx;
> >  };
> >
> >  struct optee_session {
> > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > index 9f4fdd28f04a..95adf8c93c98 100644
> > --- a/drivers/tee/optee/smc_abi.c
> > +++ b/drivers/tee/optee/smc_abi.c
> > @@ -52,6 +52,43 @@
> >   */
> >  #define OPTEE_MIN_STATIC_POOL_ALIGN    9 /* 512 bytes aligned */
> >
> > +/*
> > + * Interrupt notification can be configured using Notif-IT PTA services.
> > + * Below are the PTA UUID and its API commands.
> > + */
> > +#define PTA_IT_NOTIF_UUID \
> > +     UUID_INIT(0x4461e5c7, 0xb523, 0x4b73, 0xac, 0xed, 0x75, 0xad, \
> > +               0x2b, 0x9b, 0x59, 0xa1)
> > +
> > +/*
> > + * PTA_IT_NOTIF_ACTIVATE_DETECTION - Enable a interrupt notification
> > + *
> > + * [in]  params[0].value.a     Interrupt ID
> > + */
> > +#define PTA_IT_NOTIF_ACTIVATE_DETECTION              0
> > +
> > +/*
> > + * PTA_IT_NOTIF_DEACTIVATE_DETECTION - Disable a interrupt notification
> > + *
> > + * [in]  params[0].value.a     Interrupt ID
> > + */
> > +#define PTA_IT_NOTIF_DEACTIVATE_DETECTION    1
> > +
> > +/*
> > + * PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE - Enable an interrupt wakeup source
> > + *
> > + * [in]  params[0].value.a     Interrupt ID
> > + */
> > +#define PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE    2
> > +
> > +/*
> > + * PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE - Disable an interrupt wakeup source
> > + *
> > + * [in]  params[0].value.a     Interrupt ID
> > + */
> > +#define PTA_IT_NOTIF_DISABLE_WAKEUP_SOURCE   3
> > +
> > +
> >  /*
> >   * 1. Convert between struct tee_param and struct optee_msg_param
> >   *
> > @@ -977,6 +1014,92 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
> >   * 5. Asynchronous notification
> >   */
> >
> > +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > +{
> > +     return ver->impl_id == TEE_IMPL_ID_OPTEE;
> > +}
> > +
> > +static void init_optee_pta_context(struct optee *optee)
> > +{
> > +     struct tee_context *ctx = NULL;
> > +     const uuid_t pta_uuid = PTA_IT_NOTIF_UUID;
> > +     struct tee_ioctl_open_session_arg sess_arg;
> > +     int ret;
> > +
> > +     ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);
> > +     if (IS_ERR(ctx))
> > +             return;
> > +
> > +     memset(&sess_arg, 0, sizeof(sess_arg));
> > +     export_uuid(sess_arg.uuid, &pta_uuid);
> > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > +
> > +     ret = tee_client_open_session(ctx, &sess_arg, NULL);
> > +     if ((ret < 0) || (sess_arg.ret != 0)) {
> > +             pr_err("Can't open IT_NOTIF PTA session: %#x\n", sess_arg.ret);
> > +             tee_client_close_context(ctx);
> > +             return;
> > +     }
> > +
> > +     tee_client_close_session(ctx, sess_arg.session);
> > +
> > +     optee->notif_it_pta_ctx = ctx;
> > +}
> > +
> > +static void release_optee_pta_context(struct optee *optee)
> > +{
> > +     if (optee->notif_it_pta_ctx) {
> > +             tee_client_close_context(optee->notif_it_pta_ctx);
> > +             optee->notif_it_pta_ctx = NULL;
> > +     }
> > +}
> > +
> > +static int invoke_optee_pta(struct optee *optee, unsigned int command,
> > +                         unsigned int irq_id)
> > +{
> > +     const uuid_t pta_uuid = PTA_IT_NOTIF_UUID;
> > +     struct tee_ioctl_open_session_arg sess_arg;
> > +     struct tee_ioctl_invoke_arg inv_arg;
> > +     struct tee_param param[1];
> > +     int ret;
> > +
> > +     if (!optee->notif_it_pta_ctx)
> > +             return -ENOENT;
> > +
> > +     memset(&sess_arg, 0, sizeof(sess_arg));
> > +     export_uuid(sess_arg.uuid, &pta_uuid);
> > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > +
> > +     ret = tee_client_open_session(optee->notif_it_pta_ctx, &sess_arg, NULL);
> > +     if ((ret < 0) || (sess_arg.ret != 0)) {
> > +             pr_err("tee_client_open_session failed, err: %#x\n", sess_arg.ret);
> > +             if (!ret)
> > +                     ret = -EINVAL;
> > +             return ret;
> > +     }
> > +
> > +     memset(&inv_arg, 0, sizeof(inv_arg));
> > +     inv_arg.session = sess_arg.session;
> > +     inv_arg.func = command;
> > +     inv_arg.num_params = 1;
> > +
> > +     memset(&param, 0, sizeof(param));
> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > +     param[0].u.value.a = irq_id;
> > +
> > +     ret = tee_client_invoke_func(optee->notif_it_pta_ctx, &inv_arg, param);
> > +     if ((ret < 0) || (inv_arg.ret != 0)) {
> > +             pr_err("tee_client_invoke_func failed, ret: %d, err: %#x\n",
> > +                    ret, inv_arg.ret);
> > +             if (!ret)
> > +                     ret = -EINVAL;
> > +     }
> > +
> > +     tee_client_close_session(optee->notif_it_pta_ctx, sess_arg.session);
> > +
> > +     return ret;
> > +}
> > +
> >  static void config_notif_it(optee_invoke_fn *invoke_fn, u32 it_value,
> >                           u32 op_mask, u32 val_mask)
> >  {
> > @@ -1001,10 +1124,40 @@ static void optee_it_irq_unmask(struct irq_data *d)
> >       config_notif_it(optee->smc.invoke_fn, d->hwirq, OPTEE_SMC_NOTIF_CONFIG_MASK, 0);
> >  }
> >
> > +static void optee_it_irq_disable(struct irq_data *d)
> > +{
> > +     struct optee *optee = d->domain->host_data;
> > +
> > +     (void)invoke_optee_pta(optee, PTA_IT_NOTIF_DEACTIVATE_DETECTION, d->hwirq);
> > +}
> > +
> > +static void optee_it_irq_enable(struct irq_data *d)
> > +{
> > +     struct optee *optee = d->domain->host_data;
> > +
> > +     (void)invoke_optee_pta(optee, PTA_IT_NOTIF_ACTIVATE_DETECTION, d->hwirq);
> > +}
> > +
> > +static int optee_it_irq_set_wake(struct irq_data *d, unsigned int on)
> > +{
> > +     struct optee *optee = d->domain->host_data;
> > +     u32 command;
> > +
> > +     if (on)
> > +             command = PTA_IT_NOTIF_ENABLE_WAKEUP_SOURCE;
> > +     else
> > +             command = PTA_IT_NOTIF_DISABLE_WAKEUP_SOURCE;
> > +
> > +     return invoke_optee_pta(optee, command, d->hwirq);
> > +}
> > +
> >  static struct irq_chip optee_it_irq_chip = {
> >       .name = "optee-it",
> >       .irq_mask = optee_it_irq_mask,
> >       .irq_unmask = optee_it_irq_unmask,
> > +     .irq_disable = optee_it_irq_disable,
> > +     .irq_enable = optee_it_irq_enable,
> > +     .irq_set_wake = optee_it_irq_set_wake,
> >  };
> >
> >  static int optee_it_alloc(struct irq_domain *d, unsigned int virq,
> > @@ -1463,6 +1616,7 @@ static int optee_smc_remove(struct platform_device *pdev)
> >               optee_disable_shm_cache(optee);
> >
> >       optee_smc_notif_uninit_irq(optee);
> > +     release_optee_pta_context(optee);
> >
> >       optee_remove_common(optee);
> >
> > @@ -1650,6 +1804,8 @@ static int optee_probe(struct platform_device *pdev)
> >                               irq_dispose_mapping(irq);
> >                               goto err_notif_uninit;
> >                       }
> > +
> > +                     init_optee_pta_context(optee);
> >               }
> >
> >               enable_async_notif(optee->smc.invoke_fn);
> > @@ -1687,6 +1843,7 @@ static int optee_probe(struct platform_device *pdev)
> >               optee_disable_shm_cache(optee);
> >       optee_smc_notif_uninit_irq(optee);
> >       optee_unregister_devices();
> > +     release_optee_pta_context(optee);
> >  err_notif_uninit:
> >       optee_notif_uninit(optee);
> >  err_close_ctx:
> > --
> > 2.25.1
> >

Re: [PATCH v2 1/3] dt-bindings: arm: optee: add interrupt controller properties

[Rob Herring]

+Sudeep

On Tue, Jan 24, 2023 at 11:56:41AM +0100, Etienne Carriere wrote:
> Adds an optional interrupt controller property to optee firmware node
> in the DT bindings. Optee driver may embeds an irqchip exposing
> interrupts notified by the TEE world. Optee registers up to 1 interrupt
> controller and identifies each line with a line number from 0 to
> UINT16_MAX.
> 
> In the example, the platform SCMI device uses optee interrupt irq 5
> as async signal to trigger processing of an async incoming SCMI message,
> in the scope of a CPU DVFS control. A platform can have several SCMI
> channels driven this way. Optee irqs also permits small embedded devices
> to share e.g. a gpio expander, a group of wakeup sources, etc... between
> OP-TEE world (for sensitive services) and Linux world (for non-sensitive
> services). The physical controller is driven from the TEE which exposes
> some controls to Linux kernel.
> 
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> 
> Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v1:
> - Added a description to #interrupt-cells property.
> - Changed of example. Linux wakeup event was subject to discussion and
>   i don't know much about input events in Linux. So move to SCMI.
>   In the example, an SCMI server in OP-TEE world raises optee irq 5
>   so that Linux scmi optee channel &scmi_cpu_dvfs pushed in the incoming
>   SCMI message in the scmi device for liekly later processing in threaded
>   context. The example includes all parties: optee, scmi, sram, gic.
> - Obviously rephrased the commit message.
> - Added Cc: tags
> ---
>  .../arm/firmware/linaro,optee-tz.yaml         | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> index d4dc0749f9fd..9c00c27f8b2c 100644
> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> @@ -40,6 +40,14 @@ properties:
>        HVC #0, register assignments
>        register assignments are specified in drivers/tee/optee/optee_smc.h
>  
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 1
> +    description: |
> +      OP-TEE exposes irq for irp chip controllers from OP-TEE world. Each
> +      irq is assigned a single line number identifier used as first argument.
> +
>  required:
>    - compatible
>    - method
> @@ -64,3 +72,62 @@ examples:
>              method = "hvc";
>          };
>      };
> +
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    firmware  {
> +        optee: optee {
> +            compatible = "linaro,optee-tz";
> +            method = "smc";
> +            interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> +            interrupt-controller;
> +            #interrupt-cells = <1>;
> +        };
> +
> +        scmi {
> +            compatible = "linaro,scmi-optee";
> +            linaro,optee-channel-id = <0>;
> +            interrupt-parent = <&gic>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            scmi_cpu_dvfs: protocol@13 {
> +                reg = <0x13>;
> +                linaro,optee-channel-id = <1>;
> +                shmem = <&scmi_shm_tx>, <&scmi_shm_rx>;
> +                interrupts-extended = <&optee 5>;
> +                interrupt-names = "a2p";

These properties aren't documented. Soon there will be a warning[1]. 

> +                #clock-cells = <1>;
> +            };
> +        };
> +    };
> +
> +    gic: interrupt-controller@a0021000 {
> +        compatible = "arm,cortex-a7-gic";
> +        reg = <0xa0021000 0x1000>, <0xa0022000 0x2000>;
> +        interrupt-controller;
> +        #interrupt-cells = <3>;
> +    };
> +
> +    soc {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        sram@2ffff000 {
> +            compatible = "mmio-sram";
> +            reg = <0x2ffff000 0x1000>;
> +            #address-cells = <1>;
> +            #size-cells = <1>;
> +            ranges = <0 0x2ffff000 0x1000>;
> +
> +            scmi_shm_tx: scmi-sram@0 {
> +                compatible = "arm,scmi-shmem";
> +                reg = <0 0x80>;
> +            };
> +
> +            scmi_shm_rx: scmi-sram@100 {
> +                compatible = "arm,scmi-shmem";
> +                reg = <0x100 0x80>;
> +            };

There's no need to show providers in examples (unless the example is for 
the provider).

Rob

Re: [PATCH v2 1/3] dt-bindings: arm: optee: add interrupt controller properties

[Etienne Carriere]

On Wed, 25 Jan 2023 at 21:22, Rob Herring <robh@kernel.org> wrote:
>
> +Sudeep
>
> On Tue, Jan 24, 2023 at 11:56:41AM +0100, Etienne Carriere wrote:
> > Adds an optional interrupt controller property to optee firmware node
> > in the DT bindings. Optee driver may embeds an irqchip exposing
> > interrupts notified by the TEE world. Optee registers up to 1 interrupt
> > controller and identifies each line with a line number from 0 to
> > UINT16_MAX.
> >
> > In the example, the platform SCMI device uses optee interrupt irq 5
> > as async signal to trigger processing of an async incoming SCMI message,
> > in the scope of a CPU DVFS control. A platform can have several SCMI
> > channels driven this way. Optee irqs also permits small embedded devices
> > to share e.g. a gpio expander, a group of wakeup sources, etc... between
> > OP-TEE world (for sensitive services) and Linux world (for non-sensitive
> > services). The physical controller is driven from the TEE which exposes
> > some controls to Linux kernel.
> >
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Sumit Garg <sumit.garg@linaro.org>
> >
> > Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
> > Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since v1:
> > - Added a description to #interrupt-cells property.
> > - Changed of example. Linux wakeup event was subject to discussion and
> >   i don't know much about input events in Linux. So move to SCMI.
> >   In the example, an SCMI server in OP-TEE world raises optee irq 5
> >   so that Linux scmi optee channel &scmi_cpu_dvfs pushed in the incoming
> >   SCMI message in the scmi device for liekly later processing in threaded
> >   context. The example includes all parties: optee, scmi, sram, gic.
> > - Obviously rephrased the commit message.
> > - Added Cc: tags
> > ---
> >  .../arm/firmware/linaro,optee-tz.yaml         | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > index d4dc0749f9fd..9c00c27f8b2c 100644
> > --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > @@ -40,6 +40,14 @@ properties:
> >        HVC #0, register assignments
> >        register assignments are specified in drivers/tee/optee/optee_smc.h
> >
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +    description: |
> > +      OP-TEE exposes irq for irp chip controllers from OP-TEE world. Each
> > +      irq is assigned a single line number identifier used as first argument.
> > +
> >  required:
> >    - compatible
> >    - method
> > @@ -64,3 +72,62 @@ examples:
> >              method = "hvc";
> >          };
> >      };
> > +
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    firmware  {
> > +        optee: optee {
> > +            compatible = "linaro,optee-tz";
> > +            method = "smc";
> > +            interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> > +            interrupt-controller;
> > +            #interrupt-cells = <1>;
> > +        };
> > +
> > +        scmi {
> > +            compatible = "linaro,scmi-optee";
> > +            linaro,optee-channel-id = <0>;
> > +            interrupt-parent = <&gic>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            scmi_cpu_dvfs: protocol@13 {
> > +                reg = <0x13>;
> > +                linaro,optee-channel-id = <1>;
> > +                shmem = <&scmi_shm_tx>, <&scmi_shm_rx>;
> > +                interrupts-extended = <&optee 5>;
> > +                interrupt-names = "a2p";
>
> These properties aren't documented. Soon there will be a warning[1].

They are.
https://github.com/torvalds/linux/blob/v6.1/Documentation/devicetree/bindings/firmware/arm%2Cscmi.yaml#L45-L53

In arm,scmi.yaml, interrupts optional property stands for interrupts
and interrupts-extended, no?


>
> > +                #clock-cells = <1>;
> > +            };
> > +        };
> > +    };
> > +
> > +    gic: interrupt-controller@a0021000 {
> > +        compatible = "arm,cortex-a7-gic";
> > +        reg = <0xa0021000 0x1000>, <0xa0022000 0x2000>;
> > +        interrupt-controller;
> > +        #interrupt-cells = <3>;
> > +    };
> > +
> > +    soc {
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +
> > +        sram@2ffff000 {
> > +            compatible = "mmio-sram";
> > +            reg = <0x2ffff000 0x1000>;
> > +            #address-cells = <1>;
> > +            #size-cells = <1>;
> > +            ranges = <0 0x2ffff000 0x1000>;
> > +
> > +            scmi_shm_tx: scmi-sram@0 {
> > +                compatible = "arm,scmi-shmem";
> > +                reg = <0 0x80>;
> > +            };
> > +
> > +            scmi_shm_rx: scmi-sram@100 {
> > +                compatible = "arm,scmi-shmem";
> > +                reg = <0x100 0x80>;
> > +            };
>
> There's no need to show providers in examples (unless the example is for
> the provider).

Ok, i'll simplify the example

Thanks,
Etienne

>
> Rob

Re: [PATCH v2 1/3] dt-bindings: arm: optee: add interrupt controller properties

[Rob Herring]

On Wed, Jan 25, 2023 at 3:00 PM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> On Wed, 25 Jan 2023 at 21:22, Rob Herring <robh@kernel.org> wrote:
> >
> > +Sudeep
> >
> > On Tue, Jan 24, 2023 at 11:56:41AM +0100, Etienne Carriere wrote:
> > > Adds an optional interrupt controller property to optee firmware node
> > > in the DT bindings. Optee driver may embeds an irqchip exposing
> > > interrupts notified by the TEE world. Optee registers up to 1 interrupt
> > > controller and identifies each line with a line number from 0 to
> > > UINT16_MAX.
> > >
> > > In the example, the platform SCMI device uses optee interrupt irq 5
> > > as async signal to trigger processing of an async incoming SCMI message,
> > > in the scope of a CPU DVFS control. A platform can have several SCMI
> > > channels driven this way. Optee irqs also permits small embedded devices
> > > to share e.g. a gpio expander, a group of wakeup sources, etc... between
> > > OP-TEE world (for sensitive services) and Linux world (for non-sensitive
> > > services). The physical controller is driven from the TEE which exposes
> > > some controls to Linux kernel.
> > >
> > > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Sumit Garg <sumit.garg@linaro.org>
> > >
> > > Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
> > > Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > > Changes since v1:
> > > - Added a description to #interrupt-cells property.
> > > - Changed of example. Linux wakeup event was subject to discussion and
> > >   i don't know much about input events in Linux. So move to SCMI.
> > >   In the example, an SCMI server in OP-TEE world raises optee irq 5
> > >   so that Linux scmi optee channel &scmi_cpu_dvfs pushed in the incoming
> > >   SCMI message in the scmi device for liekly later processing in threaded
> > >   context. The example includes all parties: optee, scmi, sram, gic.
> > > - Obviously rephrased the commit message.
> > > - Added Cc: tags
> > > ---
> > >  .../arm/firmware/linaro,optee-tz.yaml         | 67 +++++++++++++++++++
> > >  1 file changed, 67 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > > index d4dc0749f9fd..9c00c27f8b2c 100644
> > > --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > > @@ -40,6 +40,14 @@ properties:
> > >        HVC #0, register assignments
> > >        register assignments are specified in drivers/tee/optee/optee_smc.h
> > >
> > > +  interrupt-controller: true
> > > +
> > > +  "#interrupt-cells":
> > > +    const: 1
> > > +    description: |
> > > +      OP-TEE exposes irq for irp chip controllers from OP-TEE world. Each
> > > +      irq is assigned a single line number identifier used as first argument.
> > > +
> > >  required:
> > >    - compatible
> > >    - method
> > > @@ -64,3 +72,62 @@ examples:
> > >              method = "hvc";
> > >          };
> > >      };
> > > +
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    firmware  {
> > > +        optee: optee {
> > > +            compatible = "linaro,optee-tz";
> > > +            method = "smc";
> > > +            interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> > > +            interrupt-controller;
> > > +            #interrupt-cells = <1>;
> > > +        };
> > > +
> > > +        scmi {
> > > +            compatible = "linaro,scmi-optee";
> > > +            linaro,optee-channel-id = <0>;
> > > +            interrupt-parent = <&gic>;
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +
> > > +            scmi_cpu_dvfs: protocol@13 {
> > > +                reg = <0x13>;
> > > +                linaro,optee-channel-id = <1>;
> > > +                shmem = <&scmi_shm_tx>, <&scmi_shm_rx>;
> > > +                interrupts-extended = <&optee 5>;
> > > +                interrupt-names = "a2p";
> >
> > These properties aren't documented. Soon there will be a warning[1].
>
> They are.
> https://github.com/torvalds/linux/blob/v6.1/Documentation/devicetree/bindings/firmware/arm%2Cscmi.yaml#L45-L53

They are not. That's the scmi node, not a protocol node.

> In arm,scmi.yaml, interrupts optional property stands for interrupts
> and interrupts-extended, no?

Yes.

Rob

Re: [PATCH v2 1/3] dt-bindings: arm: optee: add interrupt controller properties

[Etienne Carriere]

On Thu, 26 Jan 2023 at 15:00, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jan 25, 2023 at 3:00 PM Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > On Wed, 25 Jan 2023 at 21:22, Rob Herring <robh@kernel.org> wrote:
> > >
> > > +Sudeep
> > >
> > > On Tue, Jan 24, 2023 at 11:56:41AM +0100, Etienne Carriere wrote:
> > > > Adds an optional interrupt controller property to optee firmware node
> > > > in the DT bindings. Optee driver may embeds an irqchip exposing
> > > > interrupts notified by the TEE world. Optee registers up to 1 interrupt
> > > > controller and identifies each line with a line number from 0 to
> > > > UINT16_MAX.
> > > >
> > > > In the example, the platform SCMI device uses optee interrupt irq 5
> > > > as async signal to trigger processing of an async incoming SCMI message,
> > > > in the scope of a CPU DVFS control. A platform can have several SCMI
> > > > channels driven this way. Optee irqs also permits small embedded devices
> > > > to share e.g. a gpio expander, a group of wakeup sources, etc... between
> > > > OP-TEE world (for sensitive services) and Linux world (for non-sensitive
> > > > services). The physical controller is driven from the TEE which exposes
> > > > some controls to Linux kernel.
> > > >
> > > > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Sumit Garg <sumit.garg@linaro.org>
> > > >
> > > > Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
> > > > Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> > > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > ---
> > > > Changes since v1:
> > > > - Added a description to #interrupt-cells property.
> > > > - Changed of example. Linux wakeup event was subject to discussion and
> > > >   i don't know much about input events in Linux. So move to SCMI.
> > > >   In the example, an SCMI server in OP-TEE world raises optee irq 5
> > > >   so that Linux scmi optee channel &scmi_cpu_dvfs pushed in the incoming
> > > >   SCMI message in the scmi device for liekly later processing in threaded
> > > >   context. The example includes all parties: optee, scmi, sram, gic.
> > > > - Obviously rephrased the commit message.
> > > > - Added Cc: tags
> > > > ---
> > > >  .../arm/firmware/linaro,optee-tz.yaml         | 67 +++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > > > index d4dc0749f9fd..9c00c27f8b2c 100644
> > > > --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > > > +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > > > @@ -40,6 +40,14 @@ properties:
> > > >        HVC #0, register assignments
> > > >        register assignments are specified in drivers/tee/optee/optee_smc.h
> > > >
> > > > +  interrupt-controller: true
> > > > +
> > > > +  "#interrupt-cells":
> > > > +    const: 1
> > > > +    description: |
> > > > +      OP-TEE exposes irq for irp chip controllers from OP-TEE world. Each
> > > > +      irq is assigned a single line number identifier used as first argument.
> > > > +
> > > >  required:
> > > >    - compatible
> > > >    - method
> > > > @@ -64,3 +72,62 @@ examples:
> > > >              method = "hvc";
> > > >          };
> > > >      };
> > > > +
> > > > +  - |
> > > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > +    firmware  {
> > > > +        optee: optee {
> > > > +            compatible = "linaro,optee-tz";
> > > > +            method = "smc";
> > > > +            interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> > > > +            interrupt-controller;
> > > > +            #interrupt-cells = <1>;
> > > > +        };
> > > > +
> > > > +        scmi {
> > > > +            compatible = "linaro,scmi-optee";
> > > > +            linaro,optee-channel-id = <0>;
> > > > +            interrupt-parent = <&gic>;
> > > > +            #address-cells = <1>;
> > > > +            #size-cells = <0>;
> > > > +
> > > > +            scmi_cpu_dvfs: protocol@13 {
> > > > +                reg = <0x13>;
> > > > +                linaro,optee-channel-id = <1>;
> > > > +                shmem = <&scmi_shm_tx>, <&scmi_shm_rx>;
> > > > +                interrupts-extended = <&optee 5>;
> > > > +                interrupt-names = "a2p";
> > >
> > > These properties aren't documented. Soon there will be a warning[1].
> >
> > They are.
> > https://github.com/torvalds/linux/blob/v6.1/Documentation/devicetree/bindings/firmware/arm%2Cscmi.yaml#L45-L53
>
> They are not. That's the scmi node, not a protocol node.

Ok,i should have written it this way then:

+        scmi {
+            compatible = "linaro,scmi-optee";
+            linaro,optee-channel-id = <0>;
+            shmem = <&scmi_shm_tx>, <&scmi_shm_rx>;
+            interrupts-extended = <&optee 5>;
+            interrupt-names = "a2p";
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            scmi_cpu_dvfs: protocol@13 {
+                reg = <0x13>;
+                #clock-cells = <1>;
+            };
+        };


>
> > In arm,scmi.yaml, interrupts optional property stands for interrupts
> > and interrupts-extended, no?
>
> Yes.
>
> Rob

Re: [PATCH v2 1/3] dt-bindings: arm: optee: add interrupt controller properties

[Sudeep Holla]

On Tue, Jan 24, 2023 at 11:56:41AM +0100, Etienne Carriere wrote:
> Adds an optional interrupt controller property to optee firmware node
> in the DT bindings. Optee driver may embeds an irqchip exposing
> interrupts notified by the TEE world. Optee registers up to 1 interrupt
> controller and identifies each line with a line number from 0 to
> UINT16_MAX.
> 
> In the example, the platform SCMI device uses optee interrupt irq 5
> as async signal to trigger processing of an async incoming SCMI message,
> in the scope of a CPU DVFS control. A platform can have several SCMI
> channels driven this way. Optee irqs also permits small embedded devices
> to share e.g. a gpio expander, a group of wakeup sources, etc... between
> OP-TEE world (for sensitive services) and Linux world (for non-sensitive
> services). The physical controller is driven from the TEE which exposes
> some controls to Linux kernel.
> 
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> 
> Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v1:
> - Added a description to #interrupt-cells property.
> - Changed of example. Linux wakeup event was subject to discussion and
>   i don't know much about input events in Linux. So move to SCMI.
>   In the example, an SCMI server in OP-TEE world raises optee irq 5
>   so that Linux scmi optee channel &scmi_cpu_dvfs pushed in the incoming
>   SCMI message in the scmi device for liekly later processing in threaded
>   context. The example includes all parties: optee, scmi, sram, gic.
> - Obviously rephrased the commit message.
> - Added Cc: tags
> ---
>  .../arm/firmware/linaro,optee-tz.yaml         | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> index d4dc0749f9fd..9c00c27f8b2c 100644
> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> @@ -40,6 +40,14 @@ properties:
>        HVC #0, register assignments
>        register assignments are specified in drivers/tee/optee/optee_smc.h
>  
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 1
> +    description: |
> +      OP-TEE exposes irq for irp chip controllers from OP-TEE world. Each
> +      irq is assigned a single line number identifier used as first argument.
> +
>  required:
>    - compatible
>    - method
> @@ -64,3 +72,62 @@ examples:
>              method = "hvc";
>          };
>      };
> +
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    firmware  {
> +        optee: optee {
> +            compatible = "linaro,optee-tz";
> +            method = "smc";
> +            interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> +            interrupt-controller;
> +            #interrupt-cells = <1>;
> +        };
> +
> +        scmi {
> +            compatible = "linaro,scmi-optee";
> +            linaro,optee-channel-id = <0>;
> +            interrupt-parent = <&gic>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            scmi_cpu_dvfs: protocol@13 {
> +                reg = <0x13>;
> +                linaro,optee-channel-id = <1>;
> +                shmem = <&scmi_shm_tx>, <&scmi_shm_rx>;
> +                interrupts-extended = <&optee 5>;

Just curious if this can discovered by some communication within OPTEE.
You know you are using optee-channel-id 0 for all SCMI and 1 for DVFS.
Is it not possible to get the information from OPTEE dynamically like
you do for shmem. It is offset within the notification bitmap IIUC, so
the question is can be get that from the firmware on the fly. It also
gives the firmware to reshuffle things around if needed and don't have to
worry about compatibility with DT ?

-- 
Regards,
Sudeep

Re: [PATCH v2 1/3] dt-bindings: arm: optee: add interrupt controller properties

[Etienne Carriere]

Hi Sudeep,

On Thu, 26 Jan 2023 at 15:53, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Jan 24, 2023 at 11:56:41AM +0100, Etienne Carriere wrote:
> > Adds an optional interrupt controller property to optee firmware node
> > in the DT bindings. Optee driver may embeds an irqchip exposing
> > interrupts notified by the TEE world. Optee registers up to 1 interrupt
> > controller and identifies each line with a line number from 0 to
> > UINT16_MAX.
> >
> > In the example, the platform SCMI device uses optee interrupt irq 5
> > as async signal to trigger processing of an async incoming SCMI message,
> > in the scope of a CPU DVFS control. A platform can have several SCMI
> > channels driven this way. Optee irqs also permits small embedded devices
> > to share e.g. a gpio expander, a group of wakeup sources, etc... between
> > OP-TEE world (for sensitive services) and Linux world (for non-sensitive
> > services). The physical controller is driven from the TEE which exposes
> > some controls to Linux kernel.
> >
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Sumit Garg <sumit.garg@linaro.org>
> >
> > Co-developed-by: Pascal Paillet <p.paillet@foss.st.com>
> > Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since v1:
> > - Added a description to #interrupt-cells property.
> > - Changed of example. Linux wakeup event was subject to discussion and
> >   i don't know much about input events in Linux. So move to SCMI.
> >   In the example, an SCMI server in OP-TEE world raises optee irq 5
> >   so that Linux scmi optee channel &scmi_cpu_dvfs pushed in the incoming
> >   SCMI message in the scmi device for liekly later processing in threaded
> >   context. The example includes all parties: optee, scmi, sram, gic.
> > - Obviously rephrased the commit message.
> > - Added Cc: tags
> > ---
> >  .../arm/firmware/linaro,optee-tz.yaml         | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > index d4dc0749f9fd..9c00c27f8b2c 100644
> > --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> > @@ -40,6 +40,14 @@ properties:
> >        HVC #0, register assignments
> >        register assignments are specified in drivers/tee/optee/optee_smc.h
> >
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +    description: |
> > +      OP-TEE exposes irq for irp chip controllers from OP-TEE world. Each
> > +      irq is assigned a single line number identifier used as first argument.
> > +
> >  required:
> >    - compatible
> >    - method
> > @@ -64,3 +72,62 @@ examples:
> >              method = "hvc";
> >          };
> >      };
> > +
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    firmware  {
> > +        optee: optee {
> > +            compatible = "linaro,optee-tz";
> > +            method = "smc";
> > +            interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> > +            interrupt-controller;
> > +            #interrupt-cells = <1>;
> > +        };
> > +
> > +        scmi {
> > +            compatible = "linaro,scmi-optee";
> > +            linaro,optee-channel-id = <0>;
> > +            interrupt-parent = <&gic>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            scmi_cpu_dvfs: protocol@13 {
> > +                reg = <0x13>;
> > +                linaro,optee-channel-id = <1>;
> > +                shmem = <&scmi_shm_tx>, <&scmi_shm_rx>;
> > +                interrupts-extended = <&optee 5>;
>
> Just curious if this can discovered by some communication within OPTEE.
> You know you are using optee-channel-id 0 for all SCMI and 1 for DVFS.
> Is it not possible to get the information from OPTEE dynamically like
> you do for shmem. It is offset within the notification bitmap IIUC, so
> the question is can be get that from the firmware on the fly. It also
> gives the firmware to reshuffle things around if needed and don't have to
> worry about compatibility with DT ?

There is not specific discussions with OP-TEE to know which
Yet, assuming we have that scmi channel notif ID from optee, how does
scmi/optee driver relates to optee device irqchip?
With DT, we get both the optee irqchip ref (phandle) and our channel
notif irq line number (phandle arg #1).
Without DT, we still need to reference optee's irqchip.

Best regards,
etienne



>
> --
> Regards,
> Sudeep