Re: [PATCH v5 02/10] dt-bindings: mailbox: Add mboxes property for CMDQ secure driver

[Jason-JH Lin (林睿祥)]

On Thu, 2024-04-04 at 15:52 +0100, Conor Dooley wrote:
> On Thu, Apr 04, 2024 at 04:31:06AM +0000, Jason-JH Lin (林睿祥) wrote:
> > Hi Conor,
> > 
> > Thanks for the reviews.
> > 
> > On Wed, 2024-04-03 at 16:46 +0100, Conor Dooley wrote:
> > > On Wed, Apr 03, 2024 at 06:25:54PM +0800, Shawn Sung wrote:
> > > > From: "Jason-JH.Lin" <jason-jh.lin@xxxxxxxxxxxx>
> > > > 
> > > > Add mboxes to define a GCE loopping thread as a secure irq
> > > > handler.
> > > > This property is only required if CMDQ secure driver is
> > > > supported.
> > > > 
> > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
> > > > Signed-off-by: Hsiao Chien Sung <shawn.sung@xxxxxxxxxxxx>
> > > > ---
> > > >  .../bindings/mailbox/mediatek,gce-mailbox.yaml         | 10
> > > > ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > > mailbox.yaml
> > > > b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > > mailbox.yaml
> > > > index cef9d76013985..c0d80cc770899 100644
> > > > --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > > mailbox.yaml
> > > > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > > mailbox.yaml
> > > > @@ -49,6 +49,16 @@ properties:
> > > >      items:
> > > >        - const: gce
> > > >  
> > > > +  mediatek,gce-events:
> > > > +    description:
> > > > +      The event id which is mapping to the specific hardware
> > > > event
> > > > signal
> > > > +      to gce. The event id is defined in the gce header
> > > > +      include/dt-bindings/gce/<chip>-gce.h of each chips.
> > > 
> > > Missing any info here about when this should be used, hint - you
> > > have
> > > it
> > > in the commit message.
> > > 
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32-arrayi
> > > 
> > > Why is the ID used by the CMDQ service not fixed for each SoC?
> > > 
> > 
> > I forgot to sync with Shawn about this:
> > https://lore.kernel.org/all/20240124011459.12204-1-jason-
> > jh.lin@xxxxxxxxxxxx
> > 
> > I'll fix it at the next version.
> 
> When I say "fixed" I don't mean "this is wrong, please fix it", I
> mean
> "why is the value not static for a particular SoC". This needs to be
> explained in the patch (and the description for the event here needs
> to
> explain what the gce-mailbox is reserving an event for).
> 
Oh, I see. Thanks for noticing me.

We do want to reserve a static event ID for gce-mailbox to different
SoCs. There are 2 mainly reasons to why we set it in DTS:
1. 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.

2. If we defined the event ID in DTS, we might know how many SW or HW
event IDs are used.
If someone wants to use a new event ID for a new feature, they could
find out the used event IDs in DTS easily and avoid the event ID
conflicting.

The reason why we define a event ID is we want to get a SW signal from
secure world. We design a GCE looping thread in gce-mailbox driver to
wait for the GCE execute done event for each cmdq secure packets from
secure world.

Regards,
Jason-JH.Lin

> Thanks,
> Conor.