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

[Jason-JH Lin (林睿祥)]

On Fri, 2024-04-05 at 17:13 +0100, Conor Dooley wrote:
> On Fri, Apr 05, 2024 at 02:33:14PM +0000, Jason-JH Lin (林睿祥) wrote:
> > 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.
> 
> That's not a problem, we have compatibles for this purpose.

I agree that compatibles can do the same things.

> 
> > 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.
> 
> Are the event IDs not documented in the reference manual for the SoC
> in
> question? Or in documentation for the secure world for these devices?
> A
> DTS should not be the authoritive source for this information for
> developers.
> 
The event IDs were defined in:
inculde/dt-bindings/mailbox/mediatek,mt8188-gce.h.

> Additionally, the driver could very easily detect if someone does
> happen
> to put in the reserved ID. That could be generically useful (IOW,
> check
> all of them for re-use) if the ID are to not allowed to be shared.
> 
> > 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.
> 
> This sort of information needs to be in the commit message, but I
> don't
> think this property is needed at all since it seems to be something
> detectable from the compatible.

I think put this event ID in driver data and distinguish them by
different compatibles can achieve the same thing.

However, I originally thought that align to the existing way like
MUTEX, CCORR, WDMA in 
https://lore.kernel.org/all/20240124011459.12204-4-jason-jh.lin@xxxxxxxxxxxx
 would be better choice.
I think their usage of gce-events are the same. 

What do you think?

Regards,
Jason-JH.Lin