* [PATCH v5 2/2] optee: multiplex tee interrupt events on optee async notif irq
2023-05-06 7:32 [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties Etienne Carriere
@ 2023-05-06 7:32 ` Etienne Carriere
2023-05-16 6:02 ` [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties Etienne Carriere
2023-06-08 17:43 ` Rob Herring
2 siblings, 0 replies; 5+ messages in thread
From: Etienne Carriere @ 2023-05-06 7:32 UTC (permalink / raw)
To: linux-kernel
Cc: devicetree, op-tee, Alexandre Belloni, Ulf Hansson,
Jonathan Cameron, Etienne Carriere, Jens Wiklander, Marc Zyngier,
Sumit Garg, Pascal Paillet, Fabrice Gasnier
Implements an irqchip in optee driver for the interrupt events notified
from OP-TEE world to the Linux kernel. Optee registers up to 1 interrupt
controller and identifies each line with a line number from 0 to
UINT16_MAX. The interrupt line identifiers and their meaning are
specific to the platform and shall be found in the OP-TEE platform
documentation
There already exists an optee asynchronous notification mechanism using
an irq for OP-TEE to signal to Linux kernel optee driver of a pending
asynchronous events. This existing implementation binds each notified
event to the awaking of a thread waiting to call the TEE. The optee
irqchip service added by this change uses that same irq for
optee interrupt notification.
When optee driver initializes, TEE tells whether it supports interrupt
notification services or not upon what optee driver registers or not
its irqchip controller.
OP-TEE SMC ABI defines 4 new SMC function IDs for non-secure world to
manage optee interrupt events.
Fastcall SMC funcID OPTEE_SMC_GET_NOTIF_ITR allows non-secure world to
retrieve pending interrupts by grapes up to 5 lines. For efficiency,
the function also reports whether there are pending async values
targeting suspended threaded sequences execution and whether TEE has
background threaded work to do.
Fastcall SMC funcID OPTEE_SMC_NOTIF_ITR_SET_MASK allows a Linux kernel
optee irq consumer to mask/unmask the irq line.
Yielded SMC funcID OPTEE_SMC_NOTIF_ITR_SET_STATE allows a Linux kernel
optee irq consumer to enable and disable the irq line
Yielded SMC funcID OPTEE_SMC_NOTIF_ITR_SET_WAKEUP allows a Linux kernel
optee irq consumer to enable and disable the wakeup from low power
capability of the interrupt.
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 v4:
- Resolved rebase conflicts since integration in v6.3-rc3 of
commit b3b4ced12c1b ("optee: add per cpu asynchronous notification").
- Slightly rephrased commit message.
Changes since v3:
- Removed OP-TEE PTA service for interrup enable/wakeup config and
replaced with OP-TEE standard SMC funcion IDs
OPTEE_SMC_NOTIF_ITR_SET_STATE and OPTEE_SMC_NOTIF_ITR_SET_WAKEUP.
With this update, patch 3/3 of v3 is no more needed.
Changes since v2:
- Renamed it_notif/IT_NOTIF to itr_notif/ITR_NOTIF.
- Updated retrieve_pending_irqs() loop from review comments.
- Changed OP-TEE ABI function CONFIGURE_IT to a only act on interrupt
mask state. The function is renamed xxx_NOTIF_ITR_SET_MASK.
- Don't set OPTEE_SMC_SEC_CAP_IT_NOTIF among non-secure capabilities, the
capability is related only to whether secure world supports interrupt
notification to normal world.
- Fixed inline description from review comments.
- Rephrased commit message and added a sentence about optee irq line
number values and meaning.
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 | 2 +
drivers/tee/optee/optee_smc.h | 133 ++++++++++++++++++++-
drivers/tee/optee/smc_abi.c | 186 +++++++++++++++++++++++++++++-
3 files changed, 315 insertions(+), 6 deletions(-)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 72685ee0d53f..3406c6c8ea30 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -113,6 +113,7 @@ struct optee_pcpu {
* @notif_pcpu_wq workqueue for per cpu asynchronous notification or NULL
* @notif_pcpu_work work for per cpu asynchronous notification
* @notif_cpuhp_state CPU hotplug state assigned for pcpu interrupt management
+ * @domain interrupt domain registered by OP-TEE driver
*/
struct optee_smc {
optee_invoke_fn *invoke_fn;
@@ -123,6 +124,7 @@ struct optee_smc {
struct workqueue_struct *notif_pcpu_wq;
struct work_struct notif_pcpu_work;
unsigned int notif_cpuhp_state;
+ struct irq_domain *domain;
};
/**
diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
index 7d9fa426505b..73160379c738 100644
--- a/drivers/tee/optee/optee_smc.h
+++ b/drivers/tee/optee/optee_smc.h
@@ -250,7 +250,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:
@@ -278,6 +279,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_ITR_NOTIF BIT(7)
+
+#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_ITR_MASK GENMASK(23, 8)
+#define OPTEE_SMC_SEC_CAP_MAX_NOTIF_ITR_SHIFT 8
#define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
#define OPTEE_SMC_EXCHANGE_CAPABILITIES \
@@ -450,6 +456,131 @@ 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 events notified by OP-TEE world,
+ * whether bottom half is to be scheduled and if there are pending
+ * async event for waiting threads, all this since the last call of
+ * this function.
+ *
+ * Interrupts notified by OP-TEE are identified by a number from 0 to
+ * the interrupt number max value for that platform. Values for each
+ * interrupt number are platform specific and shall be found in the
+ * OP-TEE platform documentation.
+ *
+ * OP-TEE keeps a record of all posted interrupt notification events.
+ * When the async notif interrupt is received by normal 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. When do bottom half event
+ * is retrieved (async value 0), it is also cleared from its related
+ * 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_ITR
+ * 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_ITR_PENDING if other interrupt(s) are pending
+ * Bit[9]: OPTEE_SMC_NOTIF_ASYNC_PENDING if a threaded event is pending
+ * excluding bottom half notification that is retrieved in Bit[10].
+ * Bit[10]: OPTEE_SMC_NOTIF_DO_BOTTOM_HALF if retrieved bottom half notif
+ * Bit[15:11]: Reserved for future 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_ITR_COUNT_MASK GENMASK(7, 0)
+#define OPTEE_SMC_NOTIF_ITR_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_ITR 20
+#define OPTEE_SMC_GET_NOTIF_ITR \
+ OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_NOTIF_ITR)
+
+/*
+ * Mask/unmask an interrupt notification
+ *
+ * Call requests usage:
+ * a0 SMC Function ID, OPTEE_SMC_NOTIF_ITR_SET_MASK
+ * a1 Interrupt number identifier value
+ * a2 1 to mask, 0 to unmask the interrupt notification.
+ * a3-6 Reserved for future use, MBZ
+ * a7 Hypervisor Client ID register
+ *
+ * Normal return register usage:
+ * a0 OPTEE_SMC_RETURN_OK
+ * a1-7 Preserved
+ *
+ * Invalid command with provided arguments return usage:
+ * a0 OPTEE_SMC_RETURN_EBADCMD
+ * a1-7 Preserved
+ *
+ * Not supported return register usage:
+ * a0 OPTEE_SMC_RETURN_ENOTAVAIL
+ * a1-7 Preserved
+ */
+#define OPTEE_SMC_FUNCID_NOTIF_ITR_SET_MASK 21
+#define OPTEE_SMC_NOTIF_ITR_SET_MASK \
+ OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_NOTIF_ITR_SET_MASK)
+
+/*
+ * Enable/disable an interrupt notification
+ *
+ * Call requests usage:
+ * a0 SMC Function ID, OPTEE_SMC_NOTIF_ITR_SET_STATE
+ * a1 Interrupt number identifier value
+ * a2 1 to enable, 0 to disable the interrupt notification
+ * a3-6 Reserved for future use, MBZ
+ * a7 Hypervisor Client ID register
+ *
+ * Normal return register usage:
+ * a0 OPTEE_SMC_RETURN_OK
+ * a1-7 Preserved
+ *
+ * Invalid command with provided arguments return usage:
+ * a0 OPTEE_SMC_RETURN_EBADCMD
+ * a1-7 Preserved
+ */
+#define OPTEE_SMC_FUNCID_NOTIF_ITR_SET_STATE 22
+#define OPTEE_SMC_NOTIF_ITR_SET_STATE \
+ OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_NOTIF_ITR_SET_STATE)
+
+/*
+ * Enable/disable the wake up from low power feature of an interrupt event
+ *
+ * Call requests usage:
+ * a0 SMC Function ID, OPTEE_SMC_NOTIF_ITR_SET_WAKEUP
+ * a1 Interrupt number identifier value
+ * a2 1 to enable, 0 to disable the interrupt wake up capability
+ * a3-6 Reserved for future use, MBZ
+ * a7 Hypervisor Client ID register
+ *
+ * Normal return register usage:
+ * a0 OPTEE_SMC_RETURN_OK
+ * a1-7 Preserved
+ *
+ * Invalid command with provided arguments return usage:
+ * a0 OPTEE_SMC_RETURN_EBADCMD
+ * a1-7 Preserved
+ */
+#define OPTEE_SMC_FUNCID_NOTIF_ITR_SET_WAKEUP 23
+#define OPTEE_SMC_NOTIF_ITR_SET_WAKEUP \
+ OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_NOTIF_ITR_SET_WAKEUP)
+
/*
* 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 49702cb08f4f..e383811a5f6d 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -997,6 +997,99 @@ static int optee_smc_stop_async_notif(struct tee_context *ctx)
* 5. Asynchronous notification
*/
+static void optee_itr_notif_mask(struct irq_data *d)
+{
+ struct optee *optee = d->domain->host_data;
+ struct arm_smccc_res res = { };
+
+ optee->smc.invoke_fn(OPTEE_SMC_NOTIF_ITR_SET_MASK, d->hwirq, 1,
+ 0, 0, 0, 0, 0, &res);
+}
+
+static void optee_itr_notif_unmask(struct irq_data *d)
+{
+ struct optee *optee = d->domain->host_data;
+ struct arm_smccc_res res = { };
+
+ optee->smc.invoke_fn(OPTEE_SMC_NOTIF_ITR_SET_MASK, d->hwirq, 0,
+ 0, 0, 0, 0, 0, &res);
+}
+
+static void optee_itr_notif_enable(struct irq_data *d)
+{
+ struct optee *optee = d->domain->host_data;
+ struct arm_smccc_res res;
+
+ optee->smc.invoke_fn(OPTEE_SMC_NOTIF_ITR_SET_STATE, d->hwirq, 1,
+ 0, 0, 0, 0, 0, &res);
+}
+
+static void optee_itr_notif_disable(struct irq_data *d)
+{
+ struct optee *optee = d->domain->host_data;
+ struct arm_smccc_res res;
+
+ optee->smc.invoke_fn(OPTEE_SMC_NOTIF_ITR_SET_STATE, d->hwirq, 0,
+ 0, 0, 0, 0, 0, &res);
+}
+
+static int optee_itr_notif_set_wake(struct irq_data *d, unsigned int on)
+{
+ struct optee *optee = d->domain->host_data;
+ struct arm_smccc_res res;
+
+ optee->smc.invoke_fn(OPTEE_SMC_NOTIF_ITR_SET_WAKEUP, d->hwirq, !!on,
+ 0, 0, 0, 0, 0, &res);
+
+ if (res.a0)
+ return -EINVAL;
+ return 0;
+}
+
+static struct irq_chip optee_irq_chip = {
+ .name = "optee-itr-notif",
+ .irq_mask = optee_itr_notif_mask,
+ .irq_unmask = optee_itr_notif_unmask,
+ .irq_disable = optee_itr_notif_disable,
+ .irq_enable = optee_itr_notif_enable,
+ .irq_set_wake = optee_itr_notif_set_wake,
+};
+
+static int optee_itr_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_irq_chip,
+ d->host_data);
+
+ return 0;
+}
+
+static const struct irq_domain_ops optee_irq_domain_ops = {
+ .alloc = optee_itr_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_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)
{
@@ -1011,6 +1104,61 @@ 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 itr_num)
+{
+ if (generic_handle_domain_irq(optee->smc.domain, itr_num)) {
+ struct arm_smccc_res res = { };
+
+ pr_err("No consumer for optee irq %u, masked\n", itr_num);
+ optee->smc.invoke_fn(OPTEE_SMC_NOTIF_ITR_SET_MASK, itr_num, 1,
+ 0, 0, 0, 0, 0, &res);
+ }
+}
+
+static void retrieve_pending_irqs(struct optee *optee, bool *async_pending,
+ bool *do_bottom_half)
+
+{
+ struct arm_smccc_res res;
+ bool irq_pending;
+ ssize_t cnt;
+ const unsigned int lsb_mask = GENMASK(15, 0);
+ const unsigned int msb_shift = 16;
+
+ *do_bottom_half = false;
+
+ do {
+ optee->smc.invoke_fn(OPTEE_SMC_GET_NOTIF_ITR, 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;
+
+ irq_pending = res.a1 & OPTEE_SMC_NOTIF_ITR_PENDING;
+ cnt = res.a1 & OPTEE_SMC_NOTIF_ITR_COUNT_MASK;
+ if (cnt > 5 || (!cnt && irq_pending)) {
+ WARN_ONCE(0, "Unexpected irq notif count %zi\n", cnt);
+ break;
+ }
+
+ if (cnt > 0)
+ forward_irq(optee, res.a1 >> msb_shift);
+ if (cnt > 1)
+ forward_irq(optee, res.a2 & lsb_mask);
+ if (cnt > 2)
+ forward_irq(optee, res.a2 >> msb_shift);
+ if (cnt > 3)
+ forward_irq(optee, res.a3 & lsb_mask);
+ if (cnt == 5)
+ forward_irq(optee, res.a3 >> msb_shift);
+ } while (irq_pending);
+
+ *async_pending = res.a1 & OPTEE_SMC_NOTIF_VALUE_PENDING;
+}
+
static irqreturn_t irq_handler(struct optee *optee)
{
bool do_bottom_half = false;
@@ -1018,9 +1166,15 @@ static irqreturn_t irq_handler(struct optee *optee)
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_ITR_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;
@@ -1028,7 +1182,7 @@ static irqreturn_t irq_handler(struct optee *optee)
do_bottom_half = true;
else
optee_notif_send(optee, value);
- } while (value_pending);
+ }
if (do_bottom_half)
return IRQ_WAKE_THREAD;
@@ -1165,6 +1319,9 @@ static void optee_smc_notif_uninit_irq(struct optee *optee)
irq_dispose_mapping(optee->smc.notif_irq);
}
+
+ if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_ITR_NOTIF)
+ irq_domain_remove(optee->smc.domain);
}
}
@@ -1320,6 +1477,7 @@ 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 {
@@ -1352,6 +1510,13 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
else
*rpc_param_count = 0;
+ if (*sec_caps & OPTEE_SMC_SEC_CAP_ITR_NOTIF)
+ *max_notif_it = (res.result.data &
+ OPTEE_SMC_SEC_CAP_MAX_NOTIF_ITR_MASK) >>
+ OPTEE_SMC_SEC_CAP_MAX_NOTIF_ITR_SHIFT;
+ else
+ *max_notif_it = 0;
+
return true;
}
@@ -1611,6 +1776,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;
@@ -1636,7 +1802,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;
@@ -1757,6 +1923,16 @@ static int optee_probe(struct platform_device *pdev)
irq_dispose_mapping(irq);
goto err_notif_uninit;
}
+
+ if (sec_caps & OPTEE_SMC_SEC_CAP_ITR_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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties
2023-05-06 7:32 [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties Etienne Carriere
2023-05-06 7:32 ` [PATCH v5 2/2] optee: multiplex tee interrupt events on optee async notif irq Etienne Carriere
@ 2023-05-16 6:02 ` Etienne Carriere
2023-05-16 8:03 ` Krzysztof Kozlowski
2023-06-08 17:43 ` Rob Herring
2 siblings, 1 reply; 5+ messages in thread
From: Etienne Carriere @ 2023-05-16 6:02 UTC (permalink / raw)
To: linux-kernel
Cc: devicetree, op-tee, Alexandre Belloni, Ulf Hansson,
Jonathan Cameron, Jens Wiklander, Krzysztof Kozlowski,
Marc Zyngier, Rob Herring, Sumit Garg, Pascal Paillet
Dear all,
On Sat, 6 May 2023 at 09:33, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Adds an optional interrupt controller property to optee firmware node
> in the DT bindings. Optee driver may embeds an irqchip exposing
> OP-TEE interrupt events 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.
>
> The identifiers and meaning of the interrupt line number are specific
> to the platform and shall be found in the OP-TEE platform documentation.
>
> In the example shown in optee DT binding documentation, the platform SCMI
> device controlled by Linux scmi driver uses optee interrupt irq 5 as
> signal to trigger processing of an asynchronous incoming SCMI message
> in the scope of a CPU DVFS control. A platform can have several SCMI
> channels driven this way. Optee irqs also permit 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>
> ---
Any feedback on this change proposal?
Regards,
Etienne
> Changes since v4:
> - Removed empty line between Cc: tags and S-o-b tags.
>
> No changes since v3
>
> Changes since v2:
> - Added a sentence on optee irq line number values and meaning, in
> DT binding doc and commit message.
> - Updated example in DT binding doc from comment, fixed misplaced
> interrupt-parent property and removed gic and sram shm nodes.
>
> 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.
> ---
> .../arm/firmware/linaro,optee-tz.yaml | 38 +++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> index 5d033570b57b..9d9a797a6b2f 100644
> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> @@ -41,6 +41,16 @@ 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.
> + Line number identifiers and their meaning shall be found in the OP-TEE
> + firmware platform documentation.
> +
> required:
> - compatible
> - method
> @@ -65,3 +75,31 @@ examples:
> method = "hvc";
> };
> };
> +
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + firmware {
> + optee: optee {
> + compatible = "linaro,optee-tz";
> + method = "smc";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + };
> +
> + 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>;
> + };
> + };
> + };
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties
2023-05-06 7:32 [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties Etienne Carriere
2023-05-06 7:32 ` [PATCH v5 2/2] optee: multiplex tee interrupt events on optee async notif irq Etienne Carriere
2023-05-16 6:02 ` [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties Etienne Carriere
@ 2023-06-08 17:43 ` Rob Herring
2 siblings, 0 replies; 5+ messages in thread
From: Rob Herring @ 2023-06-08 17:43 UTC (permalink / raw)
To: Etienne Carriere
Cc: linux-kernel, devicetree, op-tee, Alexandre Belloni, Ulf Hansson,
Jonathan Cameron, Jens Wiklander, Krzysztof Kozlowski,
Marc Zyngier, Sumit Garg, Pascal Paillet
+Sudeep, again...
Sudeep, you may want to look at v2 again.
On Sat, May 06, 2023 at 09:32:34AM +0200, Etienne Carriere wrote:
> Adds an optional interrupt controller property to optee firmware node
> in the DT bindings. Optee driver may embeds an irqchip exposing
> OP-TEE interrupt events notified by the TEE world. Optee registers up
optee, Optee, or OP-TEE?
> to 1 interrupt controller and identifies each line with a line
> number from 0 to UINT16_MAX.
>
> The identifiers and meaning of the interrupt line number are specific
> to the platform and shall be found in the OP-TEE platform documentation.
Why can't you standardize the interrupt numbering for standard events
like SCMI?
I think there's still concern as to why this can't all be discoverable.
The Optee driver should know or discover from Optee that it is an
interrupt controller and can register itself as such. Likewise, the
SCMI driver knows for scmi-optee, the interrupt comes from the Optee
node. It can find that node (by compatible) and then find the
irqchip/domain provider for that node.
OTOH, SCMI already supports interrupts in the binding, so this isn't
too big of a deal. I'm more concerned that once you have Optee
interrupts, then you are going to want a node for every Optee
sub-function with an interrupt. Then we're back to the very first Optee
binding with a child node for every sub-function. Using it for gpio-keys
as you did in the first version for example.
If Sudeep is okay with this from an SCMI perspective and Marc is from an
irqchip perspective, then I'm okay with it too.
> In the example shown in optee DT binding documentation, the platform SCMI
> device controlled by Linux scmi driver uses optee interrupt irq 5 as
> signal to trigger processing of an asynchronous incoming SCMI message
> in the scope of a CPU DVFS control. A platform can have several SCMI
> channels driven this way. Optee irqs also permit 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 v4:
> - Removed empty line between Cc: tags and S-o-b tags.
>
> No changes since v3
>
> Changes since v2:
> - Added a sentence on optee irq line number values and meaning, in
> DT binding doc and commit message.
> - Updated example in DT binding doc from comment, fixed misplaced
> interrupt-parent property and removed gic and sram shm nodes.
>
> 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.
> ---
> .../arm/firmware/linaro,optee-tz.yaml | 38 +++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> index 5d033570b57b..9d9a797a6b2f 100644
> --- a/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> +++ b/Documentation/devicetree/bindings/arm/firmware/linaro,optee-tz.yaml
> @@ -41,6 +41,16 @@ 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.
> + Line number identifiers and their meaning shall be found in the OP-TEE
> + firmware platform documentation.
> +
> required:
> - compatible
> - method
> @@ -65,3 +75,31 @@ examples:
> method = "hvc";
> };
> };
> +
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + firmware {
> + optee: optee {
> + compatible = "linaro,optee-tz";
> + method = "smc";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + };
> +
> + 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>;
> + };
> + };
> + };
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread