Message-ID: <4f1e6bdb3e266cf0e89f8a664095ea1709f9afe0.camel@mediatek.com>
Date: Tue, 16 Jan 2024 08:21:15 +0000
From: Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>
To: "conor@...nel.org" <conor@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	Johnson Wang (王聖鑫) <Johnson.Wang@...iatek.com>,
	Singo Chang (張興國) <Singo.Chang@...iatek.com>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Jason-ch Chen (陳建豪)
	<Jason-ch.Chen@...iatek.com>, Shawn Sung (宋孝謙)
	<Shawn.Sung@...iatek.com>, Nancy Lin (林欣螢)
	<Nancy.Lin@...iatek.com>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	Project_Global_Chrome_Upstream_Group
	<Project_Global_Chrome_Upstream_Group@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "krzysztof.kozlowski+dt@...aro.org"
	<krzysztof.kozlowski+dt@...aro.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "jassisinghbrar@...il.com"
	<jassisinghbrar@...il.com>, "angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v2 2/4] dt-bindings: mailbox: mediatek: gce-mailbox: Add
 reference to gce-props.yaml

On Mon, 2024-01-15 at 17:23 +0000, Conor Dooley wrote:
> On Fri, Jan 12, 2024 at 07:44:13AM +0000, Jason-JH Lin (林睿祥) wrote:
> > On Thu, 2024-01-11 at 17:31 +0000, Conor Dooley wrote:
> > > 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?
> > 
> > GCE HW have an event table in SRAM. Each event ID has a boolean
> > event
> > value and the default value is 0. There are 1024 event IDs (0~1023)
> > in
> > the event table. The mediatek,gce-events is the property to get the
> > event IDs.
> > 
> > E.g.
> > In some camera suspend/resume use cases, the may set the event ID:
> > 100
> > to 1 before suspend. When CMDQ driver resumes, all the event value
> > of
> > event IDs will be clear to 0. But camera driver won't know the
> > event
> > IDs:100 has been cleared to 0 and still send a cmd to wait for the
> > event ID:100. So CMDQ driver may need to keep the value of event
> > ID:
> > 100 before suspend and set back the value to 1 after CMDQ driver
> > clearing all the event value of event IDs.
> > 
> > CMDQ driver will need to know which event IDs' values need to be
> > backupped and restored in that camera suspend/resume use case.
> 
> Unfortunately "some use case" doesn't really help me here, it is hard
> for me to tell whether these use cases are software policy or an
> attribute of the hardware. If I had the same exact camera but was
> using
> it for a different reason, might I set it to 1 before suspend in one
> case but not in the other?
> 

It depends on the scenario, not the camera. If the same camera will use
the value of event ID after resume, it should backup the value of event
ID before suspend.

> I also don't really understand how having this property helps in this
> case either. If a camera is a mailbox consumer, it should have a
> gce-event property for the event ID that it is using in its node.
> 
> Why is that not sufficient and a gce-event also needs to be present
> in
> the consumer node? It seems to me like having it in the consumer
> alone
> should be sufficient, and the registration process would inform the
> mailbox provider driver which gce-event ID was being used by the
> camera.

Hmm...
In this camera scenario, I think gce-events can be set in consumer's
device node cloud be sufficient.

The CMDQ mailbox driver can open an API for camera driver to set its
event ID to be backup, so we don't need to get this event ID from
mailbox provider's device node.

> 
> > 
> > > At what level will that vary? Per SoC? Per board? Something else?
> > > 
> > 
> > I think the SoC supports camera feature may need this property.
> > 
> > > > 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?
> > 
> > It's a SoC level, the SoC supports secure feature will need this
> > property.
> > 
> > > 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.
> > 
> > The secure channel could be dedicated from the soc-specific
> > compatible,
> > but the event ID couldn't.
> > 
> > The same event signal corresponding event ID may changes in
> > different
> > SoC.
> > E.g.
> > The HW event signal for CMDQ_EVENT_VDO0_MUTEX_STREAM_DONE_0 is
> > corresponding to GCE event ID: 574 in MT8188, but it's
> > corresponding to
> > eventID: 597 in MT8195.
> 
> Is it always 574 in MT8188 and always 597 in MT8195?
> 
Yes, some gce-events are hardware bound and they can not change by
software. For example, in MT8195, when VDO0_MUTEX is stream done,
VDO_MUTEX will send an event signal to GCE, and the value of event
ID:597 will be set to 1. In MT8188, the value of event ID: 574 will be
set to 1 when VOD0_MUTEX is stream done.

Some of gce-events are not hardware bound and they can change by
software. For example, in MT8188, we can take the event ID: 855 that is
not bound to any hardware to set its value to 1 when the driver in
secure world completes a task. But in MT8195, the event ID: 855 is
already bound to VDEC_LAT1, so we have to take another event ID to
achieve the same purpose.
This event ID can be change any IDs that is not bound to any hardware
and is not use in any software driver yet.
We can see if the event ID is bound to the hardware or is used by
software driver in the header
include/de-bindings/mailbox/mediatek,mt8188-gce.h.

Regards,
Jason-JH.Lin

> Thanks,
> Conor.
> 
> > I can take any of the "unused" mailbox channel for the secure irq
> > handler. But without the property mediatek,gce-events, the secure
> > channel might not know what event ID to wait for.
> > 
> > Regards,
> > Jason-JH.Lin
> > > 
> > > 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@mediatek.com/
> > > > 
> > > > Regards,
> > > > Jason-JH.Lin
> > > > 
> > > > > Cheers,
> > > > > Conor.
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@...iatek.com>
> > > > > > ---
> > > > > >  .../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@...iatek.com>
> > > > > > @@ -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
> > > > > > 

Powered by blists - more mailing lists