diff mbox series

[01/15] dt-bindings: mailbox: Add property for CMDQ secure driver

Message ID 20230918192204.32263-2-jason-jh.lin@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Add CMDQ secure driver for SVP | expand

Commit Message

Jason-JH.Lin Sept. 18, 2023, 7:21 p.m. UTC
Add mboxes to define a GCE loopping thread as a secure irq handler.
Add mediatek,event to define a GCE software event siganl as a secure
irq.

These 2 properties are required for CMDQ secure driver.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 .../mailbox/mediatek,gce-mailbox.yaml         | 30 +++++++++++++++----
 1 file changed, 24 insertions(+), 6 deletions(-)

Comments

Rob Herring Sept. 19, 2023, 4:46 p.m. UTC | #1
On Tue, Sep 19, 2023 at 03:21:50AM +0800, Jason-JH.Lin wrote:
> Add mboxes to define a GCE loopping thread as a secure irq handler.
> Add mediatek,event to define a GCE software event siganl as a secure
> irq.
> 
> These 2 properties are required for CMDQ secure driver.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  .../mailbox/mediatek,gce-mailbox.yaml         | 30 +++++++++++++++----
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> index cef9d7601398..5c9aebe83d2d 100644
> --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> @@ -49,6 +49,21 @@ properties:
>      items:
>        - const: gce
>  
> +  mboxes:
> +    description:
> +      A mailbox channel used as a secure irq handler in normal world.
> +      Using mailbox to communicate with GCE to setup looping thread,
> +      it should have this property and a phandle, mailbox specifiers.

All cases of 'mboxes' have a phandle and specifiers. No need to repeat 
that here.

> +    $ref: /schemas/types.yaml#/definitions/phandle-array

Already has a type definition too. You need to define how many entries 
and what each entry is if more than one. IOW, the same thing as clocks, 
resets, interrupts, etc.

> +
> +  mediatek,gce-events:

This is used all over. It really needs a single definition which is then 
referenced by the users.

> +    description:
> +      The event id which is mapping to a software event signal to gce.
> +      It is used as a secure irq for every secure gce threads.
> +      The event id is defined in the gce header
> +      include/dt-bindings/mailbox/mediatek,<chip>-gce.h of each chips.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +
>  required:
>    - compatible
>    - "#mbox-cells"
> @@ -71,20 +86,23 @@ additionalProperties: false
>  
>  examples:
>    - |
> -    #include <dt-bindings/clock/mt8173-clk.h>
> +    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
>      #include <dt-bindings/interrupt-controller/arm-gic.h>
>      #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/mailbox/mediatek,mt8188-gce.h>
>  
>      soc {
>          #address-cells = <2>;
>          #size-cells = <2>;
>  
> -        gce: mailbox@10212000 {
> -            compatible = "mediatek,mt8173-gce";
> -            reg = <0 0x10212000 0 0x1000>;
> -            interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
> +        gce0: mailbox@10320000 {
> +            compatible = "mediatek,mt8188-gce";

Are these new properties only for mt8188? If so, then you need a schema 
saying that. If not, then this is an unnecessary change to the example.

> +            reg = <0 0x10320000 0 0x4000>;
> +            interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH 0>;
>              #mbox-cells = <2>;
> -            clocks = <&infracfg CLK_INFRA_GCE>;
> +            clocks = <&infracfg_ao CLK_INFRA_AO_GCE>;
>              clock-names = "gce";
> +            mboxes = <&gce0 15 CMDQ_THR_PRIO_1>;

The provider is also a consumer?

> +            mediatek,gce-events = <CMDQ_SYNC_TOKEN_SECURE_THR_EOF>;
>          };
>      };
> -- 
> 2.18.0
>
Jason-JH.Lin Sept. 21, 2023, 3:12 p.m. UTC | #2
Hi Rob,

Thanks for the reviews.

On Tue, 2023-09-19 at 11:46 -0500, Rob Herring wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Tue, Sep 19, 2023 at 03:21:50AM +0800, Jason-JH.Lin wrote:
> > Add mboxes to define a GCE loopping thread as a secure irq handler.
> > Add mediatek,event to define a GCE software event siganl as a
> secure
> > irq.
> > 
> > These 2 properties are required for CMDQ secure driver.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  .../mailbox/mediatek,gce-mailbox.yaml         | 30
> +++++++++++++++----
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> > 
> > diff --git
> a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml 
> b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> > index cef9d7601398..5c9aebe83d2d 100644
> > --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> mailbox.yaml
> > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> mailbox.yaml
> > @@ -49,6 +49,21 @@ properties:
> >      items:
> >        - const: gce
> >  
> > +  mboxes:
> > +    description:
> > +      A mailbox channel used as a secure irq handler in normal
> world.
> > +      Using mailbox to communicate with GCE to setup looping
> thread,
> > +      it should have this property and a phandle, mailbox
> specifiers.
> 
> All cases of 'mboxes' have a phandle and specifiers. No need to
> repeat 
> that here.

OK, I'll remove it.

> 
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> Already has a type definition too. You need to define how many
> entries 
> and what each entry is if more than one. IOW, the same thing as
> clocks, 
> resets, interrupts, etc.

OK, I'll add the maximum number to 1 for this.

> 
> > +
> > +  mediatek,gce-events:
> 
> This is used all over. It really needs a single definition which is
> then 
> referenced by the users.

OK, I think it would defined it here since it is a GCE HW event signal.
I'll try to reference to other modules and make a definition for it.

> 
> > +    description:
> > +      The event id which is mapping to a software event signal to
> gce.
> > +      It is used as a secure irq for every secure gce threads.
> > +      The event id is defined in the gce header
> > +      include/dt-bindings/mailbox/mediatek,<chip>-gce.h of each
> chips.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +
> >  required:
> >    - compatible
> >    - "#mbox-cells"
> > @@ -71,20 +86,23 @@ additionalProperties: false
> >  
> >  examples:
> >    - |
> > -    #include <dt-bindings/clock/mt8173-clk.h>
> > +    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
> >      #include <dt-bindings/interrupt-controller/arm-gic.h>
> >      #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/mailbox/mediatek,mt8188-gce.h>
> >  
> >      soc {
> >          #address-cells = <2>;
> >          #size-cells = <2>;
> >  
> > -        gce: mailbox@10212000 {
> > -            compatible = "mediatek,mt8173-gce";
> > -            reg = <0 0x10212000 0 0x1000>;
> > -            interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
> > +        gce0: mailbox@10320000 {
> > +            compatible = "mediatek,mt8188-gce";
> 
> Are these new properties only for mt8188? If so, then you need a
> schema 
> saying that. If not, then this is an unnecessary change to the
> example.

No I think all SoC can add these properties if they have secure
requirement as mt8188. So I'll revert this unnecessary change to the
example.

> 
> > +            reg = <0 0x10320000 0 0x4000>;
> > +            interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH 0>;
> >              #mbox-cells = <2>;
> > -            clocks = <&infracfg CLK_INFRA_GCE>;
> > +            clocks = <&infracfg_ao CLK_INFRA_AO_GCE>;
> >              clock-names = "gce";
> > +            mboxes = <&gce0 15 CMDQ_THR_PRIO_1>;
> 
> The provider is also a consumer?

We'll use a mbox channel for receiving GCE signal in the secure mailbox
driver. So I think it is a provider and also a consumer.

Regards,
Jason-JH.Lin

> 
> > +            mediatek,gce-events =
> <CMDQ_SYNC_TOKEN_SECURE_THR_EOF>;
> >          };
> >      };
> > -- 
> > 2.18.0
> > 
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
index cef9d7601398..5c9aebe83d2d 100644
--- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
@@ -49,6 +49,21 @@  properties:
     items:
       - const: gce
 
+  mboxes:
+    description:
+      A mailbox channel used as a secure irq handler in normal world.
+      Using mailbox to communicate with GCE to setup looping thread,
+      it should have this property and a phandle, mailbox specifiers.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
+  mediatek,gce-events:
+    description:
+      The event id which is mapping to a software event signal to gce.
+      It is used as a secure irq for every secure gce threads.
+      The event id is defined in the gce header
+      include/dt-bindings/mailbox/mediatek,<chip>-gce.h of each chips.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
 required:
   - compatible
   - "#mbox-cells"
@@ -71,20 +86,23 @@  additionalProperties: false
 
 examples:
   - |
-    #include <dt-bindings/clock/mt8173-clk.h>
+    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
     #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/mailbox/mediatek,mt8188-gce.h>
 
     soc {
         #address-cells = <2>;
         #size-cells = <2>;
 
-        gce: mailbox@10212000 {
-            compatible = "mediatek,mt8173-gce";
-            reg = <0 0x10212000 0 0x1000>;
-            interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
+        gce0: mailbox@10320000 {
+            compatible = "mediatek,mt8188-gce";
+            reg = <0 0x10320000 0 0x4000>;
+            interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH 0>;
             #mbox-cells = <2>;
-            clocks = <&infracfg CLK_INFRA_GCE>;
+            clocks = <&infracfg_ao CLK_INFRA_AO_GCE>;
             clock-names = "gce";
+            mboxes = <&gce0 15 CMDQ_THR_PRIO_1>;
+            mediatek,gce-events = <CMDQ_SYNC_TOKEN_SECURE_THR_EOF>;
         };
     };