| Message ID | 20230918192204.32263-2-jason-jh.lin@mediatek.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add CMDQ secure driver for SVP | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 >
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 --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>; }; };
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(-)