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
> >
>
