On Wed, Jan 10, 2024 at 04:36:20PM +0000, Jason-JH Lin (林睿祥) wrote: > Hi Conor, > > Thanks for the reviews. > > On Wed, 2024-01-10 at 10:36 +0000, Conor Dooley wrote: > > On Wed, Jan 10, 2024 at 02:35:30PM +0800, Jason-JH.Lin wrote: > > > 1. Add "Provider" to the title to make it clearer. > > > 2. Add reference to gce-props.yaml for adding mediatek,gce-events > > > property. > > > > I can see this from the diff. There's still no explanation here as to > > why the mailbox provider needs to have a gce-event id. NAK until you > > can > > explain that. > > > Sorry for missing the reason in commit message, I'll add it in the next > version. > > There are 2 reasons why the mailbox provider needs gce-events: > 1. The mailbox provider here is CMDQ mailbox driver. It configures GCE > hardware register by CPU directly. If we want to set the default value > from 0 to 1 for specific gce-events during the initialization of CMDQ > driver. We need to tell CMDQ driver what gce-events need to be set and > I think such GCE hardware setting can get from its device node. Why would someone want to set it to 1 or 0? At what level will that vary? Per SoC? Per board? Something else? > 2. We'll have the secure CMDQ mailbox driver in the future patch [1]. > It will request or reserve a mailbox channel, which is a dedicate GCE > thread, as a secure IRQ handler. This GCE thread executes a looping > instruction set that keeps waiting for the gce-event set from another > GCE thread in the secure world. So we also need to tell the CMDQ driver > what gce-event need to be waited. Ditto here, what level does this vary at? Do different SoCs or different boards/platforms dictate the value? Could this channel be determined from the soc-specific compatible? In other words, please explain in your commit message why this requires a property and is not detectable from any existing mechanism. From reading this I don't know what is preventing the secure mailbox channel from picking a "random" unused channel. Thanks, Conor. > [1] cmdq_sec_irq_notify_start() is where the GCE thread is requested to > prepare a looping instruction set to wait for the gce-event. > - > https://patchwork.kernel.org/project/linux-mediatek/patch/20231222045228.27826-9-jason-jh.lin@xxxxxxxxxxxx/ > > Regards, > Jason-JH.Lin > > > Cheers, > > Conor. > > > > > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx> > > > --- > > > .../devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml | 6 > > > ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/mailbox/mediatek,gce- > > > mailbox.yaml > > > b/Documentation/devicetree/bindings/mailbox/mediatek,gce- > > > mailbox.yaml > > > index cef9d7601398..728dc93117a6 100644 > > > --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce- > > > mailbox.yaml > > > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce- > > > mailbox.yaml > > > @@ -4,7 +4,7 @@ > > > $id: > > > http://devicetree.org/schemas/mailbox/mediatek,gce-mailbox.yaml# > > > $schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > -title: Mediatek Global Command Engine Mailbox > > > +title: MediaTek Global Command Engine Mailbox Provider > > > > > > maintainers: > > > - Houlong Wei <houlong.wei@xxxxxxxxxxxx> > > > @@ -57,6 +57,8 @@ required: > > > - clocks > > > > > > allOf: > > > + - $ref: mediatek,gce-props.yaml > > > + > > > - if: > > > not: > > > properties: > > > @@ -67,7 +69,7 @@ allOf: > > > required: > > > - clock-names > > > > > > -additionalProperties: false > > > +unevaluatedProperties: false > > > > > > examples: > > > - | > > > -- > > > 2.18.0 > > >
Attachment:
signature.asc
Description: PGP signature
