Re: [PATCH v2 1/9] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

Re: [PATCH v2 1/9] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Krzysztof Kozlowski]

On 23/10/2023 06:37, Jason-JH.Lin wrote:
> CMDQ_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify CMDQ
> driver in the normal world that GCE secure thread has completed a task
> in thee secure world.

s/thee/the/

> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---

This is a new patch, so you must mention it in the changelog. There is
nothing in the changelog saying about this new patch.


>  include/dt-bindings/gce/mt8195-gce.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/dt-bindings/gce/mt8195-gce.h b/include/dt-bindings/gce/mt8195-gce.h
> index dcfb302b8a5b..9f99da3363b9 100644
> --- a/include/dt-bindings/gce/mt8195-gce.h
> +++ b/include/dt-bindings/gce/mt8195-gce.h
> @@ -809,4 +809,10 @@
>  /* end of hw event */
>  #define CMDQ_MAX_HW_EVENT				1019
>  
> +/*
> + * Notify normal CMDQ there are some secure task done,
> + * this token sync with secure world.
> + */
> +#define CMDQ_SYNC_TOKEN_SECURE_THR_EOF			980

Why is this below 1019? Your driver calls it also even, so is this an
event or not?

Your driver does not use this value, so does it mean FW uses it?

Best regards,
Krzysztof

Re: [PATCH v2 1/9] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Krzysztof Kozlowski]

On 23/10/2023 09:47, Krzysztof Kozlowski wrote:
> On 23/10/2023 06:37, Jason-JH.Lin wrote:
>> CMDQ_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify CMDQ
>> driver in the normal world that GCE secure thread has completed a task
>> in thee secure world.
> 
> s/thee/the/
> 
>>
>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>> ---
> 
> This is a new patch, so you must mention it in the changelog. There is
> nothing in the changelog saying about this new patch.

Hm, now I found previous thread. I asked to squash the patch with the
binding.

Best regards,
Krzysztof

Re: [PATCH v2 1/9] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Jason-JH Lin (林睿祥)]

Dear Krzysztof,

Thanks for the reviews.

On Mon, 2023-10-23 at 09:49 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 23/10/2023 09:47, Krzysztof Kozlowski wrote:
> > On 23/10/2023 06:37, Jason-JH.Lin wrote:
> >> CMDQ_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify
> CMDQ
> >> driver in the normal world that GCE secure thread has completed a
> task
> >> in thee secure world.
> > 
> > s/thee/the/

I'll fix this typo, thanks!

> > 
> >>
> >> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> >> ---
> > 
> > This is a new patch, so you must mention it in the changelog. There
> is
> > nothing in the changelog saying about this new patch.
> 
> Hm, now I found previous thread. I asked to squash the patch with the
> binding.

OK, I thought I had to reverse the order of definition patch and the
add property patch.

I'll squash them in the next version.

Regards,
Jason-JH.Lin

> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v2 1/9] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

Thanks for the reviews.

On Mon, 2023-10-23 at 09:47 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 23/10/2023 06:37, Jason-JH.Lin wrote:
> > CMDQ_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify CMDQ
> > driver in the normal world that GCE secure thread has completed a
> task
> > in thee secure world.
> 
> s/thee/the/
> 
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> 
> This is a new patch, so you must mention it in the changelog. There
> is
> nothing in the changelog saying about this new patch.
> 
> 
> >  include/dt-bindings/gce/mt8195-gce.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/include/dt-bindings/gce/mt8195-gce.h b/include/dt-
> bindings/gce/mt8195-gce.h
> > index dcfb302b8a5b..9f99da3363b9 100644
> > --- a/include/dt-bindings/gce/mt8195-gce.h
> > +++ b/include/dt-bindings/gce/mt8195-gce.h
> > @@ -809,4 +809,10 @@
> >  /* end of hw event */
> >  #define CMDQ_MAX_HW_EVENT1019
> >  
> > +/*
> > + * Notify normal CMDQ there are some secure task done,
> > + * this token sync with secure world.
> > + */
> > +#define CMDQ_SYNC_TOKEN_SECURE_THR_EOF980
> 
> Why is this below 1019? Your driver calls it also even, so is this an
> event or not?
> 
> Your driver does not use this value, so does it mean FW uses it?

I just want to separate this kind of event (sw token) from the HW
event. So I define it after CMDQ_MAX_HW_EVENT.

But from the perspective of GCE event signal, there is no difference.
So I'll move it to the correct position.

Regards,
Jason-JH.Lin

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 1/9] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Krzysztof Kozlowski]

On 25/10/2023 08:26, Jason-JH Lin (林睿祥) wrote:
> Hi Krzysztof,
> 
> Thanks for the reviews.
> 
> On Mon, 2023-10-23 at 09:47 +0200, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 23/10/2023 06:37, Jason-JH.Lin wrote:
>>> CMDQ_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify CMDQ
>>> driver in the normal world that GCE secure thread has completed a
>> task
>>> in thee secure world.
>>
>> s/thee/the/
>>
>>>
>>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>>> ---
>>
>> This is a new patch, so you must mention it in the changelog. There
>> is
>> nothing in the changelog saying about this new patch.
>>
>>
>>>  include/dt-bindings/gce/mt8195-gce.h | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/include/dt-bindings/gce/mt8195-gce.h b/include/dt-
>> bindings/gce/mt8195-gce.h
>>> index dcfb302b8a5b..9f99da3363b9 100644
>>> --- a/include/dt-bindings/gce/mt8195-gce.h
>>> +++ b/include/dt-bindings/gce/mt8195-gce.h
>>> @@ -809,4 +809,10 @@
>>>  /* end of hw event */
>>>  #define CMDQ_MAX_HW_EVENT1019
>>>  
>>> +/*
>>> + * Notify normal CMDQ there are some secure task done,
>>> + * this token sync with secure world.
>>> + */
>>> +#define CMDQ_SYNC_TOKEN_SECURE_THR_EOF980
>>
>> Why is this below 1019? Your driver calls it also even, so is this an
>> event or not?
>>
>> Your driver does not use this value, so does it mean FW uses it?
> 
> I just want to separate this kind of event (sw token) from the HW
> event. So I define it after CMDQ_MAX_HW_EVENT.

SW event? Then why is it in the bindings?

Best regards,
Krzysztof

Re: [PATCH v2 1/9] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Jason-JH Lin (林睿祥)]

On Wed, 2023-10-25 at 09:03 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 25/10/2023 08:26, Jason-JH Lin (林睿祥) wrote:
> > Hi Krzysztof,
> > 
> > Thanks for the reviews.
> > 
> > On Mon, 2023-10-23 at 09:47 +0200, Krzysztof Kozlowski wrote:
> >>   
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >>  On 23/10/2023 06:37, Jason-JH.Lin wrote:
> >>> CMDQ_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify
> CMDQ
> >>> driver in the normal world that GCE secure thread has completed a
> >> task
> >>> in thee secure world.
> >>
> >> s/thee/the/
> >>
> >>>
> >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> >>> ---
> >>
> >> This is a new patch, so you must mention it in the changelog.
> There
> >> is
> >> nothing in the changelog saying about this new patch.
> >>
> >>
> >>>  include/dt-bindings/gce/mt8195-gce.h | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/include/dt-bindings/gce/mt8195-gce.h b/include/dt-
> >> bindings/gce/mt8195-gce.h
> >>> index dcfb302b8a5b..9f99da3363b9 100644
> >>> --- a/include/dt-bindings/gce/mt8195-gce.h
> >>> +++ b/include/dt-bindings/gce/mt8195-gce.h
> >>> @@ -809,4 +809,10 @@
> >>>  /* end of hw event */
> >>>  #define CMDQ_MAX_HW_EVENT1019
> >>>  
> >>> +/*
> >>> + * Notify normal CMDQ there are some secure task done,
> >>> + * this token sync with secure world.
> >>> + */
> >>> +#define CMDQ_SYNC_TOKEN_SECURE_THR_EOF980
> >>
> >> Why is this below 1019? Your driver calls it also even, so is this
> an
> >> event or not?
> >>
> >> Your driver does not use this value, so does it mean FW uses it?
> > 
> > I just want to separate this kind of event (sw token) from the HW
> > event. So I define it after CMDQ_MAX_HW_EVENT.
> 
> SW event? Then why is it in the bindings?
> 

Sorry, I didn't explain it clearly.

There are 2 kind of GCE event signal:
- The SW token means: a GCE HW event signal triggered by SW drivers.
e.g. SW driver append a GCE cmd to set a GCE HW event after a specific
GCE cmd. Or SW driver use CPU to write a event id to GCE register to
trigger the GCE HW event corresponding to that event id.

- The HW event means: a GCE HW event signal triggered by HW engines.
e.g. When HW OVL fetches all the data in frame buffer, HW OVL will send
a frame done irq and a frame done GCE HW event(via HW bus directly) in
the same time.

Regards,
Jason-JH.Lin

> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v2 1/9] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Krzysztof Kozlowski]

On 23/10/2023 06:37, Jason-JH.Lin wrote:
> CMDQ_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify CMDQ
> driver in the normal world that GCE secure thread has completed a task
> in thee secure world.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof

Re: [PATCH v2 1/9] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

On Mon, 2023-10-23 at 10:08 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 23/10/2023 06:37, Jason-JH.Lin wrote:
> > CMDQ_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify CMDQ
> > driver in the normal world that GCE secure thread has completed a
> task
> > in thee secure world.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary
> people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might
> be
> a waste of time, thus I will skip this patch entirely till you follow
> the process allowing the patch to be tested.
> 
> Please kindly resend and include all necessary To/Cc entries.
> 
Thanks for the reminder, I'll use scripts to check the CC list again
next time!

> Best regards,
> Krzysztof
> 

Re: [PATCH v2 2/9] dt-bindings: mailbox: Add property for CMDQ secure driver

[Krzysztof Kozlowski]

On 23/10/2023 06:37, Jason-JH.Lin wrote:
> Add mboxes to define a GCE loopping thread as a secure irq handler.
> Add mediatek,gce-events to define a GCE event siganl sent from CMDQ
> TA driver in secure world as a secure irq.
> 
> These 2 properties are required for CMDQ secure driver.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  .../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 cef9d7601398..65fb93bb53b6 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
>  
> +  mboxes:
> +    maxItems: 1
> +
> +  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-array

Nothing improved.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

Best regards,
Krzysztof

Re: [PATCH v2 2/9] dt-bindings: mailbox: Add property for CMDQ secure driver

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

Thanks for the reviews.

On Mon, 2023-10-23 at 09:49 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 23/10/2023 06:37, Jason-JH.Lin wrote:
> > Add mboxes to define a GCE loopping thread as a secure irq handler.
> > Add mediatek,gce-events to define a GCE event siganl sent from CMDQ
> > TA driver in secure world as a secure irq.
> > 
> > These 2 properties are required for CMDQ secure driver.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  .../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 cef9d7601398..65fb93bb53b6 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
> >  
> > +  mboxes:
> > +    maxItems: 1
> > +
> > +  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-array
> 
> Nothing improved.
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my

Sorry, I didn't find your comments in the previous patch here:

https://patchwork.kernel.org/project/linux-mediatek/patch/20230918192204.32263-2-jason-jh.lin@mediatek.com/

Could you please comment again?
Thank you very much.


And I do see Rob's comment, so I sync the mediatek,gce-events property
definition from other yaml where used this property.

Should I also modify the definition of mediatek,gce-events property to
reference to mediatek,gce-mailbox.yaml for other yaml?

Regards,
Jason-JH.Lin

> feedback got lost between the quotes, maybe you just forgot to apply
> it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 
> Thank you.
> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v2 2/9] dt-bindings: mailbox: Add property for CMDQ secure driver

[Krzysztof Kozlowski]

On 24/10/2023 18:37, Jason-JH Lin (林睿祥) wrote:
> Hi Krzysztof,
> 
> Thanks for the reviews.
> 
> On Mon, 2023-10-23 at 09:49 +0200, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 23/10/2023 06:37, Jason-JH.Lin wrote:
>>> Add mboxes to define a GCE loopping thread as a secure irq handler.
>>> Add mediatek,gce-events to define a GCE event siganl sent from CMDQ
>>> TA driver in secure world as a secure irq.
>>>
>>> These 2 properties are required for CMDQ secure driver.
>>>
>>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>>> ---
>>>  .../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 cef9d7601398..65fb93bb53b6 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
>>>  
>>> +  mboxes:
>>> +    maxItems: 1
>>> +
>>> +  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-array
>>
>> Nothing improved.
>>
>> This is a friendly reminder during the review process.
>>
>> It seems my previous comments were not fully addressed. Maybe my
> 
> Sorry, I didn't find your comments in the previous patch here:
> 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20230918192204.32263-2-jason-jh.lin@mediatek.com/
> 
> Could you please comment again?
> Thank you very much.

Please implement Rob's feedback.
> 
> 
> And I do see Rob's comment, so I sync the mediatek,gce-events property
> definition from other yaml where used this property.
> 
> Should I also modify the definition of mediatek,gce-events property to
> reference to mediatek,gce-mailbox.yaml for other yaml?

You need single definition in one of the bindings or in shared bindings.
Depends on the users of this property.


Best regards,
Krzysztof

Re: [PATCH v2 2/9] dt-bindings: mailbox: Add property for CMDQ secure driver

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

On Sat, 2023-10-28 at 11:10 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 24/10/2023 18:37, Jason-JH Lin (林睿祥) wrote:
> > Hi Krzysztof,
> > 
> > Thanks for the reviews.
> > 
> > On Mon, 2023-10-23 at 09:49 +0200, Krzysztof Kozlowski wrote:
> >>   
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >>  On 23/10/2023 06:37, Jason-JH.Lin wrote:
> >>> Add mboxes to define a GCE loopping thread as a secure irq
> handler.
> >>> Add mediatek,gce-events to define a GCE event siganl sent from
> CMDQ
> >>> TA driver in secure world as a secure irq.
> >>>
> >>> These 2 properties are required for CMDQ secure driver.
> >>>
> >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> >>> ---
> >>>  .../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 cef9d7601398..65fb93bb53b6 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
> >>>  
> >>> +  mboxes:
> >>> +    maxItems: 1
> >>> +
> >>> +  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-array
> >>
> >> Nothing improved.
> >>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my previous comments were not fully addressed. Maybe my
> > 
> > Sorry, I didn't find your comments in the previous patch here:
> > 
> > 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20230918192204.32263-2-jason-jh.lin@mediatek.com/
> > 
> > Could you please comment again?
> > Thank you very much.
> 
> Please implement Rob's feedback.
> > 
> > 
> > And I do see Rob's comment, so I sync the mediatek,gce-events
> property
> > definition from other yaml where used this property.
> > 
> > Should I also modify the definition of mediatek,gce-events property
> to
> > reference to mediatek,gce-mailbox.yaml for other yaml?
> 
> You need single definition in one of the bindings or in shared
> bindings.
> Depends on the users of this property.
> 

Because this property is related to GCE HW.
I think the definition should be defined in mediatek,gce-mailbox.yaml.

So I should have a single definition in mediatek,gce-mailbox.yaml:
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-array


And other bindings:
Documentation/devicetree/bindings/soc/mediatek/mediatek,mutex.yaml#L56
D
ocumentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml#L41
Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml#L37
Documentation/devicetree/bindings/media/mediatek,mdp3-wrot.yaml#L37
should modify the definition to:
mediatek,gce-events:
  description:
    Reference to
Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml.


Is that right?

Regards,
Jason-JH.Lin

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 6/9] mailbox: mediatek: Add CMDQ driver support for mt8188

[Fei Shao]

Hi Jason,

On Mon, Oct 23, 2023 at 12:58 PM Jason-JH.Lin <jason-jh.lin@mediatek.com> wrote:
>
> Add CMDQ driver support for mt8188 by adding its compatible and
> driver data in CMDQ driver.
>
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 56fe01cd9731..3bdfb9a60614 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -704,12 +704,20 @@ static const struct gce_plat gce_plat_v7 = {
>         .gce_num = 1
>  };
>
> +static const struct gce_plat gce_plat_v8 = {
> +       .thread_nr = 32,
> +       .shift = 3,
> +       .control_by_sw = true,
> +       .gce_num = 2
> +};
> +
>  static const struct of_device_id cmdq_of_ids[] = {
>         {.compatible = "mediatek,mt8173-gce", .data = (void *)&gce_plat_v2},
>         {.compatible = "mediatek,mt8183-gce", .data = (void *)&gce_plat_v3},
>         {.compatible = "mediatek,mt8186-gce", .data = (void *)&gce_plat_v7},
>         {.compatible = "mediatek,mt6779-gce", .data = (void *)&gce_plat_v4},
>         {.compatible = "mediatek,mt8192-gce", .data = (void *)&gce_plat_v5},
> +       {.compatible = "mediatek,mt8188-gce", .data = (void *)&gce_plat_v8},
I guess I understand your intention here... but the ordering doesn't
make sense to most people. Put this line after mt8186.

It's up to you to ignore the outlier mt6779 for now or send a separate
patch to fully sort the list by the compatible strings.

Regards,
Fei


>         {.compatible = "mediatek,mt8195-gce", .data = (void *)&gce_plat_v6},
>         {}
>  };
> --
> 2.18.0
>
>

Re: [PATCH v2 6/9] mailbox: mediatek: Add CMDQ driver support for mt8188

[Jason-JH Lin (林睿祥)]

Hi Fei,

Thanks for the reviews.

On Mon, 2023-10-23 at 17:41 +0800, Fei Shao wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Jason,
> 
> On Mon, Oct 23, 2023 at 12:58 PM Jason-JH.Lin <
> jason-jh.lin@mediatek.com> wrote:
> >
> > Add CMDQ driver support for mt8188 by adding its compatible and
> > driver data in CMDQ driver.
> >
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 56fe01cd9731..3bdfb9a60614 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -704,12 +704,20 @@ static const struct gce_plat gce_plat_v7 = {
> >         .gce_num = 1
> >  };
> >
> > +static const struct gce_plat gce_plat_v8 = {
> > +       .thread_nr = 32,
> > +       .shift = 3,
> > +       .control_by_sw = true,
> > +       .gce_num = 2
> > +};
> > +
> >  static const struct of_device_id cmdq_of_ids[] = {
> >         {.compatible = "mediatek,mt8173-gce", .data = (void
> *)&gce_plat_v2},
> >         {.compatible = "mediatek,mt8183-gce", .data = (void
> *)&gce_plat_v3},
> >         {.compatible = "mediatek,mt8186-gce", .data = (void
> *)&gce_plat_v7},
> >         {.compatible = "mediatek,mt6779-gce", .data = (void
> *)&gce_plat_v4},
> >         {.compatible = "mediatek,mt8192-gce", .data = (void
> *)&gce_plat_v5},
> > +       {.compatible = "mediatek,mt8188-gce", .data = (void
> *)&gce_plat_v8},
> I guess I understand your intention here... but the ordering doesn't
> make sense to most people. Put this line after mt8186.
> 
> It's up to you to ignore the outlier mt6779 for now or send a
> separate
> patch to fully sort the list by the compatible strings.
> 
> Regards,
> Fei
> 

I'll move this patch out of the secure cmdq series and fix the sorting
problem. Thanks!

Regards,
Jason-JH.Lin

> 
> >         {.compatible = "mediatek,mt8195-gce", .data = (void
> *)&gce_plat_v6},
> >         {}
> >  };
> > --
> > 2.18.0
> >
> >

Re: [PATCH v2 6/9] mailbox: mediatek: Add CMDQ driver support for mt8188

[AngeloGioacchino Del Regno]

Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> Add CMDQ driver support for mt8188 by adding its compatible and
> driver data in CMDQ driver.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>

Please fix the commit title...

mailbox: mtk-cmdq: Add support for MT8188 mailbox

Regards,
Angelo

Re: [PATCH v2 6/9] mailbox: mediatek: Add CMDQ driver support for mt8188

[AngeloGioacchino Del Regno]

Il 23/10/23 11:50, AngeloGioacchino Del Regno ha scritto:
> Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
>> Add CMDQ driver support for mt8188 by adding its compatible and
>> driver data in CMDQ driver.
>>
>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> 
> Please fix the commit title...
> 
> mailbox: mtk-cmdq: Add support for MT8188 mailbox
> 
> Regards,
> Angelo
> 

I just noticed that the secure mailbox driver will need quite a few versions
before reaching a somewhat acceptable state, so I'd also consider sending this
commit separately from the secure cmdq series, as this is simply adding the
support for mt8188.

Re: [PATCH v2 6/9] mailbox: mediatek: Add CMDQ driver support for mt8188

[Jason-JH Lin (林睿祥)]

Hi Angelo,

Thanks for the reviews.

On Mon, 2023-10-23 at 12:14 +0200, AngeloGioacchino Del Regno wrote:
> Il 23/10/23 11:50, AngeloGioacchino Del Regno ha scritto:
> > Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> > > Add CMDQ driver support for mt8188 by adding its compatible and
> > > driver data in CMDQ driver.
> > > 
> > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > 
> > Please fix the commit title...
> > 
> > mailbox: mtk-cmdq: Add support for MT8188 mailbox
> > 
> > Regards,
> > Angelo
> > 
> 
> I just noticed that the secure mailbox driver will need quite a few
> versions
> before reaching a somewhat acceptable state, so I'd also consider
> sending this
> commit separately from the secure cmdq series, as this is simply
> adding the
> support for mt8188.
> 
> 
OK, I'll send this separately from the secure cmdq series.
Thanks.

Regards,
Jason-JH.Lin

Re: [PATCH v2 5/9] soc: mailbox: Add cmdq_pkt_finalize_loop to support looping cmd with irq

[AngeloGioacchino Del Regno]

Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> Add cmdq_pkt_finalize_loop to CMDQ driver.
> 
> cmdq_pkt_finalize_loop appends end of command(EOC) instruction and
> jump to start of command buffer instruction to make the command
> buffer loopable.
> 
> GCE irq occurs when GCE executes to the EOC instruction.
> CMDQ client can use a loop flag to mark the CMDQ packet as looping
> packet. If the CMDQ packet is a loopping packet, GCE irq handler
> won't delete the CMDQ task and disable the GCE thread.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>   drivers/mailbox/mtk-cmdq-mailbox.c       | 11 +++++++++++
>   drivers/soc/mediatek/mtk-cmdq-helper.c   | 23 +++++++++++++++++++++++
>   include/linux/mailbox/mtk-cmdq-mailbox.h |  1 +
>   include/linux/soc/mediatek/mtk-cmdq.h    |  8 ++++++++
>   4 files changed, 43 insertions(+)
> 
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 4d62b07c1411..56fe01cd9731 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c

It's true that without the changes in soc/mediatek this is not doing anything, but
mailbox/mtk-cmdq-mailbox.c goes through a different maintainer...

Please split the changes to drivers/mailbox/mtk-cmdq-mailbox.c and its header to a
different commit.

Hint:
1. soc: mediatek: cmdq: Add cmdq_pkt_finalize_loop for looping cmd with irq
2. mailbox: mtk-cmdq: Support GCE loop packets in interrupt handler

Thanks,
Angelo

Re: [PATCH v2 5/9] soc: mailbox: Add cmdq_pkt_finalize_loop to support looping cmd with irq

[Jason-JH Lin (林睿祥)]

On Mon, 2023-10-23 at 11:50 +0200, AngeloGioacchino Del Regno wrote:
> Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> > Add cmdq_pkt_finalize_loop to CMDQ driver.
> > 
> > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and
> > jump to start of command buffer instruction to make the command
> > buffer loopable.
> > 
> > GCE irq occurs when GCE executes to the EOC instruction.
> > CMDQ client can use a loop flag to mark the CMDQ packet as looping
> > packet. If the CMDQ packet is a loopping packet, GCE irq handler
> > won't delete the CMDQ task and disable the GCE thread.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >   drivers/mailbox/mtk-cmdq-mailbox.c       | 11 +++++++++++
> >   drivers/soc/mediatek/mtk-cmdq-helper.c   | 23
> > +++++++++++++++++++++++
> >   include/linux/mailbox/mtk-cmdq-mailbox.h |  1 +
> >   include/linux/soc/mediatek/mtk-cmdq.h    |  8 ++++++++
> >   4 files changed, 43 insertions(+)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 4d62b07c1411..56fe01cd9731 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> 
> It's true that without the changes in soc/mediatek this is not doing
> anything, but
> mailbox/mtk-cmdq-mailbox.c goes through a different maintainer...
> 
> Please split the changes to drivers/mailbox/mtk-cmdq-mailbox.c and
> its header to a
> different commit.
> 
> Hint:
> 1. soc: mediatek: cmdq: Add cmdq_pkt_finalize_loop for looping cmd
> with irq
> 2. mailbox: mtk-cmdq: Support GCE loop packets in interrupt handler
> 

That's what I have done in the previous version.
Because of Krzysztof's comment here:
https://patchwork.kernel.org/project/linux-mediatek/patch/20230918192204.32263-8-jason-jh.lin@mediatek.com/#25527450
I squash them together. I'm not sure if I misunderstood that.

But I think I should follow the maintainer who can apply the patch.
So I'll split it in the next version.

Regards,
Jason-JH.Lin

> Thanks,
> Angelo
> 

Re: [PATCH v2 4/9] soc: mailbox: Add cmdq_pkt_write_s_reg_value to support write value to reg

[AngeloGioacchino Del Regno]

Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> Add cmdq_pkt_write_s_reg_value to support write a value to a register.
> 
> It appends write_s command to the command buffer in a CMDQ packet,
> ask GCE to excute a write instruction to write a value to a register
> with low 16 bits physical address offset.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>

Please fix the commit title:

soc: mediatek: cmdq: xxxx

Regards,
Angelo

Re: [PATCH v2 4/9] soc: mailbox: Add cmdq_pkt_write_s_reg_value to support write value to reg

[Jason-JH Lin (林睿祥)]

Hi Angelo,

Thanks for the reviews.

On Mon, 2023-10-23 at 11:50 +0200, AngeloGioacchino Del Regno wrote:
> Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> > Add cmdq_pkt_write_s_reg_value to support write a value to a
> > register.
> > 
> > It appends write_s command to the command buffer in a CMDQ packet,
> > ask GCE to excute a write instruction to write a value to a
> > register
> > with low 16 bits physical address offset.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> 
> Please fix the commit title:
> 
> soc: mediatek: cmdq: xxxx
> 
OK, I'll fix that. Thanks!

Regards,
Jason-JH.Lin


> Regards,
> Angelo
> 

Re: [PATCH v2 3/9] soc: mailbox: Add cmdq_pkt_logic_command to support math operation

[Fei Shao]

Hi Jason,

On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <jason-jh.lin@mediatek.com> wrote:
>
> Add cmdq_pkt_logic_command to support match operation.
s/match/math/

>
> cmdq_pkt_logic_command can append logic command to the CMDQ packet,
> ask GCE to execute a arithematic calculate instruction,
s/arithematic/arithmetic/

> such as add, subtract, multiply, AND, OR and NOT, etc.
>
> Note that all instructions just accept unsigned calculate.
s/calculate/calculation/

Or I'd rephrase it as:
  Note that all arithmetic instructions are unsigned calculations.

> If there are any overflows, GCE will sent the invalid IRQ to notify
> CMDQ driver.
>
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 36 ++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  | 41 ++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index b0cd071c4719..5194d66dfc0a 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -13,9 +13,18 @@
>  #define CMDQ_WRITE_ENABLE_MASK BIT(0)
>  #define CMDQ_POLL_ENABLE_MASK  BIT(0)
>  #define CMDQ_EOC_IRQ_EN                BIT(0)
> +#define CMDQ_IMMEDIATE_VALUE   0
>  #define CMDQ_REG_TYPE          1
>  #define CMDQ_JUMP_RELATIVE     1
>
> +#define CMDQ_OPERAND_GET_IDX_VALUE(operand) \
> +       ({ \
> +               struct cmdq_operand *op = operand; \
> +               op->reg ? op->idx : op->value; \
> +       })
`((operand)->reg ? (operand)->idx : (operand)->value)` fits in one line.

> +#define CMDQ_OPERAND_TYPE(operand) \
> +       ((operand)->reg ? CMDQ_REG_TYPE : CMDQ_IMMEDIATE_VALUE)
> +
>  struct cmdq_instruction {
>         union {
>                 u32 value;
> @@ -380,6 +389,33 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>  }
>  EXPORT_SYMBOL(cmdq_pkt_poll_mask);
>
> +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, enum CMDQ_LOGIC_ENUM s_op,
> +                          u16 result_reg_idx,
> +                          struct cmdq_operand *left_operand,
> +                          struct cmdq_operand *right_operand)
> +{
> +       struct cmdq_instruction inst = { {0} };
> +       u32 left_idx_value;
> +       u32 right_idx_value;
> +
> +       if (!left_operand || !right_operand)
> +               return -EINVAL;
> +
> +       left_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(left_operand);
> +       right_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(right_operand);
> +       inst.op = CMDQ_CODE_LOGIC;
> +       inst.dst_t = CMDQ_REG_TYPE;
> +       inst.src_t = CMDQ_OPERAND_TYPE(left_operand);
> +       inst.arg_c_t = CMDQ_OPERAND_TYPE(right_operand);
> +       inst.sop = s_op;
> +       inst.arg_c = right_idx_value;
> +       inst.src_reg = left_idx_value;
> +       inst.reg_dst = result_reg_idx;
> +
> +       return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_logic_command);
> +
>  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
>  {
>         struct cmdq_instruction inst = {};
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index a253c001c861..ea4fadfb5443 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -26,6 +26,30 @@
>
>  struct cmdq_pkt;
>
> +enum CMDQ_LOGIC_ENUM {
Use lower case, and perhaps use `cmdq_logic_type` or `cmdq_operator_type`.

Regards,
Fei

Re: [PATCH v2 3/9] soc: mailbox: Add cmdq_pkt_logic_command to support math operation

[Fei Shao]

On Mon, Oct 23, 2023 at 4:26 PM Fei Shao <fshao@chromium.org> wrote:
>
> Hi Jason,
>
> On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <jason-jh.lin@mediatek.com> wrote:
<snip>

> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index a253c001c861..ea4fadfb5443 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -26,6 +26,30 @@
> >
> >  struct cmdq_pkt;
> >
> > +enum CMDQ_LOGIC_ENUM {
> Use lower case, and perhaps use `cmdq_logic_type` or `cmdq_operator_type`.
I overlooked the CMDQ_CODE_LOGIC line, so cmdq_logic_type makes more sense.

Regards,
Fei

Re: [PATCH v2 3/9] soc: mailbox: Add cmdq_pkt_logic_command to support math operation

[Jason-JH Lin (林睿祥)]

Hi Fei,

Thanks for the reviews.

On Mon, 2023-10-23 at 16:26 +0800, Fei Shao wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Jason,
> 
> On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <
> jason-jh.lin@mediatek.com> wrote:
> >
> > Add cmdq_pkt_logic_command to support match operation.
> s/match/math/
> 
I'll fix this typo. Thanks.

> >
> > cmdq_pkt_logic_command can append logic command to the CMDQ packet,
> > ask GCE to execute a arithematic calculate instruction,
> s/arithematic/arithmetic/
> 
I'll fix this typo. Thanks.

> > such as add, subtract, multiply, AND, OR and NOT, etc.
> >
> > Note that all instructions just accept unsigned calculate.
> s/calculate/calculation/
> 
> 
> Or I'd rephrase it as:
>   Note that all arithmetic instructions are unsigned calculations.
> 
OK, I'll rephrase as yours. Thanks!

> > If there are any overflows, GCE will sent the invalid IRQ to notify
> > CMDQ driver.
> >
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c | 36 ++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-cmdq.h  | 41
> ++++++++++++++++++++++++++
> >  2 files changed, 77 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index b0cd071c4719..5194d66dfc0a 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -13,9 +13,18 @@
> >  #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> >  #define CMDQ_POLL_ENABLE_MASK  BIT(0)
> >  #define CMDQ_EOC_IRQ_EN                BIT(0)
> > +#define CMDQ_IMMEDIATE_VALUE   0
> >  #define CMDQ_REG_TYPE          1
> >  #define CMDQ_JUMP_RELATIVE     1
> >
> > +#define CMDQ_OPERAND_GET_IDX_VALUE(operand) \
> > +       ({ \
> > +               struct cmdq_operand *op = operand; \
> > +               op->reg ? op->idx : op->value; \
> > +       })
> `((operand)->reg ? (operand)->idx : (operand)->value)` fits in one
> line.
> 

I had use that way, but it will get this CHECK warning:
CHECK: Macro argument reuse '_stat' - possible side-effects?

So I change to current way.

> > +#define CMDQ_OPERAND_TYPE(operand) \
> > +       ((operand)->reg ? CMDQ_REG_TYPE : CMDQ_IMMEDIATE_VALUE)
> > +
> >  struct cmdq_instruction {
> >         union {
> >                 u32 value;
> > @@ -380,6 +389,33 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt,
> u8 subsys,
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> >
> > +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, enum
> CMDQ_LOGIC_ENUM s_op,
> > +                          u16 result_reg_idx,
> > +                          struct cmdq_operand *left_operand,
> > +                          struct cmdq_operand *right_operand)
> > +{
> > +       struct cmdq_instruction inst = { {0} };
> > +       u32 left_idx_value;
> > +       u32 right_idx_value;
> > +
> > +       if (!left_operand || !right_operand)
> > +               return -EINVAL;
> > +
> > +       left_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(left_operand);
> > +       right_idx_value =
> CMDQ_OPERAND_GET_IDX_VALUE(right_operand);
> > +       inst.op = CMDQ_CODE_LOGIC;
> > +       inst.dst_t = CMDQ_REG_TYPE;
> > +       inst.src_t = CMDQ_OPERAND_TYPE(left_operand);
> > +       inst.arg_c_t = CMDQ_OPERAND_TYPE(right_operand);
> > +       inst.sop = s_op;
> > +       inst.arg_c = right_idx_value;
> > +       inst.src_reg = left_idx_value;
> > +       inst.reg_dst = result_reg_idx;
> > +
> > +       return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_logic_command);
> > +
> >  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> >  {
> >         struct cmdq_instruction inst = {};
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> > index a253c001c861..ea4fadfb5443 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -26,6 +26,30 @@
> >
> >  struct cmdq_pkt;
> >
> > +enum CMDQ_LOGIC_ENUM {
> Use lower case, and perhaps use `cmdq_logic_type` or
> `cmdq_operator_type`.
> 
Thanks! I think I would change to `cmdq_logic_op`.

Regards,
Jason-JH.Lin

> Regards,
> Fei

Re: [PATCH v2 3/9] soc: mailbox: Add cmdq_pkt_logic_command to support math operation

[AngeloGioacchino Del Regno]

Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> Add cmdq_pkt_logic_command to support match operation.
> 
> cmdq_pkt_logic_command can append logic command to the CMDQ packet,
> ask GCE to execute a arithematic calculate instruction,
> such as add, subtract, multiply, AND, OR and NOT, etc.
> 
> Note that all instructions just accept unsigned calculate.
> If there are any overflows, GCE will sent the invalid IRQ to notify
> CMDQ driver.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>

Please always run `git log` on the file that you're changing to properly format the
commit title.

In this case, this should be:

soc: mediatek: cmdq: .....

> ---
>   drivers/soc/mediatek/mtk-cmdq-helper.c | 36 ++++++++++++++++++++++
>   include/linux/soc/mediatek/mtk-cmdq.h  | 41 ++++++++++++++++++++++++++
>   2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index b0cd071c4719..5194d66dfc0a 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -13,9 +13,18 @@
>   #define CMDQ_WRITE_ENABLE_MASK	BIT(0)
>   #define CMDQ_POLL_ENABLE_MASK	BIT(0)
>   #define CMDQ_EOC_IRQ_EN		BIT(0)
> +#define CMDQ_IMMEDIATE_VALUE	0
>   #define CMDQ_REG_TYPE		1
>   #define CMDQ_JUMP_RELATIVE	1
>   
> +#define CMDQ_OPERAND_GET_IDX_VALUE(operand) \
> +	({ \
> +		struct cmdq_operand *op = operand; \
> +		op->reg ? op->idx : op->value; \
> +	})
> +#define CMDQ_OPERAND_TYPE(operand) \
> +	((operand)->reg ? CMDQ_REG_TYPE : CMDQ_IMMEDIATE_VALUE)
> +
>   struct cmdq_instruction {
>   	union {
>   		u32 value;
> @@ -380,6 +389,33 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>   }
>   EXPORT_SYMBOL(cmdq_pkt_poll_mask);
>   
> +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, enum CMDQ_LOGIC_ENUM s_op,
> +			   u16 result_reg_idx,
> +			   struct cmdq_operand *left_operand,
> +			   struct cmdq_operand *right_operand)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +	u32 left_idx_value;
> +	u32 right_idx_value;
> +
> +	if (!left_operand || !right_operand)

I would also check that the requested logic operation is actually implemented:

	if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
		return -EINVAL;

> +		return -EINVAL;
> +
> +	left_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(left_operand);
> +	right_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(right_operand);
> +	inst.op = CMDQ_CODE_LOGIC;
> +	inst.dst_t = CMDQ_REG_TYPE;
> +	inst.src_t = CMDQ_OPERAND_TYPE(left_operand);
> +	inst.arg_c_t = CMDQ_OPERAND_TYPE(right_operand);
> +	inst.sop = s_op;
> +	inst.arg_c = right_idx_value;
> +	inst.src_reg = left_idx_value;
> +	inst.reg_dst = result_reg_idx;
> +
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_logic_command);
> +
>   int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
>   {
>   	struct cmdq_instruction inst = {};
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index a253c001c861..ea4fadfb5443 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -26,6 +26,30 @@
>   
>   struct cmdq_pkt;
>   
> +enum CMDQ_LOGIC_ENUM {

Please no capital letters in enumeration names.
Besides, it's clearer if you named this

enum cmdq_logic_operation {

or

enum cmdq_logic_op {

> +	CMDQ_LOGIC_ASSIGN = 0,
> +	CMDQ_LOGIC_ADD = 1,
> +	CMDQ_LOGIC_SUBTRACT = 2,
> +	CMDQ_LOGIC_MULTIPLY = 3,
> +	CMDQ_LOGIC_XOR = 8,
> +	CMDQ_LOGIC_NOT = 9,
> +	CMDQ_LOGIC_OR = 10,
> +	CMDQ_LOGIC_AND = 11,
> +	CMDQ_LOGIC_LEFT_SHIFT = 12,
> +	CMDQ_LOGIC_RIGHT_SHIFT = 13,

You should also add a `CMDQ_LOGIC_MAX,` here to use it in the code for validation.

> +};
> +
> +struct cmdq_operand {
> +	/* register type */
> +	bool reg;
> +	union {
> +		/* index */
> +		u16 idx;
> +		/* value */
> +		u16 value;
> +	};
> +};
> +
>   struct cmdq_client_reg {
>   	u8 subsys;
>   	u16 offset;
> @@ -244,6 +268,23 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
>   int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>   		       u16 offset, u32 value, u32 mask);
>   
> +/**
> + * cmdq_pkt_logic_command() - Append logic command to the CMDQ packet, ask GCE to
> + *		          execute an instruction that store the result of logic operation
> + *		          with left and right operand into result_reg_idx.
> + * @pkt:		the CMDQ packet
> + * @s_op:		the logic operator enum
> + * @result_reg_idx:	SPR index that store operation result of left_operand and right_operand
> + * @left_operand:	left operand
> + * @right_operand:	right operand
> + *

I think that there's a way to dramatically increase human readability of calls to
this function: being this a request to perform calculations, it would be way easier
to read it like an actual calculation :-)

cmdq_pkt_logic_command(pkt, result, op_x, CMDQ_LOGIC_ADD, op_y);

At least in my head, this easily resembles "result = op_x + op_y".

I therefore propose to change this to:

int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 result_reg_idx,
			   struct cmdq_operand *left_operand,
			   enum cmdq_logic_operation s_op,
			   struct cmdq_operand *right_operand);

> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, enum CMDQ_LOGIC_ENUM s_op,
> +			   u16 result_reg_idx,
> +			   struct cmdq_operand *left_operand,
> +			   struct cmdq_operand *right_operand);
> +
>   /**
>    * cmdq_pkt_assign() - Append logic assign command to the CMDQ packet, ask GCE
>    *		       to execute an instruction that set a constant value into


Regards,
Angelo

Re: [PATCH v2 3/9] soc: mailbox: Add cmdq_pkt_logic_command to support math operation

[Jason-JH Lin (林睿祥)]

Hi Angelo,

Thanks for the reviews.

On Mon, 2023-10-23 at 11:50 +0200, AngeloGioacchino Del Regno wrote:
> Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> > Add cmdq_pkt_logic_command to support match operation.
> > 
> > cmdq_pkt_logic_command can append logic command to the CMDQ packet,
> > ask GCE to execute a arithematic calculate instruction,
> > such as add, subtract, multiply, AND, OR and NOT, etc.
> > 
> > Note that all instructions just accept unsigned calculate.
> > If there are any overflows, GCE will sent the invalid IRQ to notify
> > CMDQ driver.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> 
> Please always run `git log` on the file that you're changing to
> properly format the
> commit title.
> 
> In this case, this should be:
> 
> soc: mediatek: cmdq: .....

I think it happended because I deleted the `\n` in the first line of
the title when I review these patches in `git send-email` cmd.

I won't do that in the next version. Thanks.

> 
> > ---
> >   drivers/soc/mediatek/mtk-cmdq-helper.c | 36
> > ++++++++++++++++++++++
> >   include/linux/soc/mediatek/mtk-cmdq.h  | 41
> > ++++++++++++++++++++++++++
> >   2 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index b0cd071c4719..5194d66dfc0a 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -13,9 +13,18 @@
> >   #define CMDQ_WRITE_ENABLE_MASK	BIT(0)
> >   #define CMDQ_POLL_ENABLE_MASK	BIT(0)
> >   #define CMDQ_EOC_IRQ_EN		BIT(0)
> > +#define CMDQ_IMMEDIATE_VALUE	0
> >   #define CMDQ_REG_TYPE		1
> >   #define CMDQ_JUMP_RELATIVE	1
> >   
> > +#define CMDQ_OPERAND_GET_IDX_VALUE(operand) \
> > +	({ \
> > +		struct cmdq_operand *op = operand; \
> > +		op->reg ? op->idx : op->value; \
> > +	})
> > +#define CMDQ_OPERAND_TYPE(operand) \
> > +	((operand)->reg ? CMDQ_REG_TYPE : CMDQ_IMMEDIATE_VALUE)
> > +
> >   struct cmdq_instruction {
> >   	union {
> >   		u32 value;
> > @@ -380,6 +389,33 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt,
> > u8 subsys,
> >   }
> >   EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> >   
> > +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, enum
> > CMDQ_LOGIC_ENUM s_op,
> > +			   u16 result_reg_idx,
> > +			   struct cmdq_operand *left_operand,
> > +			   struct cmdq_operand *right_operand)
> > +{
> > +	struct cmdq_instruction inst = { {0} };
> > +	u32 left_idx_value;
> > +	u32 right_idx_value;
> > +
> > +	if (!left_operand || !right_operand)
> 
> I would also check that the requested logic operation is actually
> implemented:
> 
> 	if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
> 		return -EINVAL;
> 
OK, I'll add this check.

> > +		return -EINVAL;
> > +
> > +	left_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(left_operand);
> > +	right_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(right_operand);
> > +	inst.op = CMDQ_CODE_LOGIC;
> > +	inst.dst_t = CMDQ_REG_TYPE;
> > +	inst.src_t = CMDQ_OPERAND_TYPE(left_operand);
> > +	inst.arg_c_t = CMDQ_OPERAND_TYPE(right_operand);
> > +	inst.sop = s_op;
> > +	inst.arg_c = right_idx_value;
> > +	inst.src_reg = left_idx_value;
> > +	inst.reg_dst = result_reg_idx;
> > +
> > +	return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_logic_command);
> > +
> >   int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> >   {
> >   	struct cmdq_instruction inst = {};
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> > b/include/linux/soc/mediatek/mtk-cmdq.h
> > index a253c001c861..ea4fadfb5443 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -26,6 +26,30 @@
> >   
> >   struct cmdq_pkt;
> >   
> > +enum CMDQ_LOGIC_ENUM {
> 
> Please no capital letters in enumeration names.
> Besides, it's clearer if you named this
> 
> enum cmdq_logic_operation {
> 
> or
> 
> enum cmdq_logic_op {

OK, I'll change to this one. Thanks.

> 
> > +	CMDQ_LOGIC_ASSIGN = 0,
> > +	CMDQ_LOGIC_ADD = 1,
> > +	CMDQ_LOGIC_SUBTRACT = 2,
> > +	CMDQ_LOGIC_MULTIPLY = 3,
> > +	CMDQ_LOGIC_XOR = 8,
> > +	CMDQ_LOGIC_NOT = 9,
> > +	CMDQ_LOGIC_OR = 10,
> > +	CMDQ_LOGIC_AND = 11,
> > +	CMDQ_LOGIC_LEFT_SHIFT = 12,
> > +	CMDQ_LOGIC_RIGHT_SHIFT = 13,
> 
> You should also add a `CMDQ_LOGIC_MAX,` here to use it in the code
> for validation.

OK, I'll add this. Thanks.

> 
> > +};
> > +
> > +struct cmdq_operand {
> > +	/* register type */
> > +	bool reg;
> > +	union {
> > +		/* index */
> > +		u16 idx;
> > +		/* value */
> > +		u16 value;
> > +	};
> > +};
> > +
> >   struct cmdq_client_reg {
> >   	u8 subsys;
> >   	u16 offset;
> > @@ -244,6 +268,23 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8
> > subsys,
> >   int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> >   		       u16 offset, u32 value, u32 mask);
> >   
> > +/**
> > + * cmdq_pkt_logic_command() - Append logic command to the CMDQ
> > packet, ask GCE to
> > + *		          execute an instruction that store the result
> > of logic operation
> > + *		          with left and right operand into
> > result_reg_idx.
> > + * @pkt:		the CMDQ packet
> > + * @s_op:		the logic operator enum
> > + * @result_reg_idx:	SPR index that store operation result
> > of left_operand and right_operand
> > + * @left_operand:	left operand
> > + * @right_operand:	right operand
> > + *
> 
> I think that there's a way to dramatically increase human readability
> of calls to
> this function: being this a request to perform calculations, it would
> be way easier
> to read it like an actual calculation :-)
> 
> cmdq_pkt_logic_command(pkt, result, op_x, CMDQ_LOGIC_ADD, op_y);
> 
> At least in my head, this easily resembles "result = op_x + op_y".
> 
> I therefore propose to change this to:
> 
> int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 result_reg_idx,
> 			   struct cmdq_operand *left_operand,
> 			   enum cmdq_logic_operation s_op,
> 			   struct cmdq_operand *right_operand);
> 
OK, I think it's a good idea.
I'll change that. Thanks!

Regards,
Jason-JH.Lin

> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, enum
> > CMDQ_LOGIC_ENUM s_op,
> > +			   u16 result_reg_idx,
> > +			   struct cmdq_operand *left_operand,
> > +			   struct cmdq_operand *right_operand);
> > +
> >   /**
> >    * cmdq_pkt_assign() - Append logic assign command to the CMDQ
> > packet, ask GCE
> >    *		       to execute an instruction that set a
> > constant value into
> 
> 
> Regards,
> Angelo
> 

Re: [PATCH v2 7/9] mailbox: mediatek: Add secure CMDQ driver support for CMDQ driver

[Fei Shao]

Hi Jason,

On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <jason-jh.lin@mediatek.com> wrote:
>
> CMDQ driver will probe a secure CMDQ driver when has_sec flag
> in platform data is true and its device node in dts has defined a
> event id of CMDQ_SYNC_TOKEN_SEC_EOF.
>
> Secure CMDQ driver support on mt8188 and mt8195 currently.
> So add a has_sec flag to their driver data to probe it.
>
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c       | 42 ++++++++++++++++++++++--
>  include/linux/mailbox/mtk-cmdq-mailbox.h | 11 +++++++
>  2 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 3bdfb9a60614..4db5eb76f353 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -87,6 +87,7 @@ struct gce_plat {
>         u8 shift;
>         bool control_by_sw;
>         bool sw_ddr_en;
> +       bool has_sec;
>         u32 gce_num;
>  };
>
> @@ -560,14 +561,23 @@ static int cmdq_probe(struct platform_device *pdev)
>         int alias_id = 0;
>         static const char * const clk_name = "gce";
>         static const char * const clk_names[] = { "gce0", "gce1" };
> +       struct resource *res;
> +       struct platform_device *mtk_cmdq_sec;
> +       u32 hwid = 0;
>
>         cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
>         if (!cmdq)
>                 return -ENOMEM;
>
> -       cmdq->base = devm_platform_ioremap_resource(pdev, 0);
> -       if (IS_ERR(cmdq->base))
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
The devm_platform_ioremap_resource() helper was added on purpose [1].
Please stick to it unless you have a strong reason to re-split it.

[1]: a04f30356e75 ("mailbox: mtk-cmdq: Make use of the helper function
devm_platform_ioremap_resource()")

> +       if (!res)
> +               return -EINVAL;
> +
> +       cmdq->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(cmdq->base)) {
> +               dev_err(dev, "failed to ioremap cmdq\n");
>                 return PTR_ERR(cmdq->base);
> +       }
>
>         cmdq->irq = platform_get_irq(pdev, 0);
>         if (cmdq->irq < 0)
> @@ -585,6 +595,8 @@ static int cmdq_probe(struct platform_device *pdev)
>                 dev, cmdq->base, cmdq->irq);
>
>         if (cmdq->pdata->gce_num > 1) {
> +               hwid = of_alias_get_id(dev->of_node, clk_name);
Why get hwid here while it's only used in the has_sec branch?

> +
>                 for_each_child_of_node(phandle->parent, node) {
>                         alias_id = of_alias_get_id(node, clk_name);
>                         if (alias_id >= 0 && alias_id < cmdq->pdata->gce_num) {
> @@ -653,6 +665,30 @@ static int cmdq_probe(struct platform_device *pdev)
>                 return err;
>         }
>
> +       if (cmdq->pdata->has_sec) {
> +               struct cmdq_sec_plat gce_sec_plat;
> +
> +               if (of_property_read_u32_index(dev->of_node, "mediatek,gce-events", 0,
> +                                              &gce_sec_plat.cmdq_event) == 0) {
> +                       gce_sec_plat.gce_dev = dev;
> +                       gce_sec_plat.base = cmdq->base;
> +                       gce_sec_plat.base_pa = res->start;
> +                       gce_sec_plat.hwid = hwid;
> +                       gce_sec_plat.gce_num = cmdq->pdata->gce_num;
> +                       gce_sec_plat.clocks = cmdq->clocks;
> +                       gce_sec_plat.thread_nr = cmdq->pdata->thread_nr;
> +
> +                       mtk_cmdq_sec = platform_device_register_data(dev, "mtk_cmdq_sec",
> +                                                                    PLATFORM_DEVID_AUTO,
> +                                                                    &gce_sec_plat,
> +                                                                    sizeof(gce_sec_plat));
> +                       if (IS_ERR(mtk_cmdq_sec)) {
> +                               dev_err(dev, "failed to register platform_device mtk_cmdq_sec\n");
> +                               return PTR_ERR(mtk_cmdq_sec);
> +                       }
> +               }
> +       }
> +
>         return 0;
>  }
>
> @@ -693,6 +729,7 @@ static const struct gce_plat gce_plat_v6 = {
>         .thread_nr = 24,
>         .shift = 3,
>         .control_by_sw = true,
> +       .has_sec = true,
>         .gce_num = 2
>  };
>
> @@ -708,6 +745,7 @@ static const struct gce_plat gce_plat_v8 = {
>         .thread_nr = 32,
>         .shift = 3,
>         .control_by_sw = true,
> +       .has_sec = true,
>         .gce_num = 2
>  };
>
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index f78a08e7c6ed..fdda995a69ce 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -79,6 +79,17 @@ struct cmdq_pkt {
>         bool                    loop;
>  };
>
> +struct cmdq_sec_plat {
> +       struct device *gce_dev;
> +       void __iomem *base;
> +       dma_addr_t base_pa;
> +       u32 hwid;
> +       u32 gce_num;
> +       struct clk_bulk_data *clocks;
> +       u32 thread_nr;
> +       u32 cmdq_event;
> +};
I feel this should be in mtk-cmdq-sec-mailbox.h.

Regards,
Fei



> +
>  u8 cmdq_get_shift_pa(struct mbox_chan *chan);
>
>  #endif /* __MTK_CMDQ_MAILBOX_H__ */
> --
> 2.18.0
>
>

Re: [PATCH v2 7/9] mailbox: mediatek: Add secure CMDQ driver support for CMDQ driver

[Jason-JH Lin (林睿祥)]

Hi Fei,

Thanks for the reviews.

On Mon, 2023-10-23 at 18:47 +0800, Fei Shao wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Jason,
> 
> On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <
> jason-jh.lin@mediatek.com> wrote:
> >
> > CMDQ driver will probe a secure CMDQ driver when has_sec flag
> > in platform data is true and its device node in dts has defined a
> > event id of CMDQ_SYNC_TOKEN_SEC_EOF.
> >
> > Secure CMDQ driver support on mt8188 and mt8195 currently.
> > So add a has_sec flag to their driver data to probe it.
> >
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c       | 42
> ++++++++++++++++++++++--
> >  include/linux/mailbox/mtk-cmdq-mailbox.h | 11 +++++++
> >  2 files changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 3bdfb9a60614..4db5eb76f353 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -87,6 +87,7 @@ struct gce_plat {
> >         u8 shift;
> >         bool control_by_sw;
> >         bool sw_ddr_en;
> > +       bool has_sec;
> >         u32 gce_num;
> >  };
> >
> > @@ -560,14 +561,23 @@ static int cmdq_probe(struct platform_device
> *pdev)
> >         int alias_id = 0;
> >         static const char * const clk_name = "gce";
> >         static const char * const clk_names[] = { "gce0", "gce1" };
> > +       struct resource *res;
> > +       struct platform_device *mtk_cmdq_sec;
> > +       u32 hwid = 0;
> >
> >         cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
> >         if (!cmdq)
> >                 return -ENOMEM;
> >
> > -       cmdq->base = devm_platform_ioremap_resource(pdev, 0);
> > -       if (IS_ERR(cmdq->base))
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> The devm_platform_ioremap_resource() helper was added on purpose [1].
> Please stick to it unless you have a strong reason to re-split it.
> 
> [1]: a04f30356e75 ("mailbox: mtk-cmdq: Make use of the helper
> function
> devm_platform_ioremap_resource()")
> 
OK, because want to get the pa of GCE by resource like this:
gce_sec_plat.base_pa = res->start;

I'll find another way to do that.

> > +       if (!res)
> > +               return -EINVAL;
> > +
> > +       cmdq->base = devm_ioremap_resource(dev, res);
> > +       if (IS_ERR(cmdq->base)) {
> > +               dev_err(dev, "failed to ioremap cmdq\n");
> >                 return PTR_ERR(cmdq->base);
> > +       }
> >
> >         cmdq->irq = platform_get_irq(pdev, 0);
> >         if (cmdq->irq < 0)
> > @@ -585,6 +595,8 @@ static int cmdq_probe(struct platform_device
> *pdev)
> >                 dev, cmdq->base, cmdq->irq);
> >
> >         if (cmdq->pdata->gce_num > 1) {
> > +               hwid = of_alias_get_id(dev->of_node, clk_name);
> Why get hwid here while it's only used in the has_sec branch?

Because I want to pass the hwid to secure world and let it know what
GCE pa should use in TEE rather than passing GCE pa.

> 
> > +
> >                 for_each_child_of_node(phandle->parent, node) {
> >                         alias_id = of_alias_get_id(node, clk_name);
> >                         if (alias_id >= 0 && alias_id < cmdq-
> >pdata->gce_num) {
> > @@ -653,6 +665,30 @@ static int cmdq_probe(struct platform_device
> *pdev)
> >                 return err;
> >         }
> >
> > +       if (cmdq->pdata->has_sec) {
> > +               struct cmdq_sec_plat gce_sec_plat;
> > +
> > +               if (of_property_read_u32_index(dev->of_node,
> "mediatek,gce-events", 0,
> >
> +                                              &gce_sec_plat.cmdq_eve
> nt) == 0) {
> > +                       gce_sec_plat.gce_dev = dev;
> > +                       gce_sec_plat.base = cmdq->base;
> > +                       gce_sec_plat.base_pa = res->start;
> > +                       gce_sec_plat.hwid = hwid;
> > +                       gce_sec_plat.gce_num = cmdq->pdata-
> >gce_num;
> > +                       gce_sec_plat.clocks = cmdq->clocks;
> > +                       gce_sec_plat.thread_nr = cmdq->pdata-
> >thread_nr;
> > +
> > +                       mtk_cmdq_sec =
> platform_device_register_data(dev, "mtk_cmdq_sec",
> >
> +                                                                    
> PLATFORM_DEVID_AUTO,
> >
> +                                                                    
> &gce_sec_plat,
> >
> +                                                                    
> sizeof(gce_sec_plat));
> > +                       if (IS_ERR(mtk_cmdq_sec)) {
> > +                               dev_err(dev, "failed to register
> platform_device mtk_cmdq_sec\n");
> > +                               return PTR_ERR(mtk_cmdq_sec);
> > +                       }
> > +               }
> > +       }
> > +
> >         return 0;
> >  }
> >
> > @@ -693,6 +729,7 @@ static const struct gce_plat gce_plat_v6 = {
> >         .thread_nr = 24,
> >         .shift = 3,
> >         .control_by_sw = true,
> > +       .has_sec = true,
> >         .gce_num = 2
> >  };
> >
> > @@ -708,6 +745,7 @@ static const struct gce_plat gce_plat_v8 = {
> >         .thread_nr = 32,
> >         .shift = 3,
> >         .control_by_sw = true,
> > +       .has_sec = true,
> >         .gce_num = 2
> >  };
> >
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h
> b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index f78a08e7c6ed..fdda995a69ce 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -79,6 +79,17 @@ struct cmdq_pkt {
> >         bool                    loop;
> >  };
> >
> > +struct cmdq_sec_plat {
> > +       struct device *gce_dev;
> > +       void __iomem *base;
> > +       dma_addr_t base_pa;
> > +       u32 hwid;
> > +       u32 gce_num;
> > +       struct clk_bulk_data *clocks;
> > +       u32 thread_nr;
> > +       u32 cmdq_event;
> > +};
> I feel this should be in mtk-cmdq-sec-mailbox.h.
> 
That means we have to include <mtk-cmdq-sec-mailbox.h> in this file,
but actually mtk-cmdq-sec-mailbox.h have to include <mtk-cmdq-
mailbox.h>. So I add this here finally.

But I agree with you, I'll try to move this struct into mtk-cmdq-sec-
mailbox.h and include <mtk-cmdq-sec-mailbox.h> in mtk-cmdq-mailbox.c.

Regards,
Jason-JH.Lin

> Regards,
> Fei
> 
> 
> 
> > +
> >  u8 cmdq_get_shift_pa(struct mbox_chan *chan);
> >
> >  #endif /* __MTK_CMDQ_MAILBOX_H__ */
> > --
> > 2.18.0
> >
> >
> 

Re: [PATCH v2 8/9] mailbox: mediatek: Add CMDQ secure mailbox driver

[AngeloGioacchino Del Regno]

Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> To support secure video path feature, GCE have to read/write registgers
> in the secure world. GCE will enable the secure access permission to the
> HW who wants to access the secure content buffer.
> 
> Add CMDQ secure mailbox driver to make CMDQ client user is able to
> sending their HW settings to the secure world. So that GCE can execute
> all instructions to configure HW in the secure world.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>

This driver needs to be cleaned up quite a bit, and I'd expect this to take a few
versions before being upstreamable.

> ---
>   drivers/mailbox/Makefile                      |    2 +-
>   drivers/mailbox/mtk-cmdq-sec-mailbox.c        | 1102 +++++++++++++++++
>   drivers/mailbox/mtk-cmdq-sec-tee.c            |  202 +++
>   include/linux/mailbox/mtk-cmdq-mailbox.h      |    2 +
>   .../linux/mailbox/mtk-cmdq-sec-iwc-common.h   |  293 +++++
>   include/linux/mailbox/mtk-cmdq-sec-mailbox.h  |   83 ++
>   include/linux/mailbox/mtk-cmdq-sec-tee.h      |   31 +
>   7 files changed, 1714 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/mailbox/mtk-cmdq-sec-mailbox.c
>   create mode 100644 drivers/mailbox/mtk-cmdq-sec-tee.c
>   create mode 100644 include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
>   create mode 100644 include/linux/mailbox/mtk-cmdq-sec-mailbox.h
>   create mode 100644 include/linux/mailbox/mtk-cmdq-sec-tee.h
> 
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index fc9376117111..82da2f4ee81a 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -51,7 +51,7 @@ obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
>   
>   obj-$(CONFIG_MTK_ADSP_MBOX)	+= mtk-adsp-mailbox.o
>   
> -obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
> +obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o mtk-cmdq-sec-mailbox.o mtk-cmdq-sec-tee.o
>   
>   obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
>   
> diff --git a/drivers/mailbox/mtk-cmdq-sec-mailbox.c b/drivers/mailbox/mtk-cmdq-sec-mailbox.c
> new file mode 100644
> index 000000000000..dac867386cf1
> --- /dev/null
> +++ b/drivers/mailbox/mtk-cmdq-sec-mailbox.c
> @@ -0,0 +1,1102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2019 MediaTek Inc.
> +

Fix comment style please:

// SPDX...
/*
  * Copyright ...
  */

> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/io.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/of_platform.h>
> +#include <linux/sched/clock.h>
> +#include <linux/timer.h>
> +
> +#include <linux/mailbox/mtk-cmdq-sec-mailbox.h>
> +
> +#define CMDQ_THR_BASE		(0x100)
> +#define CMDQ_THR_SIZE		(0x80)

Those two definitions are the same as in mtk-cmdq-mailbox.c: please avoid code
duplication around, it's harder to maintain.

> +#define CMDQ_THR_EXEC_CNT_PA	(0x28)
> +
> +#define CMDQ_BUF_ALLOC_SIZE		(PAGE_SIZE)
> +#define CMDQ_TIMEOUT_DEFAULT		(1000)
> +#define CMDQ_NO_TIMEOUT			(0xffffffff)
> +#define ADDR_METADATA_MAX_COUNT_ORIGIN	(8)
> +
> +/*
> + * CMDQ secure context struct
> + * note it is not global data, each process has its own CMDQ sec context
> + */

This structure is practically fully documented: please use kerneldoc at this point.

> +struct cmdq_sec_context {
> +	struct list_head		listEntry;

listEntry -> list_entry or listentry - no camelcase please.

> +
> +	/* basic info */
> +	u32				tgid; /* tgid of process context */
> +	u32				referCount; /* reference count for open cmdq device node */
> +

referCount -> refcount?

> +	/* iwc state */
> +	enum cmdq_iwc_state_enum	state;

iwc_state

> +
> +	/* iwc information */
> +	void				*iwc_msg; /* message buffer */
> +
> +	struct cmdq_sec_tee_context	tee; /* trustzone parameters */

tee_ctx

> +};
> +
> +struct cmdq_sec_task {
> +	struct list_head	list_entry;
> +	dma_addr_t		pa_base;
> +	struct cmdq_sec_thread	*thread;
> +	struct cmdq_pkt		*pkt;
> +	u64			exec_time;
> +	struct work_struct	exec_work;
> +
> +	bool			resetExecCnt;

Again no camelcase - here and everywhere else.

Also, I see a pattern in all of the _sec_ structures: those seem to be the same
as the non-sec ones, as defined in mtk-cmdq-mailbox, but with more params.

I understand the memory concerns, but there's an easy way out of this... we could
get this done like:

struct cmdq_sec_task {
	struct cmdq_task	tsk;

	/* Secure CMDQ */
	u32			wait_cookie;
	s32			scenario;
	u64			engine_flag;
	u64			trigger;
}

...and this can be done for all of the _sec_ variants of the cmdq mbox structs
as far as I can see.

> +	u32			waitCookie;
> +
> +	u64			engineFlag;
> +	s32			scenario;
> +	u64			trigger;
> +};
> +
> +struct cmdq_sec_thread {
> +	struct mbox_chan	*chan;
> +	void __iomem		*base;
> +	struct list_head	task_list;
> +	struct timer_list	timeout;
> +	u32			timeout_ms;
> +	struct work_struct	timeout_work;
> +	u32			priority;
> +	u32			idx;
> +	bool			occupied;
> +
> +	/* following part only secure ctrl */
> +	u32			wait_cookie;
> +	u32			next_cookie;
> +	u32			task_cnt;
> +	struct workqueue_struct	*task_exec_wq;
> +};
> +
> +/**

This is not valid kerneldoc.

> + * shared memory between normal and secure world
> + */
> +struct cmdq_sec_shared_mem {
> +	void		*va;
> +	dma_addr_t	pa;
> +	u32		size;
> +};
> +
> +struct cmdq_sec {
> +	struct mbox_controller		mbox;
> +	void __iomem			*base;
> +	phys_addr_t			base_pa;
> +	u32				hwid;
> +	u32				gce_num;
> +	struct clk_bulk_data		*clocks;
> +	u32				thread_nr;
> +	struct cmdq_sec_thread		*thread;
> +	struct cmdq_client		*clt;
> +	struct cmdq_pkt			*clt_pkt;
> +
> +	atomic_t			path_res;
> +	struct cmdq_sec_shared_mem	*shared_mem;
> +	struct cmdq_sec_context		*context;
> +	struct iwcCmdqCancelTask_t	cancel;
> +
> +	struct workqueue_struct		*timeout_wq;
> +	u64				sec_invoke;
> +	u64				sec_done;
> +
> +	bool				notify_run;
> +	struct work_struct		irq_notify_work;
> +	struct workqueue_struct		*notify_wq;
> +	u32				cmdq_event;
> +	/* mutex for cmdq_sec_thread excuting cmdq_sec_task */
> +	struct mutex			exec_lock;
> +};
> +
> +static atomic_t cmdq_path_res = ATOMIC_INIT(0);
> +
> +static int cmdq_sec_task_submit(struct cmdq_sec *cmdq, struct cmdq_sec_task *task,
> +				const u32 iwc_cmd, const u32 thrd_idx, void *data);
> +
> +static inline void cmdq_sec_setup_tee_context_base(struct cmdq_sec_context *context)

You don't need this function, as it's just wrapping over another one: you can just
call cmdq_sec_setup_tee_context.

> +{
> +	cmdq_sec_setup_tee_context(&context->tee);
> +}
> +
> +static inline int cmdq_sec_init_context_base(struct cmdq_sec_context *context)
> +{
> +	int status;
> +
> +	status = cmdq_sec_init_context(&context->tee);
> +	if (status < 0)
> +		return status;

if (a < 0) return a;
else return a;

really? :-)

Besides, again, you don't need this wrapper function.

> +
> +	return status;
> +}
> +
> +int cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_client *cl = (struct cmdq_client *)pkt->cl;
> +	struct cmdq_sec_thread *thread = ((struct mbox_chan *)(cl->chan))->con_priv;
> +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct cmdq_sec, mbox);
> +	struct cmdq_operand left, right;
> +	dma_addr_t addr;
> +
> +	if (!thread->occupied || !cmdq->shared_mem)
> +		return -EFAULT;
> +
> +	pr_debug("%s %d: pkt:%p thread:%u gce:%#lx",

All those pr_xxxx shall become dev_debug, dev_err, etc.

> +		 __func__, __LINE__, pkt, thread->idx, (unsigned long)cmdq->base_pa);
> +
> +	addr = (u32)(cmdq->base_pa + CMDQ_THR_BASE +
> +		CMDQ_THR_SIZE * thread->idx + CMDQ_THR_EXEC_CNT_PA);
> +
> +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_HIGH(addr));
> +	cmdq_pkt_read_s(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_LOW(addr), CMDQ_THR_SPR_IDX1);
> +
> +	left.reg = true;
> +	left.idx = CMDQ_THR_SPR_IDX1;
> +	right.reg = false;
> +	right.value = 1;
> +	cmdq_pkt_logic_command(pkt, CMDQ_LOGIC_ADD, CMDQ_THR_SPR_IDX1, &left, &right);
> +
> +	addr = cmdq->shared_mem->pa + CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> +		thread->idx * sizeof(u32);

Fits in one line.

> +
> +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_HIGH(addr));
> +	cmdq_pkt_write_s(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_LOW(addr), CMDQ_THR_SPR_IDX1);
> +	cmdq_pkt_set_event(pkt, cmdq->cmdq_event);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(cmdq_sec_insert_backup_cookie);
> +
> +static int cmdq_sec_realloc_addr_list(struct cmdq_pkt *pkt, const u32 count)
> +{
> +	struct cmdq_sec_data *sec_data = (struct cmdq_sec_data *)pkt->sec_data;
> +	void *prev = (void *)(unsigned long)sec_data->addrMetadatas, *curr;
> +
> +	if (count <= sec_data->addrMetadataMaxCount)
> +		return 0;
> +
> +	curr = kcalloc(count, sizeof(*sec_data), GFP_KERNEL);
> +	if (!curr)
> +		return -ENOMEM;
> +
> +	if (count && sec_data->addrMetadatas)
> +		memcpy(curr, prev, sizeof(*sec_data) * sec_data->addrMetadataMaxCount);
> +
> +	kfree(prev);
> +
> +	sec_data->addrMetadatas = (uintptr_t)curr;
> +	sec_data->addrMetadataMaxCount = count;
> +	return 0;
> +}
> +
> +static int cmdq_sec_check_sec(struct cmdq_pkt *pkt)

This is not just a check: it is *allocating* memory, so please either find a better
name for this function, or just make sure that the sec_data is already allocated
before calling into functions that are calling this one, as I can foresee memory
leaks because of confusion deriving from this function.

> +{
> +	struct cmdq_sec_data *sec_data;
> +
> +	if (pkt->sec_data)
> +		return 0;
> +
> +	sec_data = kzalloc(sizeof(*sec_data), GFP_KERNEL);
> +	if (!sec_data)
> +		return -ENOMEM;
> +
> +	pkt->sec_data = (void *)sec_data;
> +
> +	return 0;
> +}
> +
> +static int cmdq_sec_append_metadata(struct cmdq_pkt *pkt,
> +				    const enum cmdq_iwc_addr_metadata_type type,
> +				    const u64 base, const u32 offset, const u32 size,
> +				    const u32 port)
> +{
> +	struct cmdq_sec_data *sec_data;
> +	struct iwcCmdqAddrMetadata_t *meta;

No camelcase please.

> +	int idx, max, ret;
> +
> +	pr_debug("[%s %d] pkt:%p type:%u base:%#llx offset:%#x size:%#x port:%#x",
> +		 __func__, __LINE__, pkt, type, base, offset, size, port);
> +
> +	ret = cmdq_sec_check_sec(pkt);
> +	if (ret < 0)
> +		return ret;
> +
> +	sec_data = (struct cmdq_sec_data *)pkt->sec_data;
> +	idx = sec_data->addrMetadataCount;
> +	if (idx >= CMDQ_IWC_MAX_ADDR_LIST_LENGTH) {
> +		pr_err("idx:%u reach over:%u", idx, CMDQ_IWC_MAX_ADDR_LIST_LENGTH);
> +		return -EFAULT;
> +	}
> +
> +	if (!sec_data->addrMetadataMaxCount)
> +		max = ADDR_METADATA_MAX_COUNT_ORIGIN;
> +	else if (idx >= sec_data->addrMetadataMaxCount)
> +		max = sec_data->addrMetadataMaxCount * 2;
> +	else
> +		max = sec_data->addrMetadataMaxCount;
> +
> +	ret = cmdq_sec_realloc_addr_list(pkt, max);
> +	if (ret)
> +		return ret;
> +
> +	if (!sec_data->addrMetadatas) {
> +		pr_info("addrMetadatas is missing");
> +
> +		meta = kzalloc(sizeof(*meta), GFP_KERNEL);
> +		if (!meta)
> +			return -ENOMEM;
> +
> +		sec_data->addrMetadatas = (uintptr_t)(void *)meta;
> +	}
> +	meta = (struct iwcCmdqAddrMetadata_t *)(uintptr_t)sec_data->addrMetadatas;
> +
> +	meta[idx].instrIndex = pkt->cmd_buf_size / CMDQ_INST_SIZE - 1;
> +	meta[idx].type = type;
> +	meta[idx].baseHandle = base;
> +	meta[idx].offset = offset;
> +	meta[idx].size = size;
> +	meta[idx].port = port;
> +	sec_data->addrMetadataCount += 1;
> +	return 0;
> +}
> +
> +int cmdq_sec_pkt_set_data(struct cmdq_pkt *pkt, const u64 dapc_engine,
> +			  const u64 port_sec_engine, const enum cmdq_sec_scenario scenario)
> +{
> +	struct cmdq_sec_data *sec_data;
> +	int ret;
> +
> +	if (!pkt) {
> +		pr_err("invalid pkt:%p", pkt);
> +		return -EINVAL;
> +	}
> +
> +	ret = cmdq_sec_check_sec(pkt);
> +	if (ret < 0)
> +		return ret;
> +
> +	pr_debug("[%s %d] pkt:%p sec_data:%p dapc:%llu port_sec:%llu scen:%u",
> +		 __func__, __LINE__, pkt, pkt->sec_data, dapc_engine, port_sec_engine, scenario);
> +
> +	sec_data = (struct cmdq_sec_data *)pkt->sec_data;
> +	sec_data->enginesNeedDAPC |= dapc_engine;
> +	sec_data->enginesNeedPortSecurity |= port_sec_engine;
> +	sec_data->scenario = scenario;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(cmdq_sec_pkt_set_data);
> +
> +int cmdq_sec_pkt_write(struct cmdq_pkt *pkt, u32 addr, u64 base,
> +		       const enum cmdq_iwc_addr_metadata_type type,
> +		       const u32 offset, const u32 size, const u32 port)
> +{
> +	int ret;
> +
> +	ret = cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX0, addr);
> +	if (ret)
> +		return ret;
> +
> +	ret = cmdq_pkt_write_s_reg_value(pkt, CMDQ_THR_SPR_IDX0, (u32)base);
> +	if (ret)
> +		return ret;
> +
> +	return cmdq_sec_append_metadata(pkt, type, base, offset, size, port);
> +}
> +EXPORT_SYMBOL(cmdq_sec_pkt_write);
> +
> +static u32 cmdq_sec_get_cookie(struct cmdq_sec *cmdq, u32 idx)
> +{
> +	return *(u32 *)(cmdq->shared_mem->va +
> +		CMDQ_SEC_SHARED_THR_CNT_OFFSET + idx * sizeof(u32));
> +}
> +
> +static void cmdq_sec_task_done(struct cmdq_sec_task *task, int sta)
> +{
> +	struct cmdq_cb_data data;
> +
> +	data.sta = sta;
> +	data.pkt = task->pkt;
> +
> +	pr_debug("%s task:%p pkt:%p err:%d",
> +		 __func__, task, task->pkt, sta);
> +
> +	mbox_chan_received_data(task->thread->chan, &data);
> +
> +	list_del_init(&task->list_entry);
> +	kfree(task);
> +}
> +
> +static bool cmdq_sec_irq_handler(struct cmdq_sec_thread *thread, const u32 cookie, const int err)
> +{
> +	struct cmdq_sec_task *task, *temp, *cur_task = NULL;
> +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct cmdq_sec, mbox);
> +	unsigned long flags;
> +	int done;
> +
> +	spin_lock_irqsave(&thread->chan->lock, flags);
> +	if (thread->wait_cookie <= cookie)
> +		done = cookie - thread->wait_cookie + 1;
> +	else if (thread->wait_cookie == (cookie + 1) % CMDQ_MAX_COOKIE_VALUE)
> +		done = 0;
> +	else
> +		done = CMDQ_MAX_COOKIE_VALUE - thread->wait_cookie + 1 +
> +			cookie + 1;
> +
> +	list_for_each_entry_safe(task, temp, &thread->task_list, list_entry) {
> +		if (!done)
> +			break;
> +
> +		cmdq_sec_task_done(task, err);
> +
> +		if (thread->task_cnt)
> +			thread->task_cnt -= 1;
> +
> +		done--;
> +	}
> +
> +	cur_task = list_first_entry_or_null(&thread->task_list,
> +					    struct cmdq_sec_task, list_entry);
> +
> +	if (err && cur_task) {
> +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> +
> +		/* for error task, cancel, callback and done */
> +		memset(&cmdq->cancel, 0, sizeof(cmdq->cancel));
> +		cmdq_sec_task_submit(cmdq, cur_task, CMD_CMDQ_IWC_CANCEL_TASK,
> +				     thread->idx, &cmdq->cancel);
> +
> +		cmdq_sec_task_done(task, err);
> +
> +		spin_lock_irqsave(&thread->chan->lock, flags);
> +
> +		task = list_first_entry_or_null(&thread->task_list,
> +						struct cmdq_sec_task, list_entry);
> +		if (cur_task == task)
> +			cmdq_sec_task_done(cur_task, err);
> +		else
> +			pr_err("task list changed");
> +
> +		/*
> +		 * error case stop all task for secure,
> +		 * since secure tdrv always remove all when cancel
> +		 */
> +		while (!list_empty(&thread->task_list)) {
> +			cur_task = list_first_entry(&thread->task_list,
> +						    struct cmdq_sec_task, list_entry);
> +
> +			cmdq_sec_task_done(cur_task, -ECONNABORTED);
> +		}
> +	} else if (err) {
> +		pr_debug("error but all task done, check notify callback");
> +	}
> +
> +	if (list_empty(&thread->task_list)) {
> +		thread->wait_cookie = 0;
> +		thread->next_cookie = 0;
> +		thread->task_cnt = 0;
> +		__raw_writel(0, (void __iomem *)cmdq->shared_mem->va +
> +			     CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> +			     thread->idx * sizeof(u32));
> +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> +		del_timer(&thread->timeout);
> +		return true;
> +	}
> +
> +	thread->wait_cookie = cookie % CMDQ_MAX_COOKIE_VALUE + 1;
> +
> +	mod_timer(&thread->timeout, jiffies + msecs_to_jiffies(thread->timeout_ms));
> +	spin_unlock_irqrestore(&thread->chan->lock, flags);
> +
> +	return false;
> +}
> +
> +static void cmdq_sec_irq_notify_work(struct work_struct *work_item)
> +{
> +	struct cmdq_sec *cmdq = container_of(work_item, struct cmdq_sec, irq_notify_work);
> +	int i;
> +
> +	mutex_lock(&cmdq->exec_lock);
> +
> +	for (i = 0; i < CMDQ_MAX_SECURE_THREAD_COUNT; i++) {
> +		struct cmdq_sec_thread *thread = &cmdq->thread[CMDQ_MIN_SECURE_THREAD_ID + i];
> +		u32 cookie = cmdq_sec_get_cookie(cmdq, thread->idx);
> +
> +		if (cookie < thread->wait_cookie || !thread->task_cnt)
> +			continue;
> +
> +		cmdq_sec_irq_handler(thread, cookie, 0);
> +	}
> +
> +	mutex_unlock(&cmdq->exec_lock);
> +}
> +
> +static void cmdq_sec_irq_notify_callback(struct mbox_client *cl, void *mssg)
> +{
> +	struct cmdq_cb_data *data = (struct cmdq_cb_data *)mssg;
> +	struct cmdq_sec *cmdq = container_of(((struct cmdq_client *)data->pkt->cl)->chan->mbox,
> +					     struct cmdq_sec, mbox);
> +

	if (work_pending...) {
		dev_dbg
		return;
	}

	queue_work

> +	if (!work_pending(&cmdq->irq_notify_work))
> +		queue_work(cmdq->notify_wq, &cmdq->irq_notify_work);
> +	else
> +		pr_debug("%s last notify callback working", __func__);
> +}
> +
> +static int cmdq_sec_irq_notify_start(struct cmdq_sec *cmdq)
> +{
> +	int err;
> +
> +	if (cmdq->notify_run)
> +		return 0;
> +
> +	if (!cmdq->clt_pkt) {
> +		cmdq->clt = cmdq_mbox_create(cmdq->mbox.dev, 0);
> +		if (!cmdq->clt || IS_ERR(cmdq->clt)) {
> +			pr_err("clt mbox_create failed clt:%p index:%d",
> +			       cmdq->clt, CMDQ_SEC_IRQ_THREAD);
> +			return -EINVAL;
> +		}
> +		cmdq->clt->client.rx_callback = cmdq_sec_irq_notify_callback;
> +
> +		cmdq->clt_pkt = cmdq_pkt_create(cmdq->clt, CMDQ_BUF_ALLOC_SIZE);
> +		if (!cmdq->clt_pkt || IS_ERR(cmdq->clt_pkt)) {
> +			pr_err("clt_pkt cmdq_pkt_create failed pkt:%p index:%d",
> +			       cmdq->clt_pkt, CMDQ_SEC_IRQ_THREAD);
> +			return -EINVAL;
> +		}
> +
> +		INIT_WORK(&cmdq->irq_notify_work, cmdq_sec_irq_notify_work);
> +	}
> +
> +	cmdq_pkt_wfe(cmdq->clt_pkt, cmdq->cmdq_event, true);
> +	cmdq_pkt_finalize_loop(cmdq->clt_pkt);
> +	dma_sync_single_for_device(cmdq->mbox.dev,
> +				   cmdq->clt_pkt->pa_base,
> +				   cmdq->clt_pkt->cmd_buf_size,
> +				   DMA_TO_DEVICE);
> +	err = mbox_send_message(cmdq->clt->chan, cmdq->clt_pkt);
> +	mbox_client_txdone(cmdq->clt->chan, 0);
> +	if (err < 0) {
> +		pr_err("%s failed:%d", __func__, err);
> +		cmdq_pkt_destroy(cmdq->clt_pkt);
> +		cmdq_mbox_destroy(cmdq->clt);

		return err;
	}
	cmdq->notify_run = true;

> +	} else {
> +		cmdq->notify_run = true;
> +		pr_debug("%s success!", __func__);
> +	}
> +
> +	return err;

return 0 here

> +}
> +
> +static int cmdq_sec_session_init(struct cmdq_sec_context *context)
> +{
> +	int err = 0;
> +
> +	if (context->state >= IWC_SES_OPENED) {
> +		pr_debug("session opened:%u", context->state);
> +		return err;

		return 0;

> +	}
> +
> +	switch (context->state) {
> +	case IWC_INIT:
> +		err = cmdq_sec_init_context_base(context);
> +		if (err)
		if (err)
			return err;

> +			break;
> +		context->state = IWC_CONTEXT_INITED;
> +	fallthrough;
> +	case IWC_CONTEXT_INITED:
> +		if (context->iwc_msg) {
> +			pr_err("iwcMessage not NULL:%p", context->iwc_msg);
> +			err = -EINVAL;

return -EINVAL;

> +			break;
> +		}
> +
> +		err = cmdq_sec_allocate_wsm(&context->tee, &context->iwc_msg,
> +					    sizeof(struct iwcCmdqMessage_t));
> +		if (err)
> +			break;

return err

> +
> +		context->state = IWC_WSM_ALLOCATED;
> +	fallthrough;
> +	case IWC_WSM_ALLOCATED:
> +		err = cmdq_sec_open_session(&context->tee, context->iwc_msg);
> +		if (err)
> +			break;

return err

> +
> +		context->state = IWC_SES_OPENED;
> +	fallthrough;
> +	default:
> +		break;
> +	}

	return 0;

> +	return err;
> +}
> +
> +static int cmdq_sec_fill_iwc_msg(struct cmdq_sec_context *context,
> +				 struct cmdq_sec_task *task, u32 thrd_idx)
> +{
> +	struct iwcCmdqMessage_t *iwc_msg = NULL;
> +	struct cmdq_sec_data *data = (struct cmdq_sec_data *)task->pkt->sec_data;
> +	u32 size = 0, offset = 0, *instr;
> +
> +	iwc_msg = (struct iwcCmdqMessage_t *)context->iwc_msg;
> +
> +	if (task->pkt->cmd_buf_size + 4 * CMDQ_INST_SIZE > CMDQ_TZ_CMD_BLOCK_SIZE) {
> +		pr_err("task:%p size:%zu > %u",
> +		       task, task->pkt->cmd_buf_size, CMDQ_TZ_CMD_BLOCK_SIZE);
> +		return -EFAULT;
> +	}
> +
> +	if (thrd_idx == CMDQ_INVALID_THREAD) {
> +		iwc_msg->command.commandSize = 0;
> +		iwc_msg->command.metadata.addrListLength = 0;
> +		return 0;

if invalid thread, return 0?! Are you sure?!

> +	}
> +

	if (iwc_msg->command.size == 0 && task->pkt->cmd_buf_size == 0) {
		dev_dbg(...... command size is zero)

		return (something);
	}

> +	iwc_msg->command.thread = thrd_idx;
> +	iwc_msg->command.scenario = task->scenario;
> +	iwc_msg->command.engineFlag = task->engineFlag;
> +	size = task->pkt->cmd_buf_size;
> +	memcpy(iwc_msg->command.pVABase + offset, task->pkt->va_base, size);
> +	iwc_msg->command.commandSize += size;
> +	offset += size / 4;
> +
> +	if (iwc_msg->command.commandSize == 0) {
> +		pr_err("%s %d: commandSize is 0\n", __func__, __LINE__);

So this being zero is valid? Again, are you sure?

> +		return 0;
> +	}
> +
> +	instr = &iwc_msg->command.pVABase[iwc_msg->command.commandSize / 4 - 4];
> +	if (instr[0] == 0x1 && instr[1] == 0x40000000)

Please define this, I guess that instr[] is a 64-bits instruction, is it?

So this could be... #define SOMETHING 0x140000000ULL
...but however you choose to define it is fine, just .. define it please :-)

> +		instr[0] = 0;
> +	else
> +		pr_err("%s %d: find EOC failed: %#x %#x",
> +		       __func__, __LINE__, instr[1], instr[0]);
> +
> +	iwc_msg->command.waitCookie = task->waitCookie;
> +	iwc_msg->command.resetExecCnt = task->resetExecCnt;
> +
> +	if (data->addrMetadataCount) {
> +		iwc_msg->command.metadata.addrListLength = data->addrMetadataCount;
> +		memcpy(iwc_msg->command.metadata.addrList,
> +		       (u32 *)(unsigned long)data->addrMetadatas,
> +		       data->addrMetadataCount * sizeof(struct iwcCmdqAddrMetadata_t));
> +	}
> +
> +	iwc_msg->command.metadata.enginesNeedDAPC = data->enginesNeedDAPC;
> +	iwc_msg->command.metadata.enginesNeedPortSecurity =
> +		data->enginesNeedPortSecurity;
> +	iwc_msg->command.hNormalTask = (unsigned long)task->pkt;
> +
> +	return 0;
> +}
> +
> +static int cmdq_sec_session_send(struct cmdq_sec_context *context,
> +				 struct cmdq_sec_task *task, const u32 iwc_cmd,
> +				 const u32 thrd_idx, struct cmdq_sec *cmdq)
> +{
> +	int err = 0;
> +	u64 cost;
> +	struct iwcCmdqMessage_t *iwc_msg = NULL;
> +
> +	iwc_msg = (struct iwcCmdqMessage_t *)context->iwc_msg;
> +
> +	memset(iwc_msg, 0, sizeof(*iwc_msg));
> +	iwc_msg->cmd = iwc_cmd;
> +	iwc_msg->cmdq_id = cmdq->hwid;
> +	iwc_msg->command.thread = thrd_idx;
> +
> +	switch (iwc_cmd) {
> +	case CMD_CMDQ_IWC_SUBMIT_TASK:
> +		err = cmdq_sec_fill_iwc_msg(context, task, thrd_idx);
> +		if (err)
> +			return err;
> +		break;
> +	case CMD_CMDQ_IWC_CANCEL_TASK:
> +		iwc_msg->cancelTask.waitCookie = task->waitCookie;
> +		iwc_msg->cancelTask.thread = thrd_idx;
> +		break;
> +	case CMD_CMDQ_IWC_PATH_RES_ALLOCATE:
> +		if (!cmdq->shared_mem || !cmdq->shared_mem->va) {
> +			pr_err("%s %d: shared_mem is NULL", __func__, __LINE__);
> +			return -EFAULT;
> +		}
> +		iwc_msg->pathResource.size = cmdq->shared_mem->size;
> +		iwc_msg->pathResource.shareMemoyPA = cmdq->shared_mem->pa;
> +		iwc_msg->pathResource.useNormalIRQ = 1;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	cmdq->sec_invoke = sched_clock();
> +	pr_debug("%s execute cmdq:%p task:%lx command:%u thread:%u cookie:%d",
> +		 __func__, cmdq, (unsigned long)task, iwc_cmd, thrd_idx,
> +		 task ? task->waitCookie : -1);
> +
> +	/* send message */
> +	err = cmdq_sec_execute_session(&context->tee, iwc_cmd, CMDQ_TIMEOUT_DEFAULT);
> +
> +	cmdq->sec_done = sched_clock();
> +	cost = div_u64(cmdq->sec_done - cmdq->sec_invoke, 1000000);
> +	if (cost >= CMDQ_TIMEOUT_DEFAULT)
> +		pr_err("%s execute timeout cmdq:%p task:%lx cost:%lluus",
> +		       __func__, cmdq, (unsigned long)task, cost);
> +	else
> +		pr_debug("%s execute done cmdq:%p task:%lx cost:%lluus",
> +			 __func__, cmdq, (unsigned long)task, cost);
> +

	if (err)
		return err;

	context->state = IWC_SES_ON_TRANSACTED;
	return 0;

> +	if (err == 0)
> +		context->state = IWC_SES_ON_TRANSACTED;
> +
> +	return err;
> +}
> +
> +static int cmdq_sec_session_reply(const u32 iwc_cmd, struct iwcCmdqMessage_t *iwc_msg,
> +				  void *data, struct cmdq_sec_task *task)
> +{
> +	struct iwcCmdqCancelTask_t *cancel = data;
> +	struct cmdq_sec_data *sec_data = task->pkt->sec_data;
> +
> +	if (iwc_cmd == CMD_CMDQ_IWC_SUBMIT_TASK) {

	if (iwc_cmd == CMD_CMDQ_IWC_SUBMIT_TASK && iwc_msg->rsp < 0) {
		....
	} else if (...) {
		....
	}

	return ...

> +		if (iwc_msg->rsp < 0) {
> +			/* submit fail case copy status */
> +			memcpy(&sec_data->sec_status, &iwc_msg->secStatus,
> +			       sizeof(sec_data->sec_status));
> +			sec_data->response = iwc_msg->rsp;
> +		}
> +	} else if (iwc_cmd == CMD_CMDQ_IWC_CANCEL_TASK && cancel) {
> +		/* cancel case only copy cancel result */
> +		memcpy(cancel, &iwc_msg->cancelTask, sizeof(*cancel));
> +	}
> +
> +	return iwc_msg->rsp;
> +}
> +
> +static int cmdq_sec_task_submit(struct cmdq_sec *cmdq, struct cmdq_sec_task *task,
> +				const u32 iwc_cmd, const u32 thrd_idx, void *data)
> +{
> +	struct cmdq_sec_context *context;
> +	int err;
> +
> +	do {
> +		if (!cmdq->context) {
> +			context = kzalloc(sizeof(*cmdq->context), GFP_ATOMIC);
> +			if (!context) {
> +				err = -ENOMEM;
> +				break;
> +			}
> +			cmdq->context = context;
> +			cmdq->context->state = IWC_INIT;
> +			cmdq->context->tgid = current->tgid;
> +		}
> +
> +		if (cmdq->context->state == IWC_INIT)
> +			cmdq_sec_setup_tee_context_base(cmdq->context);
> +
> +		err = cmdq_sec_session_init(cmdq->context);
> +		if (err) {
> +			pr_err("%s %d: cmdq_sec_session_init fail", __func__, __LINE__);
> +			break;
> +		}
> +
> +		err = cmdq_sec_irq_notify_start(cmdq);
> +		if (err < 0) {
> +			pr_err("%s %d: cmdq_sec_irq_notify_start fail", __func__, __LINE__);
> +			break;
> +		}
> +
> +		err = cmdq_sec_session_send(cmdq->context, task, iwc_cmd, thrd_idx, cmdq);
> +		if (err)
> +			pr_err("%s %d: cmdq_sec_session_send fail: %d\n", __func__, __LINE__, err);
> +		else
> +			err = cmdq_sec_session_reply(iwc_cmd, cmdq->context->iwc_msg, data, task);
> +	} while (0);

Seriously? do { ... } while(0) ?!?!?!

No!!!!

> +
> +	if (err)
> +		pr_err("%s %d: sec invoke err:%d task:%p thread:%u gce:%#lx",
> +		       __func__, __LINE__, err, task, thrd_idx, (unsigned long)cmdq->base_pa);
> +
> +	return err;
> +}
> +
> +static int cmdq_sec_suspend(struct device *dev)
> +{
> +	struct cmdq_sec *cmdq = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable_unprepare(cmdq->gce_num, cmdq->clocks);
> +	return 0;
> +}
> +
> +static int cmdq_sec_resume(struct device *dev)
> +{
> +	struct cmdq_sec *cmdq = dev_get_drvdata(dev);
> +
> +	WARN_ON(clk_bulk_prepare_enable(cmdq->gce_num, cmdq->clocks));
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops cmdq_sec_pm_ops = {
> +	.suspend = cmdq_sec_suspend,
> +	.resume = cmdq_sec_resume,
> +};
> +
> +static void cmdq_sec_task_exec_work(struct work_struct *work_item)
> +{
> +	struct cmdq_sec_task *task = container_of(work_item, struct cmdq_sec_task, exec_work);
> +	struct cmdq_sec *cmdq = container_of(task->thread->chan->mbox, struct cmdq_sec, mbox);
> +	struct cmdq_sec_data *data;
> +	unsigned long flags;
> +	int err;
> +
> +	pr_debug("%s gce:%#lx task:%p pkt:%p thread:%u",
> +		 __func__, (unsigned long)cmdq->base_pa, task, task->pkt, task->thread->idx);
> +
> +	if (!task->pkt->sec_data) {
> +		pr_err("pkt:%p without sec_data", task->pkt);
> +		return;
> +	}
> +	data = (struct cmdq_sec_data *)task->pkt->sec_data;
> +
> +	mutex_lock(&cmdq->exec_lock);
> +
> +	spin_lock_irqsave(&task->thread->chan->lock, flags);
> +	if (!task->thread->task_cnt) {
> +		mod_timer(&task->thread->timeout, jiffies +
> +			msecs_to_jiffies(task->thread->timeout_ms));
> +		task->thread->wait_cookie = 1;
> +		task->thread->next_cookie = 1;
> +		task->thread->task_cnt = 0;
> +		__raw_writel(0, (void __iomem *)cmdq->shared_mem->va +
> +			     CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> +			     task->thread->idx * sizeof(u32));
> +	}
> +
> +	task->resetExecCnt = task->thread->task_cnt ? false : true;
> +	task->waitCookie = task->thread->next_cookie;
> +	task->thread->next_cookie = (task->thread->next_cookie + 1) % CMDQ_MAX_COOKIE_VALUE;
> +	list_add_tail(&task->list_entry, &task->thread->task_list);
> +	task->thread->task_cnt += 1;
> +	spin_unlock_irqrestore(&task->thread->chan->lock, flags);
> +	task->trigger = sched_clock();
> +
> +	if (!atomic_cmpxchg(&cmdq_path_res, 0, 1)) {
> +		err = cmdq_sec_task_submit(cmdq, NULL, CMD_CMDQ_IWC_PATH_RES_ALLOCATE,
> +					   CMDQ_INVALID_THREAD, NULL);
> +		if (err) {
> +			atomic_set(&cmdq_path_res, 0);
> +			goto task_err_callback;
> +		}
> +	}
> +
> +	if (task->thread->task_cnt > CMDQ_MAX_TASK_IN_SECURE_THREAD) {
> +		pr_err("task_cnt:%u cannot more than %u task:%p thrd-idx:%u",
> +		       task->thread->task_cnt, CMDQ_MAX_TASK_IN_SECURE_THREAD,
> +		       task, task->thread->idx);
> +		err = -EMSGSIZE;
> +		goto task_err_callback;
> +	}
> +
> +	err = cmdq_sec_task_submit(cmdq, task, CMD_CMDQ_IWC_SUBMIT_TASK, task->thread->idx, NULL);
> +	if (err)
> +		pr_err("task submit CMD_CMDQ_IWC_SUBMIT_TASK failed:%d gce:%#lx task:%p thread:%u",
> +		       err, (unsigned long)cmdq->base_pa, task, task->thread->idx);
> +
> +task_err_callback:

end:

> +	if (err) {
> +		struct cmdq_cb_data cb_data;
> +
> +		cb_data.sta = err;
> +		cb_data.pkt = task->pkt;
> +		mbox_chan_received_data(task->thread->chan, &cb_data);
> +
> +		spin_lock_irqsave(&task->thread->chan->lock, flags);
> +		if (!task->thread->task_cnt)
> +			pr_err("thread:%u task_cnt:%u cannot below zero",
> +			       task->thread->idx, task->thread->task_cnt);
> +		else
> +			task->thread->task_cnt -= 1;
> +
> +		task->thread->next_cookie = (task->thread->next_cookie - 1 +
> +			CMDQ_MAX_COOKIE_VALUE) % CMDQ_MAX_COOKIE_VALUE;
> +		list_del(&task->list_entry);
> +		pr_debug("gce:%#lx err:%d task:%p pkt:%p thread:%u task_cnt:%u wait_cookie:%u next_cookie:%u",
> +			 (unsigned long)cmdq->base_pa, err, task, task->pkt,
> +			 task->thread->idx, task->thread->task_cnt,
> +			 task->thread->wait_cookie, task->thread->next_cookie);
> +		spin_unlock_irqrestore(&task->thread->chan->lock, flags);
> +		kfree(task);
> +	}
> +	mutex_unlock(&cmdq->exec_lock);
> +}
> +
> +static int cmdq_sec_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct cmdq_pkt *pkt = (struct cmdq_pkt *)data;
> +	struct cmdq_sec_data *sec_data = (struct cmdq_sec_data *)pkt->sec_data;
> +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread *)chan->con_priv;
> +	struct cmdq_sec_task *task;
> +
> +	if (!sec_data)
> +		return -EINVAL;
> +
> +	task = kzalloc(sizeof(*task), GFP_ATOMIC);
> +	if (!task)
> +		return -ENOMEM;
> +
> +	task->pkt = pkt;
> +	task->thread = thread;
> +	task->scenario = sec_data->scenario;
> +	task->engineFlag = sec_data->enginesNeedDAPC | sec_data->enginesNeedPortSecurity;
> +
> +	INIT_WORK(&task->exec_work, cmdq_sec_task_exec_work);
> +	queue_work(thread->task_exec_wq, &task->exec_work);
> +	return 0;
> +}
> +
> +static void cmdq_sec_thread_timeout(struct timer_list *t)
> +{
> +	struct cmdq_sec_thread *thread = from_timer(thread, t, timeout);
> +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct cmdq_sec, mbox);
> +
> +	if (!work_pending(&thread->timeout_work))
> +		queue_work(cmdq->timeout_wq, &thread->timeout_work);
> +}
> +
> +static void cmdq_sec_task_timeout_work(struct work_struct *work_item)
> +{
> +	struct cmdq_sec_thread *thread = container_of(work_item, struct cmdq_sec_thread,
> +						      timeout_work);
> +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct cmdq_sec, mbox);
> +	struct cmdq_sec_task *task;
> +	unsigned long flags;
> +	u64 duration;
> +	u32 cookie;
> +
> +	mutex_lock(&cmdq->exec_lock);
> +
> +	spin_lock_irqsave(&thread->chan->lock, flags);
> +	if (list_empty(&thread->task_list)) {
> +		pr_err("thread:%u task_list is empty", thread->idx);
> +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> +		goto done;
> +	}
> +
> +	task = list_first_entry(&thread->task_list, struct cmdq_sec_task, list_entry);
> +	duration = div_u64(sched_clock() - task->trigger, 1000000);
> +	if (duration < thread->timeout_ms) {
> +		mod_timer(&thread->timeout, jiffies +
> +			  msecs_to_jiffies(thread->timeout_ms - duration));
> +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> +		goto done;
> +	}
> +
> +	cookie = cmdq_sec_get_cookie(cmdq, thread->idx);
> +	spin_unlock_irqrestore(&thread->chan->lock, flags);
> +
> +	pr_err("%s duration:%llu cookie:%u thread:%u",
> +	       __func__, duration, cookie, thread->idx);
> +	cmdq_sec_irq_handler(thread, cookie, -ETIMEDOUT);
> +
> +done:
> +	mutex_unlock(&cmdq->exec_lock);
> +}
> +
> +static int cmdq_sec_mbox_startup(struct mbox_chan *chan)
> +{
> +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread *)chan->con_priv;
> +	char name[20];
> +
> +	timer_setup(&thread->timeout, cmdq_sec_thread_timeout, 0);
> +
> +	INIT_WORK(&thread->timeout_work, cmdq_sec_task_timeout_work);
> +	snprintf(name, sizeof(name), "task_exec_wq_%u", thread->idx);
> +	thread->task_exec_wq = create_singlethread_workqueue(name);
> +	thread->occupied = true;
> +	return 0;
> +}
> +
> +static void cmdq_sec_mbox_shutdown(struct mbox_chan *chan)
> +{
> +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread *)chan->con_priv;
> +
> +	thread->occupied = false;
> +}
> +
> +static int cmdq_sec_mbox_flush(struct mbox_chan *chan, unsigned long timeout)
> +{
> +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread *)chan->con_priv;
> +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct cmdq_sec, mbox);
> +	int i;
> +
> +	mutex_lock(&cmdq->exec_lock);
> +
> +	if (list_empty(&thread->task_list))

if (list_empty..) {
	mutex_unlock(...)
	return 0;
}

> +		goto out;
> +
> +	for (i = 0; i < CMDQ_MAX_SECURE_THREAD_COUNT; i++) {
> +		struct cmdq_sec_thread *thread = &cmdq->thread[CMDQ_MIN_SECURE_THREAD_ID + i];
> +		u32 cookie = cmdq_sec_get_cookie(cmdq, thread->idx);
> +
> +		if (cookie < thread->wait_cookie || !thread->task_cnt)
> +			continue;
> +
> +		cmdq_sec_irq_handler(thread, cookie, -ECONNABORTED);
> +	}
> +
> +out:
> +	mutex_unlock(&cmdq->exec_lock);
> +	return 0;
> +}
> +
> +static bool cmdq_sec_mbox_last_tx_done(struct mbox_chan *chan)
> +{
> +	return true;

Noupe. This is a hack.

You want to do some check here to return if the last TX is done, you can't
just return true and call it a day.

> +}
> +
> +static const struct mbox_chan_ops cmdq_sec_mbox_chan_ops = {
> +	.send_data = cmdq_sec_mbox_send_data,
> +	.startup = cmdq_sec_mbox_startup,
> +	.shutdown = cmdq_sec_mbox_shutdown,
> +	.flush = cmdq_sec_mbox_flush,
> +	.last_tx_done = cmdq_sec_mbox_last_tx_done,
> +};
> +
> +static struct mbox_chan *cmdq_sec_mbox_of_xlate(struct mbox_controller *mbox,
> +						const struct of_phandle_args *sp)
> +{
> +	struct cmdq_sec_thread *thread;
> +	int idx = sp->args[0];
> +
> +	if (mbox->num_chans <= idx) {
> +		pr_err("invalid thrd-idx:%u", idx);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	thread = (struct cmdq_sec_thread *)mbox->chans[idx].con_priv;
> +	thread->chan = &mbox->chans[idx];
> +	thread->timeout_ms = CMDQ_NO_TIMEOUT;
> +	thread->priority = sp->args[1];
> +
> +	return &mbox->chans[idx];
> +}
> +
> +static int cmdq_sec_probe(struct platform_device *pdev)
> +{
> +	int i, err;
> +	struct cmdq_sec *cmdq;
> +	struct cmdq_sec_plat *plat_data;
> +	struct device *dev = &pdev->dev;
> +
> +	plat_data = (struct cmdq_sec_plat *)pdev->dev.platform_data;
> +	if (!plat_data) {
> +		dev_err(dev, "no valid platform data!\n");
> +		return -EINVAL;
> +	}
> +
> +	cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
> +	if (!cmdq)
> +		return -ENOMEM;
> +
> +	cmdq->base_pa = plat_data->base_pa;
> +	cmdq->base = plat_data->base;
> +	cmdq->thread_nr = plat_data->thread_nr;
> +	cmdq->gce_num = plat_data->gce_num;
> +	cmdq->clocks = devm_kcalloc(dev, cmdq->gce_num,
> +				    sizeof(struct clk_bulk_data), GFP_KERNEL);
> +	if (!cmdq->clocks)
> +		return -ENOMEM;
> +
> +	for (i = 0 ; i < cmdq->gce_num; i++)
> +		cmdq->clocks[i] = plat_data->clocks[i];
> +
> +	cmdq->hwid = plat_data->hwid;
> +	cmdq->cmdq_event = plat_data->cmdq_event;
> +
> +	cmdq->mbox.dev = plat_data->gce_dev;
> +	cmdq->mbox.chans = devm_kcalloc(dev, cmdq->thread_nr,
> +					sizeof(*cmdq->mbox.chans), GFP_KERNEL);
> +	if (!cmdq->mbox.chans)
> +		return -ENOMEM;
> +
> +	cmdq->mbox.ops = &cmdq_sec_mbox_chan_ops;
> +	cmdq->mbox.num_chans = cmdq->thread_nr;
> +	cmdq->mbox.of_xlate = cmdq_sec_mbox_of_xlate;
> +
> +	/* make use of TXDONE_BY_ACK */
> +	cmdq->mbox.txdone_irq = false;
> +	cmdq->mbox.txdone_poll = false;
> +
> +	cmdq->thread = devm_kcalloc(dev, cmdq->thread_nr,
> +				    sizeof(*cmdq->thread), GFP_KERNEL);
> +	if (!cmdq->thread)
> +		return -ENOMEM;
> +
> +	mutex_init(&cmdq->exec_lock);
> +	for (i = 0; i < cmdq->thread_nr; i++) {
> +		cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE + CMDQ_THR_SIZE * i;
> +		INIT_LIST_HEAD(&cmdq->thread[i].task_list);
> +		cmdq->thread[i].idx = i;
> +		cmdq->thread[i].occupied = false;
> +		cmdq->mbox.chans[i].con_priv = (void *)&cmdq->thread[i];
> +	}
> +
> +	cmdq->notify_wq = create_singlethread_workqueue("mtk_cmdq_sec_notify_wq");
> +	cmdq->timeout_wq = create_singlethread_workqueue("mtk_cmdq_sec_timeout_wq");
> +	err = devm_mbox_controller_register(dev, &cmdq->mbox);
> +	if (err)
> +		return err;
> +
> +	cmdq->shared_mem = devm_kzalloc(dev, sizeof(*cmdq->shared_mem), GFP_KERNEL);
> +	if (!cmdq->shared_mem)
> +		return -ENOMEM;
> +
> +	cmdq->shared_mem->va = dma_alloc_coherent(dev, PAGE_SIZE,
> +						  &cmdq->shared_mem->pa, GFP_KERNEL);
> +	cmdq->shared_mem->size = PAGE_SIZE;
> +
> +	platform_set_drvdata(pdev, cmdq);
> +	WARN_ON(clk_bulk_prepare_enable(cmdq->gce_num, cmdq->clocks));
> +
> +	return 0;
> +}
> +
> +static int cmdq_sec_remove(struct platform_device *pdev)
> +{
> +	struct cmdq_sec *cmdq = platform_get_drvdata(pdev);
> +
> +	if (cmdq->context)
> +		cmdq_sec_free_wsm(&cmdq->context->tee, &cmdq->context->iwc_msg);
> +
> +	mbox_controller_unregister(&cmdq->mbox);
> +
> +	clk_bulk_disable_unprepare(cmdq->gce_num, cmdq->clocks);
> +	return 0;
> +}
> +
> +static struct platform_driver cmdq_sec_drv = {
> +	.probe = cmdq_sec_probe,
> +	.remove = cmdq_sec_remove,
> +	.driver = {
> +		.name = "mtk_cmdq_sec",

"mtk-cmdq-secure"

> +		.pm = &cmdq_sec_pm_ops,
> +	},
> +};
> +
> +static int __init cmdq_sec_init(void)
> +{
> +	int err;
> +
> +	err = platform_driver_register(&cmdq_sec_drv);
> +	if (err)
> +		pr_err("platform_driver_register failed:%d", err);
> +	return err;
> +}
> +
> +arch_initcall(cmdq_sec_init);

Do you *really* need this to be an arch_initcall?! That's a bit early, and I don't
really see any reason for this.

> +
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mailbox/mtk-cmdq-sec-tee.c b/drivers/mailbox/mtk-cmdq-sec-tee.c
> new file mode 100644
> index 000000000000..e42e9dba343c
> --- /dev/null
> +++ b/drivers/mailbox/mtk-cmdq-sec-tee.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2019 MediaTek Inc.

// SPDX...
/*
  * Copyright..
  */


> +
> +#include <linux/math64.h>
> +#include <linux/sched/clock.h>
> +
> +#include <linux/mailbox/mtk-cmdq-sec-tee.h>
> +#include <linux/soc/mediatek/mtk-cmdq.h>
> +
> +/* lock to protect atomic secure task execution */
> +static DEFINE_MUTEX(cmdq_sec_exec_lock);
> +
> +static void cmdq_sec_lock_secure_path(void)
> +{
> +	mutex_lock(&cmdq_sec_exec_lock);
> +	smp_mb();	/*memory barrier*/

I'm sure that you don't need a memory barrier after locking this mutex.

Besides, you don't need this function at all: just call mutex_lock and mutex_unlock
where appropriate.

> +}
> +
> +static void cmdq_sec_unlock_secure_path(void)
> +{
> +	mutex_unlock(&cmdq_sec_exec_lock);
> +}
> +
> +void cmdq_sec_setup_tee_context(struct cmdq_sec_tee_context *tee)
> +{
> +	/* 09010000 0000 0000 0000000000000000 */
> +	memset(tee->uuid, 0, sizeof(tee->uuid));
> +	tee->uuid[0] = 0x9;
> +	tee->uuid[1] = 0x1;
> +}
> +EXPORT_SYMBOL(cmdq_sec_setup_tee_context);
> +
> +#if IS_ENABLED(CONFIG_TEE)

Can this driver really work if CONFIG_TEE=n ?!

I guess it wouldn't, so this is not right at all. The solution is to depend on
CONFIG_TEE to *even compile* this driver....

> +static int tee_dev_match(struct tee_ioctl_version_data *t, const void *v)
> +{
> +#if IS_ENABLED(CONFIG_OPTEE)
> +	if (t->impl_id == TEE_IMPL_ID_OPTEE)
> +		return 1;
> +#endif
> +	return 0;
> +}
> +#endif
> +

..snip..

> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index fdda995a69ce..997a75c529fb 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -7,6 +7,7 @@
>   #ifndef __MTK_CMDQ_MAILBOX_H__
>   #define __MTK_CMDQ_MAILBOX_H__
>   
> +#include <linux/mailbox_controller.h>

Why are you adding this header?

No implicit inclusions in drivers please.

>   #include <linux/platform_device.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> @@ -77,6 +78,7 @@ struct cmdq_pkt {
>   	size_t			buf_size; /* real buffer size */
>   	void			*cl;
>   	bool			loop;
> +	void			*sec_data;
>   };
>   
>   struct cmdq_sec_plat {
> diff --git a/include/linux/mailbox/mtk-cmdq-sec-iwc-common.h b/include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> new file mode 100644
> index 000000000000..addc515df279
> --- /dev/null
> +++ b/include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> @@ -0,0 +1,293 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + */
> +
> +#ifndef __CMDQ_SEC_IWC_COMMON_H__
> +#define __CMDQ_SEC_IWC_COMMON_H__
> +
> +/*
> + * shared DRAM
> + * bit x = 1 means thread x raise IRQ
> + */
> +#define CMDQ_SEC_SHARED_IRQ_RAISED_OFFSET	0
> +#define CMDQ_SEC_SHARED_THR_CNT_OFFSET		0x100
> +#define CMDQ_SEC_SHARED_TASK_VA_OFFSET		0x200
> +#define CMDQ_SEC_SHARED_OP_OFFSET		0x300
> +#define CMDQ_SEC_SHARED_SUSPEND_CNT		0x304
> +#define CMDQ_SEC_SHARED_RESET_CNT		0x308
> +
> +/* commanad buffer & metadata */
> +#define CMDQ_TZ_CMD_BLOCK_SIZE		(20 << 12) /* MDP readback may request 20 pages */
> +#define CMDQ_IWC_MAX_CMD_LENGTH		(CMDQ_TZ_CMD_BLOCK_SIZE / 4)
> +
> +#define CMDQ_IWC_MAX_ADDR_LIST_LENGTH	(30)
> +
> +#define CMDQ_IWC_CLIENT_NAME		(16)
> +
> +#define CMDQ_SEC_MESSAGE_INST_LEN	(8)
> +#define CMDQ_SEC_DISPATCH_LEN		(8)
> +#define CMDQ_MAX_READBACK_ENG		(8)
> +
> +/*
> + * Command IDs for normal world(TLC or linux kernel) to secure world
> + */
> +/* submit current task */
> +#define CMD_CMDQ_IWC_SUBMIT_TASK	(1)
> +/* (not used)release resource in secure path per session */

If this is unused, it shouldn't be here at all.

Also, you don't need those parenthesis, remove.

> +#define CMD_CMDQ_IWC_RES_RELEASE	(2)
> +/* cancel current task */
> +#define CMD_CMDQ_IWC_CANCEL_TASK	(3)
> +/* create global resource for secure path */
> +#define CMD_CMDQ_IWC_PATH_RES_ALLOCATE	(4)
> +/* destroy globacl resource for secure path */
> +#define CMD_CMDQ_IWC_PATH_RES_RELEASE	(5)
> +
> +/*
> + * ERROR code number (ERRNO)
> + * note the error result returns negative value, i.e, -(ERRNO)
> + */
> +#define CMDQ_ERR_NOMEM					(12) /* out of memory */
> +#define CMDQ_ERR_FAULT					(14) /* bad address */
> +
> +#define CMDQ_ERR_ADDR_CONVERT_HANDLE_2_PA		(1000)
> +#define CMDQ_ERR_ADDR_CONVERT_ALLOC_MVA			(1100)
> +#define CMDQ_ERR_ADDR_CONVERT_ALLOC_MVA_N2S		(1101)
> +#define CMDQ_ERR_ADDR_CONVERT_FREE_MVA			(1200)
> +#define CMDQ_ERR_PORT_CONFIG				(1300)
> +
> +/* param check */
> +#define CMDQ_ERR_UNKNOWN_ADDR_METADATA_TYPE		(1400)
> +#define CMDQ_ERR_TOO_MANY_SEC_HANDLE			(1401)
> +/* security check */
> +#define CMDQ_ERR_SECURITY_INVALID_INSTR			(1500)
> +#define CMDQ_ERR_SECURITY_INVALID_SEC_HANDLE		(1501)
> +#define CMDQ_ERR_SECURITY_INVALID_DAPC_FALG		(1502)
> +#define CMDQ_ERR_INSERT_DAPC_INSTR_FAILED		(1503)
> +#define CMDQ_ERR_INSERT_PORT_SECURITY_INSTR_FAILED	(1504)
> +#define CMDQ_ERR_INVALID_SECURITY_THREAD		(1505)
> +#define CMDQ_ERR_PATH_RESOURCE_NOT_READY		(1506)
> +#define CMDQ_ERR_NULL_TASK				(1507)
> +#define CMDQ_ERR_SECURITY_INVALID_SOP			(1508)
> +#define CMDQ_ERR_SECURITY_INVALID_SEC_PORT_FALG		(1511)
> +

Fully documented enumeration, but no kerneldoc? :-)

> +enum cmdq_iwc_addr_metadata_type {
> +	CMDQ_IWC_H_2_PA		= 0, /* sec handle to sec PA */
> +	CMDQ_IWC_H_2_MVA	= 1, /* sec handle to sec MVA */
> +	CMDQ_IWC_NMVA_2_MVA	= 2, /* map normal MVA to secure world */
> +	CMDQ_IWC_PH_2_MVA	= 3, /* protected handle to sec MVA */
> +};
> +
> +enum cmdq_sec_engine_enum {
> +	/* MDP */
> +	CMDQ_SEC_MDP_RDMA0		= 0,
> +	CMDQ_SEC_MDP_RDMA1		= 1,
> +	CMDQ_SEC_MDP_WDMA		= 2,
> +	CMDQ_SEC_MDP_RDMA2		= 3,
> +	CMDQ_SEC_MDP_RDMA3		= 4,
> +	CMDQ_SEC_MDP_WROT0		= 5,
> +	CMDQ_SEC_MDP_WROT1		= 6,
> +	CMDQ_SEC_MDP_WROT2		= 7,
> +	CMDQ_SEC_MDP_WROT3		= 8,
> +	CMDQ_SEC_MDP_HDR0		= 9,
> +	CMDQ_SEC_MDP_HDR1		= 10,
> +	CMDQ_SEC_MDP_HDR2		= 11,
> +	CMDQ_SEC_MDP_HDR3		= 12,
> +	CMDQ_SEC_MDP_AAL0		= 13,
> +	CMDQ_SEC_MDP_AAL1		= 14,
> +	CMDQ_SEC_MDP_AAL2		= 15,
> +	CMDQ_SEC_MDP_AAL3		= 16,
> +
> +	/* DISP (VDOSYS0) */
> +	CMDQ_SEC_DISP_RDMA0		= 17,
> +	CMDQ_SEC_DISP_RDMA1		= 18,
> +	CMDQ_SEC_DISP_WDMA0		= 19,
> +	CMDQ_SEC_DISP_WDMA1		= 20,
> +	CMDQ_SEC_DISP_OVL0		= 21,
> +	CMDQ_SEC_DISP_OVL1		= 22,
> +	CMDQ_SEC_DISP_OVL2		= 23,
> +	CMDQ_SEC_DISP_2L_OVL0		= 24,
> +	CMDQ_SEC_DISP_2L_OVL1		= 25,
> +	CMDQ_SEC_DISP_2L_OVL2		= 26,
> +
> +	/* DSIP (VDOSYS1) */
> +	CMDQ_SEC_VDO1_DISP_RDMA_L0	= 27,
> +	CMDQ_SEC_VDO1_DISP_RDMA_L1	= 28,
> +	CMDQ_SEC_VDO1_DISP_RDMA_L2	= 29,
> +	CMDQ_SEC_VDO1_DISP_RDMA_L3	= 30,
> +
> +	/* VENC */
> +	CMDQ_SEC_VENC_BSDMA		= 31,
> +	CMDQ_SEC_VENC_CUR_LUMA		= 32,
> +	CMDQ_SEC_VENC_CUR_CHROMA	= 33,
> +	CMDQ_SEC_VENC_REF_LUMA		= 34,
> +	CMDQ_SEC_VENC_REF_CHROMA	= 35,
> +	CMDQ_SEC_VENC_REC		= 36,
> +	CMDQ_SEC_VENC_SUB_R_LUMA	= 37,
> +	CMDQ_SEC_VENC_SUB_W_LUMA	= 38,
> +	CMDQ_SEC_VENC_SV_COMV		= 39,
> +	CMDQ_SEC_VENC_RD_COMV		= 40,
> +	CMDQ_SEC_VENC_NBM_RDMA		= 41,
> +	CMDQ_SEC_VENC_NBM_WDMA		= 42,
> +	CMDQ_SEC_VENC_NBM_RDMA_LITE	= 43,
> +	CMDQ_SEC_VENC_NBM_WDMA_LITE	= 44,
> +	CMDQ_SEC_VENC_FCS_NBM_RDMA	= 45,
> +	CMDQ_SEC_VENC_FCS_NBM_WDMA	= 46,
> +
> +	CMDQ_SEC_MAX_ENG_COUNT		/* keep in the end */
> +};
> +
> +/*
> + * IWC message

kerneldoc please

> + */
> +struct iwcCmdqAddrMetadata_t {
...and no camelcase.

> +	/*
> +	 * [IN] index of instruction
> +	 * Update its arg_b value to real PA/MVA in secure world
> +	 */
> +	u32 instrIndex;
> +


..snip..

> diff --git a/include/linux/mailbox/mtk-cmdq-sec-mailbox.h b/include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> new file mode 100644
> index 000000000000..17ee0ae646ab
> --- /dev/null
> +++ b/include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + */
> +
> +#ifndef __MTK_CMDQ_SEC_MAILBOX_H__
> +#define __MTK_CMDQ_SEC_MAILBOX_H__
> +
> +#include <linux/kernel.h>
> +
> +#include <linux/mailbox/mtk-cmdq-mailbox.h>
> +#include <linux/mailbox/mtk-cmdq-sec-iwc-common.h>
> +#include <linux/mailbox/mtk-cmdq-sec-tee.h>
> +#include <linux/soc/mediatek/mtk-cmdq.h>
> +
> +#define CMDQ_INVALID_THREAD		(-1)
> +#define CMDQ_MAX_TASK_IN_SECURE_THREAD	(16)
> +#define CMDQ_SEC_IRQ_THREAD		(15)
> +
> +/* This define should sync with cmdq_sec_def.h in secure world */
> +#define CMDQ_MIN_SECURE_THREAD_ID	(8)

As you wrote, this shall be in sync with secure world, so this will either break
at the next SoC, or at the next secure world update.

This means that those definitions shall be in platform data for this driver.

> +#define CMDQ_MAX_SECURE_THREAD_ID	(10)
> +#define CMDQ_MAX_SECURE_THREAD_COUNT	(CMDQ_MAX_SECURE_THREAD_ID - CMDQ_MIN_SECURE_THREAD_ID)
> +
> +/* Max value of CMDQ_THR_EXEC_CMD_CNT (value starts from 0) */
> +#define CMDQ_MAX_COOKIE_VALUE           (0xffff)
> +
> +enum cmdq_sec_scenario {
> +	CMDQ_SEC_PRIMARY_DISP		= 1,
> +	CMDQ_SEC_SUB_DISP		= 4,
> +	CMDQ_SEC_PRIMARY_DISP_DISABLE	= 18,
> +	CMDQ_SEC_SUB_DISP_DISABLE	= 19,
> +
> +	CMDQ_MAX_SEC_COUNT	/* keep in the end */
> +};
> +
> +/* Inter-world communication state */
> +enum cmdq_iwc_state_enum {
> +	IWC_INIT,
> +	IWC_CONTEXT_INITED,
> +	IWC_WSM_ALLOCATED,
> +	IWC_SES_OPENED,
> +	IWC_SES_ON_TRANSACTED,
> +
> +	IWC_STATE_END_OF_ENUM,

Usually "END_OF_ENUM" is "MAX", so rename this to IWC_STATE_MAX

> +};
> +
> +/*

kerneldoc please.

> + * Address metadata, used to translate secure buffer PA
> + * related instruction in secure world.
> + */
> +struct cmdq_sec_data {
> +	u32 addrMetadataCount;	/* [IN] count of element in addrList */
> +	u64 addrMetadatas;		/* [IN] array of iwcCmdqAddrMetadata_t */
> +	u32 addrMetadataMaxCount;	/* [Reserved] */
> +
> +	enum cmdq_sec_scenario scenario;
> +
> +	u64 enginesNeedDAPC;
> +	u64 enginesNeedPortSecurity;
> +
> +	/*
> +	 * [Reserved]
> +	 * This is task index in thread's tasklist for CMDQ driver usage.
> +	 * Not for client. -1 means not in tasklist.
> +	 */
> +	s32 waitCookie;
> +	/* [Reserved] reset HW thread in secure world */
> +	bool resetExecCnt;
> +
> +	/* [OUT] response */
> +	s32 response;
> +	struct iwcCmdqSecStatus_t sec_status;
> +};
> +

Please document all those functions that you're exporting with kerneldoc.

> +int cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt);
> +int cmdq_sec_pkt_set_data(struct cmdq_pkt *pkt, const u64 dapc_engine,
> +			  const u64 port_sec_engine, const enum cmdq_sec_scenario scenario);
> +int cmdq_sec_pkt_write(struct cmdq_pkt *pkt, u32 addr, u64 base,
> +		       const enum cmdq_iwc_addr_metadata_type type,
> +		       const u32 offset, const u32 size, const u32 port);
> +
> +#endif /* __MTK_CMDQ_SEC_MAILBOX_H__ */
> diff --git a/include/linux/mailbox/mtk-cmdq-sec-tee.h b/include/linux/mailbox/mtk-cmdq-sec-tee.h
> new file mode 100644
> index 000000000000..77090921cdf6
> --- /dev/null
> +++ b/include/linux/mailbox/mtk-cmdq-sec-tee.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + */
> +
> +#ifndef __MTK_CMDQ_SEC_TEE_H__
> +#define __MTK_CMDQ_SEC_TEE_H__
> +
> +#include <linux/types.h>
> +#include <linux/delay.h>
> +#include <linux/tee_drv.h>
> +
> +/* context for tee vendor */

kerneldoc please.

> +struct cmdq_sec_tee_context {
> +	/* Universally Unique Identifier of secure world */
> +	u8			uuid[TEE_IOCTL_UUID_LEN];
> +	struct tee_context	*tee_context; /* basic context */
> +	u32			session; /* session handle */
> +	struct tee_shm		*shared_mem; /* shared memory */
> +};
> +

Also here, please document those functions.

> +void cmdq_sec_setup_tee_context(struct cmdq_sec_tee_context *tee);
> +int cmdq_sec_init_context(struct cmdq_sec_tee_context *tee);
> +int cmdq_sec_deinit_context(struct cmdq_sec_tee_context *tee);
> +int cmdq_sec_allocate_wsm(struct cmdq_sec_tee_context *tee, void **wsm_buffer, u32 size);
> +int cmdq_sec_free_wsm(struct cmdq_sec_tee_context *tee, void **wsm_buffer);
> +int cmdq_sec_open_session(struct cmdq_sec_tee_context *tee, void *wsm_buffer);
> +int cmdq_sec_close_session(struct cmdq_sec_tee_context *tee);
> +int cmdq_sec_execute_session(struct cmdq_sec_tee_context *tee, u32 cmd, s32 timeout_ms);
> +
> +#endif	/* __MTK_CMDQ_SEC_TEE_H__ */



Regards,
Angelo

Re: [PATCH v2 8/9] mailbox: mediatek: Add CMDQ secure mailbox driver

[Jason-JH Lin (林睿祥)]

Hi Angelo,

Thanks for the reviews.

On Mon, 2023-10-23 at 12:48 +0200, AngeloGioacchino Del Regno wrote:
> Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> > To support secure video path feature, GCE have to read/write
> > registgers
> > in the secure world. GCE will enable the secure access permission
> > to the
> > HW who wants to access the secure content buffer.
> > 
> > Add CMDQ secure mailbox driver to make CMDQ client user is able to
> > sending their HW settings to the secure world. So that GCE can
> > execute
> > all instructions to configure HW in the secure world.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> 
> This driver needs to be cleaned up quite a bit, and I'd expect this
> to take a few
> versions before being upstreamable.
> 
Sure, it's hard to make it upstreamable. It will get better and better
after your reviews. :)

> > ---
> >   drivers/mailbox/Makefile                      |    2 +-
> >   drivers/mailbox/mtk-cmdq-sec-mailbox.c        | 1102
> > +++++++++++++++++
> >   drivers/mailbox/mtk-cmdq-sec-tee.c            |  202 +++
> >   include/linux/mailbox/mtk-cmdq-mailbox.h      |    2 +
> >   .../linux/mailbox/mtk-cmdq-sec-iwc-common.h   |  293 +++++
> >   include/linux/mailbox/mtk-cmdq-sec-mailbox.h  |   83 ++
> >   include/linux/mailbox/mtk-cmdq-sec-tee.h      |   31 +
> >   7 files changed, 1714 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/mailbox/mtk-cmdq-sec-mailbox.c
> >   create mode 100644 drivers/mailbox/mtk-cmdq-sec-tee.c
> >   create mode 100644 include/linux/mailbox/mtk-cmdq-sec-iwc-
> > common.h
> >   create mode 100644 include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> >   create mode 100644 include/linux/mailbox/mtk-cmdq-sec-tee.h
> > 
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index fc9376117111..82da2f4ee81a 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -51,7 +51,7 @@ obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
> >   
> >   obj-$(CONFIG_MTK_ADSP_MBOX)	+= mtk-adsp-mailbox.o
> >   
> > -obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
> > +obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o mtk-cmdq-
> > sec-mailbox.o mtk-cmdq-sec-tee.o
> >   
> >   obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
> >   
> > diff --git a/drivers/mailbox/mtk-cmdq-sec-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-sec-mailbox.c
> > new file mode 100644
> > index 000000000000..dac867386cf1
> > --- /dev/null
> > +++ b/drivers/mailbox/mtk-cmdq-sec-mailbox.c
> > @@ -0,0 +1,1102 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2019 MediaTek Inc.
> > +
> 
> Fix comment style please:
> 
> // SPDX...
> /*
>   * Copyright ...
>   */
> 
OK, I'll fix this.

> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/io.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/sched/clock.h>
> > +#include <linux/timer.h>
> > +
> > +#include <linux/mailbox/mtk-cmdq-sec-mailbox.h>
> > +
> > +#define CMDQ_THR_BASE		(0x100)
> > +#define CMDQ_THR_SIZE		(0x80)
> 
> Those two definitions are the same as in mtk-cmdq-mailbox.c: please
> avoid code
> duplication around, it's harder to maintain.

OK, I'll move this to header file.

> 
> > +#define CMDQ_THR_EXEC_CNT_PA	(0x28)
> > +
> > +#define CMDQ_BUF_ALLOC_SIZE		(PAGE_SIZE)
> > +#define CMDQ_TIMEOUT_DEFAULT		(1000)
> > +#define CMDQ_NO_TIMEOUT			(0xffffffff)
> > +#define ADDR_METADATA_MAX_COUNT_ORIGIN	(8)
> > +
> > +/*
> > + * CMDQ secure context struct
> > + * note it is not global data, each process has its own CMDQ sec
> > context
> > + */
> 
> This structure is practically fully documented: please use kerneldoc
> at this point.
> 
OK, I'll refine these structures here.
By the way, is there any tools can scan the structures or function that
are met to kernel doc?

> > +struct cmdq_sec_context {
> > +	struct list_head		listEntry;
> 
> listEntry -> list_entry or listentry - no camelcase please.
> 
> > +
> > +	/* basic info */
> > +	u32				tgid; /* tgid of process context */
> > +	u32				referCount; /* reference count for
> > open cmdq device node */
> > +
> 
> referCount -> refcount?
> 
> > +	/* iwc state */
> > +	enum cmdq_iwc_state_enum	state;
> 
> iwc_state
> 
> > +
> > +	/* iwc information */
> > +	void				*iwc_msg; /* message buffer
> > */
> > +
> > +	struct cmdq_sec_tee_context	tee; /* trustzone parameters */
> 
> tee_ctx
> 
> > +};
> > +
> > +struct cmdq_sec_task {
> > +	struct list_head	list_entry;
> > +	dma_addr_t		pa_base;
> > +	struct cmdq_sec_thread	*thread;
> > +	struct cmdq_pkt		*pkt;
> > +	u64			exec_time;
> > +	struct work_struct	exec_work;
> > +
> > +	bool			resetExecCnt;
> 
> Again no camelcase - here and everywhere else.
> 
> Also, I see a pattern in all of the _sec_ structures: those seem to
> be the same
> as the non-sec ones, as defined in mtk-cmdq-mailbox, but with more
> params.
> 
> I understand the memory concerns, but there's an easy way out of
> this... we could
> get this done like:
> 
> struct cmdq_sec_task {
> 	struct cmdq_task	tsk;
> 
> 	/* Secure CMDQ */
> 	u32			wait_cookie;
> 	s32			scenario;
> 	u64			engine_flag;
> 	u64			trigger;
> }
> 
> ...and this can be done for all of the _sec_ variants of the cmdq
> mbox structs
> as far as I can see.

OK, Thanks for the advices.
I'll reuse the same part of struct cmdq_task.

> > +	u32			waitCookie;
> > +
> > +	u64			engineFlag;
> > +	s32			scenario;
> > +	u64			trigger;
> > +};
> > +
> > +struct cmdq_sec_thread {
> > +	struct mbox_chan	*chan;
> > +	void __iomem		*base;
> > +	struct list_head	task_list;
> > +	struct timer_list	timeout;
> > +	u32			timeout_ms;
> > +	struct work_struct	timeout_work;
> > +	u32			priority;
> > +	u32			idx;
> > +	bool			occupied;
> > +
> > +	/* following part only secure ctrl */
> > +	u32			wait_cookie;
> > +	u32			next_cookie;
> > +	u32			task_cnt;
> > +	struct workqueue_struct	*task_exec_wq;
> > +};
> > +
> > +/**
> 
> This is not valid kerneldoc.

OK, I'll refine it.

> 
> > + * shared memory between normal and secure world
> > + */
> > +struct cmdq_sec_shared_mem {
> > +	void		*va;
> > +	dma_addr_t	pa;
> > +	u32		size;
> > +};
> > +
> > +struct cmdq_sec {
> > +	struct mbox_controller		mbox;
> > +	void __iomem			*base;
> > +	phys_addr_t			base_pa;
> > +	u32				hwid;
> > +	u32				gce_num;
> > +	struct clk_bulk_data		*clocks;
> > +	u32				thread_nr;
> > +	struct cmdq_sec_thread		*thread;
> > +	struct cmdq_client		*clt;
> > +	struct cmdq_pkt			*clt_pkt;
> > +
> > +	atomic_t			path_res;
> > +	struct cmdq_sec_shared_mem	*shared_mem;
> > +	struct cmdq_sec_context		*context;
> > +	struct iwcCmdqCancelTask_t	cancel;
> > +
> > +	struct workqueue_struct		*timeout_wq;
> > +	u64				sec_invoke;
> > +	u64				sec_done;
> > +
> > +	bool				notify_run;
> > +	struct work_struct		irq_notify_work;
> > +	struct workqueue_struct		*notify_wq;
> > +	u32				cmdq_event;
> > +	/* mutex for cmdq_sec_thread excuting cmdq_sec_task */
> > +	struct mutex			exec_lock;
> > +};
> > +
> > +static atomic_t cmdq_path_res = ATOMIC_INIT(0);
> > +
> > +static int cmdq_sec_task_submit(struct cmdq_sec *cmdq, struct
> > cmdq_sec_task *task,
> > +				const u32 iwc_cmd, const u32 thrd_idx,
> > void *data);
> > +
> > +static inline void cmdq_sec_setup_tee_context_base(struct
> > cmdq_sec_context *context)
> 
> You don't need this function, as it's just wrapping over another one:
> you can just
> call cmdq_sec_setup_tee_context.

OK, I'll remove this wrapping function.

> 
> > +{
> > +	cmdq_sec_setup_tee_context(&context->tee);
> > +}
> > +
> > +static inline int cmdq_sec_init_context_base(struct
> > cmdq_sec_context *context)
> > +{
> > +	int status;
> > +
> > +	status = cmdq_sec_init_context(&context->tee);
> > +	if (status < 0)
> > +		return status;
> 
> if (a < 0) return a;
> else return a;
> 
> really? :-)
> 
Oh, this is not correct...

> Besides, again, you don't need this wrapper function.
> 
OK, I'll remove this wrapping function and avoid the wrong status
checking. Thanks!

> > +
> > +	return status;
> > +}
> > +
> > +int cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt)
> > +{
> > +	struct cmdq_client *cl = (struct cmdq_client *)pkt->cl;
> > +	struct cmdq_sec_thread *thread = ((struct mbox_chan *)(cl-
> > >chan))->con_priv;
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	struct cmdq_operand left, right;
> > +	dma_addr_t addr;
> > +
> > +	if (!thread->occupied || !cmdq->shared_mem)
> > +		return -EFAULT;
> > +
> > +	pr_debug("%s %d: pkt:%p thread:%u gce:%#lx",
> 
> All those pr_xxxx shall become dev_debug, dev_err, etc.

OK, I'll change them.

> 
> > +		 __func__, __LINE__, pkt, thread->idx, (unsigned
> > long)cmdq->base_pa);
> > +
> > +	addr = (u32)(cmdq->base_pa + CMDQ_THR_BASE +
> > +		CMDQ_THR_SIZE * thread->idx + CMDQ_THR_EXEC_CNT_PA);
> > +
> > +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_HIGH(addr));
> > +	cmdq_pkt_read_s(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_LOW(addr),
> > CMDQ_THR_SPR_IDX1);
> > +
> > +	left.reg = true;
> > +	left.idx = CMDQ_THR_SPR_IDX1;
> > +	right.reg = false;
> > +	right.value = 1;
> > +	cmdq_pkt_logic_command(pkt, CMDQ_LOGIC_ADD, CMDQ_THR_SPR_IDX1,
> > &left, &right);
> > +
> > +	addr = cmdq->shared_mem->pa + CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> > +		thread->idx * sizeof(u32);
> 
> Fits in one line.

OK. Thanks!

> 
> > +
> > +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_HIGH(addr));
> > +	cmdq_pkt_write_s(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_LOW(addr),
> > CMDQ_THR_SPR_IDX1);
> > +	cmdq_pkt_set_event(pkt, cmdq->cmdq_event);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_insert_backup_cookie);
> > +
> > +static int cmdq_sec_realloc_addr_list(struct cmdq_pkt *pkt, const
> > u32 count)
> > +{
> > +	struct cmdq_sec_data *sec_data = (struct cmdq_sec_data *)pkt-
> > >sec_data;
> > +	void *prev = (void *)(unsigned long)sec_data->addrMetadatas,
> > *curr;
> > +
> > +	if (count <= sec_data->addrMetadataMaxCount)
> > +		return 0;
> > +
> > +	curr = kcalloc(count, sizeof(*sec_data), GFP_KERNEL);
> > +	if (!curr)
> > +		return -ENOMEM;
> > +
> > +	if (count && sec_data->addrMetadatas)
> > +		memcpy(curr, prev, sizeof(*sec_data) * sec_data-
> > >addrMetadataMaxCount);
> > +
> > +	kfree(prev);
> > +
> > +	sec_data->addrMetadatas = (uintptr_t)curr;
> > +	sec_data->addrMetadataMaxCount = count;
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_check_sec(struct cmdq_pkt *pkt)
> 
> This is not just a check: it is *allocating* memory, so please either
> find a better
> name for this function, or just make sure that the sec_data is
> already allocated
> before calling into functions that are calling this one, as I can
> foresee memory
> leaks because of confusion deriving from this function.

OK, I'll rename it to cmdq_sec_pkt_create_sec_data() and add another
cmdq_sec_pkt_destory_sec_data() to free it.

> 
> > +{
> > +	struct cmdq_sec_data *sec_data;
> > +
> > +	if (pkt->sec_data)
> > +		return 0;
> > +
> > +	sec_data = kzalloc(sizeof(*sec_data), GFP_KERNEL);
> > +	if (!sec_data)
> > +		return -ENOMEM;
> > +
> > +	pkt->sec_data = (void *)sec_data;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_append_metadata(struct cmdq_pkt *pkt,
> > +				    const enum
> > cmdq_iwc_addr_metadata_type type,
> > +				    const u64 base, const u32 offset,
> > const u32 size,
> > +				    const u32 port)
> > +{
> > +	struct cmdq_sec_data *sec_data;
> > +	struct iwcCmdqAddrMetadata_t *meta;
> 
> No camelcase please.

OK, I'll rename it.

> 
> > +	int idx, max, ret;
> > +
> > +	pr_debug("[%s %d] pkt:%p type:%u base:%#llx offset:%#x size:%#x
> > port:%#x",
> > +		 __func__, __LINE__, pkt, type, base, offset, size,
> > port);
> > +
> > +	ret = cmdq_sec_check_sec(pkt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	sec_data = (struct cmdq_sec_data *)pkt->sec_data;
> > +	idx = sec_data->addrMetadataCount;
> > +	if (idx >= CMDQ_IWC_MAX_ADDR_LIST_LENGTH) {
> > +		pr_err("idx:%u reach over:%u", idx,
> > CMDQ_IWC_MAX_ADDR_LIST_LENGTH);
> > +		return -EFAULT;
> > +	}
> > +
> > +	if (!sec_data->addrMetadataMaxCount)
> > +		max = ADDR_METADATA_MAX_COUNT_ORIGIN;
> > +	else if (idx >= sec_data->addrMetadataMaxCount)
> > +		max = sec_data->addrMetadataMaxCount * 2;
> > +	else
> > +		max = sec_data->addrMetadataMaxCount;
> > +
> > +	ret = cmdq_sec_realloc_addr_list(pkt, max);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!sec_data->addrMetadatas) {
> > +		pr_info("addrMetadatas is missing");
> > +
> > +		meta = kzalloc(sizeof(*meta), GFP_KERNEL);
> > +		if (!meta)
> > +			return -ENOMEM;
> > +
> > +		sec_data->addrMetadatas = (uintptr_t)(void *)meta;
> > +	}
> > +	meta = (struct iwcCmdqAddrMetadata_t *)(uintptr_t)sec_data-
> > >addrMetadatas;
> > +
> > +	meta[idx].instrIndex = pkt->cmd_buf_size / CMDQ_INST_SIZE - 1;
> > +	meta[idx].type = type;
> > +	meta[idx].baseHandle = base;
> > +	meta[idx].offset = offset;
> > +	meta[idx].size = size;
> > +	meta[idx].port = port;
> > +	sec_data->addrMetadataCount += 1;
> > +	return 0;
> > +}
> > +
> > +int cmdq_sec_pkt_set_data(struct cmdq_pkt *pkt, const u64
> > dapc_engine,
> > +			  const u64 port_sec_engine, const enum
> > cmdq_sec_scenario scenario)
> > +{
> > +	struct cmdq_sec_data *sec_data;
> > +	int ret;
> > +
> > +	if (!pkt) {
> > +		pr_err("invalid pkt:%p", pkt);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = cmdq_sec_check_sec(pkt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pr_debug("[%s %d] pkt:%p sec_data:%p dapc:%llu port_sec:%llu
> > scen:%u",
> > +		 __func__, __LINE__, pkt, pkt->sec_data, dapc_engine,
> > port_sec_engine, scenario);
> > +
> > +	sec_data = (struct cmdq_sec_data *)pkt->sec_data;
> > +	sec_data->enginesNeedDAPC |= dapc_engine;
> > +	sec_data->enginesNeedPortSecurity |= port_sec_engine;
> > +	sec_data->scenario = scenario;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_pkt_set_data);
> > +
> > +int cmdq_sec_pkt_write(struct cmdq_pkt *pkt, u32 addr, u64 base,
> > +		       const enum cmdq_iwc_addr_metadata_type type,
> > +		       const u32 offset, const u32 size, const u32
> > port)
> > +{
> > +	int ret;
> > +
> > +	ret = cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX0, addr);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = cmdq_pkt_write_s_reg_value(pkt, CMDQ_THR_SPR_IDX0,
> > (u32)base);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return cmdq_sec_append_metadata(pkt, type, base, offset, size,
> > port);
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_pkt_write);
> > +
> > +static u32 cmdq_sec_get_cookie(struct cmdq_sec *cmdq, u32 idx)
> > +{
> > +	return *(u32 *)(cmdq->shared_mem->va +
> > +		CMDQ_SEC_SHARED_THR_CNT_OFFSET + idx * sizeof(u32));
> > +}
> > +
> > +static void cmdq_sec_task_done(struct cmdq_sec_task *task, int
> > sta)
> > +{
> > +	struct cmdq_cb_data data;
> > +
> > +	data.sta = sta;
> > +	data.pkt = task->pkt;
> > +
> > +	pr_debug("%s task:%p pkt:%p err:%d",
> > +		 __func__, task, task->pkt, sta);
> > +
> > +	mbox_chan_received_data(task->thread->chan, &data);
> > +
> > +	list_del_init(&task->list_entry);
> > +	kfree(task);
> > +}
> > +
> > +static bool cmdq_sec_irq_handler(struct cmdq_sec_thread *thread,
> > const u32 cookie, const int err)
> > +{
> > +	struct cmdq_sec_task *task, *temp, *cur_task = NULL;
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	unsigned long flags;
> > +	int done;
> > +
> > +	spin_lock_irqsave(&thread->chan->lock, flags);
> > +	if (thread->wait_cookie <= cookie)
> > +		done = cookie - thread->wait_cookie + 1;
> > +	else if (thread->wait_cookie == (cookie + 1) %
> > CMDQ_MAX_COOKIE_VALUE)
> > +		done = 0;
> > +	else
> > +		done = CMDQ_MAX_COOKIE_VALUE - thread->wait_cookie + 1
> > +
> > +			cookie + 1;
> > +
> > +	list_for_each_entry_safe(task, temp, &thread->task_list,
> > list_entry) {
> > +		if (!done)
> > +			break;
> > +
> > +		cmdq_sec_task_done(task, err);
> > +
> > +		if (thread->task_cnt)
> > +			thread->task_cnt -= 1;
> > +
> > +		done--;
> > +	}
> > +
> > +	cur_task = list_first_entry_or_null(&thread->task_list,
> > +					    struct cmdq_sec_task,
> > list_entry);
> > +
> > +	if (err && cur_task) {
> > +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +
> > +		/* for error task, cancel, callback and done */
> > +		memset(&cmdq->cancel, 0, sizeof(cmdq->cancel));
> > +		cmdq_sec_task_submit(cmdq, cur_task,
> > CMD_CMDQ_IWC_CANCEL_TASK,
> > +				     thread->idx, &cmdq->cancel);
> > +
> > +		cmdq_sec_task_done(task, err);
> > +
> > +		spin_lock_irqsave(&thread->chan->lock, flags);
> > +
> > +		task = list_first_entry_or_null(&thread->task_list,
> > +						struct cmdq_sec_task,
> > list_entry);
> > +		if (cur_task == task)
> > +			cmdq_sec_task_done(cur_task, err);
> > +		else
> > +			pr_err("task list changed");
> > +
> > +		/*
> > +		 * error case stop all task for secure,
> > +		 * since secure tdrv always remove all when cancel
> > +		 */
> > +		while (!list_empty(&thread->task_list)) {
> > +			cur_task = list_first_entry(&thread->task_list,
> > +						    struct
> > cmdq_sec_task, list_entry);
> > +
> > +			cmdq_sec_task_done(cur_task, -ECONNABORTED);
> > +		}
> > +	} else if (err) {
> > +		pr_debug("error but all task done, check notify
> > callback");
> > +	}
> > +
> > +	if (list_empty(&thread->task_list)) {
> > +		thread->wait_cookie = 0;
> > +		thread->next_cookie = 0;
> > +		thread->task_cnt = 0;
> > +		__raw_writel(0, (void __iomem *)cmdq->shared_mem->va +
> > +			     CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> > +			     thread->idx * sizeof(u32));
> > +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +		del_timer(&thread->timeout);
> > +		return true;
> > +	}
> > +
> > +	thread->wait_cookie = cookie % CMDQ_MAX_COOKIE_VALUE + 1;
> > +
> > +	mod_timer(&thread->timeout, jiffies + msecs_to_jiffies(thread-
> > >timeout_ms));
> > +	spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +
> > +	return false;
> > +}
> > +
> > +static void cmdq_sec_irq_notify_work(struct work_struct
> > *work_item)
> > +{
> > +	struct cmdq_sec *cmdq = container_of(work_item, struct
> > cmdq_sec, irq_notify_work);
> > +	int i;
> > +
> > +	mutex_lock(&cmdq->exec_lock);
> > +
> > +	for (i = 0; i < CMDQ_MAX_SECURE_THREAD_COUNT; i++) {
> > +		struct cmdq_sec_thread *thread = &cmdq-
> > >thread[CMDQ_MIN_SECURE_THREAD_ID + i];
> > +		u32 cookie = cmdq_sec_get_cookie(cmdq, thread->idx);
> > +
> > +		if (cookie < thread->wait_cookie || !thread->task_cnt)
> > +			continue;
> > +
> > +		cmdq_sec_irq_handler(thread, cookie, 0);
> > +	}
> > +
> > +	mutex_unlock(&cmdq->exec_lock);
> > +}
> > +
> > +static void cmdq_sec_irq_notify_callback(struct mbox_client *cl,
> > void *mssg)
> > +{
> > +	struct cmdq_cb_data *data = (struct cmdq_cb_data *)mssg;
> > +	struct cmdq_sec *cmdq = container_of(((struct cmdq_client
> > *)data->pkt->cl)->chan->mbox,
> > +					     struct cmdq_sec, mbox);
> > +
> 
> 	if (work_pending...) {
> 		dev_dbg
> 		return;
> 	}
> 
> 	queue_work
> 

It's much better. I'll change this. Thanks!

> > +	if (!work_pending(&cmdq->irq_notify_work))
> > +		queue_work(cmdq->notify_wq, &cmdq->irq_notify_work);
> > +	else
> > +		pr_debug("%s last notify callback working", __func__);
> > +}
> > +
> > +static int cmdq_sec_irq_notify_start(struct cmdq_sec *cmdq)
> > +{
> > +	int err;
> > +
> > +	if (cmdq->notify_run)
> > +		return 0;
> > +
> > +	if (!cmdq->clt_pkt) {
> > +		cmdq->clt = cmdq_mbox_create(cmdq->mbox.dev, 0);
> > +		if (!cmdq->clt || IS_ERR(cmdq->clt)) {
> > +			pr_err("clt mbox_create failed clt:%p
> > index:%d",
> > +			       cmdq->clt, CMDQ_SEC_IRQ_THREAD);
> > +			return -EINVAL;
> > +		}
> > +		cmdq->clt->client.rx_callback =
> > cmdq_sec_irq_notify_callback;
> > +
> > +		cmdq->clt_pkt = cmdq_pkt_create(cmdq->clt,
> > CMDQ_BUF_ALLOC_SIZE);
> > +		if (!cmdq->clt_pkt || IS_ERR(cmdq->clt_pkt)) {
> > +			pr_err("clt_pkt cmdq_pkt_create failed pkt:%p
> > index:%d",
> > +			       cmdq->clt_pkt, CMDQ_SEC_IRQ_THREAD);
> > +			return -EINVAL;
> > +		}
> > +
> > +		INIT_WORK(&cmdq->irq_notify_work,
> > cmdq_sec_irq_notify_work);
> > +	}
> > +
> > +	cmdq_pkt_wfe(cmdq->clt_pkt, cmdq->cmdq_event, true);
> > +	cmdq_pkt_finalize_loop(cmdq->clt_pkt);
> > +	dma_sync_single_for_device(cmdq->mbox.dev,
> > +				   cmdq->clt_pkt->pa_base,
> > +				   cmdq->clt_pkt->cmd_buf_size,
> > +				   DMA_TO_DEVICE);
> > +	err = mbox_send_message(cmdq->clt->chan, cmdq->clt_pkt);
> > +	mbox_client_txdone(cmdq->clt->chan, 0);
> > +	if (err < 0) {
> > +		pr_err("%s failed:%d", __func__, err);
> > +		cmdq_pkt_destroy(cmdq->clt_pkt);
> > +		cmdq_mbox_destroy(cmdq->clt);
> 
> 		return err;
> 	}
> 	cmdq->notify_run = true;
> 
> > +	} else {
> > +		cmdq->notify_run = true;
> > +		pr_debug("%s success!", __func__);
> > +	}
> > +
> > +	return err;
> 
> return 0 here
> 

OK, I'll change this. Thanks!

> > +}
> > +
> > +static int cmdq_sec_session_init(struct cmdq_sec_context *context)
> > +{
> > +	int err = 0;
> > +
> > +	if (context->state >= IWC_SES_OPENED) {
> > +		pr_debug("session opened:%u", context->state);
> > +		return err;
> 
> 		return 0;
> 
> > +	}
> > +
> > +	switch (context->state) {
> > +	case IWC_INIT:
> > +		err = cmdq_sec_init_context_base(context);
> > +		if (err)
> 
> 		if (err)
> 			return err;
> 
> > +			break;
> > +		context->state = IWC_CONTEXT_INITED;
> > +	fallthrough;
> > +	case IWC_CONTEXT_INITED:
> > +		if (context->iwc_msg) {
> > +			pr_err("iwcMessage not NULL:%p", context-
> > >iwc_msg);
> > +			err = -EINVAL;
> 
> return -EINVAL;
> 
> > +			break;
> > +		}
> > +
> > +		err = cmdq_sec_allocate_wsm(&context->tee, &context-
> > >iwc_msg,
> > +					    sizeof(struct
> > iwcCmdqMessage_t));
> > +		if (err)
> > +			break;
> 
> return err
> 
> > +
> > +		context->state = IWC_WSM_ALLOCATED;
> > +	fallthrough;
> > +	case IWC_WSM_ALLOCATED:
> > +		err = cmdq_sec_open_session(&context->tee, context-
> > >iwc_msg);
> > +		if (err)
> > +			break;
> 
> return err
> 
> > +
> > +		context->state = IWC_SES_OPENED;
> > +	fallthrough;
> > +	default:
> > +		break;
> > +	}
> 
> 	return 0;
> 

OK' I'll change these return err logic. Thanks!

> > +	return err;
> > +}
> > +
> > +static int cmdq_sec_fill_iwc_msg(struct cmdq_sec_context *context,
> > +				 struct cmdq_sec_task *task, u32
> > thrd_idx)
> > +{
> > +	struct iwcCmdqMessage_t *iwc_msg = NULL;
> > +	struct cmdq_sec_data *data = (struct cmdq_sec_data *)task->pkt-
> > >sec_data;
> > +	u32 size = 0, offset = 0, *instr;
> > +
> > +	iwc_msg = (struct iwcCmdqMessage_t *)context->iwc_msg;
> > +
> > +	if (task->pkt->cmd_buf_size + 4 * CMDQ_INST_SIZE >
> > CMDQ_TZ_CMD_BLOCK_SIZE) {
> > +		pr_err("task:%p size:%zu > %u",
> > +		       task, task->pkt->cmd_buf_size,
> > CMDQ_TZ_CMD_BLOCK_SIZE);
> > +		return -EFAULT;
> > +	}
> > +
> > +	if (thrd_idx == CMDQ_INVALID_THREAD) {
> > +		iwc_msg->command.commandSize = 0;
> > +		iwc_msg->command.metadata.addrListLength = 0;
> > +		return 0;
> 
> if invalid thread, return 0?! Are you sure?!

Oh, There is a cmd will use the invalid thread to send a message.

But I shouldn't just return 0 for all cmd with invalid thread.
I'll refine this. Thanks!

> 
> > +	}
> > +
> 
> 	if (iwc_msg->command.size == 0 && task->pkt->cmd_buf_size == 0)
> {
> 		dev_dbg(...... command size is zero)
> 
> 		return (something);
> 	}
> 
> > +	iwc_msg->command.thread = thrd_idx;
> > +	iwc_msg->command.scenario = task->scenario;
> > +	iwc_msg->command.engineFlag = task->engineFlag;
> > +	size = task->pkt->cmd_buf_size;
> > +	memcpy(iwc_msg->command.pVABase + offset, task->pkt->va_base,
> > size);
> > +	iwc_msg->command.commandSize += size;
> > +	offset += size / 4;
> > +
> > +	if (iwc_msg->command.commandSize == 0) {
> > +		pr_err("%s %d: commandSize is 0\n", __func__,
> > __LINE__);
> 
> So this being zero is valid? Again, are you sure?
> 
Oh, this may be the test code which I forgot to remove.
I'll remove this check. Thanks!

> > +		return 0;
> > +	}
> > +
> > +	instr = &iwc_msg->command.pVABase[iwc_msg->command.commandSize
> > / 4 - 4];
> > +	if (instr[0] == 0x1 && instr[1] == 0x40000000)
> 
> Please define this, I guess that instr[] is a 64-bits instruction, is
> it?
> 
Each GCE instruction is 64-bits, but we usually verify them by each 32-
bits. In this case, I think checking 64-bits is better.

> So this could be... #define SOMETHING 0x140000000ULL
> ...but however you choose to define it is fine, just .. define it
> please :-)
> 

OK, I'll add an EOC(end of command) define for that.

> > +		instr[0] = 0;
> > +	else
> > +		pr_err("%s %d: find EOC failed: %#x %#x",
> > +		       __func__, __LINE__, instr[1], instr[0]);
> > +
> > +	iwc_msg->command.waitCookie = task->waitCookie;
> > +	iwc_msg->command.resetExecCnt = task->resetExecCnt;
> > +
> > +	if (data->addrMetadataCount) {
> > +		iwc_msg->command.metadata.addrListLength = data-
> > >addrMetadataCount;
> > +		memcpy(iwc_msg->command.metadata.addrList,
> > +		       (u32 *)(unsigned long)data->addrMetadatas,
> > +		       data->addrMetadataCount * sizeof(struct
> > iwcCmdqAddrMetadata_t));
> > +	}
> > +
> > +	iwc_msg->command.metadata.enginesNeedDAPC = data-
> > >enginesNeedDAPC;
> > +	iwc_msg->command.metadata.enginesNeedPortSecurity =
> > +		data->enginesNeedPortSecurity;
> > +	iwc_msg->command.hNormalTask = (unsigned long)task->pkt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_session_send(struct cmdq_sec_context *context,
> > +				 struct cmdq_sec_task *task, const u32
> > iwc_cmd,
> > +				 const u32 thrd_idx, struct cmdq_sec
> > *cmdq)
> > +{
> > +	int err = 0;
> > +	u64 cost;
> > +	struct iwcCmdqMessage_t *iwc_msg = NULL;
> > +
> > +	iwc_msg = (struct iwcCmdqMessage_t *)context->iwc_msg;
> > +
> > +	memset(iwc_msg, 0, sizeof(*iwc_msg));
> > +	iwc_msg->cmd = iwc_cmd;
> > +	iwc_msg->cmdq_id = cmdq->hwid;
> > +	iwc_msg->command.thread = thrd_idx;
> > +
> > +	switch (iwc_cmd) {
> > +	case CMD_CMDQ_IWC_SUBMIT_TASK:
> > +		err = cmdq_sec_fill_iwc_msg(context, task, thrd_idx);
> > +		if (err)
> > +			return err;
> > +		break;
> > +	case CMD_CMDQ_IWC_CANCEL_TASK:
> > +		iwc_msg->cancelTask.waitCookie = task->waitCookie;
> > +		iwc_msg->cancelTask.thread = thrd_idx;
> > +		break;
> > +	case CMD_CMDQ_IWC_PATH_RES_ALLOCATE:
> > +		if (!cmdq->shared_mem || !cmdq->shared_mem->va) {
> > +			pr_err("%s %d: shared_mem is NULL", __func__,
> > __LINE__);
> > +			return -EFAULT;
> > +		}
> > +		iwc_msg->pathResource.size = cmdq->shared_mem->size;
> > +		iwc_msg->pathResource.shareMemoyPA = cmdq->shared_mem-
> > >pa;
> > +		iwc_msg->pathResource.useNormalIRQ = 1;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	cmdq->sec_invoke = sched_clock();
> > +	pr_debug("%s execute cmdq:%p task:%lx command:%u thread:%u
> > cookie:%d",
> > +		 __func__, cmdq, (unsigned long)task, iwc_cmd,
> > thrd_idx,
> > +		 task ? task->waitCookie : -1);
> > +
> > +	/* send message */
> > +	err = cmdq_sec_execute_session(&context->tee, iwc_cmd,
> > CMDQ_TIMEOUT_DEFAULT);
> > +
> > +	cmdq->sec_done = sched_clock();
> > +	cost = div_u64(cmdq->sec_done - cmdq->sec_invoke, 1000000);
> > +	if (cost >= CMDQ_TIMEOUT_DEFAULT)
> > +		pr_err("%s execute timeout cmdq:%p task:%lx
> > cost:%lluus",
> > +		       __func__, cmdq, (unsigned long)task, cost);
> > +	else
> > +		pr_debug("%s execute done cmdq:%p task:%lx
> > cost:%lluus",
> > +			 __func__, cmdq, (unsigned long)task, cost);
> > +
> 
> 	if (err)
> 		return err;
> 
> 	context->state = IWC_SES_ON_TRANSACTED;
> 	return 0;

OK, I'll change this. Thanks!

> 
> > +	if (err == 0)
> > +		context->state = IWC_SES_ON_TRANSACTED;
> > +
> > +	return err;
> > +}
> > +
> > +static int cmdq_sec_session_reply(const u32 iwc_cmd, struct
> > iwcCmdqMessage_t *iwc_msg,
> > +				  void *data, struct cmdq_sec_task
> > *task)
> > +{
> > +	struct iwcCmdqCancelTask_t *cancel = data;
> > +	struct cmdq_sec_data *sec_data = task->pkt->sec_data;
> > +
> > +	if (iwc_cmd == CMD_CMDQ_IWC_SUBMIT_TASK) {
> 
> 	if (iwc_cmd == CMD_CMDQ_IWC_SUBMIT_TASK && iwc_msg->rsp < 0) {
> 		....
> 	} else if (...) {
> 		....
> 	}
> 
> 	return ...
> 

OK, I'll change this. Thanks!

> > +		if (iwc_msg->rsp < 0) {
> > +			/* submit fail case copy status */
> > +			memcpy(&sec_data->sec_status, &iwc_msg-
> > >secStatus,
> > +			       sizeof(sec_data->sec_status));
> > +			sec_data->response = iwc_msg->rsp;
> > +		}
> > +	} else if (iwc_cmd == CMD_CMDQ_IWC_CANCEL_TASK && cancel) {
> > +		/* cancel case only copy cancel result */
> > +		memcpy(cancel, &iwc_msg->cancelTask, sizeof(*cancel));
> > +	}
> > +
> > +	return iwc_msg->rsp;
> > +}
> > +
> > +static int cmdq_sec_task_submit(struct cmdq_sec *cmdq, struct
> > cmdq_sec_task *task,
> > +				const u32 iwc_cmd, const u32 thrd_idx,
> > void *data)
> > +{
> > +	struct cmdq_sec_context *context;
> > +	int err;
> > +
> > +	do {
> > +		if (!cmdq->context) {
> > +			context = kzalloc(sizeof(*cmdq->context),
> > GFP_ATOMIC);
> > +			if (!context) {
> > +				err = -ENOMEM;
> > +				break;
> > +			}
> > +			cmdq->context = context;
> > +			cmdq->context->state = IWC_INIT;
> > +			cmdq->context->tgid = current->tgid;
> > +		}
> > +
> > +		if (cmdq->context->state == IWC_INIT)
> > +			cmdq_sec_setup_tee_context_base(cmdq->context);
> > +
> > +		err = cmdq_sec_session_init(cmdq->context);
> > +		if (err) {
> > +			pr_err("%s %d: cmdq_sec_session_init fail",
> > __func__, __LINE__);
> > +			break;
> > +		}
> > +
> > +		err = cmdq_sec_irq_notify_start(cmdq);
> > +		if (err < 0) {
> > +			pr_err("%s %d: cmdq_sec_irq_notify_start fail",
> > __func__, __LINE__);
> > +			break;
> > +		}
> > +
> > +		err = cmdq_sec_session_send(cmdq->context, task,
> > iwc_cmd, thrd_idx, cmdq);
> > +		if (err)
> > +			pr_err("%s %d: cmdq_sec_session_send fail:
> > %d\n", __func__, __LINE__, err);
> > +		else
> > +			err = cmdq_sec_session_reply(iwc_cmd, cmdq-
> > >context->iwc_msg, data, task);
> > +	} while (0);
> 
> Seriously? do { ... } while(0) ?!?!?!
> 
> No!!!!
> 

OK, I'll change this like the above checking logic. Thanks!

> > +
> > +	if (err)
> > +		pr_err("%s %d: sec invoke err:%d task:%p thread:%u
> > gce:%#lx",
> > +		       __func__, __LINE__, err, task, thrd_idx,
> > (unsigned long)cmdq->base_pa);
> > +
> > +	return err;
> > +}
> > +
> > +static int cmdq_sec_suspend(struct device *dev)
> > +{
> > +	struct cmdq_sec *cmdq = dev_get_drvdata(dev);
> > +
> > +	clk_bulk_disable_unprepare(cmdq->gce_num, cmdq->clocks);
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_resume(struct device *dev)
> > +{
> > +	struct cmdq_sec *cmdq = dev_get_drvdata(dev);
> > +
> > +	WARN_ON(clk_bulk_prepare_enable(cmdq->gce_num, cmdq->clocks));
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops cmdq_sec_pm_ops = {
> > +	.suspend = cmdq_sec_suspend,
> > +	.resume = cmdq_sec_resume,
> > +};
> > +
> > +static void cmdq_sec_task_exec_work(struct work_struct *work_item)
> > +{
> > +	struct cmdq_sec_task *task = container_of(work_item, struct
> > cmdq_sec_task, exec_work);
> > +	struct cmdq_sec *cmdq = container_of(task->thread->chan->mbox,
> > struct cmdq_sec, mbox);
> > +	struct cmdq_sec_data *data;
> > +	unsigned long flags;
> > +	int err;
> > +
> > +	pr_debug("%s gce:%#lx task:%p pkt:%p thread:%u",
> > +		 __func__, (unsigned long)cmdq->base_pa, task, task-
> > >pkt, task->thread->idx);
> > +
> > +	if (!task->pkt->sec_data) {
> > +		pr_err("pkt:%p without sec_data", task->pkt);
> > +		return;
> > +	}
> > +	data = (struct cmdq_sec_data *)task->pkt->sec_data;
> > +
> > +	mutex_lock(&cmdq->exec_lock);
> > +
> > +	spin_lock_irqsave(&task->thread->chan->lock, flags);
> > +	if (!task->thread->task_cnt) {
> > +		mod_timer(&task->thread->timeout, jiffies +
> > +			msecs_to_jiffies(task->thread->timeout_ms));
> > +		task->thread->wait_cookie = 1;
> > +		task->thread->next_cookie = 1;
> > +		task->thread->task_cnt = 0;
> > +		__raw_writel(0, (void __iomem *)cmdq->shared_mem->va +
> > +			     CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> > +			     task->thread->idx * sizeof(u32));
> > +	}
> > +
> > +	task->resetExecCnt = task->thread->task_cnt ? false : true;
> > +	task->waitCookie = task->thread->next_cookie;
> > +	task->thread->next_cookie = (task->thread->next_cookie + 1) %
> > CMDQ_MAX_COOKIE_VALUE;
> > +	list_add_tail(&task->list_entry, &task->thread->task_list);
> > +	task->thread->task_cnt += 1;
> > +	spin_unlock_irqrestore(&task->thread->chan->lock, flags);
> > +	task->trigger = sched_clock();
> > +
> > +	if (!atomic_cmpxchg(&cmdq_path_res, 0, 1)) {
> > +		err = cmdq_sec_task_submit(cmdq, NULL,
> > CMD_CMDQ_IWC_PATH_RES_ALLOCATE,
> > +					   CMDQ_INVALID_THREAD, NULL);
> > +		if (err) {
> > +			atomic_set(&cmdq_path_res, 0);
> > +			goto task_err_callback;
> > +		}
> > +	}
> > +
> > +	if (task->thread->task_cnt > CMDQ_MAX_TASK_IN_SECURE_THREAD) {
> > +		pr_err("task_cnt:%u cannot more than %u task:%p thrd-
> > idx:%u",
> > +		       task->thread->task_cnt,
> > CMDQ_MAX_TASK_IN_SECURE_THREAD,
> > +		       task, task->thread->idx);
> > +		err = -EMSGSIZE;
> > +		goto task_err_callback;
> > +	}
> > +
> > +	err = cmdq_sec_task_submit(cmdq, task,
> > CMD_CMDQ_IWC_SUBMIT_TASK, task->thread->idx, NULL);
> > +	if (err)
> > +		pr_err("task submit CMD_CMDQ_IWC_SUBMIT_TASK failed:%d
> > gce:%#lx task:%p thread:%u",
> > +		       err, (unsigned long)cmdq->base_pa, task, task-
> > >thread->idx);
> > +
> > +task_err_callback:
> 
> end:

OK, I'll change it.

> 
> > +	if (err) {
> > +		struct cmdq_cb_data cb_data;
> > +
> > +		cb_data.sta = err;
> > +		cb_data.pkt = task->pkt;
> > +		mbox_chan_received_data(task->thread->chan, &cb_data);
> > +
> > +		spin_lock_irqsave(&task->thread->chan->lock, flags);
> > +		if (!task->thread->task_cnt)
> > +			pr_err("thread:%u task_cnt:%u cannot below
> > zero",
> > +			       task->thread->idx, task->thread-
> > >task_cnt);
> > +		else
> > +			task->thread->task_cnt -= 1;
> > +
> > +		task->thread->next_cookie = (task->thread->next_cookie
> > - 1 +
> > +			CMDQ_MAX_COOKIE_VALUE) % CMDQ_MAX_COOKIE_VALUE;
> > +		list_del(&task->list_entry);
> > +		pr_debug("gce:%#lx err:%d task:%p pkt:%p thread:%u
> > task_cnt:%u wait_cookie:%u next_cookie:%u",
> > +			 (unsigned long)cmdq->base_pa, err, task, task-
> > >pkt,
> > +			 task->thread->idx, task->thread->task_cnt,
> > +			 task->thread->wait_cookie, task->thread-
> > >next_cookie);
> > +		spin_unlock_irqrestore(&task->thread->chan->lock,
> > flags);
> > +		kfree(task);
> > +	}
> > +	mutex_unlock(&cmdq->exec_lock);
> > +}
> > +
> > +static int cmdq_sec_mbox_send_data(struct mbox_chan *chan, void
> > *data)
> > +{
> > +	struct cmdq_pkt *pkt = (struct cmdq_pkt *)data;
> > +	struct cmdq_sec_data *sec_data = (struct cmdq_sec_data *)pkt-
> > >sec_data;
> > +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread
> > *)chan->con_priv;
> > +	struct cmdq_sec_task *task;
> > +
> > +	if (!sec_data)
> > +		return -EINVAL;
> > +
> > +	task = kzalloc(sizeof(*task), GFP_ATOMIC);
> > +	if (!task)
> > +		return -ENOMEM;
> > +
> > +	task->pkt = pkt;
> > +	task->thread = thread;
> > +	task->scenario = sec_data->scenario;
> > +	task->engineFlag = sec_data->enginesNeedDAPC | sec_data-
> > >enginesNeedPortSecurity;
> > +
> > +	INIT_WORK(&task->exec_work, cmdq_sec_task_exec_work);
> > +	queue_work(thread->task_exec_wq, &task->exec_work);
> > +	return 0;
> > +}
> > +
> > +static void cmdq_sec_thread_timeout(struct timer_list *t)
> > +{
> > +	struct cmdq_sec_thread *thread = from_timer(thread, t,
> > timeout);
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +
> > +	if (!work_pending(&thread->timeout_work))
> > +		queue_work(cmdq->timeout_wq, &thread->timeout_work);
> > +}
> > +
> > +static void cmdq_sec_task_timeout_work(struct work_struct
> > *work_item)
> > +{
> > +	struct cmdq_sec_thread *thread = container_of(work_item, struct
> > cmdq_sec_thread,
> > +						      timeout_work);
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	struct cmdq_sec_task *task;
> > +	unsigned long flags;
> > +	u64 duration;
> > +	u32 cookie;
> > +
> > +	mutex_lock(&cmdq->exec_lock);
> > +
> > +	spin_lock_irqsave(&thread->chan->lock, flags);
> > +	if (list_empty(&thread->task_list)) {
> > +		pr_err("thread:%u task_list is empty", thread->idx);
> > +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +		goto done;
> > +	}
> > +
> > +	task = list_first_entry(&thread->task_list, struct
> > cmdq_sec_task, list_entry);
> > +	duration = div_u64(sched_clock() - task->trigger, 1000000);
> > +	if (duration < thread->timeout_ms) {
> > +		mod_timer(&thread->timeout, jiffies +
> > +			  msecs_to_jiffies(thread->timeout_ms -
> > duration));
> > +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +		goto done;
> > +	}
> > +
> > +	cookie = cmdq_sec_get_cookie(cmdq, thread->idx);
> > +	spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +
> > +	pr_err("%s duration:%llu cookie:%u thread:%u",
> > +	       __func__, duration, cookie, thread->idx);
> > +	cmdq_sec_irq_handler(thread, cookie, -ETIMEDOUT);
> > +
> > +done:
> > +	mutex_unlock(&cmdq->exec_lock);
> > +}
> > +
> > +static int cmdq_sec_mbox_startup(struct mbox_chan *chan)
> > +{
> > +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread
> > *)chan->con_priv;
> > +	char name[20];
> > +
> > +	timer_setup(&thread->timeout, cmdq_sec_thread_timeout, 0);
> > +
> > +	INIT_WORK(&thread->timeout_work, cmdq_sec_task_timeout_work);
> > +	snprintf(name, sizeof(name), "task_exec_wq_%u", thread->idx);
> > +	thread->task_exec_wq = create_singlethread_workqueue(name);
> > +	thread->occupied = true;
> > +	return 0;
> > +}
> > +
> > +static void cmdq_sec_mbox_shutdown(struct mbox_chan *chan)
> > +{
> > +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread
> > *)chan->con_priv;
> > +
> > +	thread->occupied = false;
> > +}
> > +
> > +static int cmdq_sec_mbox_flush(struct mbox_chan *chan, unsigned
> > long timeout)
> > +{
> > +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread
> > *)chan->con_priv;
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	int i;
> > +
> > +	mutex_lock(&cmdq->exec_lock);
> > +
> > +	if (list_empty(&thread->task_list))
> 
> if (list_empty..) {
> 	mutex_unlock(...)
> 	return 0;
> }
> 

OK, I'll change this. Thanks!

> > +		goto out;
> > +
> > +	for (i = 0; i < CMDQ_MAX_SECURE_THREAD_COUNT; i++) {
> > +		struct cmdq_sec_thread *thread = &cmdq-
> > >thread[CMDQ_MIN_SECURE_THREAD_ID + i];
> > +		u32 cookie = cmdq_sec_get_cookie(cmdq, thread->idx);
> > +
> > +		if (cookie < thread->wait_cookie || !thread->task_cnt)
> > +			continue;
> > +
> > +		cmdq_sec_irq_handler(thread, cookie, -ECONNABORTED);
> > +	}
> > +
> > +out:
> > +	mutex_unlock(&cmdq->exec_lock);
> > +	return 0;
> > +}
> > +
> > +static bool cmdq_sec_mbox_last_tx_done(struct mbox_chan *chan)
> > +{
> > +	return true;
> 
> Noupe. This is a hack.
> 
> You want to do some check here to return if the last TX is done, you
> can't
> just return true and call it a day.
> 

Since we use TXDONE_BY_ACK, we can remove this function implement.
So I'll remove this in the next version. 

> > +}
> > +
> > +static const struct mbox_chan_ops cmdq_sec_mbox_chan_ops = {
> > +	.send_data = cmdq_sec_mbox_send_data,
> > +	.startup = cmdq_sec_mbox_startup,
> > +	.shutdown = cmdq_sec_mbox_shutdown,
> > +	.flush = cmdq_sec_mbox_flush,
> > +	.last_tx_done = cmdq_sec_mbox_last_tx_done,
> > +};
> > +
> > +static struct mbox_chan *cmdq_sec_mbox_of_xlate(struct
> > mbox_controller *mbox,
> > +						const struct
> > of_phandle_args *sp)
> > +{
> > +	struct cmdq_sec_thread *thread;
> > +	int idx = sp->args[0];
> > +
> > +	if (mbox->num_chans <= idx) {
> > +		pr_err("invalid thrd-idx:%u", idx);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	thread = (struct cmdq_sec_thread *)mbox->chans[idx].con_priv;
> > +	thread->chan = &mbox->chans[idx];
> > +	thread->timeout_ms = CMDQ_NO_TIMEOUT;
> > +	thread->priority = sp->args[1];
> > +
> > +	return &mbox->chans[idx];
> > +}
> > +
> > +static int cmdq_sec_probe(struct platform_device *pdev)
> > +{
> > +	int i, err;
> > +	struct cmdq_sec *cmdq;
> > +	struct cmdq_sec_plat *plat_data;
> > +	struct device *dev = &pdev->dev;
> > +
> > +	plat_data = (struct cmdq_sec_plat *)pdev->dev.platform_data;
> > +	if (!plat_data) {
> > +		dev_err(dev, "no valid platform data!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
> > +	if (!cmdq)
> > +		return -ENOMEM;
> > +
> > +	cmdq->base_pa = plat_data->base_pa;
> > +	cmdq->base = plat_data->base;
> > +	cmdq->thread_nr = plat_data->thread_nr;
> > +	cmdq->gce_num = plat_data->gce_num;
> > +	cmdq->clocks = devm_kcalloc(dev, cmdq->gce_num,
> > +				    sizeof(struct clk_bulk_data),
> > GFP_KERNEL);
> > +	if (!cmdq->clocks)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0 ; i < cmdq->gce_num; i++)
> > +		cmdq->clocks[i] = plat_data->clocks[i];
> > +
> > +	cmdq->hwid = plat_data->hwid;
> > +	cmdq->cmdq_event = plat_data->cmdq_event;
> > +
> > +	cmdq->mbox.dev = plat_data->gce_dev;
> > +	cmdq->mbox.chans = devm_kcalloc(dev, cmdq->thread_nr,
> > +					sizeof(*cmdq->mbox.chans),
> > GFP_KERNEL);
> > +	if (!cmdq->mbox.chans)
> > +		return -ENOMEM;
> > +
> > +	cmdq->mbox.ops = &cmdq_sec_mbox_chan_ops;
> > +	cmdq->mbox.num_chans = cmdq->thread_nr;
> > +	cmdq->mbox.of_xlate = cmdq_sec_mbox_of_xlate;
> > +
> > +	/* make use of TXDONE_BY_ACK */
> > +	cmdq->mbox.txdone_irq = false;
> > +	cmdq->mbox.txdone_poll = false;
> > +
> > +	cmdq->thread = devm_kcalloc(dev, cmdq->thread_nr,
> > +				    sizeof(*cmdq->thread), GFP_KERNEL);
> > +	if (!cmdq->thread)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&cmdq->exec_lock);
> > +	for (i = 0; i < cmdq->thread_nr; i++) {
> > +		cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
> > CMDQ_THR_SIZE * i;
> > +		INIT_LIST_HEAD(&cmdq->thread[i].task_list);
> > +		cmdq->thread[i].idx = i;
> > +		cmdq->thread[i].occupied = false;
> > +		cmdq->mbox.chans[i].con_priv = (void *)&cmdq-
> > >thread[i];
> > +	}
> > +
> > +	cmdq->notify_wq =
> > create_singlethread_workqueue("mtk_cmdq_sec_notify_wq");
> > +	cmdq->timeout_wq =
> > create_singlethread_workqueue("mtk_cmdq_sec_timeout_wq");
> > +	err = devm_mbox_controller_register(dev, &cmdq->mbox);
> > +	if (err)
> > +		return err;
> > +
> > +	cmdq->shared_mem = devm_kzalloc(dev, sizeof(*cmdq->shared_mem), 
> > GFP_KERNEL);
> > +	if (!cmdq->shared_mem)
> > +		return -ENOMEM;
> > +
> > +	cmdq->shared_mem->va = dma_alloc_coherent(dev, PAGE_SIZE,
> > +						  &cmdq->shared_mem-
> > >pa, GFP_KERNEL);
> > +	cmdq->shared_mem->size = PAGE_SIZE;
> > +
> > +	platform_set_drvdata(pdev, cmdq);
> > +	WARN_ON(clk_bulk_prepare_enable(cmdq->gce_num, cmdq->clocks));
> > +
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_remove(struct platform_device *pdev)
> > +{
> > +	struct cmdq_sec *cmdq = platform_get_drvdata(pdev);
> > +
> > +	if (cmdq->context)
> > +		cmdq_sec_free_wsm(&cmdq->context->tee, &cmdq->context-
> > >iwc_msg);
> > +
> > +	mbox_controller_unregister(&cmdq->mbox);
> > +
> > +	clk_bulk_disable_unprepare(cmdq->gce_num, cmdq->clocks);
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver cmdq_sec_drv = {
> > +	.probe = cmdq_sec_probe,
> > +	.remove = cmdq_sec_remove,
> > +	.driver = {
> > +		.name = "mtk_cmdq_sec",
> 
> "mtk-cmdq-secure"
> 
OK, I'll change it.

> > +		.pm = &cmdq_sec_pm_ops,
> > +	},
> > +};
> > +
> > +static int __init cmdq_sec_init(void)
> > +{
> > +	int err;
> > +
> > +	err = platform_driver_register(&cmdq_sec_drv);
> > +	if (err)
> > +		pr_err("platform_driver_register failed:%d", err);
> > +	return err;
> > +}
> > +
> > +arch_initcall(cmdq_sec_init);
> 
> Do you *really* need this to be an arch_initcall?! That's a bit
> early, and I don't
> really see any reason for this.

OK, It needs to be later than cmdq mailbox driver (subsys_initcall) and
earlier than user driver (module_initcall).

So I think it should be rootfs_initcall() here to ensure it can be
earlier than all the user drivers.

> 
> > +
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/mailbox/mtk-cmdq-sec-tee.c
> > b/drivers/mailbox/mtk-cmdq-sec-tee.c
> > new file mode 100644
> > index 000000000000..e42e9dba343c
> > --- /dev/null
> > +++ b/drivers/mailbox/mtk-cmdq-sec-tee.c
> > @@ -0,0 +1,202 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2019 MediaTek Inc.
> 
> // SPDX...
> /*
>   * Copyright..
>   */
> 

OK, I'll change it. Thanks!

> 
> > +
> > +#include <linux/math64.h>
> > +#include <linux/sched/clock.h>
> > +
> > +#include <linux/mailbox/mtk-cmdq-sec-tee.h>
> > +#include <linux/soc/mediatek/mtk-cmdq.h>
> > +
> > +/* lock to protect atomic secure task execution */
> > +static DEFINE_MUTEX(cmdq_sec_exec_lock);
> > +
> > +static void cmdq_sec_lock_secure_path(void)
> > +{
> > +	mutex_lock(&cmdq_sec_exec_lock);
> > +	smp_mb();	/*memory barrier*/
> 
> I'm sure that you don't need a memory barrier after locking this
> mutex.
> 
> Besides, you don't need this function at all: just call mutex_lock
> and mutex_unlock
> where appropriate.
> 
> > +}
> > +
> > +static void cmdq_sec_unlock_secure_path(void)
> > +{
> > +	mutex_unlock(&cmdq_sec_exec_lock);
> > +}

OK, I'll remove these 2 wrapping functions.

> > +
> > +void cmdq_sec_setup_tee_context(struct cmdq_sec_tee_context *tee)
> > +{
> > +	/* 09010000 0000 0000 0000000000000000 */
> > +	memset(tee->uuid, 0, sizeof(tee->uuid));
> > +	tee->uuid[0] = 0x9;
> > +	tee->uuid[1] = 0x1;
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_setup_tee_context);
> > +
> > +#if IS_ENABLED(CONFIG_TEE)
> 
> Can this driver really work if CONFIG_TEE=n ?!
> 
> I guess it wouldn't, so this is not right at all. The solution is to
> depend on
> CONFIG_TEE to *even compile* this driver....

Yes, I'll remove this and add a dependency to Makefile. Thanks!

> > +static int tee_dev_match(struct tee_ioctl_version_data *t, const
> > void *v)
> > +{
> > +#if IS_ENABLED(CONFIG_OPTEE)
> > +	if (t->impl_id == TEE_IMPL_ID_OPTEE)
> > +		return 1;
> > +#endif
> > +	return 0;
> > +}
> > +#endif
> > +
> 
> ..snip..
> 
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index fdda995a69ce..997a75c529fb 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -7,6 +7,7 @@
> >   #ifndef __MTK_CMDQ_MAILBOX_H__
> >   #define __MTK_CMDQ_MAILBOX_H__
> >   
> > +#include <linux/mailbox_controller.h>
> 
> Why are you adding this header?
> 
> No implicit inclusions in drivers please.
> 
Because I include <mtk-cmdq-mailbox.h> in mtk-cmdq-sec-mailbox.h and
cmdq_get_shift_pa() will use the `struct mbox_chan * chan` as
parameters.

So I add this to avoid the build fail in mtk-cmdq-sec-mailbox.
I think I should split it to another patch.

> >   #include <linux/platform_device.h>
> >   #include <linux/slab.h>
> >   #include <linux/types.h>
> > @@ -77,6 +78,7 @@ struct cmdq_pkt {
> >   	size_t			buf_size; /* real buffer size */
> >   	void			*cl;
> >   	bool			loop;
> > +	void			*sec_data;
> >   };
> >   
> >   struct cmdq_sec_plat {
> > diff --git a/include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> > b/include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> > new file mode 100644
> > index 000000000000..addc515df279
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> > @@ -0,0 +1,293 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#ifndef __CMDQ_SEC_IWC_COMMON_H__
> > +#define __CMDQ_SEC_IWC_COMMON_H__
> > +
> > +/*
> > + * shared DRAM
> > + * bit x = 1 means thread x raise IRQ
> > + */
> > +#define CMDQ_SEC_SHARED_IRQ_RAISED_OFFSET	0
> > +#define CMDQ_SEC_SHARED_THR_CNT_OFFSET		0x100
> > +#define CMDQ_SEC_SHARED_TASK_VA_OFFSET		0x200
> > +#define CMDQ_SEC_SHARED_OP_OFFSET		0x300
> > +#define CMDQ_SEC_SHARED_SUSPEND_CNT		0x304
> > +#define CMDQ_SEC_SHARED_RESET_CNT		0x308
> > +
> > +/* commanad buffer & metadata */
> > +#define CMDQ_TZ_CMD_BLOCK_SIZE		(20 << 12) /* MDP
> > readback may request 20 pages */
> > +#define CMDQ_IWC_MAX_CMD_LENGTH		(CMDQ_TZ_CMD_BLOCK_SIZE
> > / 4)
> > +
> > +#define CMDQ_IWC_MAX_ADDR_LIST_LENGTH	(30)
> > +
> > +#define CMDQ_IWC_CLIENT_NAME		(16)
> > +
> > +#define CMDQ_SEC_MESSAGE_INST_LEN	(8)
> > +#define CMDQ_SEC_DISPATCH_LEN		(8)
> > +#define CMDQ_MAX_READBACK_ENG		(8)
> > +
> > +/*
> > + * Command IDs for normal world(TLC or linux kernel) to secure
> > world
> > + */
> > +/* submit current task */
> > +#define CMD_CMDQ_IWC_SUBMIT_TASK	(1)
> > +/* (not used)release resource in secure path per session */
> 
> If this is unused, it shouldn't be here at all.
> 
> Also, you don't need those parenthesis, remove.
> 
> > +#define CMD_CMDQ_IWC_RES_RELEASE	(2)

OK, I'll remove this unused definition.

> > +/* cancel current task */
> > +#define CMD_CMDQ_IWC_CANCEL_TASK	(3)
> > +/* create global resource for secure path */
> > +#define CMD_CMDQ_IWC_PATH_RES_ALLOCATE	(4)
> > +/* destroy globacl resource for secure path */
> > +#define CMD_CMDQ_IWC_PATH_RES_RELEASE	(5)
> > +
> > +/*
> > + * ERROR code number (ERRNO)
> > + * note the error result returns negative value, i.e, -(ERRNO)
> > + */
> > +#define CMDQ_ERR_NOMEM					(12) /*
> > out of memory */
> > +#define CMDQ_ERR_FAULT					(14) /*
> > bad address */
> > +
> > +#define CMDQ_ERR_ADDR_CONVERT_HANDLE_2_PA		(1000)
> > +#define CMDQ_ERR_ADDR_CONVERT_ALLOC_MVA			(1100)
> > +#define CMDQ_ERR_ADDR_CONVERT_ALLOC_MVA_N2S		(1101)
> > +#define CMDQ_ERR_ADDR_CONVERT_FREE_MVA			(1200)
> > +#define CMDQ_ERR_PORT_CONFIG				(1300)
> > +
> > +/* param check */
> > +#define CMDQ_ERR_UNKNOWN_ADDR_METADATA_TYPE		(1400)
> > +#define CMDQ_ERR_TOO_MANY_SEC_HANDLE			(1401)
> > +/* security check */
> > +#define CMDQ_ERR_SECURITY_INVALID_INSTR			(1500)
> > +#define CMDQ_ERR_SECURITY_INVALID_SEC_HANDLE		(1501)
> > +#define CMDQ_ERR_SECURITY_INVALID_DAPC_FALG		(1502)
> > +#define CMDQ_ERR_INSERT_DAPC_INSTR_FAILED		(1503)
> > +#define CMDQ_ERR_INSERT_PORT_SECURITY_INSTR_FAILED	(1504)
> > +#define CMDQ_ERR_INVALID_SECURITY_THREAD		(1505)
> > +#define CMDQ_ERR_PATH_RESOURCE_NOT_READY		(1506)
> > +#define CMDQ_ERR_NULL_TASK				(1507)
> > +#define CMDQ_ERR_SECURITY_INVALID_SOP			(1508)
> > +#define CMDQ_ERR_SECURITY_INVALID_SEC_PORT_FALG		(1511)
> > +
> 
> Fully documented enumeration, but no kerneldoc? :-)
> 

I'll refine it by kerneldoc.

> > +enum cmdq_iwc_addr_metadata_type {
> > +	CMDQ_IWC_H_2_PA		= 0, /* sec handle to sec PA */
> > +	CMDQ_IWC_H_2_MVA	= 1, /* sec handle to sec MVA */
> > +	CMDQ_IWC_NMVA_2_MVA	= 2, /* map normal MVA to secure world */
> > +	CMDQ_IWC_PH_2_MVA	= 3, /* protected handle to sec MVA */
> > +};
> > +
> > +enum cmdq_sec_engine_enum {
> > +	/* MDP */
> > +	CMDQ_SEC_MDP_RDMA0		= 0,
> > +	CMDQ_SEC_MDP_RDMA1		= 1,
> > +	CMDQ_SEC_MDP_WDMA		= 2,
> > +	CMDQ_SEC_MDP_RDMA2		= 3,
> > +	CMDQ_SEC_MDP_RDMA3		= 4,
> > +	CMDQ_SEC_MDP_WROT0		= 5,
> > +	CMDQ_SEC_MDP_WROT1		= 6,
> > +	CMDQ_SEC_MDP_WROT2		= 7,
> > +	CMDQ_SEC_MDP_WROT3		= 8,
> > +	CMDQ_SEC_MDP_HDR0		= 9,
> > +	CMDQ_SEC_MDP_HDR1		= 10,
> > +	CMDQ_SEC_MDP_HDR2		= 11,
> > +	CMDQ_SEC_MDP_HDR3		= 12,
> > +	CMDQ_SEC_MDP_AAL0		= 13,
> > +	CMDQ_SEC_MDP_AAL1		= 14,
> > +	CMDQ_SEC_MDP_AAL2		= 15,
> > +	CMDQ_SEC_MDP_AAL3		= 16,
> > +
> > +	/* DISP (VDOSYS0) */
> > +	CMDQ_SEC_DISP_RDMA0		= 17,
> > +	CMDQ_SEC_DISP_RDMA1		= 18,
> > +	CMDQ_SEC_DISP_WDMA0		= 19,
> > +	CMDQ_SEC_DISP_WDMA1		= 20,
> > +	CMDQ_SEC_DISP_OVL0		= 21,
> > +	CMDQ_SEC_DISP_OVL1		= 22,
> > +	CMDQ_SEC_DISP_OVL2		= 23,
> > +	CMDQ_SEC_DISP_2L_OVL0		= 24,
> > +	CMDQ_SEC_DISP_2L_OVL1		= 25,
> > +	CMDQ_SEC_DISP_2L_OVL2		= 26,
> > +
> > +	/* DSIP (VDOSYS1) */
> > +	CMDQ_SEC_VDO1_DISP_RDMA_L0	= 27,
> > +	CMDQ_SEC_VDO1_DISP_RDMA_L1	= 28,
> > +	CMDQ_SEC_VDO1_DISP_RDMA_L2	= 29,
> > +	CMDQ_SEC_VDO1_DISP_RDMA_L3	= 30,
> > +
> > +	/* VENC */
> > +	CMDQ_SEC_VENC_BSDMA		= 31,
> > +	CMDQ_SEC_VENC_CUR_LUMA		= 32,
> > +	CMDQ_SEC_VENC_CUR_CHROMA	= 33,
> > +	CMDQ_SEC_VENC_REF_LUMA		= 34,
> > +	CMDQ_SEC_VENC_REF_CHROMA	= 35,
> > +	CMDQ_SEC_VENC_REC		= 36,
> > +	CMDQ_SEC_VENC_SUB_R_LUMA	= 37,
> > +	CMDQ_SEC_VENC_SUB_W_LUMA	= 38,
> > +	CMDQ_SEC_VENC_SV_COMV		= 39,
> > +	CMDQ_SEC_VENC_RD_COMV		= 40,
> > +	CMDQ_SEC_VENC_NBM_RDMA		= 41,
> > +	CMDQ_SEC_VENC_NBM_WDMA		= 42,
> > +	CMDQ_SEC_VENC_NBM_RDMA_LITE	= 43,
> > +	CMDQ_SEC_VENC_NBM_WDMA_LITE	= 44,
> > +	CMDQ_SEC_VENC_FCS_NBM_RDMA	= 45,
> > +	CMDQ_SEC_VENC_FCS_NBM_WDMA	= 46,
> > +
> > +	CMDQ_SEC_MAX_ENG_COUNT		/* keep in the end */
> > +};
> > +
> > +/*
> > + * IWC message
> 
> kerneldoc please

OK. Thanks!
> 
> > + */
> > +struct iwcCmdqAddrMetadata_t {
> 
> ...and no camelcase.
> 
OK, I'll fix it. Thanks!

> > +	/*
> > +	 * [IN] index of instruction
> > +	 * Update its arg_b value to real PA/MVA in secure world
> > +	 */
> > +	u32 instrIndex;
> > +
> 
> 
> ..snip..
> 
> > diff --git a/include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> > b/include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> > new file mode 100644
> > index 000000000000..17ee0ae646ab
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_CMDQ_SEC_MAILBOX_H__
> > +#define __MTK_CMDQ_SEC_MAILBOX_H__
> > +
> > +#include <linux/kernel.h>
> > +
> > +#include <linux/mailbox/mtk-cmdq-mailbox.h>
> > +#include <linux/mailbox/mtk-cmdq-sec-iwc-common.h>
> > +#include <linux/mailbox/mtk-cmdq-sec-tee.h>
> > +#include <linux/soc/mediatek/mtk-cmdq.h>
> > +
> > +#define CMDQ_INVALID_THREAD		(-1)
> > +#define CMDQ_MAX_TASK_IN_SECURE_THREAD	(16)
> > +#define CMDQ_SEC_IRQ_THREAD		(15)
> > +
> > +/* This define should sync with cmdq_sec_def.h in secure world */
> > +#define CMDQ_MIN_SECURE_THREAD_ID	(8)
> 
> As you wrote, this shall be in sync with secure world, so this will
> either break
> at the next SoC, or at the next secure world update.
> 
> This means that those definitions shall be in platform data for this
> driver.

Oh yes! I'll try to use platform data to refine this.
Thanks for your advice.

> 
> > +#define CMDQ_MAX_SECURE_THREAD_ID	(10)
> > +#define CMDQ_MAX_SECURE_THREAD_COUNT	(CMDQ_MAX_SECURE_THREAD
> > _ID - CMDQ_MIN_SECURE_THREAD_ID)
> > +
> > +/* Max value of CMDQ_THR_EXEC_CMD_CNT (value starts from 0) */
> > +#define CMDQ_MAX_COOKIE_VALUE           (0xffff)
> > +
> > +enum cmdq_sec_scenario {
> > +	CMDQ_SEC_PRIMARY_DISP		= 1,
> > +	CMDQ_SEC_SUB_DISP		= 4,
> > +	CMDQ_SEC_PRIMARY_DISP_DISABLE	= 18,
> > +	CMDQ_SEC_SUB_DISP_DISABLE	= 19,
> > +
> > +	CMDQ_MAX_SEC_COUNT	/* keep in the end */
> > +};
> > +
> > +/* Inter-world communication state */
> > +enum cmdq_iwc_state_enum {
> > +	IWC_INIT,
> > +	IWC_CONTEXT_INITED,
> > +	IWC_WSM_ALLOCATED,
> > +	IWC_SES_OPENED,
> > +	IWC_SES_ON_TRANSACTED,
> > +
> > +	IWC_STATE_END_OF_ENUM,
> 
> Usually "END_OF_ENUM" is "MAX", so rename this to IWC_STATE_MAX
> 
OK, I'll change it.

> > +};
> > +
> > +/*
> 
> kerneldoc please.
> 
OK. Thanks!

> > + * Address metadata, used to translate secure buffer PA
> > + * related instruction in secure world.
> > + */
> > +struct cmdq_sec_data {
> > +	u32 addrMetadataCount;	/* [IN] count of element in
> > addrList */
> > +	u64 addrMetadatas;		/* [IN] array of
> > iwcCmdqAddrMetadata_t */
> > +	u32 addrMetadataMaxCount;	/* [Reserved] */
> > +
> > +	enum cmdq_sec_scenario scenario;
> > +
> > +	u64 enginesNeedDAPC;
> > +	u64 enginesNeedPortSecurity;
> > +
> > +	/*
> > +	 * [Reserved]
> > +	 * This is task index in thread's tasklist for CMDQ driver
> > usage.
> > +	 * Not for client. -1 means not in tasklist.
> > +	 */
> > +	s32 waitCookie;
> > +	/* [Reserved] reset HW thread in secure world */
> > +	bool resetExecCnt;
> > +
> > +	/* [OUT] response */
> > +	s32 response;
> > +	struct iwcCmdqSecStatus_t sec_status;
> > +};
> > +
> 
> Please document all those functions that you're exporting with
> kerneldoc.
> 

OK, Thanks!

> > +int cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt);
> > +int cmdq_sec_pkt_set_data(struct cmdq_pkt *pkt, const u64
> > dapc_engine,
> > +			  const u64 port_sec_engine, const enum
> > cmdq_sec_scenario scenario);
> > +int cmdq_sec_pkt_write(struct cmdq_pkt *pkt, u32 addr, u64 base,
> > +		       const enum cmdq_iwc_addr_metadata_type type,
> > +		       const u32 offset, const u32 size, const u32
> > port);
> > +
> > +#endif /* __MTK_CMDQ_SEC_MAILBOX_H__ */
> > diff --git a/include/linux/mailbox/mtk-cmdq-sec-tee.h
> > b/include/linux/mailbox/mtk-cmdq-sec-tee.h
> > new file mode 100644
> > index 000000000000..77090921cdf6
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-cmdq-sec-tee.h
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_CMDQ_SEC_TEE_H__
> > +#define __MTK_CMDQ_SEC_TEE_H__
> > +
> > +#include <linux/types.h>
> > +#include <linux/delay.h>
> > +#include <linux/tee_drv.h>
> > +
> > +/* context for tee vendor */
> 
> kerneldoc please.
> 
OK, Thanks!

> > +struct cmdq_sec_tee_context {
> > +	/* Universally Unique Identifier of secure world */
> > +	u8			uuid[TEE_IOCTL_UUID_LEN];
> > +	struct tee_context	*tee_context; /* basic context */
> > +	u32			session; /* session handle */
> > +	struct tee_shm		*shared_mem; /* shared memory */
> > +};
> > +
> 
> Also here, please document those functions.
> 
OK, Thanks!

> > +void cmdq_sec_setup_tee_context(struct cmdq_sec_tee_context *tee);
> > +int cmdq_sec_init_context(struct cmdq_sec_tee_context *tee);
> > +int cmdq_sec_deinit_context(struct cmdq_sec_tee_context *tee);
> > +int cmdq_sec_allocate_wsm(struct cmdq_sec_tee_context *tee, void
> > **wsm_buffer, u32 size);
> > +int cmdq_sec_free_wsm(struct cmdq_sec_tee_context *tee, void
> > **wsm_buffer);
> > +int cmdq_sec_open_session(struct cmdq_sec_tee_context *tee, void
> > *wsm_buffer);
> > +int cmdq_sec_close_session(struct cmdq_sec_tee_context *tee);
> > +int cmdq_sec_execute_session(struct cmdq_sec_tee_context *tee, u32
> > cmd, s32 timeout_ms);
> > +
> > +#endif	/* __MTK_CMDQ_SEC_TEE_H__ */
> 
> 
> 
> Regards,
> Angelo

These patches are porting from the other platform which is not strongly
follow the upstream coding style. And we are asked to send the upstream
patch in a hurry schedule. Although I've remove a lot of unused code
and simplify some code flow, it still has a lot of trivial problems.
I really appreciate you taking the time to review this patch.
Thanks again!

Regards,
Jason-JH.Lin

Re: [PATCH v2 8/9] mailbox: mediatek: Add CMDQ secure mailbox driver

[CK Hu (胡俊光)]

Hi, Jason:

On Mon, 2023-10-23 at 12:37 +0800, Jason-JH.Lin wrote:
> To support secure video path feature, GCE have to read/write
> registgers
> in the secure world. GCE will enable the secure access permission to
> the
> HW who wants to access the secure content buffer.
> 
> Add CMDQ secure mailbox driver to make CMDQ client user is able to
> sending their HW settings to the secure world. So that GCE can
> execute
> all instructions to configure HW in the secure world.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/mailbox/Makefile                      |    2 +-
>  drivers/mailbox/mtk-cmdq-sec-mailbox.c        | 1102
> +++++++++++++++++
>  drivers/mailbox/mtk-cmdq-sec-tee.c            |  202 +++
>  include/linux/mailbox/mtk-cmdq-mailbox.h      |    2 +
>  .../linux/mailbox/mtk-cmdq-sec-iwc-common.h   |  293 +++++
>  include/linux/mailbox/mtk-cmdq-sec-mailbox.h  |   83 ++
>  include/linux/mailbox/mtk-cmdq-sec-tee.h      |   31 +
>  7 files changed, 1714 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mailbox/mtk-cmdq-sec-mailbox.c
>  create mode 100644 drivers/mailbox/mtk-cmdq-sec-tee.c
>  create mode 100644 include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
>  create mode 100644 include/linux/mailbox/mtk-cmdq-sec-mailbox.h
>  create mode 100644 include/linux/mailbox/mtk-cmdq-sec-tee.h
> 
> 

[snip]

> +
> +int cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_client *cl = (struct cmdq_client *)pkt->cl;
> +	struct cmdq_sec_thread *thread = ((struct mbox_chan *)(cl-
> >chan))->con_priv;
> +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> cmdq_sec, mbox);
> +	struct cmdq_operand left, right;
> +	dma_addr_t addr;
> +
> +	if (!thread->occupied || !cmdq->shared_mem)
> +		return -EFAULT;
> +
> +	pr_debug("%s %d: pkt:%p thread:%u gce:%#lx",
> +		 __func__, __LINE__, pkt, thread->idx, (unsigned
> long)cmdq->base_pa);
> +
> +	addr = (u32)(cmdq->base_pa + CMDQ_THR_BASE +
> +		CMDQ_THR_SIZE * thread->idx + CMDQ_THR_EXEC_CNT_PA);
> +
> +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_HIGH(addr));
> +	cmdq_pkt_read_s(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_LOW(addr),
> CMDQ_THR_SPR_IDX1);
> +
> +	left.reg = true;
> +	left.idx = CMDQ_THR_SPR_IDX1;
> +	right.reg = false;
> +	right.value = 1;
> +	cmdq_pkt_logic_command(pkt, CMDQ_LOGIC_ADD, CMDQ_THR_SPR_IDX1,
> &left, &right);
> +
> +	addr = cmdq->shared_mem->pa + CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> +		thread->idx * sizeof(u32);
> +
> +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_HIGH(addr));
> +	cmdq_pkt_write_s(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_LOW(addr),
> CMDQ_THR_SPR_IDX1);

This is a sample code of reading secure data and writing to normal
world. I think you have already check address of cmdq_pkt_read_s() in
TEE. This is a sample code that hacker may try to read any secure
address:

	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1,
CMDQ_ADDR_HIGH(hack_address));
	cmdq_pkt_jump(pkt, read);
	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_HIGH(addr));
read:
	cmdq_pkt_read_s(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_LOW(addr),
		CMDQ_THR_SPR_IDX1);

Please explain how you protect this hacker attack.

Regards,
CK

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(cmdq_sec_insert_backup_cookie);
> +

Re: [PATCH v2 8/9] mailbox: mediatek: Add CMDQ secure mailbox driver

[Jason-JH Lin (林睿祥)]

Hi CK,

On Mon, 2023-11-06 at 06:53 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2023-10-23 at 12:37 +0800, Jason-JH.Lin wrote:
> > To support secure video path feature, GCE have to read/write
> > registgers
> > in the secure world. GCE will enable the secure access permission
> > to
> > the
> > HW who wants to access the secure content buffer.
> > 
> > Add CMDQ secure mailbox driver to make CMDQ client user is able to
> > sending their HW settings to the secure world. So that GCE can
> > execute
> > all instructions to configure HW in the secure world.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/Makefile                      |    2 +-
> >  drivers/mailbox/mtk-cmdq-sec-mailbox.c        | 1102
> > +++++++++++++++++
> >  drivers/mailbox/mtk-cmdq-sec-tee.c            |  202 +++
> >  include/linux/mailbox/mtk-cmdq-mailbox.h      |    2 +
> >  .../linux/mailbox/mtk-cmdq-sec-iwc-common.h   |  293 +++++
> >  include/linux/mailbox/mtk-cmdq-sec-mailbox.h  |   83 ++
> >  include/linux/mailbox/mtk-cmdq-sec-tee.h      |   31 +
> >  7 files changed, 1714 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/mailbox/mtk-cmdq-sec-mailbox.c
> >  create mode 100644 drivers/mailbox/mtk-cmdq-sec-tee.c
> >  create mode 100644 include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> >  create mode 100644 include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> >  create mode 100644 include/linux/mailbox/mtk-cmdq-sec-tee.h
> > 
> > 
> 
> [snip]
> 
> > +
> > +int cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt)
> > +{
> > +	struct cmdq_client *cl = (struct cmdq_client *)pkt->cl;
> > +	struct cmdq_sec_thread *thread = ((struct mbox_chan *)(cl-
> > > chan))->con_priv;
> > 
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	struct cmdq_operand left, right;
> > +	dma_addr_t addr;
> > +
> > +	if (!thread->occupied || !cmdq->shared_mem)
> > +		return -EFAULT;
> > +
> > +	pr_debug("%s %d: pkt:%p thread:%u gce:%#lx",
> > +		 __func__, __LINE__, pkt, thread->idx, (unsigned
> > long)cmdq->base_pa);
> > +
> > +	addr = (u32)(cmdq->base_pa + CMDQ_THR_BASE +
> > +		CMDQ_THR_SIZE * thread->idx + CMDQ_THR_EXEC_CNT_PA);
> > +
> > +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_HIGH(addr));
> > +	cmdq_pkt_read_s(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_LOW(addr),
> > CMDQ_THR_SPR_IDX1);
> > +
> > +	left.reg = true;
> > +	left.idx = CMDQ_THR_SPR_IDX1;
> > +	right.reg = false;
> > +	right.value = 1;
> > +	cmdq_pkt_logic_command(pkt, CMDQ_LOGIC_ADD, CMDQ_THR_SPR_IDX1,
> > &left, &right);
> > +
> > +	addr = cmdq->shared_mem->pa + CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> > +		thread->idx * sizeof(u32);
> > +
> > +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_HIGH(addr));
> > +	cmdq_pkt_write_s(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_LOW(addr),
> > CMDQ_THR_SPR_IDX1);
> 
> This is a sample code of reading secure data and writing to normal
> world. I think you have already check address of cmdq_pkt_read_s() in
> TEE. This is a sample code that hacker may try to read any secure
> address:
> 
> 	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1,
> CMDQ_ADDR_HIGH(hack_address));
> 	cmdq_pkt_jump(pkt, read);
> 	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_HIGH(addr));
> read:
> 	cmdq_pkt_read_s(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_LOW(addr),
> 		CMDQ_THR_SPR_IDX1);
> 
> Please explain how you protect this hacker attack.
> 

Yes, hackers can use this sample code to read the HW register to the
shared memory, but they still can't get the secure content or other
useful information to get the secure content.

There are some protections we made for this action:
1. All the physical address of HW register in the read/write
instruction in secure command buffer need to be defined at whitelist.
2. Reading DRAM address instruction in secure command buffer are
forbidden.
3. The shared memory is used to store the value of cookie which is used
for secure cmdq packet scheduling. So secure GCE thread executing state
will crash by modifying it unexpectedly.

Although hackers could get the value store in the HW register by this
sample code, they still don't know what the HW register stand for and 
what is the meaning of the value they read. Additionally, if they read
unexpectedly values into shared memory, they will corrupt the secure
GCE thread state.

Regards,
Jason-JH.Lin

> Regards,
> CK
> 
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_insert_backup_cookie);
> > +