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

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 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 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 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 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 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 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 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;

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
> > 

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
> > 
> 

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 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 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 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 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 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 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 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 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 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 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 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 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 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
> 
> 

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
> 
> 

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 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

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 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 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 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
> 
> 

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 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
> 
> 

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
> 

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