[PATCH 00/15] Add CMDQ secure driver for SVP

[PATCH 00/15] Add CMDQ secure driver for SVP

[Jason-JH.Lin]

For the Secure Video Path (SVP) feature, inculding the memory stored
secure video content, the registers of display HW pipeline and the
HW configure operations are required to execute in the secure world.

So using a CMDQ secure driver to make all display HW registers
configuration secure DRAM access permision settings execute by GCE
secure thread in the secure world.

We are landing this feature on mt8188 and mt8195 currently.

Jason-JH.Lin (15):
  dt-bindings: mailbox: Add property for CMDQ secure driver
  dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id
  soc: mailbox: Add SPR definition for GCE
  soc: mailbox: Add cmdq_pkt_logic_command to support math operation
  mailbox: mediatek: Add cmdq_pkt_write_s_reg_value to CMDQ driver
  mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread
  mailbox: mediatek: Add loop pkt flag and irq handling for loop command
  soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver
  mailbox: mediatek: Add secure CMDQ driver support for CMDQ driver
  mailbox: mediatek: Add CMDQ secure mailbox driver
  soc: mediatek: Add cmdq_insert_backup_cookie before EOC for secure pkt
  mailbox: mediatek: Add CMDQ driver support for mt8188
  mailbox: mediatek: Add mt8188 support for CMDQ secure driver
  mailbox: mediatek: Add mt8195 support for CMDQ secure driver
  arm64: dts: mediatek: mt8195: Add CMDQ secure driver support for gce0

 .../mailbox/mediatek,gce-mailbox.yaml         |   30 +-
 arch/arm64/boot/dts/mediatek/mt8195.dtsi      |    2 +
 drivers/mailbox/Makefile                      |    2 +-
 drivers/mailbox/mtk-cmdq-mailbox.c            |   67 +-
 drivers/mailbox/mtk-cmdq-sec-mailbox.c        | 1103 +++++++++++++++++
 drivers/mailbox/mtk-cmdq-sec-tee.c            |  202 +++
 drivers/soc/mediatek/mtk-cmdq-helper.c        |   81 ++
 include/dt-bindings/gce/mt8195-gce.h          |    6 +
 include/linux/mailbox/mtk-cmdq-mailbox.h      |   15 +
 .../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 +
 include/linux/soc/mediatek/mtk-cmdq.h         |   65 +
 13 files changed, 1971 insertions(+), 9 deletions(-)
 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

-- 
2.18.0

[PATCH 01/15] dt-bindings: mailbox: Add property for CMDQ secure driver

[Jason-JH.Lin]

Add mboxes to define a GCE loopping thread as a secure irq handler.
Add mediatek,event to define a GCE software event siganl as a secure
irq.

These 2 properties are required for CMDQ secure driver.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 .../mailbox/mediatek,gce-mailbox.yaml         | 30 +++++++++++++++----
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
index cef9d7601398..5c9aebe83d2d 100644
--- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
@@ -49,6 +49,21 @@ properties:
     items:
       - const: gce
 
+  mboxes:
+    description:
+      A mailbox channel used as a secure irq handler in normal world.
+      Using mailbox to communicate with GCE to setup looping thread,
+      it should have this property and a phandle, mailbox specifiers.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
+  mediatek,gce-events:
+    description:
+      The event id which is mapping to a software event signal to gce.
+      It is used as a secure irq for every secure gce threads.
+      The event id is defined in the gce header
+      include/dt-bindings/mailbox/mediatek,<chip>-gce.h of each chips.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
 required:
   - compatible
   - "#mbox-cells"
@@ -71,20 +86,23 @@ additionalProperties: false
 
 examples:
   - |
-    #include <dt-bindings/clock/mt8173-clk.h>
+    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
     #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/mailbox/mediatek,mt8188-gce.h>
 
     soc {
         #address-cells = <2>;
         #size-cells = <2>;
 
-        gce: mailbox@10212000 {
-            compatible = "mediatek,mt8173-gce";
-            reg = <0 0x10212000 0 0x1000>;
-            interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
+        gce0: mailbox@10320000 {
+            compatible = "mediatek,mt8188-gce";
+            reg = <0 0x10320000 0 0x4000>;
+            interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH 0>;
             #mbox-cells = <2>;
-            clocks = <&infracfg CLK_INFRA_GCE>;
+            clocks = <&infracfg_ao CLK_INFRA_AO_GCE>;
             clock-names = "gce";
+            mboxes = <&gce0 15 CMDQ_THR_PRIO_1>;
+            mediatek,gce-events = <CMDQ_SYNC_TOKEN_SECURE_THR_EOF>;
         };
     };
-- 
2.18.0

Re: [PATCH 01/15] dt-bindings: mailbox: Add property for CMDQ secure driver

[Rob Herring]

On Tue, Sep 19, 2023 at 03:21:50AM +0800, Jason-JH.Lin wrote:
> Add mboxes to define a GCE loopping thread as a secure irq handler.
> Add mediatek,event to define a GCE software event siganl as a secure
> irq.
> 
> These 2 properties are required for CMDQ secure driver.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  .../mailbox/mediatek,gce-mailbox.yaml         | 30 +++++++++++++++----
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> index cef9d7601398..5c9aebe83d2d 100644
> --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> @@ -49,6 +49,21 @@ properties:
>      items:
>        - const: gce
>  
> +  mboxes:
> +    description:
> +      A mailbox channel used as a secure irq handler in normal world.
> +      Using mailbox to communicate with GCE to setup looping thread,
> +      it should have this property and a phandle, mailbox specifiers.

All cases of 'mboxes' have a phandle and specifiers. No need to repeat 
that here.

> +    $ref: /schemas/types.yaml#/definitions/phandle-array

Already has a type definition too. You need to define how many entries 
and what each entry is if more than one. IOW, the same thing as clocks, 
resets, interrupts, etc.

> +
> +  mediatek,gce-events:

This is used all over. It really needs a single definition which is then 
referenced by the users.

> +    description:
> +      The event id which is mapping to a software event signal to gce.
> +      It is used as a secure irq for every secure gce threads.
> +      The event id is defined in the gce header
> +      include/dt-bindings/mailbox/mediatek,<chip>-gce.h of each chips.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +
>  required:
>    - compatible
>    - "#mbox-cells"
> @@ -71,20 +86,23 @@ additionalProperties: false
>  
>  examples:
>    - |
> -    #include <dt-bindings/clock/mt8173-clk.h>
> +    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
>      #include <dt-bindings/interrupt-controller/arm-gic.h>
>      #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/mailbox/mediatek,mt8188-gce.h>
>  
>      soc {
>          #address-cells = <2>;
>          #size-cells = <2>;
>  
> -        gce: mailbox@10212000 {
> -            compatible = "mediatek,mt8173-gce";
> -            reg = <0 0x10212000 0 0x1000>;
> -            interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
> +        gce0: mailbox@10320000 {
> +            compatible = "mediatek,mt8188-gce";

Are these new properties only for mt8188? If so, then you need a schema 
saying that. If not, then this is an unnecessary change to the example.

> +            reg = <0 0x10320000 0 0x4000>;
> +            interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH 0>;
>              #mbox-cells = <2>;
> -            clocks = <&infracfg CLK_INFRA_GCE>;
> +            clocks = <&infracfg_ao CLK_INFRA_AO_GCE>;
>              clock-names = "gce";
> +            mboxes = <&gce0 15 CMDQ_THR_PRIO_1>;

The provider is also a consumer?

> +            mediatek,gce-events = <CMDQ_SYNC_TOKEN_SECURE_THR_EOF>;
>          };
>      };
> -- 
> 2.18.0
> 

Re: [PATCH 01/15] dt-bindings: mailbox: Add property for CMDQ secure driver

[Jason-JH Lin (林睿祥)]

Hi Rob,

Thanks for the reviews.

On Tue, 2023-09-19 at 11:46 -0500, Rob Herring wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Tue, Sep 19, 2023 at 03:21:50AM +0800, Jason-JH.Lin wrote:
> > Add mboxes to define a GCE loopping thread as a secure irq handler.
> > Add mediatek,event to define a GCE software event siganl as a
> secure
> > irq.
> > 
> > These 2 properties are required for CMDQ secure driver.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  .../mailbox/mediatek,gce-mailbox.yaml         | 30
> +++++++++++++++----
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> > 
> > diff --git
> a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml 
> b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml
> > index cef9d7601398..5c9aebe83d2d 100644
> > --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> mailbox.yaml
> > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> mailbox.yaml
> > @@ -49,6 +49,21 @@ properties:
> >      items:
> >        - const: gce
> >  
> > +  mboxes:
> > +    description:
> > +      A mailbox channel used as a secure irq handler in normal
> world.
> > +      Using mailbox to communicate with GCE to setup looping
> thread,
> > +      it should have this property and a phandle, mailbox
> specifiers.
> 
> All cases of 'mboxes' have a phandle and specifiers. No need to
> repeat 
> that here.

OK, I'll remove it.

> 
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> Already has a type definition too. You need to define how many
> entries 
> and what each entry is if more than one. IOW, the same thing as
> clocks, 
> resets, interrupts, etc.

OK, I'll add the maximum number to 1 for this.

> 
> > +
> > +  mediatek,gce-events:
> 
> This is used all over. It really needs a single definition which is
> then 
> referenced by the users.

OK, I think it would defined it here since it is a GCE HW event signal.
I'll try to reference to other modules and make a definition for it.

> 
> > +    description:
> > +      The event id which is mapping to a software event signal to
> gce.
> > +      It is used as a secure irq for every secure gce threads.
> > +      The event id is defined in the gce header
> > +      include/dt-bindings/mailbox/mediatek,<chip>-gce.h of each
> chips.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +
> >  required:
> >    - compatible
> >    - "#mbox-cells"
> > @@ -71,20 +86,23 @@ additionalProperties: false
> >  
> >  examples:
> >    - |
> > -    #include <dt-bindings/clock/mt8173-clk.h>
> > +    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
> >      #include <dt-bindings/interrupt-controller/arm-gic.h>
> >      #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/mailbox/mediatek,mt8188-gce.h>
> >  
> >      soc {
> >          #address-cells = <2>;
> >          #size-cells = <2>;
> >  
> > -        gce: mailbox@10212000 {
> > -            compatible = "mediatek,mt8173-gce";
> > -            reg = <0 0x10212000 0 0x1000>;
> > -            interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
> > +        gce0: mailbox@10320000 {
> > +            compatible = "mediatek,mt8188-gce";
> 
> Are these new properties only for mt8188? If so, then you need a
> schema 
> saying that. If not, then this is an unnecessary change to the
> example.

No I think all SoC can add these properties if they have secure
requirement as mt8188. So I'll revert this unnecessary change to the
example.

> 
> > +            reg = <0 0x10320000 0 0x4000>;
> > +            interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH 0>;
> >              #mbox-cells = <2>;
> > -            clocks = <&infracfg CLK_INFRA_GCE>;
> > +            clocks = <&infracfg_ao CLK_INFRA_AO_GCE>;
> >              clock-names = "gce";
> > +            mboxes = <&gce0 15 CMDQ_THR_PRIO_1>;
> 
> The provider is also a consumer?

We'll use a mbox channel for receiving GCE signal in the secure mailbox
driver. So I think it is a provider and also a consumer.

Regards,
Jason-JH.Lin

> 
> > +            mediatek,gce-events =
> <CMDQ_SYNC_TOKEN_SECURE_THR_EOF>;
> >          };
> >      };
> > -- 
> > 2.18.0
> > 
> 

[PATCH 02/15] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Jason-JH.Lin]

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>
---
 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
+
 #endif
-- 
2.18.0

Re: [PATCH 02/15] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Krzysztof Kozlowski]

On 18/09/2023 21:21, 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.

How can #define be added after its usage? Does it even make any sense of
being separate patch?

Do you folks in Mediatek have any internal review before posting?

Best regards,
Krzysztof

Re: [PATCH 02/15] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

Thanks for the reviews.

On Sat, 2023-09-23 at 20:01 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 18/09/2023 21:21, 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.
> 
> How can #define be added after its usage? Does it even make any sense
> of
> being separate patch?
> 

This definition is used in the mt8195.dts at [PATCH 15/15] and the CMDQ
driver at [PATCH 9/15] will parse it from dts, then using it as secure
irq at [PATCH 10/15].

Do you mean I should merge this patch into the [PATCH 9/15]?


> Do you folks in Mediatek have any internal review before posting?

We'll use a robot to scan the whole series for the DT_CHECK,
checkpatch, build warning and build error, etc, before sending
patches. 

I'm sorry about bothering you to take time on reviewing these patches.

But it seems the robot can't scan out which patches should be separated
or merged together. It there any rules or scripts that can train the
robot to know how it goes?

Regards,
Jason-JH.Lin

> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 02/15] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Krzysztof Kozlowski]

On 25/09/2023 07:05, Jason-JH Lin (林睿祥) wrote:
> Hi Krzysztof,
> 
> Thanks for the reviews.
> 
> On Sat, 2023-09-23 at 20:01 +0200, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 18/09/2023 21:21, 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.
>>
>> How can #define be added after its usage? Does it even make any sense
>> of
>> being separate patch?
>>
> 
> This definition is used in the mt8195.dts at [PATCH 15/15] and the CMDQ

No, the define is used in previous patch, which means your patchset is
not bisectable and not tested.

Best regards,
Krzysztof

Re: [PATCH 02/15] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Jason-JH Lin (林睿祥)]

On Mon, 2023-09-25 at 08:42 +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/09/2023 07:05, Jason-JH Lin (林睿祥) wrote:
> > Hi Krzysztof,
> > 
> > Thanks for the reviews.
> > 
> > On Sat, 2023-09-23 at 20:01 +0200, Krzysztof Kozlowski wrote:
> >>   
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >>  On 18/09/2023 21:21, 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.
> >>
> >> How can #define be added after its usage? Does it even make any
> sense
> >> of
> >> being separate patch?
> >>
> > 
> > This definition is used in the mt8195.dts at [PATCH 15/15] and the
> CMDQ
> 
> No, the define is used in previous patch, which means your patchset
> is
> not bisectable and not tested.
> 

Do you mean this patch should add before patch 1?


The example of dts in patch 1 is used the definition of mt8188, so I
think I can add this patch to define the gce event id for mt8195 after
patch 1.

I will swap the patch 1 and the patch 2 in the next version, if that
can make it more appropriate.

Regards,
Jason-JH.Lin

> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 02/15] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Krzysztof Kozlowski]

On 25/09/2023 11:11, Jason-JH Lin (林睿祥) wrote:
> On Mon, 2023-09-25 at 08:42 +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/09/2023 07:05, Jason-JH Lin (林睿祥) wrote:
>>> Hi Krzysztof,
>>>
>>> Thanks for the reviews.
>>>
>>> On Sat, 2023-09-23 at 20:01 +0200, Krzysztof Kozlowski wrote:
>>>>   
>>>> External email : Please do not click links or open attachments
>> until
>>>> you have verified the sender or the content.
>>>>  On 18/09/2023 21:21, 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.
>>>>
>>>> How can #define be added after its usage? Does it even make any
>> sense
>>>> of
>>>> being separate patch?
>>>>
>>>
>>> This definition is used in the mt8195.dts at [PATCH 15/15] and the
>> CMDQ
>>
>> No, the define is used in previous patch, which means your patchset
>> is
>> not bisectable and not tested.
>>
> 
> Do you mean this patch should add before patch 1?
> 
> 
> The example of dts in patch 1 is used the definition of mt8188, so I
> think I can add this patch to define the gce event id for mt8195 after
> patch 1.
> 
> I will swap the patch 1 and the patch 2 in the next version, if that
> can make it more appropriate.

Really, test your patches. Each of them individually. Appropriateness is
one thing, but broken bisectability is an error.

Anyway, the patch is logically part of binding adding this, so it should
be squashed. Don't create some weird patch ordering where every define
or every function is in its own patch. I commented about it multiple
times in this patchset.

Best regards,
Krzysztof

Re: [PATCH 02/15] dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event id

[Jason-JH Lin (林睿祥)]

On Mon, 2023-09-25 at 11:28 +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/09/2023 11:11, Jason-JH Lin (林睿祥) wrote:
> > On Mon, 2023-09-25 at 08:42 +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/09/2023 07:05, Jason-JH Lin (林睿祥) wrote:
> >>> Hi Krzysztof,
> >>>
> >>> Thanks for the reviews.
> >>>
> >>> On Sat, 2023-09-23 at 20:01 +0200, Krzysztof Kozlowski wrote:
> >>>>   
> >>>> External email : Please do not click links or open attachments
> >> until
> >>>> you have verified the sender or the content.
> >>>>  On 18/09/2023 21:21, 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.
> >>>>
> >>>> How can #define be added after its usage? Does it even make any
> >> sense
> >>>> of
> >>>> being separate patch?
> >>>>
> >>>
> >>> This definition is used in the mt8195.dts at [PATCH 15/15] and
> the
> >> CMDQ
> >>
> >> No, the define is used in previous patch, which means your
> patchset
> >> is
> >> not bisectable and not tested.
> >>
> > 
> > Do you mean this patch should add before patch 1?
> > 
> > 
> > The example of dts in patch 1 is used the definition of mt8188, so
> I
> > think I can add this patch to define the gce event id for mt8195
> after
> > patch 1.
> > 
> > I will swap the patch 1 and the patch 2 in the next version, if
> that
> > can make it more appropriate.
> 
> Really, test your patches. Each of them individually. Appropriateness
> is
> one thing, but broken bisectability is an error.
> 
> Anyway, the patch is logically part of binding adding this, so it
> should
> be squashed. Don't create some weird patch ordering where every
> define
> or every function is in its own patch. I commented about it multiple
> times in this patchset.
> 

OK, I'll re-order the definition before the patch using it or squash
the definition to the patch using it.

Regards,
Jason-JH.Lin

> Best regards,
> Krzysztof
> 

[PATCH 03/15] soc: mailbox: Add SPR definition for GCE

[Jason-JH.Lin]

GCE has specific purpose registers, abbreviated as SPR.
Client can us SPR to store data or programs.

In CMDQ driver, it allows client to STORE or LOAD data into SPR.
The value stored in SPR will be cleared after reset GCE HW thread.

There are 4 SPR (register index 0 - 3) in every GCE HW thread.
SPR is thread-independent and HW secure protected.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 include/linux/soc/mediatek/mtk-cmdq.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 649955d2cf5c..f49ca8bd58e8 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -11,6 +11,11 @@
 #include <linux/mailbox/mtk-cmdq-mailbox.h>
 #include <linux/timer.h>
 
+#define CMDQ_THR_SPR_IDX0		0
+#define CMDQ_THR_SPR_IDX1		1
+#define CMDQ_THR_SPR_IDX2		2
+#define CMDQ_THR_SPR_IDX3		3
+
 #define CMDQ_ADDR_HIGH(addr)	((u32)(((addr) >> 16) & GENMASK(31, 0)))
 #define CMDQ_ADDR_LOW(addr)	((u16)(addr) | BIT(1))
 
-- 
2.18.0

Re: [PATCH 03/15] soc: mailbox: Add SPR definition for GCE

[Krzysztof Kozlowski]

On 18/09/2023 21:21, Jason-JH.Lin wrote:
> GCE has specific purpose registers, abbreviated as SPR.
> Client can us SPR to store data or programs.
> 
> In CMDQ driver, it allows client to STORE or LOAD data into SPR.
> The value stored in SPR will be cleared after reset GCE HW thread.
> 
> There are 4 SPR (register index 0 - 3) in every GCE HW thread.
> SPR is thread-independent and HW secure protected.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  include/linux/soc/mediatek/mtk-cmdq.h | 5 +++++

There is no user of this... Why do you add unused defines?

Best regards,
Krzysztof

Re: [PATCH 03/15] soc: mailbox: Add SPR definition for GCE

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

Thanks for the reviews.

On Sat, 2023-09-23 at 20:02 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 18/09/2023 21:21, Jason-JH.Lin wrote:
> > GCE has specific purpose registers, abbreviated as SPR.
> > Client can us SPR to store data or programs.
> > 
> > In CMDQ driver, it allows client to STORE or LOAD data into SPR.
> > The value stored in SPR will be cleared after reset GCE HW thread.
> > 
> > There are 4 SPR (register index 0 - 3) in every GCE HW thread.
> > SPR is thread-independent and HW secure protected.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  include/linux/soc/mediatek/mtk-cmdq.h | 5 +++++
> 
> There is no user of this... Why do you add unused defines?

It'll be used in cmdq_sec_insert_backup_cookie() at [PATCH 10/15].
Should I merge this patch into [PATCH 10/15]?

Regards,
Jason-JH.Lin

> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 03/15] soc: mailbox: Add SPR definition for GCE

[Krzysztof Kozlowski]

On 25/09/2023 07:08, Jason-JH Lin (林睿祥) wrote:
> Hi Krzysztof,
> 
> Thanks for the reviews.
> 
> On Sat, 2023-09-23 at 20:02 +0200, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 18/09/2023 21:21, Jason-JH.Lin wrote:
>>> GCE has specific purpose registers, abbreviated as SPR.
>>> Client can us SPR to store data or programs.
>>>
>>> In CMDQ driver, it allows client to STORE or LOAD data into SPR.
>>> The value stored in SPR will be cleared after reset GCE HW thread.
>>>
>>> There are 4 SPR (register index 0 - 3) in every GCE HW thread.
>>> SPR is thread-independent and HW secure protected.
>>>
>>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>>> ---
>>>  include/linux/soc/mediatek/mtk-cmdq.h | 5 +++++
>>
>> There is no user of this... Why do you add unused defines?
> 
> It'll be used in cmdq_sec_insert_backup_cookie() at [PATCH 10/15].
> Should I merge this patch into [PATCH 10/15]?

Yes, because what is the purpose of adding unused defines? I asked
before and did not get answer...

Best regards,
Krzysztof

Re: [PATCH 03/15] soc: mailbox: Add SPR definition for GCE

[Jason-JH Lin (林睿祥)]

On Mon, 2023-09-25 at 08:42 +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/09/2023 07:08, Jason-JH Lin (林睿祥) wrote:
> > Hi Krzysztof,
> > 
> > Thanks for the reviews.
> > 
> > On Sat, 2023-09-23 at 20:02 +0200, Krzysztof Kozlowski wrote:
> >>   
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >>  On 18/09/2023 21:21, Jason-JH.Lin wrote:
> >>> GCE has specific purpose registers, abbreviated as SPR.
> >>> Client can us SPR to store data or programs.
> >>>
> >>> In CMDQ driver, it allows client to STORE or LOAD data into SPR.
> >>> The value stored in SPR will be cleared after reset GCE HW
> thread.
> >>>
> >>> There are 4 SPR (register index 0 - 3) in every GCE HW thread.
> >>> SPR is thread-independent and HW secure protected.
> >>>
> >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> >>> ---
> >>>  include/linux/soc/mediatek/mtk-cmdq.h | 5 +++++
> >>
> >> There is no user of this... Why do you add unused defines?
> > 
> > It'll be used in cmdq_sec_insert_backup_cookie() at [PATCH 10/15].
> > Should I merge this patch into [PATCH 10/15]?
> 
> Yes, because what is the purpose of adding unused defines? I asked
> before and did not get answer...
> 

I'm totally agree with merging this patch to the usage parts of mtk-
cmdq-sec-mailbox.c.

But I have no idea why mtk-cmdq-sec-mailbox.c and mtk-cmdq-mailbox.c
are not placed in the same maintainer's tree as mtk-cmdq.h and mtk-
cmdq-helper.c, so I just separate them to different patch by tree like
the requirement from previous sent series.

I will re-organized this series to make the definition and the usage of
the code in the same patch.

Regards,
Jason-JH.Lin

> Best regards,
> Krzysztof
> 
> 

[PATCH 04/15] soc: mailbox: Add cmdq_pkt_logic_command to support math operation

[Jason-JH.Lin]

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>
---
 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)
+		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 f49ca8bd58e8..40e6f3097319 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -21,6 +21,30 @@
 
 struct cmdq_pkt;
 
+enum CMDQ_LOGIC_ENUM {
+	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,
+};
+
+struct cmdq_operand {
+	/* register type */
+	bool reg;
+	union {
+		/* index */
+		u16 idx;
+		/* value */
+		u16 value;
+	};
+};
+
 struct cmdq_client_reg {
 	u8 subsys;
 	u16 offset;
@@ -239,6 +263,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
+ *
+ * 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
-- 
2.18.0

[PATCH 05/15] mailbox: mediatek: Add cmdq_pkt_write_s_reg_value to CMDQ driver

[Jason-JH.Lin]

Add cmdq_pkt_write_s_reg_value to CMDQ driver.

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>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 13 +++++++++++++
 include/linux/soc/mediatek/mtk-cmdq.h  | 11 +++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 5194d66dfc0a..4be2a18a4a02 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -287,6 +287,19 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 }
 EXPORT_SYMBOL(cmdq_pkt_write_s_value);
 
+int cmdq_pkt_write_s_reg_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx, u32 value)
+{
+	struct cmdq_instruction inst = {};
+
+	inst.op = CMDQ_CODE_WRITE_S;
+	inst.dst_t = CMDQ_REG_TYPE;
+	inst.reg_dst = high_addr_reg_idx;
+	inst.value = value;
+
+	return cmdq_pkt_append_command(pkt, inst);
+}
+EXPORT_SYMBOL(cmdq_pkt_write_s_reg_value);
+
 int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 				u16 addr_low, u32 value, u32 mask)
 {
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 40e6f3097319..837ad8656adc 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -202,6 +202,17 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 				u16 addr_low, u32 value, u32 mask);
 
+/**
+ * cmdq_pkt_write_s_reg_value() - append write_s command to the CMDQ packet which
+ *				  write value to a register with low address pa
+ * @pkt:	the CMDQ packet
+ * @reg_idx:	internal register ID which contains high address of pa
+ * @value:	the specified target value
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_write_s_reg_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx, u32 value);
+
 /**
  * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
  * @pkt:	the CMDQ packet
-- 
2.18.0

[PATCH 06/15] mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread

[Jason-JH.Lin]

Add cmdq_mbox_stop to disable GCE thread.

To support the error handling or the stop flow of the GCE loopping
thread, lopping thread user can call cmdq_mbox_stop to disable the
GCE HW thread.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/mailbox/mtk-cmdq-mailbox.c       | 6 ++++++
 include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index 4d62b07c1411..8bd39fecbf00 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan)
 	spin_unlock_irqrestore(&thread->chan->lock, flags);
 }
 
+void cmdq_mbox_stop(struct mbox_chan *chan)
+{
+	cmdq_mbox_shutdown(chan);
+}
+EXPORT_SYMBOL(cmdq_mbox_stop);
+
 static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long timeout)
 {
 	struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv;
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index a8f0070c7aa9..f3e577335acb 100644
--- a/include/linux/mailbox/mtk-cmdq-mailbox.h
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -79,5 +79,6 @@ struct cmdq_pkt {
 };
 
 u8 cmdq_get_shift_pa(struct mbox_chan *chan);
+void cmdq_mbox_stop(struct mbox_chan *chan);
 
 #endif /* __MTK_CMDQ_MAILBOX_H__ */
-- 
2.18.0

Re: [PATCH 06/15] mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread

[CK Hu (胡俊光)]

Hi, Jason:

On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote:
> Add cmdq_mbox_stop to disable GCE thread.
> 
> To support the error handling or the stop flow of the GCE loopping
> thread, lopping thread user can call cmdq_mbox_stop to disable the
> GCE HW thread.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c       | 6 ++++++
>  include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 4d62b07c1411..8bd39fecbf00 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct mbox_chan
> *chan)
>  	spin_unlock_irqrestore(&thread->chan->lock, flags);
>  }
>  
> +void cmdq_mbox_stop(struct mbox_chan *chan)
> +{
> +	cmdq_mbox_shutdown(chan);

cmdq_mobx_stop() is equal to cmdq_mbox_shutdown(), so client driver
could  call mbox_free_channel() to do this and this function is
redundant.

Regards,
CK

> +}
> +EXPORT_SYMBOL(cmdq_mbox_stop);
> +
>  static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long
> timeout)
>  {
>  	struct cmdq_thread *thread = (struct cmdq_thread *)chan-
> >con_priv;
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h
> b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index a8f0070c7aa9..f3e577335acb 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -79,5 +79,6 @@ struct cmdq_pkt {
>  };
>  
>  u8 cmdq_get_shift_pa(struct mbox_chan *chan);
> +void cmdq_mbox_stop(struct mbox_chan *chan);
>  
>  #endif /* __MTK_CMDQ_MAILBOX_H__ */

Re: [PATCH 06/15] mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.


On Tue, 2023-09-19 at 01:24 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote:
> > Add cmdq_mbox_stop to disable GCE thread.
> > 
> > To support the error handling or the stop flow of the GCE loopping
> > thread, lopping thread user can call cmdq_mbox_stop to disable the
> > GCE HW thread.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c       | 6 ++++++
> >  include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 4d62b07c1411..8bd39fecbf00 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct
> > mbox_chan
> > *chan)
> >  	spin_unlock_irqrestore(&thread->chan->lock, flags);
> >  }
> >  
> > +void cmdq_mbox_stop(struct mbox_chan *chan)
> > +{
> > +	cmdq_mbox_shutdown(chan);
> 
> cmdq_mobx_stop() is equal to cmdq_mbox_shutdown(), so client driver
> could  call mbox_free_channel() to do this and this function is
> redundant.
> 

I'vd tried to use cmdq->mbox.ops->shutdown(cmdq->clt->chan) in mtk-
cmdq-sec-mbox.c, but it'll call to cmdq_sec_mbox_shutdown().
If I want to call to the cmdq_mbox_shutdown in mtk-cmdq-sec-mailbox.c,
I have to find the way to get the mbox of mtk-cmdq-mailbox.c.
So I think open a API is easy solution for this.

I have called mbox_free_channel() by calling cmdq_mbox_destroy() after 
cmdq_mbox_stop() in cmdq_sec_irq_notify_start() in mtk-cmdq-sec-
mailbox.c.


Regards,
Jason-JH.Lin

> Regards,
> CK
> 
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_stop);
> > +
> >  static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long
> > timeout)
> >  {
> >  	struct cmdq_thread *thread = (struct cmdq_thread *)chan-
> > > con_priv;
> > 
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index a8f0070c7aa9..f3e577335acb 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -79,5 +79,6 @@ struct cmdq_pkt {
> >  };
> >  
> >  u8 cmdq_get_shift_pa(struct mbox_chan *chan);
> > +void cmdq_mbox_stop(struct mbox_chan *chan);
> >  
> >  #endif /* __MTK_CMDQ_MAILBOX_H__ */

Re: [PATCH 06/15] mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread

[Krzysztof Kozlowski]

On 18/09/2023 21:21, Jason-JH.Lin wrote:
> Add cmdq_mbox_stop to disable GCE thread.
> 
> To support the error handling or the stop flow of the GCE loopping
> thread, lopping thread user can call cmdq_mbox_stop to disable the
> GCE HW thread.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c       | 6 ++++++
>  include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 4d62b07c1411..8bd39fecbf00 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan)
>  	spin_unlock_irqrestore(&thread->chan->lock, flags);
>  }
>  
> +void cmdq_mbox_stop(struct mbox_chan *chan)
> +{
> +	cmdq_mbox_shutdown(chan);
> +}
> +EXPORT_SYMBOL(cmdq_mbox_stop);

1. EXPORT_SYMBOL_GPL
2. Missing kernel doc (full doc)



Best regards,
Krzysztof

Re: [PATCH 06/15] mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

Thanks for the review.

On Sat, 2023-09-23 at 20:02 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 18/09/2023 21:21, Jason-JH.Lin wrote:
> > Add cmdq_mbox_stop to disable GCE thread.
> > 
> > To support the error handling or the stop flow of the GCE loopping
> > thread, lopping thread user can call cmdq_mbox_stop to disable the
> > GCE HW thread.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c       | 6 ++++++
> >  include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 4d62b07c1411..8bd39fecbf00 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct
> mbox_chan *chan)
> >  spin_unlock_irqrestore(&thread->chan->lock, flags);
> >  }
> >  
> > +void cmdq_mbox_stop(struct mbox_chan *chan)
> > +{
> > +cmdq_mbox_shutdown(chan);
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_stop);
> 
> 1. EXPORT_SYMBOL_GPL
> 2. Missing kernel doc (full doc)
> 
> 
After reviewing by CK, I think this patch should be dropped.
Sorry for bothering you and thanks for the reviews.

Regards,
Jason-JH.Lin
> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 06/15] mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread

[Krzysztof Kozlowski]

On 18/09/2023 21:21, Jason-JH.Lin wrote:
> Add cmdq_mbox_stop to disable GCE thread.
> 
> To support the error handling or the stop flow of the GCE loopping
> thread, lopping thread user can call cmdq_mbox_stop to disable the
> GCE HW thread.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c       | 6 ++++++
>  include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 4d62b07c1411..8bd39fecbf00 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan)
>  	spin_unlock_irqrestore(&thread->chan->lock, flags);
>  }
>  
> +void cmdq_mbox_stop(struct mbox_chan *chan)
> +{
> +	cmdq_mbox_shutdown(chan);
> +}
> +EXPORT_SYMBOL(cmdq_mbox_stop);

Plus there are no users.

NAK. This is not code which should be posted upstream.

Best regards,
Krzysztof

Re: [PATCH 06/15] mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

Thanks for the reviews.

On Sat, 2023-09-23 at 20:07 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 18/09/2023 21:21, Jason-JH.Lin wrote:
> > Add cmdq_mbox_stop to disable GCE thread.
> > 
> > To support the error handling or the stop flow of the GCE loopping
> > thread, lopping thread user can call cmdq_mbox_stop to disable the
> > GCE HW thread.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c       | 6 ++++++
> >  include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 4d62b07c1411..8bd39fecbf00 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct
> mbox_chan *chan)
> >  spin_unlock_irqrestore(&thread->chan->lock, flags);
> >  }
> >  
> > +void cmdq_mbox_stop(struct mbox_chan *chan)
> > +{
> > +cmdq_mbox_shutdown(chan);
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_stop);
> 
> Plus there are no users.
> 
> NAK. This is not code which should be posted upstream.
> 
It'll be used in cmdq_sec_irq_notify_start() at [PATCH 10/15].
After reviewing by CK, I think I'll try to drop this patch.

Regards,
Jason-JH.Lin

> Best regards,
> Krzysztof
> 
> 

[PATCH 07/15] mailbox: mediatek: Add loop pkt flag and irq handling for loop command

[Jason-JH.Lin]

CMDQ client can use a loop flag for the CMDQ packet to make current
command buffer jumps to the beginning when GCE executes to the end
of commands buffer.

GCE irq occurs when GCE executes to the end of command instruction.
If the CMDQ packet is a loopping command, GCE irq handler can not
delete the CMDQ task and disable the GCE thread.

Add cmdq_mbox_stop to support thread disable

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/mailbox/mtk-cmdq-mailbox.c       | 11 +++++++++++
 include/linux/mailbox/mtk-cmdq-mailbox.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index 8bd39fecbf00..a3b831b6bab9 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -264,6 +264,17 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq,
 
 	curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR) << cmdq->pdata->shift;
 
+	task = list_first_entry_or_null(&thread->task_busy_list,
+					struct cmdq_task, list_entry);
+	if (task && task->pkt->loop) {
+		struct cmdq_cb_data data;
+
+		data.sta = err;
+		data.pkt = task->pkt;
+		mbox_chan_received_data(task->thread->chan, &data);
+		return;
+	}
+
 	list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
 				 list_entry) {
 		task_end_pa = task->pa_base + task->pkt->cmd_buf_size;
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index f3e577335acb..fc663b994b7a 100644
--- a/include/linux/mailbox/mtk-cmdq-mailbox.h
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -76,6 +76,7 @@ struct cmdq_pkt {
 	size_t			cmd_buf_size; /* command occupied size */
 	size_t			buf_size; /* real buffer size */
 	void			*cl;
+	bool			loop;
 };
 
 u8 cmdq_get_shift_pa(struct mbox_chan *chan);
-- 
2.18.0

Re: [PATCH 07/15] mailbox: mediatek: Add loop pkt flag and irq handling for loop command

[Krzysztof Kozlowski]

On 18/09/2023 21:21, Jason-JH.Lin wrote:
> CMDQ client can use a loop flag for the CMDQ packet to make current
> command buffer jumps to the beginning when GCE executes to the end
> of commands buffer.
> 
> GCE irq occurs when GCE executes to the end of command instruction.
> If the CMDQ packet is a loopping command, GCE irq handler can not
> delete the CMDQ task and disable the GCE thread.
> 
> Add cmdq_mbox_stop to support thread disable

How or where do you add it? I do not see it in this patch. Your patchset
looks randomly organized.

Best regards,
Krzysztof

Re: [PATCH 07/15] mailbox: mediatek: Add loop pkt flag and irq handling for loop command

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

Thanks for the reviews.

On Sat, 2023-09-23 at 20: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 18/09/2023 21:21, Jason-JH.Lin wrote:
> > CMDQ client can use a loop flag for the CMDQ packet to make current
> > command buffer jumps to the beginning when GCE executes to the end
> > of commands buffer.
> > 
> > GCE irq occurs when GCE executes to the end of command instruction.
> > If the CMDQ packet is a loopping command, GCE irq handler can not
> > delete the CMDQ task and disable the GCE thread.
> > 
> > Add cmdq_mbox_stop to support thread disable
> 
> How or where do you add it? I do not see it in this patch. Your
> patchset
> looks randomly organized.

This will be used in cmdq_pkt_finialize_loop() at [PATCH 8/15].

mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the
same maintainer's tree, so I separate this to another patch from [PATCH
8/15].

Regards,
Jason-JH.Lin

> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 07/15] mailbox: mediatek: Add loop pkt flag and irq handling for loop command

[Krzysztof Kozlowski]

On 25/09/2023 07:21, Jason-JH Lin (林睿祥) wrote:
> Hi Krzysztof,
> 
> Thanks for the reviews.
> 
> On Sat, 2023-09-23 at 20: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 18/09/2023 21:21, Jason-JH.Lin wrote:
>>> CMDQ client can use a loop flag for the CMDQ packet to make current
>>> command buffer jumps to the beginning when GCE executes to the end
>>> of commands buffer.
>>>
>>> GCE irq occurs when GCE executes to the end of command instruction.
>>> If the CMDQ packet is a loopping command, GCE irq handler can not
>>> delete the CMDQ task and disable the GCE thread.
>>>
>>> Add cmdq_mbox_stop to support thread disable
>>
>> How or where do you add it? I do not see it in this patch. Your
>> patchset
>> looks randomly organized.
> 
> This will be used in cmdq_pkt_finialize_loop() at [PATCH 8/15].
> 
> mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the
> same maintainer's tree, so I separate this to another patch from [PATCH
> 8/15].

Why? Anyway it has to go through same tree. You have dependencies. Such
artificial split makes it only difficult to review and understand.
Re-organize your patchset to be correctly split per each logical
feature/change. Split per subsystems is not the same.

Best regards,
Krzysztof

Re: [PATCH 07/15] mailbox: mediatek: Add loop pkt flag and irq handling for loop command

[Jason-JH Lin (林睿祥)]

On Mon, 2023-09-25 at 08:44 +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/09/2023 07:21, Jason-JH Lin (林睿祥) wrote:
> > Hi Krzysztof,
> > 
> > Thanks for the reviews.
> > 
> > On Sat, 2023-09-23 at 20: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 18/09/2023 21:21, Jason-JH.Lin wrote:
> >>> CMDQ client can use a loop flag for the CMDQ packet to make
> current
> >>> command buffer jumps to the beginning when GCE executes to the
> end
> >>> of commands buffer.
> >>>
> >>> GCE irq occurs when GCE executes to the end of command
> instruction.
> >>> If the CMDQ packet is a loopping command, GCE irq handler can not
> >>> delete the CMDQ task and disable the GCE thread.
> >>>
> >>> Add cmdq_mbox_stop to support thread disable
> >>
> >> How or where do you add it? I do not see it in this patch. Your
> >> patchset
> >> looks randomly organized.
> > 
> > This will be used in cmdq_pkt_finialize_loop() at [PATCH 8/15].
> > 
> > mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the
> > same maintainer's tree, so I separate this to another patch from
> [PATCH
> > 8/15].
> 
> Why? Anyway it has to go through same tree. You have dependencies.
> Such
> artificial split makes it only difficult to review and understand.
> Re-organize your patchset to be correctly split per each logical
> feature/change. Split per subsystems is not the same.
> 

Yes, these related files are in the different maintainer's tree.
Refer to https://www.kernel.org/doc/linux/MAINTAINERS

MAILBOX API
M: Jassi Brar
F: drivers/mailbox/
- drivers/mailbox/mtk-cmdq-mailbox.c
- drivers/mailbox/mtk-cmdq-sec-
mailbox.c

ARM/Mediatek SoC support
M: Matthias Brugger
F: drivers/soc/mediatek/
K: mediatek
- drivers/soc/mediatek/mtk-cmdq-helper.c
-
include/linux/soc/mediatek/mtk-cmdq.h

I think we should add a new MAINTAINER label for mediatek CMDQ mailbox
and put these files together, such as "MAILBOX ARM MHUv2" and "QUALCOM
IPCC MAILBOX DRIVER".
But I don't know how to make a request for that.

Anyway, I'll squash this logical feature to the same patch, no matter
these files are not in the same tree.

Regards,
Jason-JH.Lin

> Best regards,
> Krzysztof
> 

Re: [PATCH 07/15] mailbox: mediatek: Add loop pkt flag and irq handling for loop command

[Krzysztof Kozlowski]

On 26/09/2023 05:20, Jason-JH Lin (林睿祥) wrote:
mdq_pkt_finialize_loop() at [PATCH 8/15].
>>>
>>> mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the
>>> same maintainer's tree, so I separate this to another patch from
>> [PATCH
>>> 8/15].
>>
>> Why? Anyway it has to go through same tree. You have dependencies.
>> Such
>> artificial split makes it only difficult to review and understand.
>> Re-organize your patchset to be correctly split per each logical
>> feature/change. Split per subsystems is not the same.
>>
> 
> Yes, these related files are in the different maintainer's tree.
> Refer to https://www.kernel.org/doc/linux/MAINTAINERS
> 
> MAILBOX API
> M: Jassi Brar
> F: drivers/mailbox/
> - drivers/mailbox/mtk-cmdq-mailbox.c
> - drivers/mailbox/mtk-cmdq-sec-
> mailbox.c
> 
> ARM/Mediatek SoC support
> M: Matthias Brugger
> F: drivers/soc/mediatek/
> K: mediatek
> - drivers/soc/mediatek/mtk-cmdq-helper.c
> -
> include/linux/soc/mediatek/mtk-cmdq.h
> 
> I think we should add a new MAINTAINER label for mediatek CMDQ mailbox
> and put these files together, such as "MAILBOX ARM MHUv2" and "QUALCOM
> IPCC MAILBOX DRIVER".

Why? It's not related to the topic of splitting patchset into patches.
There is no problem of patchsets touching multiple subsystems. We
already solved this problem many years ago...


> But I don't know how to make a request for that.

Anyway, you would not be a maintainer taking patches, just a reviewer
called "M:" here...

> 
> Anyway, I'll squash this logical feature to the same patch, no matter
> these files are not in the same tree.
> 
Best regards,
Krzysztof

[PATCH 08/15] soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver

[Jason-JH.Lin]

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.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++
 include/linux/soc/mediatek/mtk-cmdq.h  |  8 ++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 4be2a18a4a02..bbb127620bb3 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 }
 EXPORT_SYMBOL(cmdq_pkt_finalize);
 
+int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt)
+{
+	struct cmdq_instruction inst = { {0} };
+	int err;
+
+	/* insert EOC and generate IRQ for each command iteration */
+	inst.op = CMDQ_CODE_EOC;
+	inst.value = CMDQ_EOC_IRQ_EN;
+	err = cmdq_pkt_append_command(pkt, inst);
+	if (err < 0)
+		return err;
+
+	/* JUMP to start of pkt */
+	err = cmdq_pkt_jump(pkt, pkt->pa_base);
+	if (err < 0)
+		return err;
+
+	pkt->loop = true;
+
+	return err;
+}
+EXPORT_SYMBOL(cmdq_pkt_finalize_loop);
+
 int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
 {
 	int err;
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 837ad8656adc..38a8e47da338 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -323,6 +323,14 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr);
  */
 int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
 
+/**
+ * cmdq_pkt_finalize_loop() - Append EOC and jump to start command.
+ * @pkt:	the CMDQ packet
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt);
+
 /**
  * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
  *                          packet and call back at the end of done packet
-- 
2.18.0

Re: [PATCH 08/15] soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver

[CK Hu (胡俊光)]

Hi, Jason:

On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote:
> 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.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  |  8 ++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 4be2a18a4a02..bbb127620bb3 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>  }
>  EXPORT_SYMBOL(cmdq_pkt_finalize);
>  
> +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +	int err;
> +
> +	/* insert EOC and generate IRQ for each command iteration */
> +	inst.op = CMDQ_CODE_EOC;
> +	inst.value = CMDQ_EOC_IRQ_EN;
> +	err = cmdq_pkt_append_command(pkt, inst);
> +	if (err < 0)
> +		return err;
> +
> +	/* JUMP to start of pkt */
> +	err = cmdq_pkt_jump(pkt, pkt->pa_base);
> +	if (err < 0)
> +		return err;

Could you explain the case that a loop thread would trigger an
interrupt? In DRM crc function, the loop thread need not to trigger
interrupt, so I'm curious about this.

Regards,
CK

> +
> +	pkt->loop = true;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_finalize_loop);
> +
>  int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
>  {
>  	int err;
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> index 837ad8656adc..38a8e47da338 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -323,6 +323,14 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt,
> dma_addr_t addr);
>   */
>  int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
>  
> +/**
> + * cmdq_pkt_finalize_loop() - Append EOC and jump to start command.
> + * @pkt:	the CMDQ packet
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt);
> +
>  /**
>   * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute
> the CMDQ
>   *                          packet and call back at the end of done
> packet

Re: [PATCH 08/15] soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver

[Jason-JH Lin (林睿祥)]

On Tue, 2023-09-19 at 01:38 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote:
> > 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.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c | 23
> > +++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-cmdq.h  |  8 ++++++++
> >  2 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 4be2a18a4a02..bbb127620bb3 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_finalize);
> >  
> > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt)
> > +{
> > +	struct cmdq_instruction inst = { {0} };
> > +	int err;
> > +
> > +	/* insert EOC and generate IRQ for each command iteration */
> > +	inst.op = CMDQ_CODE_EOC;
> > +	inst.value = CMDQ_EOC_IRQ_EN;
> > +	err = cmdq_pkt_append_command(pkt, inst);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	/* JUMP to start of pkt */
> > +	err = cmdq_pkt_jump(pkt, pkt->pa_base);
> > +	if (err < 0)
> > +		return err;
> 
> Could you explain the case that a loop thread would trigger an
> interrupt? In DRM crc function, the loop thread need not to trigger
> interrupt, so I'm curious about this.
> 
The looping thread in DRM crc funtion is for update CRC value to DRAM
during every vblank event coming. It doesn't need to handle the
software flow after the EOC.

The looping thread in cmdq_sec_irq_notify_start() is waiting for every
CMDQ_SYNC_TOKEN_SEC_THR_EOF being set, that means the GCE in secure
world has finished all commands in a command buffer. Then it needs a
GCE irq to trigger secure mailbox rx_callback() to handle the task of
secure cmdq_pkt done in software.

Regards,
Jason-JH.Lin

> Regards,
> CK
> 
> > +
> > +	pkt->loop = true;
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop);
> > +
> >  int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
> >  {
> >  	int err;
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> > b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 837ad8656adc..38a8e47da338 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -323,6 +323,14 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt,
> > dma_addr_t addr);
> >   */
> >  int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
> >  
> > +/**
> > + * cmdq_pkt_finalize_loop() - Append EOC and jump to start
> > command.
> > + * @pkt:	the CMDQ packet
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt);
> > +
> >  /**
> >   * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute
> > the CMDQ
> >   *                          packet and call back at the end of
> > done
> > packet

Re: [PATCH 08/15] soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver

[Krzysztof Kozlowski]

On 18/09/2023 21:21, Jason-JH.Lin wrote:
> 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.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  |  8 ++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 4be2a18a4a02..bbb127620bb3 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>  }
>  EXPORT_SYMBOL(cmdq_pkt_finalize);
>  
> +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +	int err;
> +
> +	/* insert EOC and generate IRQ for each command iteration */
> +	inst.op = CMDQ_CODE_EOC;
> +	inst.value = CMDQ_EOC_IRQ_EN;
> +	err = cmdq_pkt_append_command(pkt, inst);
> +	if (err < 0)
> +		return err;
> +
> +	/* JUMP to start of pkt */
> +	err = cmdq_pkt_jump(pkt, pkt->pa_base);
> +	if (err < 0)
> +		return err;
> +
> +	pkt->loop = true;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_finalize_loop);

1. Missing GPL
2. Missing kerneldoc
3. Missing user
4. Are you going to split the patchset into one function per patch? No.

Best regards,
Krzysztof

Re: [PATCH 08/15] soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver

[Krzysztof Kozlowski]

On 18/09/2023 21:21, Jason-JH.Lin wrote:
> 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.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  |  8 ++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 4be2a18a4a02..bbb127620bb3 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>  }
>  EXPORT_SYMBOL(cmdq_pkt_finalize);
>  
> +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +	int err;
> +
> +	/* insert EOC and generate IRQ for each command iteration */
> +	inst.op = CMDQ_CODE_EOC;
> +	inst.value = CMDQ_EOC_IRQ_EN;
> +	err = cmdq_pkt_append_command(pkt, inst);
> +	if (err < 0)
> +		return err;
> +
> +	/* JUMP to start of pkt */
> +	err = cmdq_pkt_jump(pkt, pkt->pa_base);
> +	if (err < 0)
> +		return err;
> +
> +	pkt->loop = true;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_finalize_loop);

NAK. No users (and please carefully think before you answer that your
other patch uses it).

Best regards,
Krzysztof

Re: [PATCH 08/15] soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

Thanks for the reviews.

On Sat, 2023-09-23 at 20: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 18/09/2023 21:21, Jason-JH.Lin wrote:
> > 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.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c | 23
> +++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-cmdq.h  |  8 ++++++++
> >  2 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 4be2a18a4a02..bbb127620bb3 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_finalize);
> >  
> > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt)
> > +{
> > +struct cmdq_instruction inst = { {0} };
> > +int err;
> > +
> > +/* insert EOC and generate IRQ for each command iteration */
> > +inst.op = CMDQ_CODE_EOC;
> > +inst.value = CMDQ_EOC_IRQ_EN;
> > +err = cmdq_pkt_append_command(pkt, inst);
> > +if (err < 0)
> > +return err;
> > +
> > +/* JUMP to start of pkt */
> > +err = cmdq_pkt_jump(pkt, pkt->pa_base);
> > +if (err < 0)
> > +return err;
> > +
> > +pkt->loop = true;
> > +
> > +return err;
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop);
> 
> NAK. No users (and please carefully think before you answer that your
> other patch uses it).
> 

As I know, the API with EXPORT_SYMBOL means it can be used by a dynamic
loaded module.

Do you means that mtk-cmdq-sec-mailbox.c in [PATCH 10/15] is a built-in 
module currently, so I should not add EXPORT_SYMBOL to this API?

Regards,
Jason-JH.Lin

> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 08/15] soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver

[Krzysztof Kozlowski]

On 25/09/2023 08:04, Jason-JH Lin (林睿祥) wrote:
> Hi Krzysztof,
> 
> Thanks for the reviews.
> 
> On Sat, 2023-09-23 at 20: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 18/09/2023 21:21, Jason-JH.Lin wrote:
>>> 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.
>>>
>>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>>> ---
>>>  drivers/soc/mediatek/mtk-cmdq-helper.c | 23
>> +++++++++++++++++++++++
>>>  include/linux/soc/mediatek/mtk-cmdq.h  |  8 ++++++++
>>>  2 files changed, 31 insertions(+)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
>> b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index 4be2a18a4a02..bbb127620bb3 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>>>  }
>>>  EXPORT_SYMBOL(cmdq_pkt_finalize);
>>>  
>>> +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt)
>>> +{
>>> +struct cmdq_instruction inst = { {0} };
>>> +int err;
>>> +
>>> +/* insert EOC and generate IRQ for each command iteration */
>>> +inst.op = CMDQ_CODE_EOC;
>>> +inst.value = CMDQ_EOC_IRQ_EN;
>>> +err = cmdq_pkt_append_command(pkt, inst);
>>> +if (err < 0)
>>> +return err;
>>> +
>>> +/* JUMP to start of pkt */
>>> +err = cmdq_pkt_jump(pkt, pkt->pa_base);
>>> +if (err < 0)
>>> +return err;
>>> +
>>> +pkt->loop = true;
>>> +
>>> +return err;
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_finalize_loop);
>>
>> NAK. No users (and please carefully think before you answer that your
>> other patch uses it).
>>
> 
> As I know, the API with EXPORT_SYMBOL means it can be used by a dynamic
> loaded module.
> 
> Do you means that mtk-cmdq-sec-mailbox.c in [PATCH 10/15] is a built-in 
> module currently, so I should not add EXPORT_SYMBOL to this API?

I mean there is no need for this. Please name the name of module where
this function will be defined and name of module(s) using it. ".c" is
not a module. ".ko" is.


Best regards,
Krzysztof

[PATCH 09/15] mailbox: mediatek: Add secure CMDQ driver support for CMDQ driver

[Jason-JH.Lin]

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.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/mailbox/mtk-cmdq-mailbox.c       | 40 ++++++++++++++++++++++--
 include/linux/mailbox/mtk-cmdq-mailbox.h | 11 +++++++
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index a3b831b6bab9..cd4a8f0ef6ad 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;
 };
 
@@ -566,14 +567,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);
+	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)
@@ -591,6 +601,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);
+
 		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) {
@@ -659,6 +671,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;
 }
 
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index fc663b994b7a..a8f4f78f21d8 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;
+};
+
 u8 cmdq_get_shift_pa(struct mbox_chan *chan);
 void cmdq_mbox_stop(struct mbox_chan *chan);
 
-- 
2.18.0

[PATCH 10/15] mailbox: mediatek: Add CMDQ secure mailbox driver

[Jason-JH.Lin]

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        | 1103 +++++++++++++++++
 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, 1715 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..954dfe7a57ed
--- /dev/null
+++ b/drivers/mailbox/mtk-cmdq-sec-mailbox.c
@@ -0,0 +1,1103 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2019 MediaTek Inc.
+
+#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)
+#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
+ */
+struct cmdq_sec_context {
+	struct list_head		listEntry;
+
+	/* basic info */
+	u32				tgid; /* tgid of process context */
+	u32				referCount; /* reference count for open cmdq device node */
+
+	/* iwc state */
+	enum cmdq_iwc_state_enum	state;
+
+	/* iwc information */
+	void				*iwc_msg; /* message buffer */
+
+	struct cmdq_sec_tee_context	tee; /* trustzone parameters */
+};
+
+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;
+	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;
+};
+
+/**
+ * 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 s32 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)
+{
+	cmdq_sec_setup_tee_context(&context->tee);
+}
+
+static inline s32 cmdq_sec_init_context_base(struct cmdq_sec_context *context)
+{
+	s32 status;
+
+	status = cmdq_sec_init_context(&context->tee);
+	if (status < 0)
+		return status;
+
+	return status;
+}
+
+s32 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);
+	cmdq_pkt_set_event(pkt, cmdq->cmdq_event);
+
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_sec_insert_backup_cookie);
+
+static s32 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 s32 cmdq_sec_check_sec(struct cmdq_pkt *pkt)
+{
+	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 s32 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;
+	s32 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;
+}
+
+s32 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;
+	s32 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);
+
+s32 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)
+{
+	s32 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 s32 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;
+	s32 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(s32));
+		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);
+	s32 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(&cmdq->irq_notify_work))
+		queue_work(cmdq->notify_wq, &cmdq->irq_notify_work);
+	else
+		pr_debug("%s last notify callback working", __func__);
+}
+
+static s32 cmdq_sec_irq_notify_start(struct cmdq_sec *cmdq)
+{
+	s32 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_stop(cmdq->clt->chan);
+		cmdq_mbox_destroy(cmdq->clt);
+	} else {
+		cmdq->notify_run = true;
+		pr_debug("%s success!", __func__);
+	}
+
+	return err;
+}
+
+static s32 cmdq_sec_session_init(struct cmdq_sec_context *context)
+{
+	s32 err = 0;
+
+	if (context->state >= IWC_SES_OPENED) {
+		pr_debug("session opened:%u", context->state);
+		return err;
+	}
+
+	switch (context->state) {
+	case IWC_INIT:
+		err = cmdq_sec_init_context_base(context);
+		if (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;
+			break;
+		}
+
+		err = cmdq_sec_allocate_wsm(&context->tee, &context->iwc_msg,
+					    sizeof(struct iwcCmdqMessage_t));
+		if (err)
+			break;
+
+		context->state = IWC_WSM_ALLOCATED;
+	fallthrough;
+	case IWC_WSM_ALLOCATED:
+		err = cmdq_sec_open_session(&context->tee, context->iwc_msg);
+		if (err)
+			break;
+
+		context->state = IWC_SES_OPENED;
+	fallthrough;
+	default:
+		break;
+	}
+	return err;
+}
+
+static s32 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;
+	}
+
+	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__);
+		return 0;
+	}
+
+	instr = &iwc_msg->command.pVABase[iwc_msg->command.commandSize / 4 - 4];
+	if (instr[0] == 0x1 && instr[1] == 0x40000000)
+		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 s32 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)
+{
+	s32 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 == 0)
+		context->state = IWC_SES_ON_TRANSACTED;
+
+	return err;
+}
+
+static s32 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_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 s32 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;
+	s32 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);
+
+	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;
+	s32 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(s32));
+	}
+
+	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:
+	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);
+	s32 i;
+
+	mutex_lock(&cmdq->exec_lock);
+
+	if (list_empty(&thread->task_list))
+		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;
+}
+
+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;
+	s32 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)
+{
+	s32 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",
+		.pm = &cmdq_sec_pm_ops,
+	},
+};
+
+static int __init cmdq_sec_init(void)
+{
+	s32 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);
+
+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..2b69862d8d0e
--- /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.
+
+#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*/
+}
+
+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)
+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
+
+s32 cmdq_sec_init_context(struct cmdq_sec_tee_context *tee)
+{
+	s32 status;
+
+#if IS_ENABLED(CONFIG_TEE)
+	tee->tee_context = tee_client_open_context(NULL, tee_dev_match, NULL, NULL);
+#endif
+	if (tee->tee_context)
+		status = 0;
+	else
+		status = -EINVAL;
+
+	pr_info("[%s][%d]status=%d\n", __func__, __LINE__, status);
+
+	return status;
+}
+EXPORT_SYMBOL(cmdq_sec_init_context);
+
+s32 cmdq_sec_deinit_context(struct cmdq_sec_tee_context *tee)
+{
+#if IS_ENABLED(CONFIG_TEE)
+	if (tee)
+		tee_client_close_context(tee->tee_context);
+#endif
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_sec_deinit_context);
+
+s32 cmdq_sec_allocate_wsm(struct cmdq_sec_tee_context *tee, void **wsm_buffer, u32 size)
+{
+	void *buffer;
+
+	if (!wsm_buffer)
+		return -EINVAL;
+
+	if (!size)
+		size = 8;
+
+	buffer = kmalloc(size, GFP_KERNEL);
+	if (!buffer)
+		return -EINVAL;
+
+#if IS_ENABLED(CONFIG_TEE)
+	tee->shared_mem = tee_shm_register_kernel_buf(tee->tee_context,
+						      buffer,
+						      size);
+#endif
+	if (!tee->shared_mem) {
+		kfree(buffer);
+		return -EINVAL;
+	}
+
+	*wsm_buffer = buffer;
+
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_sec_allocate_wsm);
+
+s32 cmdq_sec_free_wsm(struct cmdq_sec_tee_context *tee, void **wsm_buffer)
+{
+	if (!wsm_buffer)
+		return -EINVAL;
+
+#if IS_ENABLED(CONFIG_TEE)
+	tee_shm_put(tee->shared_mem);
+#endif
+	tee->shared_mem = NULL;
+	kfree(*wsm_buffer);
+	*wsm_buffer = NULL;
+
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_sec_free_wsm);
+
+s32 cmdq_sec_open_session(struct cmdq_sec_tee_context *tee, void *wsm_buffer)
+{
+	struct tee_ioctl_open_session_arg osarg;
+	struct tee_param params;
+	int rc = 0;
+
+	if (!wsm_buffer)
+		return -EINVAL;
+
+	memset(&params, 0, sizeof(struct tee_param));
+
+	memset(&osarg, 0, sizeof(osarg));
+	osarg.num_params = 1;
+	memcpy(osarg.uuid, tee->uuid, sizeof(osarg.uuid));
+	osarg.clnt_login = 0;
+#if IS_ENABLED(CONFIG_TEE)
+	rc = tee_client_open_session(tee->tee_context, &osarg, &params);
+#endif
+	if (rc)
+		return -EINVAL;
+
+	if (!osarg.ret)
+		tee->session = osarg.session;
+
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_sec_open_session);
+
+s32 cmdq_sec_close_session(struct cmdq_sec_tee_context *tee)
+{
+#if IS_ENABLED(CONFIG_TEE)
+	tee_client_close_session(tee->tee_context, tee->session);
+#endif
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_sec_close_session);
+
+s32 cmdq_sec_execute_session(struct cmdq_sec_tee_context *tee, u32 cmd, s32 timeout_ms)
+{
+	s32 status;
+	struct tee_ioctl_invoke_arg ivarg;
+	struct tee_param params;
+	u64 ts = sched_clock();
+	int rc = 0;
+
+	cmdq_sec_lock_secure_path();
+
+	memset(&params, 0, sizeof(struct tee_param));
+	params.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
+	params.u.memref.shm = tee->shared_mem;
+	params.u.memref.shm_offs = 0;
+	params.u.memref.size = tee->shared_mem->size;
+
+	memset(&ivarg, 0, sizeof(ivarg));
+	ivarg.num_params = 1;
+	ivarg.session = tee->session;
+	ivarg.func = cmd;
+
+#if IS_ENABLED(CONFIG_TEE)
+	rc = tee_client_invoke_func(tee->tee_context, &ivarg, &params);
+#endif
+	if (rc) {
+		pr_err("tee_client_invoke_func failed, ret=%d\n", rc);
+		return -EINVAL;
+	}
+
+	status = ivarg.ret;
+
+	cmdq_sec_unlock_secure_path();
+
+	ts = div_u64(sched_clock() - ts, 1000000);
+
+	if (status != 0)
+		pr_err("[SEC]execute: TEEC_InvokeCommand:%u err:%d cost:%lluus", cmd, status, ts);
+	else if (ts > timeout_ms)
+		pr_err("[SEC]execute: TEEC_InvokeCommand:%u ret:%d cost:%lluus", cmd, status, ts);
+	else
+		pr_debug("[SEC]execute: TEEC_InvokeCommand:%u ret:%d cost:%lluus", cmd, status, ts);
+
+	return status;
+}
+EXPORT_SYMBOL(cmdq_sec_execute_session);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index a8f4f78f21d8..89deb06413c3 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>
 #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 */
+#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)
+
+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
+ */
+struct iwcCmdqAddrMetadata_t {
+	/*
+	 * [IN] index of instruction
+	 * Update its arg_b value to real PA/MVA in secure world
+	 */
+	u32 instrIndex;
+
+	/*
+	 * Note: Buffer and offset
+	 *
+	 *   -------------
+	 *   |     |     |
+	 *   -------------
+	 *   ^     ^  ^  ^
+	 *   A     B  C  D
+	 *
+	 *	A: baseHandle
+	 *	B: baseHandle + blockOffset
+	 *	C: baseHandle + blockOffset + offset
+	 *	A~B or B~D: size
+	 */
+
+	u32 type;		/* [IN] addr handle type */
+	u64 baseHandle;	/* [IN] secure address handle */
+	u32 blockOffset;	/* [IN] block offset from handle(PA) to current block(plane) */
+	u32 offset;	/* [IN]_b, buffser offset to secure handle */
+	u32 size;		/* [IN] buffer size */
+	u32 port;		/* [IN] HW port id (i.e. M4U port id)*/
+};
+
+struct iwcCmdqDebugConfig_t {
+	s32 logLevel;
+	s32 enableProfile;
+};
+
+struct iwcCmdqSecStatus_t {
+	u32 step;
+	s32 status;
+	u32 args[4];
+	u32 sec_inst[CMDQ_SEC_MESSAGE_INST_LEN];
+	u32 inst_index;
+	char dispatch[CMDQ_SEC_DISPATCH_LEN];
+};
+
+struct iwcCmdqMetadata_t {
+	u32 addrListLength;
+	struct iwcCmdqAddrMetadata_t addrList[CMDQ_IWC_MAX_ADDR_LIST_LENGTH];
+
+	u64 enginesNeedDAPC;
+	u64 enginesNeedPortSecurity;
+};
+
+struct iwcCmdqPathResource_t {
+	long long shareMemoyPA; /* use long long for 64 bit compatible support */
+	u32 size;
+	bool useNormalIRQ; /* use normal IRQ in SWd */
+};
+
+struct iwcCmdqCancelTask_t {
+	/* [IN] */
+	s32 thread;
+	u32 waitCookie;
+
+	/* [OUT] */
+	bool throwAEE;
+	bool hasReset;
+	s32 irqStatus; /* global secure IRQ flag */
+	s32 irqFlag; /* thread IRQ flag */
+	u32 errInstr[2]; /* errInstr[0] = instB, errInstr[1] = instA */
+	u32 regValue;
+	u32 pc;
+};
+
+struct iwcCmdqMetaBuf {
+	u64 va;
+	u64 size;
+};
+
+/* extension flag for secure driver, must sync with def */
+enum sec_extension_iwc {
+	IWC_MDP_AAL = 0,
+	IWC_MDP_TDSHP,
+};
+
+struct readback_engine {
+	u32 engine;
+	u32 start;
+	u32 count;
+	u32 param;
+};
+
+struct iwcCmdqCommand_t {
+	/* basic execution data */
+	u32 thread;
+	u32 scenario;
+	u32 priority;
+	u32 commandSize;
+	u64 engineFlag;
+	u32 pVABase[CMDQ_IWC_MAX_CMD_LENGTH];
+
+	/* exec order data */
+	u32 waitCookie; /* [IN] index in thread's task list, it should be (nextCookie - 1) */
+	bool resetExecCnt;   /* [IN] reset HW thread */
+
+	/* client info */
+	s32 callerPid;
+	char callerName[CMDQ_IWC_CLIENT_NAME];
+
+	/* metadata */
+	struct iwcCmdqMetadata_t metadata;
+
+	/* client extension bits */
+	u64 extension;
+	u64 readback_pa;
+
+	/* debug */
+	u64 hNormalTask; /* handle to reference task in normal world */
+
+	/* SVP HDR */
+	u32 mdp_extension;
+	struct readback_engine readback_engs[CMDQ_MAX_READBACK_ENG];
+	u32 readback_cnt;
+};
+
+/*
+ * Linex kernel and mobicore has their own MMU tables,
+ * the latter's is used to map world shared memory and physical address
+ * so mobicore dose not understand linux virtual address mapping.
+ * If we want to transact a large buffer in TCI/DCI, there are 2 method (both need 1 copy):
+ * 1. use mc_map, to map normal world buffer to WSM, and pass secure_virt_addr in TCI/DCI buffer
+ *    note mc_map implies a memcopy to copy content from normal world to WSM.
+ * 2. declare a fixed length array in TCI/DCI struct, and its size must be < 1M.
+ */
+struct iwcCmdqMessage_t {
+	union {
+		u32 cmd;	/* [IN] command id */
+		s32 rsp;	/* [OUT] 0 for success, < 0 for error */
+	};
+
+	union {
+		struct iwcCmdqCommand_t command;
+		struct iwcCmdqCancelTask_t cancelTask;
+		struct iwcCmdqPathResource_t pathResource;
+	};
+
+	struct iwcCmdqDebugConfig_t debug;
+	struct iwcCmdqSecStatus_t secStatus;
+
+	u8 cmdq_id;
+};
+#endif /* __CMDQ_SEC_IWC_COMMON_H__ */
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..848fbcfdd59a
--- /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)
+#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,
+};
+
+/*
+ * 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;
+};
+
+s32 cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt);
+s32 cmdq_sec_pkt_set_data(struct cmdq_pkt *pkt, const u64 dapc_engine,
+			  const u64 port_sec_engine, const enum cmdq_sec_scenario scenario);
+s32 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..2596d2866837
--- /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 */
+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 */
+};
+
+void cmdq_sec_setup_tee_context(struct cmdq_sec_tee_context *tee);
+s32 cmdq_sec_init_context(struct cmdq_sec_tee_context *tee);
+s32 cmdq_sec_deinit_context(struct cmdq_sec_tee_context *tee);
+s32 cmdq_sec_allocate_wsm(struct cmdq_sec_tee_context *tee, void **wsm_buffer, u32 size);
+s32 cmdq_sec_free_wsm(struct cmdq_sec_tee_context *tee, void **wsm_buffer);
+s32 cmdq_sec_open_session(struct cmdq_sec_tee_context *tee, void *wsm_buffer);
+s32 cmdq_sec_close_session(struct cmdq_sec_tee_context *tee);
+s32 cmdq_sec_execute_session(struct cmdq_sec_tee_context *tee, u32 cmd, s32 timeout_ms);
+
+#endif	/* __MTK_CMDQ_SEC_TEE_H__ */
-- 
2.18.0

[PATCH 11/15] soc: mediatek: Add cmdq_insert_backup_cookie before EOC for secure pkt

[Jason-JH.Lin]

Add cmdq_insert_backup_cookie to append some commands before EOC:
1. Get GCE HW thread execute count from the GCE HW register.
2. Add 1 to the execute count and then store into a shared memory.
3. Set a software event siganl as secure irq to GCE HW.

Since the value of execute count + 1 is stored in a shared memory,
CMDQ driver in the normal world can use it to handle task done in irq
handler and CMDQ driver in the secure world will use it to schedule
the task slot for each secure thread.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index bbb127620bb3..7b5392878aba 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/mailbox_controller.h>
 #include <linux/of.h>
+#include <linux/mailbox/mtk-cmdq-sec-mailbox.h>
 #include <linux/soc/mediatek/mtk-cmdq.h>
 
 #define CMDQ_WRITE_ENABLE_MASK	BIT(0)
@@ -153,7 +154,9 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
 
 	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
 			 DMA_TO_DEVICE);
+
 	kfree(pkt->va_base);
+	kfree(pkt->sec_data);
 	kfree(pkt);
 }
 EXPORT_SYMBOL(cmdq_pkt_destroy);
@@ -458,6 +461,12 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 	struct cmdq_instruction inst = { {0} };
 	int err;
 
+	if (pkt->sec_data) {
+		err = cmdq_sec_insert_backup_cookie(pkt);
+		if (err < 0)
+			return err;
+	}
+
 	/* insert EOC and generate IRQ for each command iteration */
 	inst.op = CMDQ_CODE_EOC;
 	inst.value = CMDQ_EOC_IRQ_EN;
-- 
2.18.0

Re: [PATCH 11/15] soc: mediatek: Add cmdq_insert_backup_cookie before EOC for secure pkt

[CK Hu (胡俊光)]

Hi, Jason:

On Tue, 2023-09-19 at 03:22 +0800, Jason-JH.Lin wrote:
> Add cmdq_insert_backup_cookie to append some commands before EOC:
> 1. Get GCE HW thread execute count from the GCE HW register.
> 2. Add 1 to the execute count and then store into a shared memory.
> 3. Set a software event siganl as secure irq to GCE HW.
> 
> Since the value of execute count + 1 is stored in a shared memory,
> CMDQ driver in the normal world can use it to handle task done in irq
> handler and CMDQ driver in the secure world will use it to schedule
> the task slot for each secure thread.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index bbb127620bb3..7b5392878aba 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -8,6 +8,7 @@
>  #include <linux/module.h>
>  #include <linux/mailbox_controller.h>
>  #include <linux/of.h>
> +#include <linux/mailbox/mtk-cmdq-sec-mailbox.h>
>  #include <linux/soc/mediatek/mtk-cmdq.h>
>  
>  #define CMDQ_WRITE_ENABLE_MASK	BIT(0)
> @@ -153,7 +154,9 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
>  
>  	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt-
> >buf_size,
>  			 DMA_TO_DEVICE);
> +
>  	kfree(pkt->va_base);
> +	kfree(pkt->sec_data);
>  	kfree(pkt);
>  }
>  EXPORT_SYMBOL(cmdq_pkt_destroy);
> @@ -458,6 +461,12 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>  	struct cmdq_instruction inst = { {0} };
>  	int err;
>  
> +	if (pkt->sec_data) {
> +		err = cmdq_sec_insert_backup_cookie(pkt);
> +		if (err < 0)
> +			return err;
> +	}

Client driver could directly call cmdq_sec_insert_backup_cookie()
before call cmdq_pkt_finalize(). I would like helper provide simple API
and client driver would integrate simple API to what they want.

Regards,
CK

> +
>  	/* insert EOC and generate IRQ for each command iteration */
>  	inst.op = CMDQ_CODE_EOC;
>  	inst.value = CMDQ_EOC_IRQ_EN;

Re: [PATCH 11/15] soc: mediatek: Add cmdq_insert_backup_cookie before EOC for secure pkt

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reivews.

On Tue, 2023-09-19 at 03:04 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Tue, 2023-09-19 at 03:22 +0800, Jason-JH.Lin wrote:
> > Add cmdq_insert_backup_cookie to append some commands before EOC:
> > 1. Get GCE HW thread execute count from the GCE HW register.
> > 2. Add 1 to the execute count and then store into a shared memory.
> > 3. Set a software event siganl as secure irq to GCE HW.
> > 
> > Since the value of execute count + 1 is stored in a shared memory,
> > CMDQ driver in the normal world can use it to handle task done in
> > irq
> > handler and CMDQ driver in the secure world will use it to schedule
> > the task slot for each secure thread.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index bbb127620bb3..7b5392878aba 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/module.h>
> >  #include <linux/mailbox_controller.h>
> >  #include <linux/of.h>
> > +#include <linux/mailbox/mtk-cmdq-sec-mailbox.h>
> >  #include <linux/soc/mediatek/mtk-cmdq.h>
> >  
> >  #define CMDQ_WRITE_ENABLE_MASK	BIT(0)
> > @@ -153,7 +154,9 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
> >  
> >  	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt-
> > > buf_size,
> > 
> >  			 DMA_TO_DEVICE);
> > +
> >  	kfree(pkt->va_base);
> > +	kfree(pkt->sec_data);
> >  	kfree(pkt);
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_destroy);
> > @@ -458,6 +461,12 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >  	struct cmdq_instruction inst = { {0} };
> >  	int err;
> >  
> > +	if (pkt->sec_data) {
> > +		err = cmdq_sec_insert_backup_cookie(pkt);
> > +		if (err < 0)
> > +			return err;
> > +	}
> 
> Client driver could directly call cmdq_sec_insert_backup_cookie()
> before call cmdq_pkt_finalize(). I would like helper provide simple
> API
> and client driver would integrate simple API to what they want.
> 
OK, I'll move this to the client driver.
Thanks.

Regards,
Jason-JH.Lin

> Regards,
> CK
> 
> > +
> >  	/* insert EOC and generate IRQ for each command iteration */
> >  	inst.op = CMDQ_CODE_EOC;
> >  	inst.value = CMDQ_EOC_IRQ_EN;

[PATCH 12/15] mailbox: mediatek: Add CMDQ driver support for mt8188

[Jason-JH.Lin]

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 cd4a8f0ef6ad..3940b9f8e774 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -746,12 +746,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},
 	{.compatible = "mediatek,mt8195-gce", .data = (void *)&gce_plat_v6},
 	{}
 };
-- 
2.18.0

[PATCH 13/15] mailbox: mediatek: Add mt8188 support for CMDQ secure driver

[Jason-JH.Lin]

Add mt8188 support for CMDQ secure driver.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/mailbox/mtk-cmdq-mailbox.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index 3940b9f8e774..4e047dc916b9 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -750,6 +750,7 @@ static const struct gce_plat gce_plat_v8 = {
 	.thread_nr = 32,
 	.shift = 3,
 	.control_by_sw = true,
+	.has_sec = true,
 	.gce_num = 2
 };
 
-- 
2.18.0

Re: [PATCH 13/15] mailbox: mediatek: Add mt8188 support for CMDQ secure driver

[Krzysztof Kozlowski]

On 18/09/2023 21:22, Jason-JH.Lin wrote:
> Add mt8188 support for CMDQ secure driver.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 3940b9f8e774..4e047dc916b9 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -750,6 +750,7 @@ static const struct gce_plat gce_plat_v8 = {
>  	.thread_nr = 32,
>  	.shift = 3,
>  	.control_by_sw = true,
> +	.has_sec = true,

No, you just added it patch ago. Do not add broken code and fix it. Are
there some KPIs in Mediatek to have patch count?

Best regards,
Krzysztof

Re: [PATCH 13/15] mailbox: mediatek: Add mt8188 support for CMDQ secure driver

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

On Sat, 2023-09-23 at 20:09 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 18/09/2023 21:22, Jason-JH.Lin wrote:
> > Add mt8188 support for CMDQ secure driver.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 3940b9f8e774..4e047dc916b9 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -750,6 +750,7 @@ static const struct gce_plat gce_plat_v8 = {
> >  .thread_nr = 32,
> >  .shift = 3,
> >  .control_by_sw = true,
> > +.has_sec = true,
> 
> No, you just added it patch ago. Do not add broken code and fix it.
> Are
> there some KPIs in Mediatek to have patch count?
> 

This patch is different from [PATCH 14/15] at the gce_plat:
[PATCH 13/15] is adding the flag to gce_plat_v8 for mediatek,mt8188-gce
[PATCH 14/15] is adding the flag to gce_plat_v6 for mediatek,mt8195-gce

I'm sorry about that gce_plat are too similar to cause the confusion.

I'vd built the whole series before sending it, so I think it won't
break the code and I think there are no KPIs on the patch count.

Should I merge [PATCH 13/15] and [PATCH 14/15] in to [PATCH 9/15] to
show how it works?

Regards,
Jason-JH.Lin

> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 13/15] mailbox: mediatek: Add mt8188 support for CMDQ secure driver

[Krzysztof Kozlowski]

On 25/09/2023 08:01, Jason-JH Lin (林睿祥) wrote:
> Hi Krzysztof,
> 
> On Sat, 2023-09-23 at 20:09 +0200, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 18/09/2023 21:22, Jason-JH.Lin wrote:
>>> Add mt8188 support for CMDQ secure driver.
>>>
>>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>>> ---
>>>  drivers/mailbox/mtk-cmdq-mailbox.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
>> b/drivers/mailbox/mtk-cmdq-mailbox.c
>>> index 3940b9f8e774..4e047dc916b9 100644
>>> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
>>> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
>>> @@ -750,6 +750,7 @@ static const struct gce_plat gce_plat_v8 = {
>>>  .thread_nr = 32,
>>>  .shift = 3,
>>>  .control_by_sw = true,
>>> +.has_sec = true,
>>
>> No, you just added it patch ago. Do not add broken code and fix it.
>> Are
>> there some KPIs in Mediatek to have patch count?
>>
> 
> This patch is different from [PATCH 14/15] at the gce_plat:
> [PATCH 13/15] is adding the flag to gce_plat_v8 for mediatek,mt8188-gce
> [PATCH 14/15] is adding the flag to gce_plat_v6 for mediatek,mt8195-gce
???

I talked about patch 12! Why do you add incomplete code?


Best regards,
Krzysztof

Re: [PATCH 13/15] mailbox: mediatek: Add mt8188 support for CMDQ secure driver

[Jason-JH Lin (林睿祥)]

On Mon, 2023-09-25 at 08:45 +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/09/2023 08:01, Jason-JH Lin (林睿祥) wrote:
> > Hi Krzysztof,
> > 
> > On Sat, 2023-09-23 at 20:09 +0200, Krzysztof Kozlowski wrote:
> >>   
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >>  On 18/09/2023 21:22, Jason-JH.Lin wrote:
> >>> Add mt8188 support for CMDQ secure driver.
> >>>
> >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> >>> ---
> >>>  drivers/mailbox/mtk-cmdq-mailbox.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> >> b/drivers/mailbox/mtk-cmdq-mailbox.c
> >>> index 3940b9f8e774..4e047dc916b9 100644
> >>> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> >>> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> >>> @@ -750,6 +750,7 @@ static const struct gce_plat gce_plat_v8 = {
> >>>  .thread_nr = 32,
> >>>  .shift = 3,
> >>>  .control_by_sw = true,
> >>> +.has_sec = true,
> >>
> >> No, you just added it patch ago. Do not add broken code and fix
> it.
> >> Are
> >> there some KPIs in Mediatek to have patch count?
> >>
> > 
> > This patch is different from [PATCH 14/15] at the gce_plat:
> > [PATCH 13/15] is adding the flag to gce_plat_v8 for
> mediatek,mt8188-gce
> > [PATCH 14/15] is adding the flag to gce_plat_v6 for
> mediatek,mt8195-gce
> ???
> 
> I talked about patch 12! Why do you add incomplete code?

Do you mean I should merge the patch 12 and 13?

I think separating the new supported mt8188 and the new supported
secure feature in different patch can make the CMDQ user know how to
make a CMDQ mailbox driver support secure feature.

Since mt8188 is a new supported IC for mailbox, so merging patch 12 and
13 into the same patch is OK.
I'll merge them at the next version. Thanks.

Regards,
Jason-JH.Lin
> 
> 
> Best regards,
> Krzysztof
> 
> 

[PATCH 14/15] mailbox: mediatek: Add mt8195 support for CMDQ secure driver

[Jason-JH.Lin]

Add mt8195 support for CMDQ secure driver.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/mailbox/mtk-cmdq-mailbox.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index 4e047dc916b9..d27d033c587d 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -735,6 +735,7 @@ static const struct gce_plat gce_plat_v6 = {
 	.thread_nr = 24,
 	.shift = 3,
 	.control_by_sw = true,
+	.has_sec = true,
 	.gce_num = 2
 };
 
-- 
2.18.0

Re: [PATCH 14/15] mailbox: mediatek: Add mt8195 support for CMDQ secure driver

[Krzysztof Kozlowski]

On 18/09/2023 21:22, Jason-JH.Lin wrote:
> Add mt8195 support for CMDQ secure driver.

How is it anyhow related to your patch content?

> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 4e047dc916b9..d27d033c587d 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -735,6 +735,7 @@ static const struct gce_plat gce_plat_v6 = {
>  	.thread_nr = 24,
>  	.shift = 3,
>  	.control_by_sw = true,
> +	.has_sec = true,

Really, how?

Best regards,
Krzysztof

Re: [PATCH 14/15] mailbox: mediatek: Add mt8195 support for CMDQ secure driver

[Jason-JH Lin (林睿祥)]

Hi Krzysztof,

Thanks for the reviews.

On Sat, 2023-09-23 at 20: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 18/09/2023 21:22, Jason-JH.Lin wrote:
> > Add mt8195 support for CMDQ secure driver.
> 
> How is it anyhow related to your patch content?

This path is dependent on cmdq_probe() in [PATCH 9/15].

> 
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 4e047dc916b9..d27d033c587d 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -735,6 +735,7 @@ static const struct gce_plat gce_plat_v6 = {
> >  .thread_nr = 24,
> >  .shift = 3,
> >  .control_by_sw = true,
> > +.has_sec = true,
> 
> Really, how?

Within this flag, cmdq_probe() will call
platform_device_register_data() to setup a CMDQ secure driver.

Regards,
Jason-JH.Lin

> 
> Best regards,
> Krzysztof
> 
> 

[PATCH 15/15] arm64: dts: mediatek: mt8195: Add CMDQ secure driver support for gce0

[Jason-JH.Lin]

Add CMDQ secure driver support for GCE0.

CMDQ secure driver will requset a GCE HW thread in GCE0 core to support
sending a CMDQ packet through secure mailbox.
Then ask GCE HW thread to excute commands in the secure world.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8195.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
index 4dbbf8fdab75..201eb3bc0273 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
@@ -910,6 +910,8 @@
 			interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH 0>;
 			#mbox-cells = <2>;
 			clocks = <&infracfg_ao CLK_INFRA_AO_GCE>;
+			mboxes = <&gce0 15 CMDQ_THR_PRIO_1>;
+			mediatek,gce-events = <CMDQ_SYNC_TOKEN_SECURE_THR_EOF>;
 		};
 
 		gce1: mailbox@10330000 {
-- 
2.18.0

Re: [PATCH 00/15] Add CMDQ secure driver for SVP

[CK Hu (胡俊光)]

Hi, Jason:

On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote:
> For the Secure Video Path (SVP) feature, inculding the memory stored
> secure video content, the registers of display HW pipeline and the
> HW configure operations are required to execute in the secure world.
> 
> So using a CMDQ secure driver to make all display HW registers
> configuration secure DRAM access permision settings execute by GCE
> secure thread in the secure world.
> 
> We are landing this feature on mt8188 and mt8195 currently.

I'm doubt that GCE could be secure enough. Hacker would try any way to
hack TEE. If the interface is simple, it's easy to check in the
interface entry. But GCE command has enormous combination, how to check
it's not hacked? One example is that hacker could use cmdq_pkt_read_s()
and cmdq_pkt_write_s() to do memory copy and send this packet to secure
GCE. Could this memory copy touch secure memory? Or don't worry about 
this, once hacker find a way to break this, just find a way to fix it?

> 
> Jason-JH.Lin (15):
>   dt-bindings: mailbox: Add property for CMDQ secure driver
>   dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event
> id
>   soc: mailbox: Add SPR definition for GCE
>   soc: mailbox: Add cmdq_pkt_logic_command to support math operation
>   mailbox: mediatek: Add cmdq_pkt_write_s_reg_value to CMDQ driver
>   mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread
>   mailbox: mediatek: Add loop pkt flag and irq handling for loop
> command
>   soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver
>   mailbox: mediatek: Add secure CMDQ driver support for CMDQ driver
>   mailbox: mediatek: Add CMDQ secure mailbox driver
>   soc: mediatek: Add cmdq_insert_backup_cookie before EOC for secure
> pkt
>   mailbox: mediatek: Add CMDQ driver support for mt8188
>   mailbox: mediatek: Add mt8188 support for CMDQ secure driver
>   mailbox: mediatek: Add mt8195 support for CMDQ secure driver
>   arm64: dts: mediatek: mt8195: Add CMDQ secure driver support for
> gce0
> 
>  .../mailbox/mediatek,gce-mailbox.yaml         |   30 +-
>  arch/arm64/boot/dts/mediatek/mt8195.dtsi      |    2 +
>  drivers/mailbox/Makefile                      |    2 +-
>  drivers/mailbox/mtk-cmdq-mailbox.c            |   67 +-
>  drivers/mailbox/mtk-cmdq-sec-mailbox.c        | 1103
> +++++++++++++++++
>  drivers/mailbox/mtk-cmdq-sec-tee.c            |  202 +++
>  drivers/soc/mediatek/mtk-cmdq-helper.c        |   81 ++
>  include/dt-bindings/gce/mt8195-gce.h          |    6 +
>  include/linux/mailbox/mtk-cmdq-mailbox.h      |   15 +
>  .../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 +
>  include/linux/soc/mediatek/mtk-cmdq.h         |   65 +
>  13 files changed, 1971 insertions(+), 9 deletions(-)
>  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
> 

Re: [PATCH 00/15] Add CMDQ secure driver for SVP

[Jason-JH Lin (林睿祥)]

Hi CK,

On Wed, 2023-09-20 at 03:08 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote:
> > For the Secure Video Path (SVP) feature, inculding the memory
> > stored
> > secure video content, the registers of display HW pipeline and the
> > HW configure operations are required to execute in the secure
> > world.
> > 
> > So using a CMDQ secure driver to make all display HW registers
> > configuration secure DRAM access permision settings execute by GCE
> > secure thread in the secure world.
> > 
> > We are landing this feature on mt8188 and mt8195 currently.
> 
> I'm doubt that GCE could be secure enough. Hacker would try any way
> to
> hack TEE. If the interface is simple, it's easy to check in the
> interface entry. But GCE command has enormous combination, how to
> check
> it's not hacked? One example is that hacker could use
> cmdq_pkt_read_s()
> and cmdq_pkt_write_s() to do memory copy and send this packet to
> secure
> GCE. Could this memory copy touch secure memory? Or don't worry
> about 
> this, once hacker find a way to break this, just find a way to fix
> it?

We have put a lot protection mechanism in the secure world to avoid the
secure video content reveal from our SoC.

The hackers may try to add some commands in to cmdq secure pkt, but
we'll check all the commands in secure world for every secure pkt and
find out the invalid one with whitelist registers checking, DAPC HW
protection for write instruction to DRAM with configuring WDMA, larb
secure protection for read secure DRAM from normal world, checking
read_s instructions for memory copy to an invalid DRAM addr,etc.

Maybe hackers would find the way to break the video playback, but they
can not find the way to steal the secure video content by adding a
commands into secure cmdq pkt.

Regards,
Jason-JH.Lin
> 
> > 
> > Jason-JH.Lin (15):
> >   dt-bindings: mailbox: Add property for CMDQ secure driver
> >   dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF
> > event
> > id
> >   soc: mailbox: Add SPR definition for GCE
> >   soc: mailbox: Add cmdq_pkt_logic_command to support math
> > operation
> >   mailbox: mediatek: Add cmdq_pkt_write_s_reg_value to CMDQ driver
> >   mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread
> >   mailbox: mediatek: Add loop pkt flag and irq handling for loop
> > command
> >   soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver
> >   mailbox: mediatek: Add secure CMDQ driver support for CMDQ driver
> >   mailbox: mediatek: Add CMDQ secure mailbox driver
> >   soc: mediatek: Add cmdq_insert_backup_cookie before EOC for
> > secure
> > pkt
> >   mailbox: mediatek: Add CMDQ driver support for mt8188
> >   mailbox: mediatek: Add mt8188 support for CMDQ secure driver
> >   mailbox: mediatek: Add mt8195 support for CMDQ secure driver
> >   arm64: dts: mediatek: mt8195: Add CMDQ secure driver support for
> > gce0
> > 
> >  .../mailbox/mediatek,gce-mailbox.yaml         |   30 +-
> >  arch/arm64/boot/dts/mediatek/mt8195.dtsi      |    2 +
> >  drivers/mailbox/Makefile                      |    2 +-
> >  drivers/mailbox/mtk-cmdq-mailbox.c            |   67 +-
> >  drivers/mailbox/mtk-cmdq-sec-mailbox.c        | 1103
> > +++++++++++++++++
> >  drivers/mailbox/mtk-cmdq-sec-tee.c            |  202 +++
> >  drivers/soc/mediatek/mtk-cmdq-helper.c        |   81 ++
> >  include/dt-bindings/gce/mt8195-gce.h          |    6 +
> >  include/linux/mailbox/mtk-cmdq-mailbox.h      |   15 +
> >  .../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 +
> >  include/linux/soc/mediatek/mtk-cmdq.h         |   65 +
> >  13 files changed, 1971 insertions(+), 9 deletions(-)
> >  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
> >