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@xxxxxxxxxx> wrote:
>
> On Wed, Jan 25, 2023 at 3:00 PM Etienne Carriere
> <etienne.carriere@xxxxxxxxxx> wrote:
> >
> > On Wed, 25 Jan 2023 at 21:22, Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>
> > > > Cc: Marc Zyngier <maz@xxxxxxxxxx>
> > > > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > > > Cc: Sumit Garg <sumit.garg@xxxxxxxxxx>
> > > >
> > > > Co-developed-by: Pascal Paillet <p.paillet@xxxxxxxxxxx>
> > > > Signed-off-by: Pascal Paillet <p.paillet@xxxxxxxxxxx>
> > > > Signed-off-by: Etienne Carriere <etienne.carriere@xxxxxxxxxx>
> > > > ---
> > > > 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