Message-ID: <3541454c6f60b8a16f4930dd91173ed24c9b5fd5.camel@mediatek.com>
Date: Tue, 28 May 2024 14:38:46 +0000
From: Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>
To: "conor@...nel.org" <conor@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	Singo Chang (張興國) <Singo.Chang@...iatek.com>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	Jason-ch Chen (陳建豪)
	<Jason-ch.Chen@...iatek.com>, "chunkuang.hu@...nel.org"
	<chunkuang.hu@...nel.org>, Shawn Sung (宋孝謙)
	<Shawn.Sung@...iatek.com>, Nancy Lin (林欣螢)
	<Nancy.Lin@...iatek.com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	Project_Global_Chrome_Upstream_Group
	<Project_Global_Chrome_Upstream_Group@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "krzysztof.kozlowski+dt@...aro.org"
	<krzysztof.kozlowski+dt@...aro.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "jassisinghbrar@...il.com"
	<jassisinghbrar@...il.com>, "angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH RESEND,v6 2/8] dt-bindings: mailbox: Add property for CMDQ
 secure driver

Hi Conor,

On Mon, 2024-05-27 at 18:36 +0100, Conor Dooley wrote:
> On Sun, May 26, 2024 at 10:44:37PM +0800, Jason-JH.Lin wrote:
> > 1. Add mboxes property to define a GCE loopping thread as a secure
> > IRQ
> > handler.
> > The CMDQ secure driver requests a mbox channel and sends a looping
> > command to the GCE thread. The looping command will wait for a
> > secure
> > packet done event signal from secure world and then jump back to
> > the
> > first instuction. Each time it waits for an event, it notifies the
> > CMDQ driver to perform the same action as the IRQ handler.
> > 
> > 2. Add gce-events property from gce-props.yaml to define a
> > secure packet done signal in secure world.
> > There are 1024 events IDs for GCE to use to execute instructions in
> > the specific event happened. These events could be signaled by HW
> > or SW
> > and their value would be different in different SoC because of HW
> > event
> > IDs distribution range from 0 to 1023.
> > If we set a static event ID: 855 for mt8188, it might be conflict
> > the
> > event ID original set in mt8195.
> 
> Two different SoCs, two different compatibles, no problem.
> I'm almost certain you previously told me that the firmware changing
> could result in a different event ID, but I see no mention of that
> here.

Yes, it could be, but we don't use it that way.

> The commit messages makes it seem like this can be determined by the
> compatible, so either give me a commit message that explains why the
> compatible is not sufficient or drop the patch.
> 

Yes, this can be determined by compatible in CMDQ mailbox driver,
so I think it's possible to put this in the CMDQ mailbox driver data
and configure by different SoC.

The problem is these events are defined in include/dt-
bindings/mailbox/mediatek,mt8188-gce.h and include/dt-
bindings/gce/mt8195-gce.h.
I can only use them in their mt8188.dts or mt8195.dts.

If I want to use the same event define in 2 different headers in the
same CMDQ mailbox driver, I think I just can parse their dts to get the
corresponding one.
Do you know how to generally deal with this problem?
Or I can just use the number of event ID in driver data without the
event define in dt-bindings header.

> > So we define an event ID that will be set when GCE runs to the end
> > of
> > secure cmdq packet in the secure world.
> > 
> > This can reduce the latency of software communication between
> > normal
> > world and secure world. In addition, we can also remove the complex
> > logic after the secure packet done in the secure world.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@...iatek.com>
> > Signed-off-by: Hsiao Chien Sung <shawn.sung@...iatek.com>
> > ---
> >  .../devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml | 8
> > +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > mailbox.yaml
> > b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > mailbox.yaml
> > index cef9d7601398..6e5e848d61d9 100644
> > --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > mailbox.yaml
> > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > mailbox.yaml
> > @@ -49,6 +49,10 @@ properties:
> >      items:
> >        - const: gce
> >  
> > +  mboxes:
> > +    items:
> > +      - description: GCE looping thread as a secure IRQ handler
> 
> Why is this needed? It's going to be a reference to itself, right?
> Why can't you just reserve a channel in the driver?
> 
I think you're right.
CMDQ mailbox driver can reserved a channel itself and it will know
which channel has occupied.
CMDQ users will request fail if they want to use the reserved channel.

I'll drop this and move it into CMDQ mailbox driver data for mt8188 and
mt8195.

Regards,
Jason-JH.Lin

> Thanks,
> Conor.
> 
> > +
> >  required:
> >    - compatible
> >    - "#mbox-cells"
> > @@ -57,6 +61,8 @@ required:
> >    - clocks
> >  
> >  allOf:
> > +  - $ref: /schemas/mailbox/mediatek,gce-props.yaml#
> > +
> >    - if:
> >        not:
> >          properties:
> > @@ -67,7 +73,7 @@ allOf:
> >        required:
> >          - clock-names
> >  
> > -additionalProperties: false
> > +unevaluatedProperties: false
> >  
> >  examples:
> >    - |
> > -- 
> > 2.18.0
> > 

Powered by blists - more mailing lists