2023-05-06 07:54:45

by Etienne Carriere

[permalink] [raw]
Subject: [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties

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 <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Sumit Garg <[email protected]>
Co-developed-by: Pascal Paillet <[email protected]>
Signed-off-by: Pascal Paillet <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
---
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


2023-05-16 06:17:52

by Etienne Carriere

[permalink] [raw]
Subject: Re: [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties

Dear all,

On Sat, 6 May 2023 at 09:33, Etienne Carriere
<[email protected]> 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 <[email protected]>
> Cc: Krzysztof Kozlowski <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Sumit Garg <[email protected]>
> Co-developed-by: Pascal Paillet <[email protected]>
> Signed-off-by: Pascal Paillet <[email protected]>
> Signed-off-by: Etienne Carriere <[email protected]>
> ---

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
>

2023-05-16 08:35:13

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties

On 16/05/2023 08:02, Etienne Carriere wrote:
> Dear all,
>
> On Sat, 6 May 2023 at 09:33, Etienne Carriere
> <[email protected]> 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 <[email protected]>
>> Cc: Krzysztof Kozlowski <[email protected]>
>> Cc: Marc Zyngier <[email protected]>
>> Cc: Rob Herring <[email protected]>
>> Cc: Sumit Garg <[email protected]>
>> Co-developed-by: Pascal Paillet <[email protected]>
>> Signed-off-by: Pascal Paillet <[email protected]>
>> Signed-off-by: Etienne Carriere <[email protected]>
>> ---
>
> Any feedback on this change proposal?

Rob had here several comments, so I will defer it to him.

I don't get why this is not part of linaro,scmi-optee driver directly. I
think it's the only valid use case because the others like GPIO
expanders seem a stretch.


Best regards,
Krzysztof


2023-06-08 18:21:00

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v5 1/2] dt-bindings: arm: optee: add interrupt controller properties

+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 <[email protected]>
> Cc: Krzysztof Kozlowski <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Sumit Garg <[email protected]>
> Co-developed-by: Pascal Paillet <[email protected]>
> Signed-off-by: Pascal Paillet <[email protected]>
> Signed-off-by: Etienne Carriere <[email protected]>
> ---
> 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
>