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
