[v5,00/13] support gce on mt6779 platform

This patch support gce on mt6779 platform.

Change since v4:
- do not clear disp event again in drm driver
- symbolize value 1 to jump relative

Change since v3:
- refine code for local variable usage
- use cmdq error code to consistent with current design
- return error directly after send if error code return
- also modify drm driver which uses cmdq_pkt_wfe api
- add finalize in drm driver

[... snip ...]



Dennis YC Hsieh (13):
  dt-binding: gce: add gce header file for mt6779
  mailbox: cmdq: variablize address shift in platform
  mailbox: cmdq: support mt6779 gce platform definition
  mailbox: mediatek: cmdq: clear task in channel before shutdown
  soc: mediatek: cmdq: return send msg error code
  soc: mediatek: cmdq: add assign function
  soc: mediatek: cmdq: add write_s function
  soc: mediatek: cmdq: add read_s function
  soc: mediatek: cmdq: add write_s value function
  soc: mediatek: cmdq: export finalize function
  soc: mediatek: cmdq: add jump function
  soc: mediatek: cmdq: add clear option in cmdq_pkt_wfe api
  soc: mediatek: cmdq: add set event function

 .../devicetree/bindings/mailbox/mtk-gce.txt   |   8 +-
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c       |   3 +-
 drivers/mailbox/mtk-cmdq-mailbox.c            | 101 ++++++--
 drivers/soc/mediatek/mtk-cmdq-helper.c        | 144 +++++++++++-
 include/dt-bindings/gce/mt6779-gce.h          | 222 ++++++++++++++++++
 include/linux/mailbox/mtk-cmdq-mailbox.h      |  10 +-
 include/linux/soc/mediatek/mtk-cmdq.h         |  94 +++++++-
 7 files changed, 549 insertions(+), 33 deletions(-)
 create mode 100644 include/dt-bindings/gce/mt6779-gce.h

Hi, Dennis:

On Sun, 2020-03-08 at 18:52 +0800, Dennis YC Hsieh wrote:
> Add jump function so that client can jump to any address which
> contains instruction.
> 

Reviewed-by: CK Hu <ck.hu@mediatek.com>

> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@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 59bc1164b411..bb5be20fc70a 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -13,6 +13,7 @@
>  #define CMDQ_POLL_ENABLE_MASK	BIT(0)
>  #define CMDQ_EOC_IRQ_EN		BIT(0)
>  #define CMDQ_REG_TYPE		1
> +#define CMDQ_JUMP_RELATIVE	1
>  
>  struct cmdq_instruction {
>  	union {
> @@ -372,6 +373,18 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
>  }
>  EXPORT_SYMBOL(cmdq_pkt_assign);
>  
> +int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +
> +	inst.op = CMDQ_CODE_JUMP;
> +	inst.offset = CMDQ_JUMP_RELATIVE;
> +	inst.value = addr >>
> +		cmdq_mbox_shift(((struct cmdq_client *)pkt->cl)->chan);
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_jump);
> +
>  int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>  {
>  	struct cmdq_instruction inst = { {0} };
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index 99e77155f967..1a6c56f3bec1 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -213,6 +213,17 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>   */
>  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
>  
> +/**
> + * cmdq_pkt_jump() - Append jump command to the CMDQ packet, ask GCE
> + *		     to execute an instruction that change current thread PC to
> + *		     a physical address which should contains more instruction.
> + * @pkt:        the CMDQ packet
> + * @addr:       physical address of target instruction buffer
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr);
> +
>  /**
>   * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
>   * @pkt:	the CMDQ packet

Hi, Dennis:

On Sun, 2020-03-08 at 18:52 +0800, Dennis YC Hsieh wrote:
> Add clear parameter to let client decide if
> event should be clear to 0 after GCE receive it.
> 

Reviewed-by: CK Hu <ck.hu@mediatek.com>

> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 2 +-
>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 5 +++--
>  include/linux/mailbox/mtk-cmdq-mailbox.h | 3 +--
>  include/linux/soc/mediatek/mtk-cmdq.h    | 5 +++--
>  4 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 7daaabc26eb1..a065b3a412cf 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -488,7 +488,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
>  	if (mtk_crtc->cmdq_client) {
>  		cmdq_handle = cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
>  		cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
> -		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
> +		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event, false);
>  		mtk_crtc_ddp_config(crtc, cmdq_handle);
>  		cmdq_pkt_finalize(cmdq_handle);
>  		cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index bb5be20fc70a..ec5637d43254 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -296,15 +296,16 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>  }
>  EXPORT_SYMBOL(cmdq_pkt_write_s_value);
>  
> -int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
>  {
>  	struct cmdq_instruction inst = { {0} };
> +	u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>  
>  	if (event >= CMDQ_MAX_EVENT)
>  		return -EINVAL;
>  
>  	inst.op = CMDQ_CODE_WFE;
> -	inst.value = CMDQ_WFE_OPTION;
> +	inst.value = CMDQ_WFE_OPTION | clear_option;
>  	inst.event = event;
>  
>  	return cmdq_pkt_append_command(pkt, inst);
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index 3f6bc0dfd5da..42d2a30e6a70 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -27,8 +27,7 @@
>   * bit 16-27: update value
>   * bit 31: 1 - update, 0 - no update
>   */
> -#define CMDQ_WFE_OPTION			(CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | \
> -					CMDQ_WFE_WAIT_VALUE)
> +#define CMDQ_WFE_OPTION			(CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE)
>  
>  /** cmdq event maximum */
>  #define CMDQ_MAX_EVENT			0x3ff
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index 1a6c56f3bec1..d63749440697 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -152,11 +152,12 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>  /**
>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>   * @pkt:	the CMDQ packet
> - * @event:	the desired event type to "wait and CLEAR"
> + * @event:	the desired event type to wait
> + * @clear:	clear event or not after event arrive
>   *
>   * Return: 0 for success; else the error code is returned
>   */
> -int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event);
> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
>  
>  /**
>   * cmdq_pkt_clear_event() - append clear event command to the CMDQ packet

On Sun, Mar 8, 2020 at 5:53 AM Dennis YC Hsieh
<dennis-yc.hsieh@mediatek.com> wrote:
>
> Some gce hardware shift pc and end address in register to support
> large dram addressing.
> Implement gce address shift when write or read pc and end register.
> And add shift bit in platform definition.
>
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c       | 61 ++++++++++++++++++------
>  drivers/soc/mediatek/mtk-cmdq-helper.c   |  3 +-
>  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 +
>
Please segregate this patch, and any other if, into mailbox and
platform specific patchsets. Ideally soc/client specific changes later
on top of mailbox/provider changes.

Thanks

Hi Jassi,


On Thu, 2020-03-19 at 20:05 -0500, Jassi Brar wrote:
> On Sun, Mar 8, 2020 at 5:53 AM Dennis YC Hsieh
> <dennis-yc.hsieh@mediatek.com> wrote:
> >
> > Some gce hardware shift pc and end address in register to support
> > large dram addressing.
> > Implement gce address shift when write or read pc and end register.
> > And add shift bit in platform definition.
> >
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c       | 61 ++++++++++++++++++------
> >  drivers/soc/mediatek/mtk-cmdq-helper.c   |  3 +-
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 +
> >
> Please segregate this patch, and any other if, into mailbox and
> platform specific patchsets. Ideally soc/client specific changes later
> on top of mailbox/provider changes.
> 
> Thanks

Thanks for your comment. I'll separate mailbox and soc code.


Regards,
Dennis

On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> Add gce v4 hardware support with different thread number and shift.
> 
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 4dbee9258127..9994ac9426d6 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -572,10 +572,12 @@ static const struct dev_pm_ops cmdq_pm_ops = {
>  
>  static const struct gce_plat gce_plat_v2 = {.thread_nr = 16};
>  static const struct gce_plat gce_plat_v3 = {.thread_nr = 24};
> +static const struct gce_plat gce_plat_v4 = {.thread_nr = 24, .shift = 3};
>  
>  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,mt6779-gce", .data = (void *)&gce_plat_v4},
>  	{}
>  };
>  
>

On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> Return error code to client if send message fail,
> so that client has chance to error handling.
> 
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> Fixes: 576f1b4bc802 ("soc: mediatek: Add Mediatek CMDQ helper")
> Reviewed-by: CK Hu <ck.hu@mediatek.com>

Queued for v5.7-fixes

> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 2e1bc513569b..98f23ba3ba47 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -351,7 +351,9 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
>  		spin_unlock_irqrestore(&client->lock, flags);
>  	}
>  
> -	mbox_send_message(client->chan, pkt);
> +	err = mbox_send_message(client->chan, pkt);
> +	if (err < 0)
> +		return err;
>  	/* We can send next packet immediately, so just call txdone. */
>  	mbox_client_txdone(client->chan, 0);
>  
>

On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> Add assign function in cmdq helper which assign constant value into
> internal register by index.
> 
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 24 +++++++++++++++++++++++-
>  include/linux/mailbox/mtk-cmdq-mailbox.h |  1 +
>  include/linux/soc/mediatek/mtk-cmdq.h    | 14 ++++++++++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 98f23ba3ba47..33153d17c9d9 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -12,6 +12,7 @@
>  #define CMDQ_WRITE_ENABLE_MASK	BIT(0)
>  #define CMDQ_POLL_ENABLE_MASK	BIT(0)
>  #define CMDQ_EOC_IRQ_EN		BIT(0)
> +#define CMDQ_REG_TYPE		1
>  
>  struct cmdq_instruction {
>  	union {
> @@ -21,8 +22,17 @@ struct cmdq_instruction {
>  	union {
>  		u16 offset;
>  		u16 event;
> +		u16 reg_dst;
> +	};
> +	union {
> +		u8 subsys;
> +		struct {
> +			u8 sop:5;
> +			u8 arg_c_t:1;
> +			u8 arg_b_t:1;
> +			u8 dst_t:1;
> +		};

This union seems without context in this patch. Please drop.

Regards,
Matthias

>  	};
> -	u8 subsys;
>  	u8 op;
>  };
>  
> @@ -277,6 +287,18 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>  }
>  EXPORT_SYMBOL(cmdq_pkt_poll_mask);
>  
> +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +
> +	inst.op = CMDQ_CODE_LOGIC;
> +	inst.dst_t = CMDQ_REG_TYPE;
> +	inst.reg_dst = reg_idx;
> +	inst.value = value;
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_assign);
> +
>  static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>  {
>  	struct cmdq_instruction inst = { {0} };
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index dfe5b2eb85cc..121c3bb6d3de 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -59,6 +59,7 @@ enum cmdq_code {
>  	CMDQ_CODE_JUMP = 0x10,
>  	CMDQ_CODE_WFE = 0x20,
>  	CMDQ_CODE_EOC = 0x40,
> +	CMDQ_CODE_LOGIC = 0xa0,
>  };
>  
>  enum cmdq_cb_status {
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index a74c1d5acdf3..83340211e1d3 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -152,6 +152,20 @@ 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_assign() - Append logic assign command to the CMDQ packet, ask GCE
> + *		       to execute an instruction that set a constant value into
> + *		       internal register and use as value, mask or address in
> + *		       read/write instruction.
> + * @pkt:	the CMDQ packet
> + * @reg_idx:	the CMDQ internal register ID
> + * @value:	the specified value
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
> +
>  /**
>   * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
>   *                          packet and call back at the end of done packet
>

On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> add write_s function in cmdq helper functions which
> writes value contains in internal register to address
> with large dma access support.
> 
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 34 +++++++++++++++++++++++-
>  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 ++
>  include/linux/soc/mediatek/mtk-cmdq.h    | 20 ++++++++++++++
>  3 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 33153d17c9d9..90f1ff2b4b00 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -18,6 +18,10 @@ struct cmdq_instruction {
>  	union {
>  		u32 value;
>  		u32 mask;
> +		struct {
> +			u16 arg_c;
> +			u16 src_reg;
> +		};
>  	};
>  	union {
>  		u16 offset;
> @@ -29,7 +33,7 @@ struct cmdq_instruction {
>  		struct {
>  			u8 sop:5;
>  			u8 arg_c_t:1;
> -			u8 arg_b_t:1;
> +			u8 src_t:1;

fixing patch 6/13 please. seems the struct should be added in this patch.

>  			u8 dst_t:1;
>  		};
>  	};
> @@ -222,6 +226,34 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
>  }
>  EXPORT_SYMBOL(cmdq_pkt_write_mask);
>  
> +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> +		     u16 addr_low, u16 src_reg_idx, u32 mask)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +	int err;
> +
> +	if (mask != U32_MAX) {
> +		inst.op = CMDQ_CODE_MASK;
> +		inst.mask = ~mask;
> +		err = cmdq_pkt_append_command(pkt, inst);
> +		if (err < 0)
> +			return err;
> +
> +		inst.mask = 0;
> +		inst.op = CMDQ_CODE_WRITE_S_MASK;
> +	} else {
> +		inst.op = CMDQ_CODE_WRITE_S;
> +	}
> +
> +	inst.src_t = CMDQ_REG_TYPE;

Not defined.
Please make sure that every patch compiles on it's own and does not add a
regression. This is very helpful if we have to bisect the kernel in the future.

> +	inst.sop = high_addr_reg_idx;
> +	inst.offset = addr_low;
> +	inst.src_reg = src_reg_idx;
> +
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_write_s);
> +
>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
>  {
>  	struct cmdq_instruction inst = { {0} };
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index 121c3bb6d3de..8ef87e1bd03b 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -59,6 +59,8 @@ enum cmdq_code {
>  	CMDQ_CODE_JUMP = 0x10,
>  	CMDQ_CODE_WFE = 0x20,
>  	CMDQ_CODE_EOC = 0x40,
> +	CMDQ_CODE_WRITE_S = 0x90,
> +	CMDQ_CODE_WRITE_S_MASK = 0x91,
>  	CMDQ_CODE_LOGIC = 0xa0,
>  };
>  
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index 83340211e1d3..c72d826d8934 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -12,6 +12,8 @@
>  #include <linux/timer.h>
>  
>  #define CMDQ_NO_TIMEOUT		0xffffffffu
> +#define CMDQ_ADDR_HIGH(addr)	((u32)(((addr) >> 16) & GENMASK(31, 0)))
> +#define CMDQ_ADDR_LOW(addr)	((u16)(addr) | BIT(1))
>  
>  struct cmdq_pkt;
>  
> @@ -102,6 +104,24 @@ int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value);
>  int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
>  			u16 offset, u32 value, u32 mask);
>  
> +/**
> + * cmdq_pkt_write_s() - append write_s command to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa

s/regisger/register

> + * @addr_low:	low address of pa
> + * @src_reg_idx:	the CMDQ internal register ID which cache source value
> + * @mask:	the specified target address mask, use U32_MAX if no need
> + *
> + * Return: 0 for success; else the error code is returned
> + *
> + * Support write value to physical address without subsys. Use CMDQ_ADDR_HIGH()
> + * to get high addrees and call cmdq_pkt_assign() to assign value into internal

s/addrees/address

> + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameterwhen

s/parameterwhen/parameter when

> + * call to this function.
> + */
> +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> +		     u16 addr_low, u16 src_reg_idx, u32 mask);
> +

In general I wonder if we shouldn't provide two functions, one that writes a
mask and on for the else case.

Regards,
Matthias

>  /**
>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>   * @pkt:	the CMDQ packet
>

On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> add write_s function in cmdq helper functions which
> writes a constant value to address with large dma
> access support.
> 
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  | 14 ++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 03c129230cd7..a9ebbabb7439 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>  }
>  EXPORT_SYMBOL(cmdq_pkt_write_s);
>  
> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> +			   u16 addr_low, u32 value, u32 mask)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +	int err;
> +
> +	if (mask != U32_MAX) {
> +		inst.op = CMDQ_CODE_MASK;
> +		inst.mask = ~mask;
> +		err = cmdq_pkt_append_command(pkt, inst);
> +		if (err < 0)
> +			return err;
> +
> +		inst.op = CMDQ_CODE_WRITE_S_MASK;
> +	} else {
> +		inst.op = CMDQ_CODE_WRITE_S;
> +	}
> +
> +	inst.sop = high_addr_reg_idx;

Writing u16 value in a 5 bit wide variable?

> +	inst.offset = addr_low;
> +	inst.value = value;
> +
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_write_s_value);
> +
>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
>  {
>  	struct cmdq_instruction inst = { {0} };
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index 01b4184af310..fec292aac83c 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
>  int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>  		     u16 addr_low, u16 src_reg_idx, u32 mask);
>  
> +/**
> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ
> + *			      packet which write value to a physical address
> + * @pkt:	the CMDQ packet
> + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa

register

> + * @addr_low:	low address of pa
> + * @value:	the specified target value
> + * @mask:	the specified target mask
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> +			   u16 addr_low, u32 value, u32 mask);
> +
>  /**
>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>   * @pkt:	the CMDQ packet
>

On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> Export finalize function to client which helps append eoc and jump
> command to pkt. Let client decide call finalize or not.
> 
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 +
>  drivers/soc/mediatek/mtk-cmdq-helper.c  | 7 ++-----
>  include/linux/soc/mediatek/mtk-cmdq.h   | 8 ++++++++
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 0dfcd1787e65..7daaabc26eb1 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -490,6 +490,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
>  		cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
>  		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
>  		mtk_crtc_ddp_config(crtc, cmdq_handle);
> +		cmdq_pkt_finalize(cmdq_handle);
>  		cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
>  	}
>  #endif

This should be a independent patch.
Other then that patch looks good.

> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index a9ebbabb7439..59bc1164b411 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -372,7 +372,7 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
>  }
>  EXPORT_SYMBOL(cmdq_pkt_assign);
>  
> -static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> +int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>  {
>  	struct cmdq_instruction inst = { {0} };
>  	int err;
> @@ -392,6 +392,7 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>  
>  	return err;
>  }
> +EXPORT_SYMBOL(cmdq_pkt_finalize);
>  
>  static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
>  {
> @@ -426,10 +427,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
>  	unsigned long flags = 0;
>  	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
>  
> -	err = cmdq_pkt_finalize(pkt);
> -	if (err < 0)
> -		return err;
> -
>  	pkt->cb.cb = cb;
>  	pkt->cb.data = data;
>  	pkt->async_cb.cb = cmdq_pkt_flush_async_cb;
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index fec292aac83c..99e77155f967 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -213,6 +213,14 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>   */
>  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
>  
> +/**
> + * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
> + * @pkt:	the CMDQ packet
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_finalize(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
>

On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> Add clear parameter to let client decide if
> event should be clear to 0 after GCE receive it.
> 
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 2 +-
>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 5 +++--
>  include/linux/mailbox/mtk-cmdq-mailbox.h | 3 +--
>  include/linux/soc/mediatek/mtk-cmdq.h    | 5 +++--
>  4 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 7daaabc26eb1..a065b3a412cf 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -488,7 +488,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
>  	if (mtk_crtc->cmdq_client) {
>  		cmdq_handle = cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
>  		cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
> -		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
> +		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event, false);
>  		mtk_crtc_ddp_config(crtc, cmdq_handle);
>  		cmdq_pkt_finalize(cmdq_handle);
>  		cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);

This should be an independent patch

> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index bb5be20fc70a..ec5637d43254 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -296,15 +296,16 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>  }
>  EXPORT_SYMBOL(cmdq_pkt_write_s_value);
>  
> -int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
>  {
>  	struct cmdq_instruction inst = { {0} };
> +	u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
>  
>  	if (event >= CMDQ_MAX_EVENT)
>  		return -EINVAL;
>  
>  	inst.op = CMDQ_CODE_WFE;
> -	inst.value = CMDQ_WFE_OPTION;
> +	inst.value = CMDQ_WFE_OPTION | clear_option;
>  	inst.event = event;
>  
>  	return cmdq_pkt_append_command(pkt, inst);
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index 3f6bc0dfd5da..42d2a30e6a70 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -27,8 +27,7 @@
>   * bit 16-27: update value
>   * bit 31: 1 - update, 0 - no update
>   */
> -#define CMDQ_WFE_OPTION			(CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | \
> -					CMDQ_WFE_WAIT_VALUE)
> +#define CMDQ_WFE_OPTION			(CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE)
>  
>  /** cmdq event maximum */
>  #define CMDQ_MAX_EVENT			0x3ff
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index 1a6c56f3bec1..d63749440697 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -152,11 +152,12 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>  /**
>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>   * @pkt:	the CMDQ packet
> - * @event:	the desired event type to "wait and CLEAR"
> + * @event:	the desired event type to wait
> + * @clear:	clear event or not after event arrive
>   *
>   * Return: 0 for success; else the error code is returned
>   */
> -int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event);
> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
>  
>  /**
>   * cmdq_pkt_clear_event() - append clear event command to the CMDQ packet
>

On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> Add set event function in cmdq helper functions to set specific event.
> 
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 15 +++++++++++++++
>  include/linux/mailbox/mtk-cmdq-mailbox.h |  1 +
>  include/linux/soc/mediatek/mtk-cmdq.h    |  9 +++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index ec5637d43254..3294c9285994 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -327,6 +327,21 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>  }
>  EXPORT_SYMBOL(cmdq_pkt_clear_event);
>  
> +int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +
> +	if (event >= CMDQ_MAX_EVENT)
> +		return -EINVAL;
> +
> +	inst.op = CMDQ_CODE_WFE;
> +	inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE;
> +	inst.event = event;
> +
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_set_event);
> +
>  int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
>  		  u16 offset, u32 value)
>  {
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index 42d2a30e6a70..ba2d811183a9 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -17,6 +17,7 @@
>  #define CMDQ_JUMP_PASS			CMDQ_INST_SIZE
>  
>  #define CMDQ_WFE_UPDATE			BIT(31)
> +#define CMDQ_WFE_UPDATE_VALUE		BIT(16)
>  #define CMDQ_WFE_WAIT			BIT(15)
>  #define CMDQ_WFE_WAIT_VALUE		0x1
>  
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index d63749440697..ca70296ae120 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -168,6 +168,15 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
>   */
>  int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event);
>  
> +/**
> + * cmdq_pkt_set_event() - append set event command to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @event:	the desired event to be set

Can we add the events and their code, so that later on, when a consumer calls
cmdq_pkt_set_event() we don't have any magic values that are hard to understand?

Regards,
Matthias

> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event);
> +
>  /**
>   * cmdq_pkt_poll() - Append polling command to the CMDQ packet, ask GCE to
>   *		     execute an instruction that wait for a specified
>

Hi Matthias,

Thanks for your comment.

On Sat, 2020-05-16 at 19:59 +0200, Matthias Brugger wrote:
> 
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > Add assign function in cmdq helper which assign constant value into
> > internal register by index.
> > 
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c   | 24 +++++++++++++++++++++++-
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |  1 +
> >  include/linux/soc/mediatek/mtk-cmdq.h    | 14 ++++++++++++++
> >  3 files changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 98f23ba3ba47..33153d17c9d9 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -12,6 +12,7 @@
> >  #define CMDQ_WRITE_ENABLE_MASK	BIT(0)
> >  #define CMDQ_POLL_ENABLE_MASK	BIT(0)
> >  #define CMDQ_EOC_IRQ_EN		BIT(0)
> > +#define CMDQ_REG_TYPE		1
> >  
> >  struct cmdq_instruction {
> >  	union {
> > @@ -21,8 +22,17 @@ struct cmdq_instruction {
> >  	union {
> >  		u16 offset;
> >  		u16 event;
> > +		u16 reg_dst;
> > +	};
> > +	union {
> > +		u8 subsys;
> > +		struct {
> > +			u8 sop:5;
> > +			u8 arg_c_t:1;
> > +			u8 arg_b_t:1;
> > +			u8 dst_t:1;
> > +		};
> 
> This union seems without context in this patch. Please drop.
> 

The dst_t use in cmdq_pkt_assign function so how about merge other
variables to reserved and leave dst_t ?

struct {
	u8 reserved_t:7;
	u8 dst_t:1;
};


Regards,
Dennis


> Regards,
> Matthias
> 
> >  	};
> > -	u8 subsys;
> >  	u8 op;
> >  };
> >  
> > @@ -277,6 +287,18 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> >  
> > +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> > +{
> > +	struct cmdq_instruction inst = { {0} };
> > +
> > +	inst.op = CMDQ_CODE_LOGIC;
> > +	inst.dst_t = CMDQ_REG_TYPE;
> > +	inst.reg_dst = reg_idx;
> > +	inst.value = value;
> > +	return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_assign);
> > +
> >  static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >  {
> >  	struct cmdq_instruction inst = { {0} };
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index dfe5b2eb85cc..121c3bb6d3de 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -59,6 +59,7 @@ enum cmdq_code {
> >  	CMDQ_CODE_JUMP = 0x10,
> >  	CMDQ_CODE_WFE = 0x20,
> >  	CMDQ_CODE_EOC = 0x40,
> > +	CMDQ_CODE_LOGIC = 0xa0,
> >  };
> >  
> >  enum cmdq_cb_status {
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index a74c1d5acdf3..83340211e1d3 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -152,6 +152,20 @@ 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_assign() - Append logic assign command to the CMDQ packet, ask GCE
> > + *		       to execute an instruction that set a constant value into
> > + *		       internal register and use as value, mask or address in
> > + *		       read/write instruction.
> > + * @pkt:	the CMDQ packet
> > + * @reg_idx:	the CMDQ internal register ID
> > + * @value:	the specified value
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
> > +
> >  /**
> >   * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
> >   *                          packet and call back at the end of done packet
> >

Hi Mattias,

Thanks for your comment.

On Sat, 2020-05-16 at 20:14 +0200, Matthias Brugger wrote:
> 
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > add write_s function in cmdq helper functions which
> > writes value contains in internal register to address
> > with large dma access support.
> > 
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c   | 34 +++++++++++++++++++++++-
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 ++
> >  include/linux/soc/mediatek/mtk-cmdq.h    | 20 ++++++++++++++
> >  3 files changed, 55 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 33153d17c9d9..90f1ff2b4b00 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -18,6 +18,10 @@ struct cmdq_instruction {
> >  	union {
> >  		u32 value;
> >  		u32 mask;
> > +		struct {
> > +			u16 arg_c;
> > +			u16 src_reg;
> > +		};
> >  	};
> >  	union {
> >  		u16 offset;
> > @@ -29,7 +33,7 @@ struct cmdq_instruction {
> >  		struct {
> >  			u8 sop:5;
> >  			u8 arg_c_t:1;
> > -			u8 arg_b_t:1;
> > +			u8 src_t:1;
> 
> fixing patch 6/13 please. seems the struct should be added in this patch.

ok, will move to this patch.

> 
> >  			u8 dst_t:1;
> >  		};
> >  	};
> > @@ -222,6 +226,34 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_write_mask);
> >  
> > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > +		     u16 addr_low, u16 src_reg_idx, u32 mask)
> > +{
> > +	struct cmdq_instruction inst = { {0} };
> > +	int err;
> > +
> > +	if (mask != U32_MAX) {
> > +		inst.op = CMDQ_CODE_MASK;
> > +		inst.mask = ~mask;
> > +		err = cmdq_pkt_append_command(pkt, inst);
> > +		if (err < 0)
> > +			return err;
> > +
> > +		inst.mask = 0;
> > +		inst.op = CMDQ_CODE_WRITE_S_MASK;
> > +	} else {
> > +		inst.op = CMDQ_CODE_WRITE_S;
> > +	}
> > +
> > +	inst.src_t = CMDQ_REG_TYPE;
> 
> Not defined.
> Please make sure that every patch compiles on it's own and does not add a
> regression. This is very helpful if we have to bisect the kernel in the future.

May I know which part not defined? The src_t defined on top of this
patch and CMDQ_REG_TYPE defined in last patc (see 06/13).

> 
> > +	inst.sop = high_addr_reg_idx;
> > +	inst.offset = addr_low;
> > +	inst.src_reg = src_reg_idx;
> > +
> > +	return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_write_s);
> > +
> >  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> >  {
> >  	struct cmdq_instruction inst = { {0} };
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index 121c3bb6d3de..8ef87e1bd03b 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -59,6 +59,8 @@ enum cmdq_code {
> >  	CMDQ_CODE_JUMP = 0x10,
> >  	CMDQ_CODE_WFE = 0x20,
> >  	CMDQ_CODE_EOC = 0x40,
> > +	CMDQ_CODE_WRITE_S = 0x90,
> > +	CMDQ_CODE_WRITE_S_MASK = 0x91,
> >  	CMDQ_CODE_LOGIC = 0xa0,
> >  };
> >  
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 83340211e1d3..c72d826d8934 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -12,6 +12,8 @@
> >  #include <linux/timer.h>
> >  
> >  #define CMDQ_NO_TIMEOUT		0xffffffffu
> > +#define CMDQ_ADDR_HIGH(addr)	((u32)(((addr) >> 16) & GENMASK(31, 0)))
> > +#define CMDQ_ADDR_LOW(addr)	((u16)(addr) | BIT(1))
> >  
> >  struct cmdq_pkt;
> >  
> > @@ -102,6 +104,24 @@ int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value);
> >  int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
> >  			u16 offset, u32 value, u32 mask);
> >  
> > +/**
> > + * cmdq_pkt_write_s() - append write_s command to the CMDQ packet
> > + * @pkt:	the CMDQ packet
> > + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa
> 
> s/regisger/register

will fix

> 
> > + * @addr_low:	low address of pa
> > + * @src_reg_idx:	the CMDQ internal register ID which cache source value
> > + * @mask:	the specified target address mask, use U32_MAX if no need
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + *
> > + * Support write value to physical address without subsys. Use CMDQ_ADDR_HIGH()
> > + * to get high addrees and call cmdq_pkt_assign() to assign value into internal
> 
> s/addrees/address

will fix

> 
> > + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameterwhen
> 
> s/parameterwhen/parameter when

will fix

> 
> > + * call to this function.
> > + */
> > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > +		     u16 addr_low, u16 src_reg_idx, u32 mask);
> > +
> 
> In general I wonder if we shouldn't provide two functions, one that writes a
> mask and on for the else case.

ok, I'll separate this function to cmdq_pkt_write_s and
cmdq_pkt_write_s_mask. Let the client choose which case is more
suitable.


> 
> Regards,
> Matthias
> 
> >  /**
> >   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> >   * @pkt:	the CMDQ packet
> >

Hi Matthias,

Thanks for your comment.

On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote:
> 
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > add write_s function in cmdq helper functions which
> > writes a constant value to address with large dma
> > access support.
> > 
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-cmdq.h  | 14 ++++++++++++++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 03c129230cd7..a9ebbabb7439 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_write_s);
> >  
> > +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > +			   u16 addr_low, u32 value, u32 mask)
> > +{
> > +	struct cmdq_instruction inst = { {0} };
> > +	int err;
> > +
> > +	if (mask != U32_MAX) {
> > +		inst.op = CMDQ_CODE_MASK;
> > +		inst.mask = ~mask;
> > +		err = cmdq_pkt_append_command(pkt, inst);
> > +		if (err < 0)
> > +			return err;
> > +
> > +		inst.op = CMDQ_CODE_WRITE_S_MASK;
> > +	} else {
> > +		inst.op = CMDQ_CODE_WRITE_S;
> > +	}
> > +
> > +	inst.sop = high_addr_reg_idx;
> 
> Writing u16 value in a 5 bit wide variable?

We need only 5 bits in this case. I'll change high_addr_reg_idx
parameter to u8.

> 
> > +	inst.offset = addr_low;
> > +	inst.value = value;
> > +
> > +	return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_write_s_value);
> > +
> >  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> >  {
> >  	struct cmdq_instruction inst = { {0} };
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 01b4184af310..fec292aac83c 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
> >  int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >  		     u16 addr_low, u16 src_reg_idx, u32 mask);
> >  
> > +/**
> > + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ
> > + *			      packet which write value to a physical address
> > + * @pkt:	the CMDQ packet
> > + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa
> 
> register

will fix


Regards,
Dennis

> 
> > + * @addr_low:	low address of pa
> > + * @value:	the specified target value
> > + * @mask:	the specified target mask
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > +			   u16 addr_low, u32 value, u32 mask);
> > +
> >  /**
> >   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> >   * @pkt:	the CMDQ packet
> >

Hi Matthias,

Thanks for your comment.

On Sat, 2020-05-16 at 20:22 +0200, Matthias Brugger wrote:
> 
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > Export finalize function to client which helps append eoc and jump
> > command to pkt. Let client decide call finalize or not.
> > 
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 +
> >  drivers/soc/mediatek/mtk-cmdq-helper.c  | 7 ++-----
> >  include/linux/soc/mediatek/mtk-cmdq.h   | 8 ++++++++
> >  3 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 0dfcd1787e65..7daaabc26eb1 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -490,6 +490,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
> >  		cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
> >  		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
> >  		mtk_crtc_ddp_config(crtc, cmdq_handle);
> > +		cmdq_pkt_finalize(cmdq_handle);
> >  		cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
> >  	}
> >  #endif
> 
> This should be a independent patch.
> Other then that patch looks good.

ok, I'll separate this part.


Regards,
Dennis

> 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index a9ebbabb7439..59bc1164b411 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -372,7 +372,7 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_assign);
> >  
> > -static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > +int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >  {
> >  	struct cmdq_instruction inst = { {0} };
> >  	int err;
> > @@ -392,6 +392,7 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >  
> >  	return err;
> >  }
> > +EXPORT_SYMBOL(cmdq_pkt_finalize);
> >  
> >  static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> >  {
> > @@ -426,10 +427,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> >  	unsigned long flags = 0;
> >  	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> >  
> > -	err = cmdq_pkt_finalize(pkt);
> > -	if (err < 0)
> > -		return err;
> > -
> >  	pkt->cb.cb = cb;
> >  	pkt->cb.data = data;
> >  	pkt->async_cb.cb = cmdq_pkt_flush_async_cb;
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index fec292aac83c..99e77155f967 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -213,6 +213,14 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> >   */
> >  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
> >  
> > +/**
> > + * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
> > + * @pkt:	the CMDQ packet
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_finalize(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
> >

Hi Matthias,

Thanks for your comment.


On Sat, 2020-05-16 at 20:30 +0200, Matthias Brugger wrote:
> 
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > Add clear parameter to let client decide if
> > event should be clear to 0 after GCE receive it.
> > 
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 2 +-
> >  drivers/soc/mediatek/mtk-cmdq-helper.c   | 5 +++--
> >  include/linux/mailbox/mtk-cmdq-mailbox.h | 3 +--
> >  include/linux/soc/mediatek/mtk-cmdq.h    | 5 +++--
> >  4 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 7daaabc26eb1..a065b3a412cf 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -488,7 +488,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
> >  	if (mtk_crtc->cmdq_client) {
> >  		cmdq_handle = cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
> >  		cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
> > -		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
> > +		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event, false);
> >  		mtk_crtc_ddp_config(crtc, cmdq_handle);
> >  		cmdq_pkt_finalize(cmdq_handle);
> >  		cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
> 
> This should be an independent patch

ok, I'll separate this part.


Regards,
Dennis

> 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index bb5be20fc70a..ec5637d43254 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -296,15 +296,16 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_write_s_value);
> >  
> > -int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> > +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
> >  {
> >  	struct cmdq_instruction inst = { {0} };
> > +	u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
> >  
> >  	if (event >= CMDQ_MAX_EVENT)
> >  		return -EINVAL;
> >  
> >  	inst.op = CMDQ_CODE_WFE;
> > -	inst.value = CMDQ_WFE_OPTION;
> > +	inst.value = CMDQ_WFE_OPTION | clear_option;
> >  	inst.event = event;
> >  
> >  	return cmdq_pkt_append_command(pkt, inst);
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index 3f6bc0dfd5da..42d2a30e6a70 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -27,8 +27,7 @@
> >   * bit 16-27: update value
> >   * bit 31: 1 - update, 0 - no update
> >   */
> > -#define CMDQ_WFE_OPTION			(CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | \
> > -					CMDQ_WFE_WAIT_VALUE)
> > +#define CMDQ_WFE_OPTION			(CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE)
> >  
> >  /** cmdq event maximum */
> >  #define CMDQ_MAX_EVENT			0x3ff
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 1a6c56f3bec1..d63749440697 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -152,11 +152,12 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >  /**
> >   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> >   * @pkt:	the CMDQ packet
> > - * @event:	the desired event type to "wait and CLEAR"
> > + * @event:	the desired event type to wait
> > + * @clear:	clear event or not after event arrive
> >   *
> >   * Return: 0 for success; else the error code is returned
> >   */
> > -int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event);
> > +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
> >  
> >  /**
> >   * cmdq_pkt_clear_event() - append clear event command to the CMDQ packet
> >

Hi Matthias,

Thanks for your comment.


On Sat, 2020-05-16 at 20:32 +0200, Matthias Brugger wrote:
> 
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > Add set event function in cmdq helper functions to set specific event.
> > 
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c   | 15 +++++++++++++++
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |  1 +
> >  include/linux/soc/mediatek/mtk-cmdq.h    |  9 +++++++++
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index ec5637d43254..3294c9285994 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -327,6 +327,21 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_clear_event);
> >  
> > +int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
> > +{
> > +	struct cmdq_instruction inst = { {0} };
> > +
> > +	if (event >= CMDQ_MAX_EVENT)
> > +		return -EINVAL;
> > +
> > +	inst.op = CMDQ_CODE_WFE;
> > +	inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE;
> > +	inst.event = event;
> > +
> > +	return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_set_event);
> > +
> >  int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
> >  		  u16 offset, u32 value)
> >  {
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index 42d2a30e6a70..ba2d811183a9 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -17,6 +17,7 @@
> >  #define CMDQ_JUMP_PASS			CMDQ_INST_SIZE
> >  
> >  #define CMDQ_WFE_UPDATE			BIT(31)
> > +#define CMDQ_WFE_UPDATE_VALUE		BIT(16)
> >  #define CMDQ_WFE_WAIT			BIT(15)
> >  #define CMDQ_WFE_WAIT_VALUE		0x1
> >  
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index d63749440697..ca70296ae120 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -168,6 +168,15 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
> >   */
> >  int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event);
> >  
> > +/**
> > + * cmdq_pkt_set_event() - append set event command to the CMDQ packet
> > + * @pkt:	the CMDQ packet
> > + * @event:	the desired event to be set
> 
> Can we add the events and their code, so that later on, when a consumer calls
> cmdq_pkt_set_event() we don't have any magic values that are hard to understand?

Please see patch 02/13:
http://lists.infradead.org/pipermail/linux-mediatek/2020-March/027801.html

Definitions begin with CMDQ_EVENT_ is the event id to this function.
Since the event id is different between platform, client must parse it
from device tree. So no magic values require when call this function.


Regard,
Dennis


> 
> Regards,
> Matthias
> 
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event);
> > +
> >  /**
> >   * cmdq_pkt_poll() - Append polling command to the CMDQ packet, ask GCE to
> >   *		     execute an instruction that wait for a specified
> >

On 24/05/2020 19:01, Dennis-YC Hsieh wrote:
> Hi Matthias,
> 
> Thanks for your comment.
> 
> On Sat, 2020-05-16 at 19:59 +0200, Matthias Brugger wrote:
>>
>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>> Add assign function in cmdq helper which assign constant value into
>>> internal register by index.
>>>
>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> ---
>>>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 24 +++++++++++++++++++++++-
>>>  include/linux/mailbox/mtk-cmdq-mailbox.h |  1 +
>>>  include/linux/soc/mediatek/mtk-cmdq.h    | 14 ++++++++++++++
>>>  3 files changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index 98f23ba3ba47..33153d17c9d9 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -12,6 +12,7 @@
>>>  #define CMDQ_WRITE_ENABLE_MASK	BIT(0)
>>>  #define CMDQ_POLL_ENABLE_MASK	BIT(0)
>>>  #define CMDQ_EOC_IRQ_EN		BIT(0)
>>> +#define CMDQ_REG_TYPE		1
>>>  
>>>  struct cmdq_instruction {
>>>  	union {
>>> @@ -21,8 +22,17 @@ struct cmdq_instruction {
>>>  	union {
>>>  		u16 offset;
>>>  		u16 event;
>>> +		u16 reg_dst;
>>> +	};
>>> +	union {
>>> +		u8 subsys;
>>> +		struct {
>>> +			u8 sop:5;
>>> +			u8 arg_c_t:1;
>>> +			u8 arg_b_t:1;
>>> +			u8 dst_t:1;
>>> +		};
>>
>> This union seems without context in this patch. Please drop.
>>
> 
> The dst_t use in cmdq_pkt_assign function so how about merge other

Ah didn't realize this. Then I think it's OK like it is.

Regards,
Matthias

> variables to reserved and leave dst_t ?
> 
> struct {
> 	u8 reserved_t:7;
> 	u8 dst_t:1;
> };
> 
> 
> Regards,
> Dennis
> 
> 
>> Regards,
>> Matthias
>>
>>>  	};
>>> -	u8 subsys;
>>>  	u8 op;
>>>  };
>>>  
>>> @@ -277,6 +287,18 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>>>  }
>>>  EXPORT_SYMBOL(cmdq_pkt_poll_mask);
>>>  
>>> +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
>>> +{
>>> +	struct cmdq_instruction inst = { {0} };
>>> +
>>> +	inst.op = CMDQ_CODE_LOGIC;
>>> +	inst.dst_t = CMDQ_REG_TYPE;
>>> +	inst.reg_dst = reg_idx;
>>> +	inst.value = value;
>>> +	return cmdq_pkt_append_command(pkt, inst);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_assign);
>>> +
>>>  static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>>>  {
>>>  	struct cmdq_instruction inst = { {0} };
>>> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> index dfe5b2eb85cc..121c3bb6d3de 100644
>>> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> @@ -59,6 +59,7 @@ enum cmdq_code {
>>>  	CMDQ_CODE_JUMP = 0x10,
>>>  	CMDQ_CODE_WFE = 0x20,
>>>  	CMDQ_CODE_EOC = 0x40,
>>> +	CMDQ_CODE_LOGIC = 0xa0,
>>>  };
>>>  
>>>  enum cmdq_cb_status {
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index a74c1d5acdf3..83340211e1d3 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -152,6 +152,20 @@ 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_assign() - Append logic assign command to the CMDQ packet, ask GCE
>>> + *		       to execute an instruction that set a constant value into
>>> + *		       internal register and use as value, mask or address in
>>> + *		       read/write instruction.
>>> + * @pkt:	the CMDQ packet
>>> + * @reg_idx:	the CMDQ internal register ID
>>> + * @value:	the specified value
>>> + *
>>> + * Return: 0 for success; else the error code is returned
>>> + */
>>> +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
>>> +
>>>  /**
>>>   * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
>>>   *                          packet and call back at the end of done packet
>>>
>

On 24/05/2020 19:26, Dennis-YC Hsieh wrote:
> Hi Mattias,
> 
> Thanks for your comment.
> 
> On Sat, 2020-05-16 at 20:14 +0200, Matthias Brugger wrote:
>>
>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>> add write_s function in cmdq helper functions which
>>> writes value contains in internal register to address
>>> with large dma access support.
>>>
>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> ---
>>>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 34 +++++++++++++++++++++++-
>>>  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 ++
>>>  include/linux/soc/mediatek/mtk-cmdq.h    | 20 ++++++++++++++
>>>  3 files changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index 33153d17c9d9..90f1ff2b4b00 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -18,6 +18,10 @@ struct cmdq_instruction {
>>>  	union {
>>>  		u32 value;
>>>  		u32 mask;
>>> +		struct {
>>> +			u16 arg_c;
>>> +			u16 src_reg;
>>> +		};
>>>  	};
>>>  	union {
>>>  		u16 offset;
>>> @@ -29,7 +33,7 @@ struct cmdq_instruction {
>>>  		struct {
>>>  			u8 sop:5;
>>>  			u8 arg_c_t:1;
>>> -			u8 arg_b_t:1;
>>> +			u8 src_t:1;
>>
>> fixing patch 6/13 please. seems the struct should be added in this patch.
> 
> ok, will move to this patch.
> 
>>
>>>  			u8 dst_t:1;
>>>  		};
>>>  	};
>>> @@ -222,6 +226,34 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
>>>  }
>>>  EXPORT_SYMBOL(cmdq_pkt_write_mask);
>>>  
>>> +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> +		     u16 addr_low, u16 src_reg_idx, u32 mask)
>>> +{
>>> +	struct cmdq_instruction inst = { {0} };
>>> +	int err;
>>> +
>>> +	if (mask != U32_MAX) {
>>> +		inst.op = CMDQ_CODE_MASK;
>>> +		inst.mask = ~mask;
>>> +		err = cmdq_pkt_append_command(pkt, inst);
>>> +		if (err < 0)
>>> +			return err;
>>> +
>>> +		inst.mask = 0;
>>> +		inst.op = CMDQ_CODE_WRITE_S_MASK;
>>> +	} else {
>>> +		inst.op = CMDQ_CODE_WRITE_S;
>>> +	}
>>> +
>>> +	inst.src_t = CMDQ_REG_TYPE;
>>
>> Not defined.
>> Please make sure that every patch compiles on it's own and does not add a
>> regression. This is very helpful if we have to bisect the kernel in the future.
> 
> May I know which part not defined? The src_t defined on top of this
> patch and CMDQ_REG_TYPE defined in last patc (see 06/13).

correct, sorry for the noise.

> 
>>
>>> +	inst.sop = high_addr_reg_idx;
>>> +	inst.offset = addr_low;
>>> +	inst.src_reg = src_reg_idx;
>>> +
>>> +	return cmdq_pkt_append_command(pkt, inst);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_write_s);
>>> +
>>>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
>>>  {
>>>  	struct cmdq_instruction inst = { {0} };
>>> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> index 121c3bb6d3de..8ef87e1bd03b 100644
>>> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> @@ -59,6 +59,8 @@ enum cmdq_code {
>>>  	CMDQ_CODE_JUMP = 0x10,
>>>  	CMDQ_CODE_WFE = 0x20,
>>>  	CMDQ_CODE_EOC = 0x40,
>>> +	CMDQ_CODE_WRITE_S = 0x90,
>>> +	CMDQ_CODE_WRITE_S_MASK = 0x91,
>>>  	CMDQ_CODE_LOGIC = 0xa0,
>>>  };
>>>  
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index 83340211e1d3..c72d826d8934 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -12,6 +12,8 @@
>>>  #include <linux/timer.h>
>>>  
>>>  #define CMDQ_NO_TIMEOUT		0xffffffffu
>>> +#define CMDQ_ADDR_HIGH(addr)	((u32)(((addr) >> 16) & GENMASK(31, 0)))
>>> +#define CMDQ_ADDR_LOW(addr)	((u16)(addr) | BIT(1))
>>>  
>>>  struct cmdq_pkt;
>>>  
>>> @@ -102,6 +104,24 @@ int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value);
>>>  int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
>>>  			u16 offset, u32 value, u32 mask);
>>>  
>>> +/**
>>> + * cmdq_pkt_write_s() - append write_s command to the CMDQ packet
>>> + * @pkt:	the CMDQ packet
>>> + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa
>>
>> s/regisger/register
> 
> will fix
> 
>>
>>> + * @addr_low:	low address of pa
>>> + * @src_reg_idx:	the CMDQ internal register ID which cache source value
>>> + * @mask:	the specified target address mask, use U32_MAX if no need
>>> + *
>>> + * Return: 0 for success; else the error code is returned
>>> + *
>>> + * Support write value to physical address without subsys. Use CMDQ_ADDR_HIGH()
>>> + * to get high addrees and call cmdq_pkt_assign() to assign value into internal
>>
>> s/addrees/address
> 
> will fix
> 
>>
>>> + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameterwhen
>>
>> s/parameterwhen/parameter when
> 
> will fix
> 
>>
>>> + * call to this function.
>>> + */
>>> +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> +		     u16 addr_low, u16 src_reg_idx, u32 mask);
>>> +
>>
>> In general I wonder if we shouldn't provide two functions, one that writes a
>> mask and on for the else case.
> 
> ok, I'll separate this function to cmdq_pkt_write_s and
> cmdq_pkt_write_s_mask. Let the client choose which case is more
> suitable.

Sound good, thanks.


> 
> 
>>
>> Regards,
>> Matthias
>>
>>>  /**
>>>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>>>   * @pkt:	the CMDQ packet
>>>
>

On 24/05/2020 19:31, Dennis-YC Hsieh wrote:
> Hi Matthias,
> 
> Thanks for your comment.
> 
> On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote:
>>
>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>> add write_s function in cmdq helper functions which
>>> writes a constant value to address with large dma
>>> access support.
>>>
>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> ---
>>>  drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
>>>  include/linux/soc/mediatek/mtk-cmdq.h  | 14 ++++++++++++++
>>>  2 files changed, 40 insertions(+)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index 03c129230cd7..a9ebbabb7439 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>  }
>>>  EXPORT_SYMBOL(cmdq_pkt_write_s);
>>>  
>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> +			   u16 addr_low, u32 value, u32 mask)
>>> +{
>>> +	struct cmdq_instruction inst = { {0} };
>>> +	int err;
>>> +
>>> +	if (mask != U32_MAX) {
>>> +		inst.op = CMDQ_CODE_MASK;
>>> +		inst.mask = ~mask;
>>> +		err = cmdq_pkt_append_command(pkt, inst);
>>> +		if (err < 0)
>>> +			return err;
>>> +
>>> +		inst.op = CMDQ_CODE_WRITE_S_MASK;
>>> +	} else {
>>> +		inst.op = CMDQ_CODE_WRITE_S;
>>> +	}
>>> +
>>> +	inst.sop = high_addr_reg_idx;
>>
>> Writing u16 value in a 5 bit wide variable?
> 
> We need only 5 bits in this case. I'll change high_addr_reg_idx
> parameter to u8.
> 

Ok, please make sure to mask the value, so that it's explicit in the code that
we only use the lowest 5 bits of high_addr_reg_idx.

Regards,
Matthias

>>
>>> +	inst.offset = addr_low;
>>> +	inst.value = value;
>>> +
>>> +	return cmdq_pkt_append_command(pkt, inst);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value);
>>> +
>>>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
>>>  {
>>>  	struct cmdq_instruction inst = { {0} };
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index 01b4184af310..fec292aac83c 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
>>>  int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>  		     u16 addr_low, u16 src_reg_idx, u32 mask);
>>>  
>>> +/**
>>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ
>>> + *			      packet which write value to a physical address
>>> + * @pkt:	the CMDQ packet
>>> + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa
>>
>> register
> 
> will fix
> 
> 
> Regards,
> Dennis
> 
>>
>>> + * @addr_low:	low address of pa
>>> + * @value:	the specified target value
>>> + * @mask:	the specified target mask
>>> + *
>>> + * Return: 0 for success; else the error code is returned
>>> + */
>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> +			   u16 addr_low, u32 value, u32 mask);
>>> +
>>>  /**
>>>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>>>   * @pkt:	the CMDQ packet
>>>
>

On 24/05/2020 19:39, Dennis-YC Hsieh wrote:
> Hi Matthias,
> 
> Thanks for your comment.
> 
> 
> On Sat, 2020-05-16 at 20:32 +0200, Matthias Brugger wrote:
>>
>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>> Add set event function in cmdq helper functions to set specific event.
>>>
>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> ---
>>>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 15 +++++++++++++++
>>>  include/linux/mailbox/mtk-cmdq-mailbox.h |  1 +
>>>  include/linux/soc/mediatek/mtk-cmdq.h    |  9 +++++++++
>>>  3 files changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index ec5637d43254..3294c9285994 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -327,6 +327,21 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>>>  }
>>>  EXPORT_SYMBOL(cmdq_pkt_clear_event);
>>>  
>>> +int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>>> +{
>>> +	struct cmdq_instruction inst = { {0} };
>>> +
>>> +	if (event >= CMDQ_MAX_EVENT)
>>> +		return -EINVAL;
>>> +
>>> +	inst.op = CMDQ_CODE_WFE;
>>> +	inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE;
>>> +	inst.event = event;
>>> +
>>> +	return cmdq_pkt_append_command(pkt, inst);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_set_event);
>>> +
>>>  int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
>>>  		  u16 offset, u32 value)
>>>  {
>>> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> index 42d2a30e6a70..ba2d811183a9 100644
>>> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> @@ -17,6 +17,7 @@
>>>  #define CMDQ_JUMP_PASS			CMDQ_INST_SIZE
>>>  
>>>  #define CMDQ_WFE_UPDATE			BIT(31)
>>> +#define CMDQ_WFE_UPDATE_VALUE		BIT(16)
>>>  #define CMDQ_WFE_WAIT			BIT(15)
>>>  #define CMDQ_WFE_WAIT_VALUE		0x1
>>>  
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index d63749440697..ca70296ae120 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -168,6 +168,15 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
>>>   */
>>>  int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event);
>>>  
>>> +/**
>>> + * cmdq_pkt_set_event() - append set event command to the CMDQ packet
>>> + * @pkt:	the CMDQ packet
>>> + * @event:	the desired event to be set
>>
>> Can we add the events and their code, so that later on, when a consumer calls
>> cmdq_pkt_set_event() we don't have any magic values that are hard to understand?
> 
> Please see patch 02/13:
> http://lists.infradead.org/pipermail/linux-mediatek/2020-March/027801.html
> 
> Definitions begin with CMDQ_EVENT_ is the event id to this function.
> Since the event id is different between platform, client must parse it
> from device tree. So no magic values require when call this function.
> 
> 

Got it, thanks for clarification.
Matthias

Hi, Matthias:

Matthias Brugger <matthias.bgg@gmail.com> 於 2020年5月17日 週日 上午2:22寫道：
>
>
>
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > Export finalize function to client which helps append eoc and jump
> > command to pkt. Let client decide call finalize or not.
> >
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 +
> >  drivers/soc/mediatek/mtk-cmdq-helper.c  | 7 ++-----
> >  include/linux/soc/mediatek/mtk-cmdq.h   | 8 ++++++++
> >  3 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 0dfcd1787e65..7daaabc26eb1 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -490,6 +490,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
> >               cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
> >               cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
> >               mtk_crtc_ddp_config(crtc, cmdq_handle);
> > +             cmdq_pkt_finalize(cmdq_handle);
> >               cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
> >       }
> >  #endif
>
> This should be a independent patch.
> Other then that patch looks good.

Apply only drm part or only cmdq helpr part, it would be abnormal.
Shall we seperate this patch?
Or seperate it but make sure these two patches be in the same tree?

Regards,
Chun-Kuang.

>
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index a9ebbabb7439..59bc1164b411 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -372,7 +372,7 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_assign);
> >
> > -static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > +int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >  {
> >       struct cmdq_instruction inst = { {0} };
> >       int err;
> > @@ -392,6 +392,7 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >
> >       return err;
> >  }
> > +EXPORT_SYMBOL(cmdq_pkt_finalize);
> >
> >  static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> >  {
> > @@ -426,10 +427,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> >       unsigned long flags = 0;
> >       struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> >
> > -     err = cmdq_pkt_finalize(pkt);
> > -     if (err < 0)
> > -             return err;
> > -
> >       pkt->cb.cb = cb;
> >       pkt->cb.data = data;
> >       pkt->async_cb.cb = cmdq_pkt_flush_async_cb;
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index fec292aac83c..99e77155f967 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -213,6 +213,14 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> >   */
> >  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
> >
> > +/**
> > + * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
> > + * @pkt:     the CMDQ packet
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_finalize(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
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sun, 2020-05-24 at 20:13 +0200, Matthias Brugger wrote:
> 
> On 24/05/2020 19:31, Dennis-YC Hsieh wrote:
> > Hi Matthias,
> > 
> > Thanks for your comment.
> > 
> > On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote:
> >>
> >> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> >>> add write_s function in cmdq helper functions which
> >>> writes a constant value to address with large dma
> >>> access support.
> >>>
> >>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> >>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> >>> ---
> >>>  drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
> >>>  include/linux/soc/mediatek/mtk-cmdq.h  | 14 ++++++++++++++
> >>>  2 files changed, 40 insertions(+)
> >>>
> >>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>> index 03c129230cd7..a9ebbabb7439 100644
> >>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >>>  }
> >>>  EXPORT_SYMBOL(cmdq_pkt_write_s);
> >>>  
> >>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >>> +			   u16 addr_low, u32 value, u32 mask)
> >>> +{
> >>> +	struct cmdq_instruction inst = { {0} };
> >>> +	int err;
> >>> +
> >>> +	if (mask != U32_MAX) {
> >>> +		inst.op = CMDQ_CODE_MASK;
> >>> +		inst.mask = ~mask;
> >>> +		err = cmdq_pkt_append_command(pkt, inst);
> >>> +		if (err < 0)
> >>> +			return err;
> >>> +
> >>> +		inst.op = CMDQ_CODE_WRITE_S_MASK;
> >>> +	} else {
> >>> +		inst.op = CMDQ_CODE_WRITE_S;
> >>> +	}
> >>> +
> >>> +	inst.sop = high_addr_reg_idx;
> >>
> >> Writing u16 value in a 5 bit wide variable?
> > 
> > We need only 5 bits in this case. I'll change high_addr_reg_idx
> > parameter to u8.
> > 
> 
> Ok, please make sure to mask the value, so that it's explicit in the code that
> we only use the lowest 5 bits of high_addr_reg_idx.

Is it necessary to mask the value?
Since sop already defined as "u8 sop:5;", I thought it is explicit that
only use 5 bits and compiler should do the rest jobs.


Regards,
Dennis

> 
> Regards,
> Matthias
> 
> >>
> >>> +	inst.offset = addr_low;
> >>> +	inst.value = value;
> >>> +
> >>> +	return cmdq_pkt_append_command(pkt, inst);
> >>> +}
> >>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value);
> >>> +
> >>>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> >>>  {
> >>>  	struct cmdq_instruction inst = { {0} };
> >>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> >>> index 01b4184af310..fec292aac83c 100644
> >>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> >>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> >>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
> >>>  int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >>>  		     u16 addr_low, u16 src_reg_idx, u32 mask);
> >>>  
> >>> +/**
> >>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ
> >>> + *			      packet which write value to a physical address
> >>> + * @pkt:	the CMDQ packet
> >>> + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa
> >>
> >> register
> > 
> > will fix
> > 
> > 
> > Regards,
> > Dennis
> > 
> >>
> >>> + * @addr_low:	low address of pa
> >>> + * @value:	the specified target value
> >>> + * @mask:	the specified target mask
> >>> + *
> >>> + * Return: 0 for success; else the error code is returned
> >>> + */
> >>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >>> +			   u16 addr_low, u32 value, u32 mask);
> >>> +
> >>>  /**
> >>>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> >>>   * @pkt:	the CMDQ packet
> >>>
> >

On 25/05/2020 02:23, Chun-Kuang Hu wrote:
> Hi, Matthias:
> 
> Matthias Brugger <matthias.bgg@gmail.com> 於 2020年5月17日 週日 上午2:22寫道：
>>
>>
>>
>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>> Export finalize function to client which helps append eoc and jump
>>> command to pkt. Let client decide call finalize or not.
>>>
>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> ---
>>>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 +
>>>  drivers/soc/mediatek/mtk-cmdq-helper.c  | 7 ++-----
>>>  include/linux/soc/mediatek/mtk-cmdq.h   | 8 ++++++++
>>>  3 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>> index 0dfcd1787e65..7daaabc26eb1 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>> @@ -490,6 +490,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
>>>               cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
>>>               cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
>>>               mtk_crtc_ddp_config(crtc, cmdq_handle);
>>> +             cmdq_pkt_finalize(cmdq_handle);
>>>               cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
>>>       }
>>>  #endif
>>
>> This should be a independent patch.
>> Other then that patch looks good.
> 
> Apply only drm part or only cmdq helpr part, it would be abnormal.

Right it would break DRM driver (if only applied to cmdq) or compilation if only
applied to DRM.

> Shall we seperate this patch?

After thinking twice, I think we can leave it as it is. If you provide your
Acked-by I can take it thorugh my tree, if that's OK for you.

Regards,
Matthias

> Or seperate it but make sure these two patches be in the same tree?
> 
> Regards,
> Chun-Kuang.
> 
>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index a9ebbabb7439..59bc1164b411 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -372,7 +372,7 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
>>>  }
>>>  EXPORT_SYMBOL(cmdq_pkt_assign);
>>>
>>> -static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>>> +int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>>>  {
>>>       struct cmdq_instruction inst = { {0} };
>>>       int err;
>>> @@ -392,6 +392,7 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>>>
>>>       return err;
>>>  }
>>> +EXPORT_SYMBOL(cmdq_pkt_finalize);
>>>
>>>  static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
>>>  {
>>> @@ -426,10 +427,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
>>>       unsigned long flags = 0;
>>>       struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
>>>
>>> -     err = cmdq_pkt_finalize(pkt);
>>> -     if (err < 0)
>>> -             return err;
>>> -
>>>       pkt->cb.cb = cb;
>>>       pkt->cb.data = data;
>>>       pkt->async_cb.cb = cmdq_pkt_flush_async_cb;
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index fec292aac83c..99e77155f967 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -213,6 +213,14 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>>>   */
>>>  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
>>>
>>> +/**
>>> + * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
>>> + * @pkt:     the CMDQ packet
>>> + *
>>> + * Return: 0 for success; else the error code is returned
>>> + */
>>> +int cmdq_pkt_finalize(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
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

On 25/05/2020 04:27, Dennis-YC Hsieh wrote:
> 
> On Sun, 2020-05-24 at 20:13 +0200, Matthias Brugger wrote:
>>
>> On 24/05/2020 19:31, Dennis-YC Hsieh wrote:
>>> Hi Matthias,
>>>
>>> Thanks for your comment.
>>>
>>> On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote:
>>>>
>>>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>>>> add write_s function in cmdq helper functions which
>>>>> writes a constant value to address with large dma
>>>>> access support.
>>>>>
>>>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>>>> ---
>>>>>  drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
>>>>>  include/linux/soc/mediatek/mtk-cmdq.h  | 14 ++++++++++++++
>>>>>  2 files changed, 40 insertions(+)
>>>>>
>>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>> index 03c129230cd7..a9ebbabb7439 100644
>>>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>>  }
>>>>>  EXPORT_SYMBOL(cmdq_pkt_write_s);
>>>>>  
>>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>> +			   u16 addr_low, u32 value, u32 mask)
>>>>> +{
>>>>> +	struct cmdq_instruction inst = { {0} };
>>>>> +	int err;
>>>>> +
>>>>> +	if (mask != U32_MAX) {
>>>>> +		inst.op = CMDQ_CODE_MASK;
>>>>> +		inst.mask = ~mask;
>>>>> +		err = cmdq_pkt_append_command(pkt, inst);
>>>>> +		if (err < 0)
>>>>> +			return err;
>>>>> +
>>>>> +		inst.op = CMDQ_CODE_WRITE_S_MASK;
>>>>> +	} else {
>>>>> +		inst.op = CMDQ_CODE_WRITE_S;
>>>>> +	}
>>>>> +
>>>>> +	inst.sop = high_addr_reg_idx;
>>>>
>>>> Writing u16 value in a 5 bit wide variable?
>>>
>>> We need only 5 bits in this case. I'll change high_addr_reg_idx
>>> parameter to u8.
>>>
>>
>> Ok, please make sure to mask the value, so that it's explicit in the code that
>> we only use the lowest 5 bits of high_addr_reg_idx.
> 
> Is it necessary to mask the value?
> Since sop already defined as "u8 sop:5;", I thought it is explicit that
> only use 5 bits and compiler should do the rest jobs.

Yes but it makes the code more explicit if we have a
inst.sop = high_addr_reg_idx & 0x1f;

What do you think?

Regards,
Matthias

> 
> 
> Regards,
> Dennis
> 
>>
>> Regards,
>> Matthias
>>
>>>>
>>>>> +	inst.offset = addr_low;
>>>>> +	inst.value = value;
>>>>> +
>>>>> +	return cmdq_pkt_append_command(pkt, inst);
>>>>> +}
>>>>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value);
>>>>> +
>>>>>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
>>>>>  {
>>>>>  	struct cmdq_instruction inst = { {0} };
>>>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>>>> index 01b4184af310..fec292aac83c 100644
>>>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>>>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
>>>>>  int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>>  		     u16 addr_low, u16 src_reg_idx, u32 mask);
>>>>>  
>>>>> +/**
>>>>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ
>>>>> + *			      packet which write value to a physical address
>>>>> + * @pkt:	the CMDQ packet
>>>>> + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa
>>>>
>>>> register
>>>
>>> will fix
>>>
>>>
>>> Regards,
>>> Dennis
>>>
>>>>
>>>>> + * @addr_low:	low address of pa
>>>>> + * @value:	the specified target value
>>>>> + * @mask:	the specified target mask
>>>>> + *
>>>>> + * Return: 0 for success; else the error code is returned
>>>>> + */
>>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>> +			   u16 addr_low, u32 value, u32 mask);
>>>>> +
>>>>>  /**
>>>>>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>>>>>   * @pkt:	the CMDQ packet
>>>>>
>>>
>

On Mon, 2020-05-25 at 10:39 +0200, Matthias Brugger wrote:
> 
> On 25/05/2020 04:27, Dennis-YC Hsieh wrote:
> > 
> > On Sun, 2020-05-24 at 20:13 +0200, Matthias Brugger wrote:
> >>
> >> On 24/05/2020 19:31, Dennis-YC Hsieh wrote:
> >>> Hi Matthias,
> >>>
> >>> Thanks for your comment.
> >>>
> >>> On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote:
> >>>>
> >>>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> >>>>> add write_s function in cmdq helper functions which
> >>>>> writes a constant value to address with large dma
> >>>>> access support.
> >>>>>
> >>>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> >>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> >>>>> ---
> >>>>>  drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
> >>>>>  include/linux/soc/mediatek/mtk-cmdq.h  | 14 ++++++++++++++
> >>>>>  2 files changed, 40 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>>>> index 03c129230cd7..a9ebbabb7439 100644
> >>>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>>>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >>>>>  }
> >>>>>  EXPORT_SYMBOL(cmdq_pkt_write_s);
> >>>>>  
> >>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >>>>> +			   u16 addr_low, u32 value, u32 mask)
> >>>>> +{
> >>>>> +	struct cmdq_instruction inst = { {0} };
> >>>>> +	int err;
> >>>>> +
> >>>>> +	if (mask != U32_MAX) {
> >>>>> +		inst.op = CMDQ_CODE_MASK;
> >>>>> +		inst.mask = ~mask;
> >>>>> +		err = cmdq_pkt_append_command(pkt, inst);
> >>>>> +		if (err < 0)
> >>>>> +			return err;
> >>>>> +
> >>>>> +		inst.op = CMDQ_CODE_WRITE_S_MASK;
> >>>>> +	} else {
> >>>>> +		inst.op = CMDQ_CODE_WRITE_S;
> >>>>> +	}
> >>>>> +
> >>>>> +	inst.sop = high_addr_reg_idx;
> >>>>
> >>>> Writing u16 value in a 5 bit wide variable?
> >>>
> >>> We need only 5 bits in this case. I'll change high_addr_reg_idx
> >>> parameter to u8.
> >>>
> >>
> >> Ok, please make sure to mask the value, so that it's explicit in the code that
> >> we only use the lowest 5 bits of high_addr_reg_idx.
> > 
> > Is it necessary to mask the value?
> > Since sop already defined as "u8 sop:5;", I thought it is explicit that
> > only use 5 bits and compiler should do the rest jobs.
> 
> Yes but it makes the code more explicit if we have a
> inst.sop = high_addr_reg_idx & 0x1f;
> 
> What do you think?

The value assign to sop will restrict by hardware spec. Clients call
this function will define constant value and use it as parameter. So I
think we don't worry about client call this api with wrong value.


Regards,
Dennis

> 
> Regards,
> Matthias
> 
> > 
> > 
> > Regards,
> > Dennis
> > 
> >>
> >> Regards,
> >> Matthias
> >>
> >>>>
> >>>>> +	inst.offset = addr_low;
> >>>>> +	inst.value = value;
> >>>>> +
> >>>>> +	return cmdq_pkt_append_command(pkt, inst);
> >>>>> +}
> >>>>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value);
> >>>>> +
> >>>>>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> >>>>>  {
> >>>>>  	struct cmdq_instruction inst = { {0} };
> >>>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> >>>>> index 01b4184af310..fec292aac83c 100644
> >>>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> >>>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> >>>>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
> >>>>>  int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >>>>>  		     u16 addr_low, u16 src_reg_idx, u32 mask);
> >>>>>  
> >>>>> +/**
> >>>>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ
> >>>>> + *			      packet which write value to a physical address
> >>>>> + * @pkt:	the CMDQ packet
> >>>>> + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa
> >>>>
> >>>> register
> >>>
> >>> will fix
> >>>
> >>>
> >>> Regards,
> >>> Dennis
> >>>
> >>>>
> >>>>> + * @addr_low:	low address of pa
> >>>>> + * @value:	the specified target value
> >>>>> + * @mask:	the specified target mask
> >>>>> + *
> >>>>> + * Return: 0 for success; else the error code is returned
> >>>>> + */
> >>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> >>>>> +			   u16 addr_low, u32 value, u32 mask);
> >>>>> +
> >>>>>  /**
> >>>>>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> >>>>>   * @pkt:	the CMDQ packet
> >>>>>
> >>>
> >

Hi, Matthias:

Matthias Brugger <matthias.bgg@gmail.com> 於 2020年5月25日 週一 下午4:38寫道：
>
>
>
> On 25/05/2020 02:23, Chun-Kuang Hu wrote:
> > Hi, Matthias:
> >
> > Matthias Brugger <matthias.bgg@gmail.com> 於 2020年5月17日 週日 上午2:22寫道：
> >>
> >>
> >>
> >> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> >>> Export finalize function to client which helps append eoc and jump
> >>> command to pkt. Let client decide call finalize or not.
> >>>
> >>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> >>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> >>> ---
> >>>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 +
> >>>  drivers/soc/mediatek/mtk-cmdq-helper.c  | 7 ++-----
> >>>  include/linux/soc/mediatek/mtk-cmdq.h   | 8 ++++++++
> >>>  3 files changed, 11 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> >>> index 0dfcd1787e65..7daaabc26eb1 100644
> >>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> >>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> >>> @@ -490,6 +490,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
> >>>               cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
> >>>               cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
> >>>               mtk_crtc_ddp_config(crtc, cmdq_handle);
> >>> +             cmdq_pkt_finalize(cmdq_handle);
> >>>               cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
> >>>       }
> >>>  #endif
> >>
> >> This should be a independent patch.
> >> Other then that patch looks good.
> >
> > Apply only drm part or only cmdq helpr part, it would be abnormal.
>
> Right it would break DRM driver (if only applied to cmdq) or compilation if only
> applied to DRM.
>
> > Shall we seperate this patch?
>
> After thinking twice, I think we can leave it as it is. If you provide your
> Acked-by I can take it thorugh my tree, if that's OK for you.

This is OK for me, so

Acked-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>

>
> Regards,
> Matthias
>
> > Or seperate it but make sure these two patches be in the same tree?
> >
> > Regards,
> > Chun-Kuang.
> >
> >>
> >>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>> index a9ebbabb7439..59bc1164b411 100644
> >>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>> @@ -372,7 +372,7 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> >>>  }
> >>>  EXPORT_SYMBOL(cmdq_pkt_assign);
> >>>
> >>> -static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >>> +int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >>>  {
> >>>       struct cmdq_instruction inst = { {0} };
> >>>       int err;
> >>> @@ -392,6 +392,7 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >>>
> >>>       return err;
> >>>  }
> >>> +EXPORT_SYMBOL(cmdq_pkt_finalize);
> >>>
> >>>  static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> >>>  {
> >>> @@ -426,10 +427,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> >>>       unsigned long flags = 0;
> >>>       struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> >>>
> >>> -     err = cmdq_pkt_finalize(pkt);
> >>> -     if (err < 0)
> >>> -             return err;
> >>> -
> >>>       pkt->cb.cb = cb;
> >>>       pkt->cb.data = data;
> >>>       pkt->async_cb.cb = cmdq_pkt_flush_async_cb;
> >>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> >>> index fec292aac83c..99e77155f967 100644
> >>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> >>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> >>> @@ -213,6 +213,14 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> >>>   */
> >>>  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
> >>>
> >>> +/**
> >>> + * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
> >>> + * @pkt:     the CMDQ packet
> >>> + *
> >>> + * Return: 0 for success; else the error code is returned
> >>> + */
> >>> +int cmdq_pkt_finalize(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
> >>>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel

On 25/05/2020 12:38, Dennis-YC Hsieh wrote:
> 
> On Mon, 2020-05-25 at 10:39 +0200, Matthias Brugger wrote:
>>
>> On 25/05/2020 04:27, Dennis-YC Hsieh wrote:
>>>
>>> On Sun, 2020-05-24 at 20:13 +0200, Matthias Brugger wrote:
>>>>
>>>> On 24/05/2020 19:31, Dennis-YC Hsieh wrote:
>>>>> Hi Matthias,
>>>>>
>>>>> Thanks for your comment.
>>>>>
>>>>> On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote:
>>>>>>
>>>>>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>>>>>> add write_s function in cmdq helper functions which
>>>>>>> writes a constant value to address with large dma
>>>>>>> access support.
>>>>>>>
>>>>>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>>>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>>>>>> ---
>>>>>>>  drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
>>>>>>>  include/linux/soc/mediatek/mtk-cmdq.h  | 14 ++++++++++++++
>>>>>>>  2 files changed, 40 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>>>> index 03c129230cd7..a9ebbabb7439 100644
>>>>>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>>>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>>>>  }
>>>>>>>  EXPORT_SYMBOL(cmdq_pkt_write_s);
>>>>>>>  
>>>>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>>>> +			   u16 addr_low, u32 value, u32 mask)
>>>>>>> +{
>>>>>>> +	struct cmdq_instruction inst = { {0} };
>>>>>>> +	int err;
>>>>>>> +
>>>>>>> +	if (mask != U32_MAX) {
>>>>>>> +		inst.op = CMDQ_CODE_MASK;
>>>>>>> +		inst.mask = ~mask;
>>>>>>> +		err = cmdq_pkt_append_command(pkt, inst);
>>>>>>> +		if (err < 0)
>>>>>>> +			return err;
>>>>>>> +
>>>>>>> +		inst.op = CMDQ_CODE_WRITE_S_MASK;
>>>>>>> +	} else {
>>>>>>> +		inst.op = CMDQ_CODE_WRITE_S;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	inst.sop = high_addr_reg_idx;
>>>>>>
>>>>>> Writing u16 value in a 5 bit wide variable?
>>>>>
>>>>> We need only 5 bits in this case. I'll change high_addr_reg_idx
>>>>> parameter to u8.
>>>>>
>>>>
>>>> Ok, please make sure to mask the value, so that it's explicit in the code that
>>>> we only use the lowest 5 bits of high_addr_reg_idx.
>>>
>>> Is it necessary to mask the value?
>>> Since sop already defined as "u8 sop:5;", I thought it is explicit that
>>> only use 5 bits and compiler should do the rest jobs.
>>
>> Yes but it makes the code more explicit if we have a
>> inst.sop = high_addr_reg_idx & 0x1f;
>>
>> What do you think?
> 
> The value assign to sop will restrict by hardware spec. Clients call
> this function will define constant value and use it as parameter. So I
> think we don't worry about client call this api with wrong value.
> 

Ok, then let's change the parameter to u8 and don't add a mask.

Regards,
Matthias