[PATCH v1 0/11] support cmdq helper function on mt6779 platform

[PATCH v1 0/11] support cmdq helper function on mt6779 platform

[Dennis YC Hsieh]

This patch support cmdq helper function on mt6779 platform,
based on "support gce on mt6779 platform" patchset.


Dennis YC Hsieh (11):
  soc: mediatek: cmdq: add address shift in jump
  soc: mediatek: cmdq: add assign function
  soc: mediatek: cmdq: add write_s function
  soc: mediatek: cmdq: add write_s_mask function
  soc: mediatek: cmdq: add read_s function
  soc: mediatek: cmdq: add write_s value function
  soc: mediatek: cmdq: add write_s_mask 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

 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  |   3 +-
 drivers/soc/mediatek/mtk-cmdq-helper.c   | 159 +++++++++++++++++++++--
 include/linux/mailbox/mtk-cmdq-mailbox.h |   8 +-
 include/linux/soc/mediatek/mtk-cmdq.h    | 124 +++++++++++++++++-
 4 files changed, 280 insertions(+), 14 deletions(-)

-- 
2.18.0

[PATCH v1 01/11] soc: mediatek: cmdq: add address shift in jump

[Dennis YC Hsieh]

Add address shift when compose jump instruction
to compatible with 35bit format.

Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index c67081759728..98f23ba3ba47 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -291,7 +291,8 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 
 	/* JUMP to end */
 	inst.op = CMDQ_CODE_JUMP;
-	inst.value = CMDQ_JUMP_PASS;
+	inst.value = CMDQ_JUMP_PASS >>
+		cmdq_mbox_shift(((struct cmdq_client *)pkt->cl)->chan);
 	err = cmdq_pkt_append_command(pkt, inst);
 
 	return err;
-- 
1.7.9.5

Re: [PATCH v1 01/11] soc: mediatek: cmdq: add address shift in jump

[Bibby Hsieh]

Reviewed-by: Bibby Hsieh <bibby.hsieh@mediatek.com>

Thanks.

On Sun, 2020-06-21 at 22:18 +0800, Dennis YC Hsieh wrote:
> Add address shift when compose jump instruction
> to compatible with 35bit format.
> 
> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index c67081759728..98f23ba3ba47 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -291,7 +291,8 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>  
>  	/* JUMP to end */
>  	inst.op = CMDQ_CODE_JUMP;
> -	inst.value = CMDQ_JUMP_PASS;
> +	inst.value = CMDQ_JUMP_PASS >>
> +		cmdq_mbox_shift(((struct cmdq_client *)pkt->cl)->chan);
>  	err = cmdq_pkt_append_command(pkt, inst);
>  
>  	return err;

[PATCH v1 02/11] soc: mediatek: cmdq: add assign function

[Dennis YC Hsieh]

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>
---
 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..bf32e3b2ca6c 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 src_t:1;
+			u8 dst_t:1;
+		};
 	};
-	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 = {};
+
+	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
-- 
1.7.9.5

Re: [PATCH v1 02/11] soc: mediatek: cmdq: add assign function

[Matthias Brugger]

On 21/06/2020 16:18, 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>

Applied to v5.8-next/soc

Thanks

> ---
>  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..bf32e3b2ca6c 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 src_t:1;
> +			u8 dst_t:1;
> +		};
>  	};
> -	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 = {};
> +
> +	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
> 

[PATCH v1 03/11] soc: mediatek: cmdq: add write_s function

[Dennis YC Hsieh]

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>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c   |   19 +++++++++++++++++++
 include/linux/mailbox/mtk-cmdq-mailbox.h |    1 +
 include/linux/soc/mediatek/mtk-cmdq.h    |   19 +++++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index bf32e3b2ca6c..817a5a97dbe5 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;
@@ -222,6 +226,21 @@ 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)
+{
+	struct cmdq_instruction inst = {};
+
+	inst.op = CMDQ_CODE_WRITE_S;
+	inst.src_t = CMDQ_REG_TYPE;
+	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..ee67dd3b86f5 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_WRITE_S = 0x90,
 	CMDQ_CODE_LOGIC = 0xa0,
 };
 
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 83340211e1d3..e1c5a7549b4f 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;
 
@@ -103,6 +105,23 @@ 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 register ID which contains high address of pa
+ * @addr_low:	low address of pa
+ * @src_reg_idx:	the CMDQ internal register ID which cache source value
+ *
+ * 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 address and call cmdq_pkt_assign() to assign value into internal
+ * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low 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);
+
+/**
  * 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"
-- 
1.7.9.5

Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function

[Matthias Brugger]

On 21/06/2020 16:18, 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>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c   |   19 +++++++++++++++++++
>  include/linux/mailbox/mtk-cmdq-mailbox.h |    1 +
>  include/linux/soc/mediatek/mtk-cmdq.h    |   19 +++++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index bf32e3b2ca6c..817a5a97dbe5 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;
> @@ -222,6 +226,21 @@ 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)
> +{

Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and
CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low
respectively?

In that case I think a better interface would be to pass the address and do the
high/low calculation in the cmdq_pkt_write_s

Regards,
Matthias

> +	struct cmdq_instruction inst = {};
> +
> +	inst.op = CMDQ_CODE_WRITE_S;
> +	inst.src_t = CMDQ_REG_TYPE;
> +	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..ee67dd3b86f5 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_WRITE_S = 0x90,
>  	CMDQ_CODE_LOGIC = 0xa0,
>  };
>  
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index 83340211e1d3..e1c5a7549b4f 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;
>  
> @@ -103,6 +105,23 @@ 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 register ID which contains high address of pa
> + * @addr_low:	low address of pa
> + * @src_reg_idx:	the CMDQ internal register ID which cache source value
> + *
> + * 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 address and call cmdq_pkt_assign() to assign value into internal
> + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low 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);
> +
> +/**
>   * 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"
> 

Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function

[Dennis-YC Hsieh]

Hi Matthias,

thanks for your comment.

On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote:
> 
> On 21/06/2020 16:18, 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>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c   |   19 +++++++++++++++++++
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |    1 +
> >  include/linux/soc/mediatek/mtk-cmdq.h    |   19 +++++++++++++++++++
> >  3 files changed, 39 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index bf32e3b2ca6c..817a5a97dbe5 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;
> > @@ -222,6 +226,21 @@ 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)
> > +{
> 
> Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and
> CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low
> respectively?
> 
> In that case I think a better interface would be to pass the address and do the
> high/low calculation in the cmdq_pkt_write_s

Not exactly. The high_addr_reg_idx parameter is index of internal
register (which store address bit[47:16]), not result of
CMDQ_ADDR_HIGH(addr). 

The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This
api helps assign address bit[47:16] into one of internal register by
index. And same index could be use in cmdq_pkt_write_s(). The gce
combine bit[47:16] in internal register and bit[15:0] in addr_low
parameter to final address. So it is better to keep interface in this
way.


Regards,
Dennis

> 
> Regards,
> Matthias
> 
> > +	struct cmdq_instruction inst = {};
> > +
> > +	inst.op = CMDQ_CODE_WRITE_S;
> > +	inst.src_t = CMDQ_REG_TYPE;
> > +	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..ee67dd3b86f5 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_WRITE_S = 0x90,
> >  	CMDQ_CODE_LOGIC = 0xa0,
> >  };
> >  
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 83340211e1d3..e1c5a7549b4f 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;
> >  
> > @@ -103,6 +105,23 @@ 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 register ID which contains high address of pa
> > + * @addr_low:	low address of pa
> > + * @src_reg_idx:	the CMDQ internal register ID which cache source value
> > + *
> > + * 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 address and call cmdq_pkt_assign() to assign value into internal
> > + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low 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);
> > +
> > +/**
> >   * 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"
> > 

Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function

[Matthias Brugger]

On 22/06/2020 17:36, Dennis-YC Hsieh wrote:
> Hi Matthias,
> 
> thanks for your comment.
> 
> On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote:
>>
>> On 21/06/2020 16:18, 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>
>>> ---
>>>  drivers/soc/mediatek/mtk-cmdq-helper.c   |   19 +++++++++++++++++++
>>>  include/linux/mailbox/mtk-cmdq-mailbox.h |    1 +
>>>  include/linux/soc/mediatek/mtk-cmdq.h    |   19 +++++++++++++++++++
>>>  3 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index bf32e3b2ca6c..817a5a97dbe5 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;
>>> @@ -222,6 +226,21 @@ 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)
>>> +{
>>
>> Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and
>> CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low
>> respectively?
>>
>> In that case I think a better interface would be to pass the address and do the
>> high/low calculation in the cmdq_pkt_write_s
> 
> Not exactly. The high_addr_reg_idx parameter is index of internal
> register (which store address bit[47:16]), not result of
> CMDQ_ADDR_HIGH(addr). 
> 
> The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This
> api helps assign address bit[47:16] into one of internal register by
> index. And same index could be use in cmdq_pkt_write_s(). The gce
> combine bit[47:16] in internal register and bit[15:0] in addr_low
> parameter to final address. So it is better to keep interface in this
> way.
> 

Got it, but then why don't we call cmdq_pkt_assign() in cmdq_pkt_write_s()? This
way we would get a clean API for what we want to do.
Do we expect other users of cmdq_pkt_assign()? Otherwise we could keep it
private the this file and don't export it.

By the way, why do you postfix the _s, I understand that it reflects the large
DMA access but I wonder why you choose '_s'.

Regards,
Matthias

> 
> Regards,
> Dennis
> 
>>
>> Regards,
>> Matthias
>>
>>> +	struct cmdq_instruction inst = {};
>>> +
>>> +	inst.op = CMDQ_CODE_WRITE_S;
>>> +	inst.src_t = CMDQ_REG_TYPE;
>>> +	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..ee67dd3b86f5 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_WRITE_S = 0x90,
>>>  	CMDQ_CODE_LOGIC = 0xa0,
>>>  };
>>>  
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index 83340211e1d3..e1c5a7549b4f 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;
>>>  
>>> @@ -103,6 +105,23 @@ 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 register ID which contains high address of pa
>>> + * @addr_low:	low address of pa
>>> + * @src_reg_idx:	the CMDQ internal register ID which cache source value
>>> + *
>>> + * 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 address and call cmdq_pkt_assign() to assign value into internal
>>> + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low 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);
>>> +
>>> +/**
>>>   * 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"
>>>
> 

Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function

[Dennis-YC Hsieh]

Hi Matthias,

On Mon, 2020-06-22 at 17:54 +0200, Matthias Brugger wrote:
> 
> On 22/06/2020 17:36, Dennis-YC Hsieh wrote:
> > Hi Matthias,
> > 
> > thanks for your comment.
> > 
> > On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote:
> >>
> >> On 21/06/2020 16:18, 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>
> >>> ---
> >>>  drivers/soc/mediatek/mtk-cmdq-helper.c   |   19 +++++++++++++++++++
> >>>  include/linux/mailbox/mtk-cmdq-mailbox.h |    1 +
> >>>  include/linux/soc/mediatek/mtk-cmdq.h    |   19 +++++++++++++++++++
> >>>  3 files changed, 39 insertions(+)
> >>>
> >>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>> index bf32e3b2ca6c..817a5a97dbe5 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;
> >>> @@ -222,6 +226,21 @@ 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)
> >>> +{
> >>
> >> Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and
> >> CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low
> >> respectively?
> >>
> >> In that case I think a better interface would be to pass the address and do the
> >> high/low calculation in the cmdq_pkt_write_s
> > 
> > Not exactly. The high_addr_reg_idx parameter is index of internal
> > register (which store address bit[47:16]), not result of
> > CMDQ_ADDR_HIGH(addr). 
> > 
> > The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This
> > api helps assign address bit[47:16] into one of internal register by
> > index. And same index could be use in cmdq_pkt_write_s(). The gce
> > combine bit[47:16] in internal register and bit[15:0] in addr_low
> > parameter to final address. So it is better to keep interface in this
> > way.
> > 
> 
> Got it, but then why don't we call cmdq_pkt_assign() in cmdq_pkt_write_s()? This
> way we would get a clean API for what we want to do.
> Do we expect other users of cmdq_pkt_assign()? Otherwise we could keep it
> private the this file and don't export it.

Considering this case: write 2 register 0xaabb00c0 0xaabb00d0.

If we call assign inside write_s api it will be:
assign aabb to internal reg 0
write reg 0 + 0x00c0
assign aabb to internal reg 0
write reg 0 + 0x00d0


But if we let client decide timing to call assign, it will be like:
assign aabb to internal reg 0
write reg 0 + 0x00c0
write reg 0 + 0x00d0


The first way uses 4 command and second one uses only 3 command.
Thus it is better to let client call assign explicitly.

> 
> By the way, why do you postfix the _s, I understand that it reflects the large
> DMA access but I wonder why you choose '_s'.
> 

The name of this command is "write_s" which is hardware spec.
I'm just following it since it is a common language between gce sw/hw
designers.


Regards,
Dennis

> Regards,
> Matthias
> 
> > 
> > Regards,
> > Dennis
> > 
> >>
> >> Regards,
> >> Matthias
> >>
> >>> +	struct cmdq_instruction inst = {};
> >>> +
> >>> +	inst.op = CMDQ_CODE_WRITE_S;
> >>> +	inst.src_t = CMDQ_REG_TYPE;
> >>> +	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..ee67dd3b86f5 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_WRITE_S = 0x90,
> >>>  	CMDQ_CODE_LOGIC = 0xa0,
> >>>  };
> >>>  
> >>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> >>> index 83340211e1d3..e1c5a7549b4f 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;
> >>>  
> >>> @@ -103,6 +105,23 @@ 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 register ID which contains high address of pa
> >>> + * @addr_low:	low address of pa
> >>> + * @src_reg_idx:	the CMDQ internal register ID which cache source value
> >>> + *
> >>> + * 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 address and call cmdq_pkt_assign() to assign value into internal
> >>> + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low 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);
> >>> +
> >>> +/**
> >>>   * 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"
> >>>
> > 

Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function

[Matthias Brugger]

On 22/06/2020 18:12, Dennis-YC Hsieh wrote:
> Hi Matthias,
> 
> On Mon, 2020-06-22 at 17:54 +0200, Matthias Brugger wrote:
>>
>> On 22/06/2020 17:36, Dennis-YC Hsieh wrote:
>>> Hi Matthias,
>>>
>>> thanks for your comment.
>>>
>>> On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote:
>>>>
>>>> On 21/06/2020 16:18, 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>
>>>>> ---
>>>>>  drivers/soc/mediatek/mtk-cmdq-helper.c   |   19 +++++++++++++++++++
>>>>>  include/linux/mailbox/mtk-cmdq-mailbox.h |    1 +
>>>>>  include/linux/soc/mediatek/mtk-cmdq.h    |   19 +++++++++++++++++++
>>>>>  3 files changed, 39 insertions(+)
>>>>>
>>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>> index bf32e3b2ca6c..817a5a97dbe5 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;
>>>>> @@ -222,6 +226,21 @@ 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)
>>>>> +{
>>>>
>>>> Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and
>>>> CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low
>>>> respectively?
>>>>
>>>> In that case I think a better interface would be to pass the address and do the
>>>> high/low calculation in the cmdq_pkt_write_s
>>>
>>> Not exactly. The high_addr_reg_idx parameter is index of internal
>>> register (which store address bit[47:16]), not result of
>>> CMDQ_ADDR_HIGH(addr). 
>>>
>>> The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This
>>> api helps assign address bit[47:16] into one of internal register by
>>> index. And same index could be use in cmdq_pkt_write_s(). The gce
>>> combine bit[47:16] in internal register and bit[15:0] in addr_low
>>> parameter to final address. So it is better to keep interface in this
>>> way.
>>>
>>
>> Got it, but then why don't we call cmdq_pkt_assign() in cmdq_pkt_write_s()? This
>> way we would get a clean API for what we want to do.
>> Do we expect other users of cmdq_pkt_assign()? Otherwise we could keep it
>> private the this file and don't export it.
> 
> Considering this case: write 2 register 0xaabb00c0 0xaabb00d0.
> 
> If we call assign inside write_s api it will be:
> assign aabb to internal reg 0
> write reg 0 + 0x00c0
> assign aabb to internal reg 0
> write reg 0 + 0x00d0
> 
> 
> But if we let client decide timing to call assign, it will be like:
> assign aabb to internal reg 0
> write reg 0 + 0x00c0
> write reg 0 + 0x00d0
> 

Ok, thanks for clarification. Is this something you exepect to see in the gce
consumer driver?

> 
> The first way uses 4 command and second one uses only 3 command.
> Thus it is better to let client call assign explicitly.
> 
>>
>> By the way, why do you postfix the _s, I understand that it reflects the large
>> DMA access but I wonder why you choose '_s'.
>>
> 
> The name of this command is "write_s" which is hardware spec.
> I'm just following it since it is a common language between gce sw/hw
> designers.
> 

Ok, I will probably have to look that up every time have a look at the driver,
but that's OK.

Regards,
Matthias

> 
> Regards,
> Dennis
> 
>> Regards,
>> Matthias
>>
>>>
>>> Regards,
>>> Dennis
>>>
>>>>
>>>> Regards,
>>>> Matthias
>>>>
>>>>> +	struct cmdq_instruction inst = {};
>>>>> +
>>>>> +	inst.op = CMDQ_CODE_WRITE_S;
>>>>> +	inst.src_t = CMDQ_REG_TYPE;
>>>>> +	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..ee67dd3b86f5 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_WRITE_S = 0x90,
>>>>>  	CMDQ_CODE_LOGIC = 0xa0,
>>>>>  };
>>>>>  
>>>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>>>> index 83340211e1d3..e1c5a7549b4f 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;
>>>>>  
>>>>> @@ -103,6 +105,23 @@ 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 register ID which contains high address of pa
>>>>> + * @addr_low:	low address of pa
>>>>> + * @src_reg_idx:	the CMDQ internal register ID which cache source value
>>>>> + *
>>>>> + * 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 address and call cmdq_pkt_assign() to assign value into internal
>>>>> + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low 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);
>>>>> +
>>>>> +/**
>>>>>   * 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"
>>>>>
>>>
> 

Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function

[Dennis-YC Hsieh]

Hi Matthias,


On Mon, 2020-06-22 at 19:08 +0200, Matthias Brugger wrote:
> 
> On 22/06/2020 18:12, Dennis-YC Hsieh wrote:
> > Hi Matthias,
> > 
> > On Mon, 2020-06-22 at 17:54 +0200, Matthias Brugger wrote:
> >>
> >> On 22/06/2020 17:36, Dennis-YC Hsieh wrote:
> >>> Hi Matthias,
> >>>
> >>> thanks for your comment.
> >>>
> >>> On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote:
> >>>>
> >>>> On 21/06/2020 16:18, 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>
> >>>>> ---
> >>>>>  drivers/soc/mediatek/mtk-cmdq-helper.c   |   19 +++++++++++++++++++
> >>>>>  include/linux/mailbox/mtk-cmdq-mailbox.h |    1 +
> >>>>>  include/linux/soc/mediatek/mtk-cmdq.h    |   19 +++++++++++++++++++
> >>>>>  3 files changed, 39 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> >>>>> index bf32e3b2ca6c..817a5a97dbe5 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;
> >>>>> @@ -222,6 +226,21 @@ 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)
> >>>>> +{
> >>>>
> >>>> Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and
> >>>> CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low
> >>>> respectively?
> >>>>
> >>>> In that case I think a better interface would be to pass the address and do the
> >>>> high/low calculation in the cmdq_pkt_write_s
> >>>
> >>> Not exactly. The high_addr_reg_idx parameter is index of internal
> >>> register (which store address bit[47:16]), not result of
> >>> CMDQ_ADDR_HIGH(addr). 
> >>>
> >>> The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This
> >>> api helps assign address bit[47:16] into one of internal register by
> >>> index. And same index could be use in cmdq_pkt_write_s(). The gce
> >>> combine bit[47:16] in internal register and bit[15:0] in addr_low
> >>> parameter to final address. So it is better to keep interface in this
> >>> way.
> >>>
> >>
> >> Got it, but then why don't we call cmdq_pkt_assign() in cmdq_pkt_write_s()? This
> >> way we would get a clean API for what we want to do.
> >> Do we expect other users of cmdq_pkt_assign()? Otherwise we could keep it
> >> private the this file and don't export it.
> > 
> > Considering this case: write 2 register 0xaabb00c0 0xaabb00d0.
> > 
> > If we call assign inside write_s api it will be:
> > assign aabb to internal reg 0
> > write reg 0 + 0x00c0
> > assign aabb to internal reg 0
> > write reg 0 + 0x00d0
> > 
> > 
> > But if we let client decide timing to call assign, it will be like:
> > assign aabb to internal reg 0
> > write reg 0 + 0x00c0
> > write reg 0 + 0x00d0
> > 
> 
> Ok, thanks for clarification. Is this something you exepect to see in the gce
> consumer driver?
> 

yes it is, less command means better performance and save memory, so it
is a good practice for consumer.

> > 
> > The first way uses 4 command and second one uses only 3 command.
> > Thus it is better to let client call assign explicitly.
> > 
> >>
> >> By the way, why do you postfix the _s, I understand that it reflects the large
> >> DMA access but I wonder why you choose '_s'.
> >>
> > 
> > The name of this command is "write_s" which is hardware spec.
> > I'm just following it since it is a common language between gce sw/hw
> > designers.
> > 
> 
> Ok, I will probably have to look that up every time have a look at the driver,
> but that's OK.
> 

ok thanks for your comment



Regards,
Dennis

> Regards,
> Matthias
> 
> > 
> > Regards,
> > Dennis
> > 
> >> Regards,
> >> Matthias
> >>
> >>>
> >>> Regards,
> >>> Dennis
> >>>
> >>>>
> >>>> Regards,
> >>>> Matthias
> >>>>
> >>>>> +	struct cmdq_instruction inst = {};
> >>>>> +
> >>>>> +	inst.op = CMDQ_CODE_WRITE_S;
> >>>>> +	inst.src_t = CMDQ_REG_TYPE;
> >>>>> +	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..ee67dd3b86f5 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_WRITE_S = 0x90,
> >>>>>  	CMDQ_CODE_LOGIC = 0xa0,
> >>>>>  };
> >>>>>  
> >>>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> >>>>> index 83340211e1d3..e1c5a7549b4f 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;
> >>>>>  
> >>>>> @@ -103,6 +105,23 @@ 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 register ID which contains high address of pa
> >>>>> + * @addr_low:	low address of pa
> >>>>> + * @src_reg_idx:	the CMDQ internal register ID which cache source value
> >>>>> + *
> >>>>> + * 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 address and call cmdq_pkt_assign() to assign value into internal
> >>>>> + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low 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);
> >>>>> +
> >>>>> +/**
> >>>>>   * 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"
> >>>>>
> >>>
> > 

[PATCH v1 04/11] soc: mediatek: cmdq: add write_s_mask function

[Dennis YC Hsieh]

add write_s_mask function in cmdq helper functions which
writes value contains in internal register to address
with mask and large dma access support.

Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c   |   23 +++++++++++++++++++++++
 include/linux/mailbox/mtk-cmdq-mailbox.h |    1 +
 include/linux/soc/mediatek/mtk-cmdq.h    |   18 ++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 817a5a97dbe5..13b888779093 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -241,6 +241,29 @@ 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_mask(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
+			  u16 addr_low, u16 src_reg_idx, u32 mask)
+{
+	struct cmdq_instruction inst = {};
+	int err;
+
+	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;
+	inst.src_t = CMDQ_REG_TYPE;
+	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_mask);
+
 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 ee67dd3b86f5..8ef87e1bd03b 100644
--- a/include/linux/mailbox/mtk-cmdq-mailbox.h
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -60,6 +60,7 @@ enum cmdq_code {
 	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 e1c5a7549b4f..ca9c75fd8125 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -122,6 +122,24 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
 		     u16 addr_low, u16 src_reg_idx);
 
 /**
+ * cmdq_pkt_write_s_mask() - append write_s with mask command to the CMDQ packet
+ * @pkt:	the CMDQ packet
+ * @high_addr_reg_idx:	internal register ID which contains high address of pa
+ * @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 address and call cmdq_pkt_assign() to assign value into internal
+ * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameter when
+ * call to this function.
+ */
+int cmdq_pkt_write_s_mask(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
+			  u16 addr_low, u16 src_reg_idx, u32 mask);
+
+/**
  * 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"
-- 
1.7.9.5

[PATCH v1 05/11] soc: mediatek: cmdq: add read_s function

[Dennis YC Hsieh]

Add read_s function in cmdq helper functions which support read value from
register or dma physical address into gce internal register.

Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@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    |   12 ++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 13b888779093..58075589509b 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -226,6 +226,21 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
 }
 EXPORT_SYMBOL(cmdq_pkt_write_mask);
 
+int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
+		    u16 reg_idx)
+{
+	struct cmdq_instruction inst = {};
+
+	inst.op = CMDQ_CODE_READ_S;
+	inst.dst_t = CMDQ_REG_TYPE;
+	inst.sop = high_addr_reg_idx;
+	inst.reg_dst = reg_idx;
+	inst.src_reg = addr_low;
+
+	return cmdq_pkt_append_command(pkt, inst);
+}
+EXPORT_SYMBOL(cmdq_pkt_read_s);
+
 int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
 		     u16 addr_low, u16 src_reg_idx)
 {
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index 8ef87e1bd03b..3f6bc0dfd5da 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_READ_S = 0x80,
 	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 ca9c75fd8125..40fe1eb52190 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -104,6 +104,18 @@ struct cmdq_client *cmdq_mbox_create(struct device *dev, int index,
 int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
 			u16 offset, u32 value, u32 mask);
 
+/*
+ * cmdq_pkt_read_s() - append read_s command to the CMDQ packet
+ * @pkt:	the CMDQ packet
+ * @high_addr_reg_idx:	internal register ID which contains high address of pa
+ * @addr_low:	low address of pa
+ * @reg_idx:	the CMDQ internal register ID to cache read data
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
+		    u16 reg_idx);
+
 /**
  * cmdq_pkt_write_s() - append write_s command to the CMDQ packet
  * @pkt:	the CMDQ packet
-- 
1.7.9.5

[PATCH v1 06/11] soc: mediatek: cmdq: add write_s value function

[Dennis YC Hsieh]

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

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 58075589509b..2ad78df46636 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -279,6 +279,20 @@ int cmdq_pkt_write_s_mask(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
 }
 EXPORT_SYMBOL(cmdq_pkt_write_s_mask);
 
+int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
+			   u16 addr_low, u32 value)
+{
+	struct cmdq_instruction inst = {};
+
+	inst.op = CMDQ_CODE_WRITE_S;
+	inst.sop = high_addr_reg_idx;
+	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 40fe1eb52190..7f1c115a66b8 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -152,6 +152,19 @@ int cmdq_pkt_write_s_mask(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 to the CMDQ packet which
+ *			      write value to a physical address
+ * @pkt:	the CMDQ packet
+ * @high_addr_reg_idx:	internal register ID which contains high address of pa
+ * @addr_low:	low address of pa
+ * @value:	the specified target value
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
+			   u16 addr_low, u32 value);
+
+/**
  * 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"
-- 
1.7.9.5

[PATCH v1 07/11] soc: mediatek: cmdq: add write_s_mask value function

[Dennis YC Hsieh]

add write_s_mask_value function in cmdq helper functions which
writes a constant value to address with mask and large dma
access support.

Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c |   21 +++++++++++++++++++++
 include/linux/soc/mediatek/mtk-cmdq.h  |   15 +++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 2ad78df46636..e372ae065240 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -293,6 +293,27 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 }
 EXPORT_SYMBOL(cmdq_pkt_write_s_value);
 
+int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
+				u16 addr_low, u32 value, u32 mask)
+{
+	struct cmdq_instruction inst = {};
+	int err;
+
+	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;
+	inst.sop = high_addr_reg_idx;
+	inst.offset = addr_low;
+	inst.value = value;
+
+	return cmdq_pkt_append_command(pkt, inst);
+}
+EXPORT_SYMBOL(cmdq_pkt_write_s_mask_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 7f1c115a66b8..6e8caacedc80 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -165,6 +165,21 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 			   u16 addr_low, u32 value);
 
 /**
+ * cmdq_pkt_write_s_mask_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 register ID which contains high address of pa
+ * @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_mask_value(struct cmdq_pkt *pkt, u8 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
  * @event:	the desired event type to "wait and CLEAR"
-- 
1.7.9.5

[PATCH v1 08/11] soc: mediatek: cmdq: export finalize function

[Dennis YC Hsieh]

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>
Acked-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 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
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index e372ae065240..248945108a36 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -391,7 +391,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;
@@ -411,6 +411,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)
 {
@@ -445,10 +446,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 6e8caacedc80..eac1405e4872 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -244,6 +244,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
  * @pkt:	the CMDQ packet
-- 
1.7.9.5

Re: [PATCH v1 08/11] soc: mediatek: cmdq: export finalize function

[Matthias Brugger]

On 21/06/2020 16:18, 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>
> Acked-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> ---
>  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(-)
> 


Applied to v5.8-next/soc

Thanks!

> 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
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index e372ae065240..248945108a36 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -391,7 +391,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;
> @@ -411,6 +411,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)
>  {
> @@ -445,10 +446,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 6e8caacedc80..eac1405e4872 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -244,6 +244,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
>   * @pkt:	the CMDQ packet
> 

[PATCH v1 09/11] soc: mediatek: cmdq: add jump function

[Dennis YC Hsieh]

Add jump function so that client can jump to any address which
contains instruction.

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 248945108a36..009f86ae72c6 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 {
@@ -391,6 +392,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 = {};
+
+	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 eac1405e4872..18364d81e8f7 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -244,6 +244,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
  *
-- 
1.7.9.5

[PATCH v1 10/11] soc: mediatek: cmdq: add clear option in cmdq_pkt_wfe api

[Dennis YC Hsieh]

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>
Reviewed-by: CK Hu <ck.hu@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 009f86ae72c6..13f78c9b5901 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -315,15 +315,16 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 }
 EXPORT_SYMBOL(cmdq_pkt_write_s_mask_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 18364d81e8f7..4b5f5d154bad 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -182,11 +182,12 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 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
-- 
1.7.9.5

Re: [PATCH v1 10/11] soc: mediatek: cmdq: add clear option in cmdq_pkt_wfe api

[Matthias Brugger]

On 21/06/2020 16:18, 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>
> Reviewed-by: CK Hu <ck.hu@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);

This does not set CMDQ_WFE_UPDATE while the old code did. Is this a bug fix or a
bug in the code?
If it's a fix, please provide a fixes tag.

Thanks,
Matthias

>  		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 009f86ae72c6..13f78c9b5901 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -315,15 +315,16 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
>  }
>  EXPORT_SYMBOL(cmdq_pkt_write_s_mask_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 18364d81e8f7..4b5f5d154bad 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -182,11 +182,12 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 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
> 

Re: [PATCH v1 10/11] soc: mediatek: cmdq: add clear option in cmdq_pkt_wfe api

[Dennis-YC Hsieh]

Hi Matthias,

thanks for your comment.

On Mon, 2020-06-22 at 13:19 +0200, Matthias Brugger wrote:
> 
> On 21/06/2020 16:18, 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>
> > Reviewed-by: CK Hu <ck.hu@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);
> 
> This does not set CMDQ_WFE_UPDATE while the old code did. Is this a bug fix or a
> bug in the code?
> If it's a fix, please provide a fixes tag.

no need to to update again since event always clear before wait.
I'll provide a fix tag, thanks.


Regards,
Dennis

> 
> Thanks,
> Matthias
> 
> >  		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 009f86ae72c6..13f78c9b5901 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -315,15 +315,16 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_write_s_mask_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 18364d81e8f7..4b5f5d154bad 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -182,11 +182,12 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 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
> > 

[PATCH v1 11/11] soc: mediatek: cmdq: add set event function

[Dennis YC Hsieh]

Add set event function in cmdq helper functions to set specific event.

Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@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 13f78c9b5901..e6133a42d229 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -346,6 +346,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 = {};
+
+	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 4b5f5d154bad..960704d75994 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -199,6 +199,15 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 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
+ *
+ * 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
  *		     hardware register to check for the value w/o mask.
-- 
1.7.9.5

Re: [PATCH v1 11/11] soc: mediatek: cmdq: add set event function

[Matthias Brugger]

On 21/06/2020 16:18, 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>
> ---
>  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(+)
> 

Applied to v5.8-next/soc

Thanks!

> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 13f78c9b5901..e6133a42d229 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -346,6 +346,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 = {};
> +
> +	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 4b5f5d154bad..960704d75994 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -199,6 +199,15 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
>  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
> + *
> + * 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
>   *		     hardware register to check for the value w/o mask.
> 

Re: [PATCH v1 0/11] support cmdq helper function on mt6779 platform

[Bibby Hsieh]

Hi, Dennis,

Please add "depends on patch: support gce on mt6779 platform" in cover
letter. Thanks

Bibby

On Sun, 2020-06-21 at 22:18 +0800, Dennis YC Hsieh wrote:
> This patch support cmdq helper function on mt6779 platform,
> based on "support gce on mt6779 platform" patchset.
> 
> 
> Dennis YC Hsieh (11):
>   soc: mediatek: cmdq: add address shift in jump
>   soc: mediatek: cmdq: add assign function
>   soc: mediatek: cmdq: add write_s function
>   soc: mediatek: cmdq: add write_s_mask function
>   soc: mediatek: cmdq: add read_s function
>   soc: mediatek: cmdq: add write_s value function
>   soc: mediatek: cmdq: add write_s_mask 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
> 
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  |   3 +-
>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 159 +++++++++++++++++++++--
>  include/linux/mailbox/mtk-cmdq-mailbox.h |   8 +-
>  include/linux/soc/mediatek/mtk-cmdq.h    | 124 +++++++++++++++++-
>  4 files changed, 280 insertions(+), 14 deletions(-)
> 

Re: [PATCH v1 0/11] support cmdq helper function on mt6779 platform

[Dennis-YC Hsieh]

Hi Bibby,


On Mon, 2020-06-22 at 10:40 +0800, Bibby Hsieh wrote:
> Hi, Dennis,
> 
> Please add "depends on patch: support gce on mt6779 platform" in cover
> letter. Thanks

ok will do, thanks


Regards,
Dennis


> 
> Bibby
> 
> On Sun, 2020-06-21 at 22:18 +0800, Dennis YC Hsieh wrote:
> > This patch support cmdq helper function on mt6779 platform,
> > based on "support gce on mt6779 platform" patchset.
> > 
> > 
> > Dennis YC Hsieh (11):
> >   soc: mediatek: cmdq: add address shift in jump
> >   soc: mediatek: cmdq: add assign function
> >   soc: mediatek: cmdq: add write_s function
> >   soc: mediatek: cmdq: add write_s_mask function
> >   soc: mediatek: cmdq: add read_s function
> >   soc: mediatek: cmdq: add write_s value function
> >   soc: mediatek: cmdq: add write_s_mask 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
> > 
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  |   3 +-
> >  drivers/soc/mediatek/mtk-cmdq-helper.c   | 159 +++++++++++++++++++++--
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |   8 +-
> >  include/linux/soc/mediatek/mtk-cmdq.h    | 124 +++++++++++++++++-
> >  4 files changed, 280 insertions(+), 14 deletions(-)
> > 
> 
>