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

[Jason-JH Lin (林睿祥)]

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@xxxxxxxxxxxx>
> > ---
> >  .../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
> > 
>