From: "Jason-JH.Lin" <[email protected]>
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 <[email protected]>
Signed-off-by: Hsiao Chien Sung <[email protected]>
---
.../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.
+ $ref: /schemas/types.yaml#/definitions/uint32-arrayi
+
+ mboxes:
+ maxItems: 1
+
required:
- compatible
- "#mbox-cells"
--
2.18.0
On Wed, 03 Apr 2024 18:25:54 +0800, Shawn Sung wrote:
> From: "Jason-JH.Lin" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Hsiao Chien Sung <[email protected]>
> ---
> .../bindings/mailbox/mediatek,gce-mailbox.yaml | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml:
Unresolvable JSON pointer: 'definitions/uint32-arrayi'
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On Wed, Apr 03, 2024 at 06:25:54PM +0800, Shawn Sung wrote:
> From: "Jason-JH.Lin" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Hsiao Chien Sung <[email protected]>
> ---
> .../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?
Cheers,
Conor
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" <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Hsiao Chien Sung <[email protected]>
> > ---
> > .../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-
[email protected]
I'll fix it at the next version.
Regards,
Jason-JH.Lin
> Cheers,
> Conor
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" <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Hsiao Chien Sung <[email protected]>
> > > ---
> > > .../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-
> [email protected]
>
> 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).
Thanks,
Conor.
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" <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Hsiao Chien Sung <[email protected]>
> > > > ---
> > > > .../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-
> > [email protected]
> >
> > 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.
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" <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > Signed-off-by: Hsiao Chien Sung <[email protected]>
> > > > > ---
> > > > > .../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-
> > > [email protected]
> > >
> > > 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.
> 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.
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.
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" <[email protected]>
> > > > > >
> > > > > > 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 <[email protected]>
> > > > > > Signed-off-by: Hsiao Chien Sung <[email protected]>
> > > > > > ---
> > > > > > .../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-
> > > > [email protected]
> > > >
> > > > 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/[email protected]
would be better choice.
I think their usage of gce-events are the same.
What do you think?
Regards,
Jason-JH.Lin
On Sat, Apr 06, 2024 at 04:15:51PM +0000, Jason-JH Lin (林睿祥) wrote:
> 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" <[email protected]>
> > > > > > >
> > > > > > > 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 <[email protected]>
> > > > > > > Signed-off-by: Hsiao Chien Sung <[email protected]>
> > > > > > > ---
> > > > > > > .../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-
> > > > > [email protected]
> > > > >
> > > > > 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/[email protected]
> would be better choice.
> I think their usage of gce-events are the same.
>
> What do you think?
To me it comes down to whether the IDs are fixed on a particular SoC (in
which case they can be deduced by the compatible) or not. I don't really
see how this is actually a fixed property of the SoC though, if you came
along tomorrow with a "gce-2.0" you could totally end up with different
numbering because (as far as I can tell) this numbering is actually a
property of the os-firmware interface, not actually a property of the
SoC itself. I was expecting you to say "no" when I asked if the IDs were
fixed for a given SoC because changing the firmware /could/ change the IDs.
Although, I think you'd likely not ever want to change them, because
that'd just be an annoying ABI break to deal with.
What I think is that you need to write a property description that
explains what the mailbox is using the gce channel for so that someone
can populate the property correctly. The commit message also needs to
explain why this is not a fixed value for a given SoC.
And yes, as you pointed out earlier in this thread, Shawn needs to
update this to have a reference to the gce-events binding which has a
great description in it of what a gce event is.
> I think their usage of gce-events are the same.
Also, just because a property got introduced doesn't mean that it is
correct and adding new instances can definitely by required to provide
justification, not just saying "it's used by xyz too".
Cheers,
Conor.
On Tue, 2024-04-09 at 18:52 +0100, Conor Dooley wrote:
> On Sat, Apr 06, 2024 at 04:15:51PM +0000, Jason-JH Lin (林睿祥) wrote:
> > 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" <[email protected]>
> > > > > > > >
> > > > > > > > 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 <[email protected]>
> > > > > > > > Signed-off-by: Hsiao Chien Sung <
> > > > > > > > [email protected]>
> > > > > > > > ---
> > > > > > > > .../bindings/mailbox/mediatek,gce-
> > > > > > > > mailbox.yaml |
> > > > > > > > 10
> > > > > > > > ++++++++++
> > > > > > > > 1 file changed, 10 insertions(+)
> > > > > > > >
> > > > > > > > diff --git
> > > > > > > > a/Documentation/devicetree/bindings/mailbox/mediatek,gc
> > > > > > > > e-
> > > > > > > > mailbox.yaml
> > > > > > > > b/Documentation/devicetree/bindings/mailbox/mediatek,gc
> > > > > > > > e-
> > > > > > > > mailbox.yaml
> > > > > > > > index cef9d76013985..c0d80cc770899 100644
> > > > > > > > ---
> > > > > > > > a/Documentation/devicetree/bindings/mailbox/mediatek,gc
> > > > > > > > e-
> > > > > > > > mailbox.yaml
> > > > > > > > +++
> > > > > > > > b/Documentation/devicetree/bindings/mailbox/mediatek,gc
> > > > > > > > e-
> > > > > > > > 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?
Did I misunderstand the ID here?
I thought we were talking about event IDs, but it looks like we are
talking about mbox IDs.
> > > > > > >
> > > > > >
> > > > > > I forgot to sync with Shawn about this:
> > > > > > https://lore.kernel.org/all/20240124011459.12204-1-jason-
> > > > > > [email protected]
> > > > > >
> > > > > > 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/[email protected]
> > would be better choice.
> > I think their usage of gce-events are the same.
> >
> > What do you think?
>
> To me it comes down to whether the IDs are fixed on a particular SoC
> (in
> which case they can be deduced by the compatible) or not. I don't
> really
> see how this is actually a fixed property of the SoC though, if you
> came
> along tomorrow with a "gce-2.0" you could totally end up with
> different
> numbering because (as far as I can tell) this numbering is actually a
> property of the os-firmware interface, not actually a property of the
> SoC itself. I was expecting you to say "no" when I asked if the IDs
> were
> fixed for a given SoC because changing the firmware /could/ change
> the IDs.
> Although, I think you'd likely not ever want to change them, because
> that'd just be an annoying ABI break to deal with.
>
mbox IDs are "not" fixed property of the SoC.
Some of the event IDs(HW event) are fixed of the SoC and some of the
event IDs are not fixed of the SoC.
So we need to change the non-fixed event IDs to make sure they won't
conflict to the fixed event IDs of another SoC.
> What I think is that you need to write a property description that
> explains what the mailbox is using the gce channel for so that
> someone
> can populate the property correctly. The commit message also needs to
> explain why this is not a fixed value for a given SoC.
>
Totally agree!
> And yes, as you pointed out earlier in this thread, Shawn needs to
> update this to have a reference to the gce-events binding which has a
> great description in it of what a gce event is.
>
> > I think their usage of gce-events are the same.
>
> Also, just because a property got introduced doesn't mean that it is
> correct and adding new instances can definitely by required to
> provide
> justification, not just saying "it's used by xyz too".
>
Sure, I think we need to add more explanation for what we use a mbox
channel for.
I think the main reason is we want to request a mbox channel for
sending a looping command to GCE thread. The looping command will get
secure task done event signal every time from secure world and notify
CMDQ driver to do the same operation as the IRQ handler.
This can reduce the latency of software communication between secure
world and we can also remove the complex logic after every secure task
done in the secure world.
Regards,
Jason-JH.Lin
> Cheers,
> Conor.
On Fri, Apr 12, 2024 at 09:00:53AM +0000, Jason-JH Lin (林睿祥) wrote:
> On Tue, 2024-04-09 at 18:52 +0100, Conor Dooley wrote:
> > On Sat, Apr 06, 2024 at 04:15:51PM +0000, Jason-JH Lin (林睿祥) wrote:
> > > 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" <[email protected]>
> > > > > > > > > + 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?
>
> Did I misunderstand the ID here?
> I thought we were talking about event IDs, but it looks like we are
> talking about mbox IDs.
We were talking about the event IDs FWIW.
Just send a new version with some actual explanations added (as we
discussed earlier on this thread) and we should be good here I think.
Cheers,
Conor.