[RESEND, PATCH 0/5] Add CMDQ API for upcoming ISP feature

[RESEND, PATCH 0/5] Add CMDQ API for upcoming ISP feature

[Jason-JH.Lin]

From: Jason-jh Lin <jason-jh.lin@mediatek.corp-partner.google.com>

In order to support the upcoming ISP functions, some CMDQ APIs
need to be prepared:
- cmdq_pkt_mem_move(): for memory or register vaule copy
- cmdq_pkt_poll_addr(): extending cmdq_pkt_poll() to support polling
                        address of register 
- cmdq_pkt_acquire_event(): to support MUTEX_LOCK protection between
                            GCE threads
- cmdq_get_event()/cmdq_set_event(): to support ISP driver runtime control
                                     some GCE events.


Change in RESEND v1:
1. Remove Change-Id in commit message.

Jason-JH.Lin (5):
  soc: mediatek: mtk-cmdq: Add specific purpose register definitions for
    GCE
  soc: mediatek: mtk-cmdq: Add cmdq_pkt_mem_move() function
  soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function
  soc: mediatek: mtk-cmdq: Add cmdq_pkt_acquire_event() function
  mailbox: mtk-cmdq: Add support runtime get and set GCE event

 drivers/mailbox/mtk-cmdq-mailbox.c       | 37 +++++++++++
 drivers/soc/mediatek/mtk-cmdq-helper.c   | 79 ++++++++++++++++++++++++
 include/linux/mailbox/mtk-cmdq-mailbox.h |  2 +
 include/linux/soc/mediatek/mtk-cmdq.h    | 44 +++++++++++++
 4 files changed, 162 insertions(+)

-- 
2.18.0

[RESEND, PATCH 1/5] soc: mediatek: mtk-cmdq: Add specific purpose register definitions for GCE

[Jason-JH.Lin]

Add specific purpose register definitions for GCE, so CMDQ users can
use them as a buffer to store data.

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

diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 649955d2cf5c..1dae80185f9f 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -14,6 +14,15 @@
 #define CMDQ_ADDR_HIGH(addr)	((u32)(((addr) >> 16) & GENMASK(31, 0)))
 #define CMDQ_ADDR_LOW(addr)	((u16)(addr) | BIT(1))
 
+/*
+ * Every cmdq thread has its own SPRs (Specific Purpose Registers),
+ * so there are 4 * N (threads) SPRs in GCE that shares the same indexes below.
+ */
+#define CMDQ_THR_SPR_IDX0	(0)
+#define CMDQ_THR_SPR_IDX1	(1)
+#define CMDQ_THR_SPR_IDX2	(2)
+#define CMDQ_THR_SPR_IDX3	(3)
+
 struct cmdq_pkt;
 
 struct cmdq_client_reg {
-- 
2.18.0

[RESEND, PATCH 2/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_mem_move() function

[Jason-JH.Lin]

Add cmdq_pkt_mem_move() function to support CMDQ user making
an instruction for moving a value from a source address to a
destination address.

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

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index b0cd071c4719..3a1e47ad8a41 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -299,6 +299,32 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 }
 EXPORT_SYMBOL(cmdq_pkt_write_s_mask_value);
 
+s32 cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr, dma_addr_t dst_addr)
+{
+	s32 err;
+	const u16 tmp_reg_idx = CMDQ_THR_SPR_IDX0;
+	const u16 swap_reg_idx = CMDQ_THR_SPR_IDX1;
+
+	/* read the value of src_addr into swap_reg_idx */
+	err = cmdq_pkt_assign(pkt, tmp_reg_idx, CMDQ_ADDR_HIGH(src_addr));
+	if (err < 0)
+		return err;
+	err = cmdq_pkt_read_s(pkt, tmp_reg_idx, CMDQ_ADDR_LOW(src_addr), swap_reg_idx);
+	if (err < 0)
+		return err;
+
+	/* write the value of swap_reg_idx into dst_addr */
+	err = cmdq_pkt_assign(pkt, tmp_reg_idx, CMDQ_ADDR_HIGH(dst_addr));
+	if (err < 0)
+		return err;
+	err = cmdq_pkt_write_s(pkt, tmp_reg_idx, CMDQ_ADDR_LOW(dst_addr), swap_reg_idx);
+	if (err < 0)
+		return err;
+
+	return err;
+}
+EXPORT_SYMBOL(cmdq_pkt_mem_move);
+
 int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
 {
 	struct cmdq_instruction inst = { {0} };
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 1dae80185f9f..b6dbe2d8f16a 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -182,6 +182,16 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
 				u16 addr_low, u32 value, u32 mask);
 
+/**
+ * cmdq_pkt_mem_move() - append memory move command to the CMDQ packet
+ * @pkt:	the CMDQ packet
+ * @src_addr:	source address
+ * @dma_addr_t: destination address
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr, dma_addr_t dst_addr);
+
 /**
  * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
  * @pkt:	the CMDQ packet
-- 
2.18.0

[RESEND, PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function

[Jason-JH.Lin]

Add cmdq_pkt_poll_addr function to support CMDQ user making
an instruction for polling a specific address of hardware rigster
to check the value with or without mask.

POLL is an old operation in GCE, so it does not support SPR and
CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
to move polling register address to GPR to make an instruction.
This will be done in cmdq_pkt_poll_addr().

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

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 3a1e47ad8a41..2e9fc9bb1183 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_POLL_HIGH_ADDR_GPR	(14)
 #define CMDQ_EOC_IRQ_EN		BIT(0)
 #define CMDQ_REG_TYPE		1
 #define CMDQ_JUMP_RELATIVE	1
@@ -406,6 +407,43 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
 }
 EXPORT_SYMBOL(cmdq_pkt_poll_mask);
 
+int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32 value, u32 mask)
+{
+	struct cmdq_instruction inst = { {0} };
+	int err;
+	u8 use_mask = 0;
+
+	if (mask != U32_MAX) {
+		inst.op = CMDQ_CODE_MASK;
+		inst.mask = ~mask;
+		err = cmdq_pkt_append_command(pkt, inst);
+		if (err != 0)
+			return err;
+		use_mask = CMDQ_POLL_ENABLE_MASK;
+	}
+
+	/*
+	 * POLL is an old operation in GCE and it does not support SPR and CMDQ_CODE_LOGIC,
+	 * so it can not use cmdq_pkt_assign to keep polling register address to SPR.
+	 * It needs to use GPR and CMDQ_CODE_MASK to move polling register address to GPR.
+	 */
+	inst.op = CMDQ_CODE_MASK;
+	inst.dst_t = CMDQ_REG_TYPE;
+	inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
+	inst.mask = addr;
+	err = cmdq_pkt_append_command(pkt, inst);
+	if (err < 0)
+		return err;
+
+	inst.op = CMDQ_CODE_POLL;
+	inst.dst_t = CMDQ_REG_TYPE;
+	inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
+	inst.offset = use_mask;
+	inst.value = value;
+	return cmdq_pkt_append_command(pkt, inst);
+}
+EXPORT_SYMBOL(cmdq_pkt_poll_addr);
+
 int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
 {
 	struct cmdq_instruction inst = {};
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index b6dbe2d8f16a..2fe9be240fbc 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -253,6 +253,22 @@ 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_poll_addr() - Append polling command to the CMDQ packet, ask GCE to
+ *			 execute an instruction that wait for a specified
+ *			 address of hardware register to check for the value
+ *			 w/ or w/o mask.
+ *			 All GCE hardware threads will be blocked by this
+ *			 instruction.
+ * @pkt:	the CMDQ packet
+ * @addr:	the hardware register address
+ * @value:	the specified target register value
+ * @mask:	the specified target register mask
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, 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
-- 
2.18.0

[RESEND, PATCH 4/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_acquire_event() function

[Jason-JH.Lin]

Add cmdq_pkt_acquire_event() function to support CMDQ user making
an instruction for acquiring event.

CMDQ users can use cmdq_pkt_acquire_event() and cmdq_pkt_clear_event()
to acquire GCE event and release GCE event and achieve the MUTEX_LOCK
protection between GCE threads.

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

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 2e9fc9bb1183..0183b40a0eff 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -342,6 +342,21 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
 }
 EXPORT_SYMBOL(cmdq_pkt_wfe);
 
+int cmdq_pkt_acquire_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 | CMDQ_WFE_WAIT;
+	inst.event = event;
+
+	return cmdq_pkt_append_command(pkt, inst);
+}
+EXPORT_SYMBOL(cmdq_pkt_acquire_event);
+
 int cmdq_pkt_clear_event(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 2fe9be240fbc..de93c0a8e8a9 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -202,6 +202,15 @@ int cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr, dma_addr_t dst_
  */
 int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
 
+/**
+ * cmdq_pkt_acquire_event() - append acquire event command to the CMDQ packet
+ * @pkt:	the CMDQ packet
+ * @event:	the desired event to be acquired
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event);
+
 /**
  * cmdq_pkt_clear_event() - append clear event command to the CMDQ packet
  * @pkt:	the CMDQ packet
-- 
2.18.0

[RESEND, PATCH 5/5] mailbox: mtk-cmdq: Add support runtime get and set GCE event

[Jason-JH.Lin]

ISP drivers need to get and set GCE event in their runtime contorl flow.
So add these functions to support get and set GCE by CPU.

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

diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index ead2200f39ba..d7c08249c898 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -25,7 +25,11 @@
 #define CMDQ_GCE_NUM_MAX		(2)
 
 #define CMDQ_CURR_IRQ_STATUS		0x10
+#define CMDQ_SYNC_TOKEN_ID		0x60
+#define CMDQ_SYNC_TOKEN_VALUE		0x64
+#define CMDQ_TOKEN_ID_MASK			GENMASK(9, 0)
 #define CMDQ_SYNC_TOKEN_UPDATE		0x68
+#define CMDQ_TOKEN_UPDATE_VALUE			BIT(16)
 #define CMDQ_THR_SLOT_CYCLES		0x30
 #define CMDQ_THR_BASE			0x100
 #define CMDQ_THR_SIZE			0x80
@@ -83,6 +87,7 @@ struct cmdq {
 	struct cmdq_thread	*thread;
 	struct clk_bulk_data	clocks[CMDQ_GCE_NUM_MAX];
 	bool			suspended;
+	spinlock_t		event_lock; /* lock for gce event */
 };
 
 struct gce_plat {
@@ -113,6 +118,38 @@ u8 cmdq_get_shift_pa(struct mbox_chan *chan)
 }
 EXPORT_SYMBOL(cmdq_get_shift_pa);
 
+void cmdq_set_event(void *chan, u16 event_id)
+{
+	struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)->mbox,
+		typeof(*cmdq), mbox);
+	unsigned long flags;
+
+	spin_lock_irqsave(&cmdq->event_lock, flags);
+
+	writel(CMDQ_TOKEN_UPDATE_VALUE | event_id, cmdq->base + CMDQ_SYNC_TOKEN_UPDATE);
+
+	spin_unlock_irqrestore(&cmdq->event_lock, flags);
+}
+EXPORT_SYMBOL(cmdq_set_event);
+
+u32 cmdq_get_event(void *chan, u16 event_id)
+{
+	struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)->mbox,
+		typeof(*cmdq), mbox);
+	unsigned long flags;
+	u32 value = 0;
+
+	spin_lock_irqsave(&cmdq->event_lock, flags);
+
+	writel(CMDQ_TOKEN_ID_MASK & event_id, cmdq->base + CMDQ_SYNC_TOKEN_ID);
+	value = readl(cmdq->base + CMDQ_SYNC_TOKEN_VALUE);
+
+	spin_unlock_irqrestore(&cmdq->event_lock, flags);
+
+	return value;
+}
+EXPORT_SYMBOL(cmdq_get_event);
+
 static int cmdq_thread_suspend(struct cmdq *cmdq, struct cmdq_thread *thread)
 {
 	u32 status;
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index a8f0070c7aa9..f05cabd230da 100644
--- a/include/linux/mailbox/mtk-cmdq-mailbox.h
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -79,5 +79,7 @@ struct cmdq_pkt {
 };
 
 u8 cmdq_get_shift_pa(struct mbox_chan *chan);
+void cmdq_set_event(void *chan, u16 event_id);
+u32 cmdq_get_event(void *chan, u16 event_id);
 
 #endif /* __MTK_CMDQ_MAILBOX_H__ */
-- 
2.18.0

Re: [RESEND, PATCH 4/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_acquire_event() function

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> Add cmdq_pkt_acquire_event() function to support CMDQ user making
> an instruction for acquiring event.
> 
> CMDQ users can use cmdq_pkt_acquire_event() and
> cmdq_pkt_clear_event()
> to acquire GCE event and release GCE event and achieve the MUTEX_LOCK
> protection between GCE threads.

I'm not clear what acquire do in detail. This is what I guess:

cmdq_pkt_acquire_event() would wait for event to be cleared. After
event is cleared, cmdq_pkt_acquire_event() would set event and keep
executing next command. So the mutex would work like this

cmdq_pkt_acquire_event() /* mutex lock */

/* critical secton */

cmdq_pkt_clear_event() /* mutex unlock */

If it's so, describe as detail as this. If not, describe how it do.

As I know, GCE is single core, so multiple thread is served by single
GCE, why need mutex lock?

Regards,
CK

> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 15 +++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  |  9 +++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 2e9fc9bb1183..0183b40a0eff 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -342,6 +342,21 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16
> event, bool clear)
>  }
>  EXPORT_SYMBOL(cmdq_pkt_wfe);
>  
> +int cmdq_pkt_acquire_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 |
> CMDQ_WFE_WAIT;
> +	inst.event = event;
> +
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_acquire_event);
> +
>  int cmdq_pkt_clear_event(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 2fe9be240fbc..de93c0a8e8a9 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -202,6 +202,15 @@ int cmdq_pkt_mem_move(struct cmdq_pkt *pkt,
> dma_addr_t src_addr, dma_addr_t dst_
>   */
>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
>  
> +/**
> + * cmdq_pkt_acquire_event() - append acquire event command to the
> CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @event:	the desired event to be acquired
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event);
> +
>  /**
>   * cmdq_pkt_clear_event() - append clear event command to the CMDQ
> packet
>   * @pkt:	the CMDQ packet

Re: [RESEND, PATCH 5/5] mailbox: mtk-cmdq: Add support runtime get and set GCE event

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> ISP drivers need to get and set GCE event in their runtime contorl
> flow.
> So add these functions to support get and set GCE by CPU.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c       | 37
> ++++++++++++++++++++++++
>  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 ++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> b/drivers/mailbox/mtk-cmdq-mailbox.c
> index ead2200f39ba..d7c08249c898 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -25,7 +25,11 @@
>  #define CMDQ_GCE_NUM_MAX		(2)
>  
>  #define CMDQ_CURR_IRQ_STATUS		0x10
> +#define CMDQ_SYNC_TOKEN_ID		0x60
> +#define CMDQ_SYNC_TOKEN_VALUE		0x64
> +#define CMDQ_TOKEN_ID_MASK			GENMASK(9, 0)
>  #define CMDQ_SYNC_TOKEN_UPDATE		0x68
> +#define CMDQ_TOKEN_UPDATE_VALUE			BIT(16)
>  #define CMDQ_THR_SLOT_CYCLES		0x30
>  #define CMDQ_THR_BASE			0x100
>  #define CMDQ_THR_SIZE			0x80
> @@ -83,6 +87,7 @@ struct cmdq {
>  	struct cmdq_thread	*thread;
>  	struct clk_bulk_data	clocks[CMDQ_GCE_NUM_MAX];
>  	bool			suspended;
> +	spinlock_t		event_lock; /* lock for gce event */
>  };
>  
>  struct gce_plat {
> @@ -113,6 +118,38 @@ u8 cmdq_get_shift_pa(struct mbox_chan *chan)
>  }
>  EXPORT_SYMBOL(cmdq_get_shift_pa);
>  
> +void cmdq_set_event(void *chan, u16 event_id)

struct mbox_chan *chan

Is the event_id the hardware event id listed in include/dt-bindings/gce 
? I mean CPU could trigger the event which should be trigger by
hardware?

> +{
> +	struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)-
> >mbox,
> +		typeof(*cmdq), mbox);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cmdq->event_lock, flags);
> +
> +	writel(CMDQ_TOKEN_UPDATE_VALUE | event_id, cmdq->base +
> CMDQ_SYNC_TOKEN_UPDATE);
> +
> +	spin_unlock_irqrestore(&cmdq->event_lock, flags);
> +}
> +EXPORT_SYMBOL(cmdq_set_event);
> +
> +u32 cmdq_get_event(void *chan, u16 event_id)

Does this get the event status? I think event has only two status, set
or cleared. So I would like this to return true for set and false for
cleared.

Regards,
CK

> +{
> +	struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)-
> >mbox,
> +		typeof(*cmdq), mbox);
> +	unsigned long flags;
> +	u32 value = 0;
> +
> +	spin_lock_irqsave(&cmdq->event_lock, flags);
> +
> +	writel(CMDQ_TOKEN_ID_MASK & event_id, cmdq->base +
> CMDQ_SYNC_TOKEN_ID);
> +	value = readl(cmdq->base + CMDQ_SYNC_TOKEN_VALUE);
> +
> +	spin_unlock_irqrestore(&cmdq->event_lock, flags);
> +
> +	return value;
> +}
> +EXPORT_SYMBOL(cmdq_get_event);
> +
>  static int cmdq_thread_suspend(struct cmdq *cmdq, struct cmdq_thread
> *thread)
>  {
>  	u32 status;
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h
> b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index a8f0070c7aa9..f05cabd230da 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -79,5 +79,7 @@ struct cmdq_pkt {
>  };
>  
>  u8 cmdq_get_shift_pa(struct mbox_chan *chan);
> +void cmdq_set_event(void *chan, u16 event_id);
> +u32 cmdq_get_event(void *chan, u16 event_id);
>  
>  #endif /* __MTK_CMDQ_MAILBOX_H__ */

Re: [RESEND, PATCH 2/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_mem_move() function

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> Add cmdq_pkt_mem_move() function to support CMDQ user making
> an instruction for moving a value from a source address to a
> destination address.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 26
> ++++++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  | 10 ++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index b0cd071c4719..3a1e47ad8a41 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -299,6 +299,32 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt
> *pkt, u8 high_addr_reg_idx,
>  }
>  EXPORT_SYMBOL(cmdq_pkt_write_s_mask_value);
>  
> +s32 cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr,
> dma_addr_t dst_addr)

return type int.

> +{
> +	s32 err;

It may not error, so I prefer to use 'ret'.
Move this after the next two declaration.

> +	const u16 tmp_reg_idx = CMDQ_THR_SPR_IDX0;
> +	const u16 swap_reg_idx = CMDQ_THR_SPR_IDX1;

I would like tmp_reg_idx to be high_addr_reg_idx and swap_reg_idx to be
value_reg_idx.

Regards,
CK

> +
> +	/* read the value of src_addr into swap_reg_idx */
> +	err = cmdq_pkt_assign(pkt, tmp_reg_idx,
> CMDQ_ADDR_HIGH(src_addr));
> +	if (err < 0)
> +		return err;
> +	err = cmdq_pkt_read_s(pkt, tmp_reg_idx,
> CMDQ_ADDR_LOW(src_addr), swap_reg_idx);
> +	if (err < 0)
> +		return err;
> +
> +	/* write the value of swap_reg_idx into dst_addr */
> +	err = cmdq_pkt_assign(pkt, tmp_reg_idx,
> CMDQ_ADDR_HIGH(dst_addr));
> +	if (err < 0)
> +		return err;
> +	err = cmdq_pkt_write_s(pkt, tmp_reg_idx,
> CMDQ_ADDR_LOW(dst_addr), swap_reg_idx);
> +	if (err < 0)
> +		return err;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_mem_move);
> +
>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
>  {
>  	struct cmdq_instruction inst = { {0} };
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> index 1dae80185f9f..b6dbe2d8f16a 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -182,6 +182,16 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt,
> u8 high_addr_reg_idx,
>  int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8
> high_addr_reg_idx,
>  				u16 addr_low, u32 value, u32 mask);
>  
> +/**
> + * cmdq_pkt_mem_move() - append memory move command to the CMDQ
> packet
> + * @pkt:	the CMDQ packet
> + * @src_addr:	source address
> + * @dma_addr_t: destination address
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr,
> dma_addr_t dst_addr);
> +
>  /**
>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>   * @pkt:	the CMDQ packet

Re: [RESEND, PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function

[CK Hu (胡俊光)]

Hi, Jason:

On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> Add cmdq_pkt_poll_addr function to support CMDQ user making
> an instruction for polling a specific address of hardware rigster
> to check the value with or without mask.
> 
> POLL is an old operation in GCE, so it does not support SPR and
> CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
> to move polling register address to GPR to make an instruction.
> This will be done in cmdq_pkt_poll_addr().
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> ++++++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  | 16 +++++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 3a1e47ad8a41..2e9fc9bb1183 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_POLL_HIGH_ADDR_GPR	(14)

I think there are multiple GPR and you use #14 to store high addr. I
would like you to list all GPR and do not limit the usage of each GPR.
The question is, why limit #14 to be high addr? If the GPR is shared by
all threads, there should be a mechanism to manage GPR usage for client
driver to allocate/free GPR.

>  #define CMDQ_EOC_IRQ_EN		BIT(0)
>  #define CMDQ_REG_TYPE		1
>  #define CMDQ_JUMP_RELATIVE	1
> @@ -406,6 +407,43 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8
> subsys,
>  }
>  EXPORT_SYMBOL(cmdq_pkt_poll_mask);
>  
> +int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32
> value, u32 mask)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +	int err;
> +	u8 use_mask = 0;
> +
> +	if (mask != U32_MAX) {
> +		inst.op = CMDQ_CODE_MASK;
> +		inst.mask = ~mask;
> +		err = cmdq_pkt_append_command(pkt, inst);
> +		if (err != 0)
> +			return err;
> +		use_mask = CMDQ_POLL_ENABLE_MASK;
> +	}
> +
> +	/*
> +	 * POLL is an old operation in GCE and it does not support SPR
> and CMDQ_CODE_LOGIC,
> +	 * so it can not use cmdq_pkt_assign to keep polling register
> address to SPR.
> +	 * It needs to use GPR and CMDQ_CODE_MASK to move polling
> register address to GPR.
> +	 */
> +	inst.op = CMDQ_CODE_MASK;
> +	inst.dst_t = CMDQ_REG_TYPE;
> +	inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
> +	inst.mask = addr;

This is full address, not high address. Why name the GPR to
CMDQ_POLL_HIGH_ADDR_GPR?

The addr is not mask, so I would like inst.value = addr.

Regards,
CK

> +	err = cmdq_pkt_append_command(pkt, inst);
> +	if (err < 0)
> +		return err;
> +
> +	inst.op = CMDQ_CODE_POLL;
> +	inst.dst_t = CMDQ_REG_TYPE;
> +	inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
> +	inst.offset = use_mask;
> +	inst.value = value;
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_poll_addr);
> +
>  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
>  {
>  	struct cmdq_instruction inst = {};
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> index b6dbe2d8f16a..2fe9be240fbc 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -253,6 +253,22 @@ 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_poll_addr() - Append polling command to the CMDQ packet,
> ask GCE to
> + *			 execute an instruction that wait for a
> specified
> + *			 address of hardware register to check for the
> value
> + *			 w/ or w/o mask.
> + *			 All GCE hardware threads will be blocked by
> this
> + *			 instruction.
> + * @pkt:	the CMDQ packet
> + * @addr:	the hardware register address
> + * @value:	the specified target register value
> + * @mask:	the specified target register mask
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, 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

Re: [RESEND, PATCH 4/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_acquire_event() function

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Mon, 2024-03-04 at 02:11 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > Add cmdq_pkt_acquire_event() function to support CMDQ user making
> > an instruction for acquiring event.
> > 
> > CMDQ users can use cmdq_pkt_acquire_event() and
> > cmdq_pkt_clear_event()
> > to acquire GCE event and release GCE event and achieve the
> > MUTEX_LOCK
> > protection between GCE threads.
> 
> I'm not clear what acquire do in detail. This is what I guess:
> 
> cmdq_pkt_acquire_event() would wait for event to be cleared. After
> event is cleared, cmdq_pkt_acquire_event() would set event and keep
> executing next command. So the mutex would work like this
> 
> cmdq_pkt_acquire_event() /* mutex lock */
> 
> /* critical secton */
> 
> cmdq_pkt_clear_event() /* mutex unlock */
> 
> If it's so, describe as detail as this. If not, describe how it do.
> 
Yes, they should be used like this.
I'll add more description in commit message.

> As I know, GCE is single core, so multiple thread is served by single
> GCE, why need mutex lock?
> 
Although GCE is single core, it will context switch to other GCE
threads with the same priority. The context switch time is set to
GCE_THR_SLOT_CYCLES during GCE initialization.

So we have to use event_lock to protect HW resource if each cmdq_pkt in
different thread may execute more than the context switch time.

Regards,
Jason-JH.Lin

> Regards,
> CK
> 
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c | 15 +++++++++++++++
> >  include/linux/soc/mediatek/mtk-cmdq.h  |  9 +++++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 2e9fc9bb1183..0183b40a0eff 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -342,6 +342,21 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16
> > event, bool clear)
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_wfe);
> >  
> > +int cmdq_pkt_acquire_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 |
> > CMDQ_WFE_WAIT;
> > +	inst.event = event;
> > +
> > +	return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_acquire_event);
> > +
> >  int cmdq_pkt_clear_event(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 2fe9be240fbc..de93c0a8e8a9 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -202,6 +202,15 @@ int cmdq_pkt_mem_move(struct cmdq_pkt *pkt,
> > dma_addr_t src_addr, dma_addr_t dst_
> >   */
> >  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
> >  
> > +/**
> > + * cmdq_pkt_acquire_event() - append acquire event command to the
> > CMDQ packet
> > + * @pkt:	the CMDQ packet
> > + * @event:	the desired event to be acquired
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event);
> > +
> >  /**
> >   * cmdq_pkt_clear_event() - append clear event command to the CMDQ
> > packet
> >   * @pkt:	the CMDQ packet

Re: [RESEND, PATCH 5/5] mailbox: mtk-cmdq: Add support runtime get and set GCE event

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Mon, 2024-03-04 at 02:30 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > ISP drivers need to get and set GCE event in their runtime contorl
> > flow.
> > So add these functions to support get and set GCE by CPU.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c       | 37
> > ++++++++++++++++++++++++
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 ++
> >  2 files changed, 39 insertions(+)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index ead2200f39ba..d7c08249c898 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -25,7 +25,11 @@
> >  #define CMDQ_GCE_NUM_MAX		(2)
> >  
> >  #define CMDQ_CURR_IRQ_STATUS		0x10
> > +#define CMDQ_SYNC_TOKEN_ID		0x60
> > +#define CMDQ_SYNC_TOKEN_VALUE		0x64
> > +#define CMDQ_TOKEN_ID_MASK			GENMASK(9, 0)
> >  #define CMDQ_SYNC_TOKEN_UPDATE		0x68
> > +#define CMDQ_TOKEN_UPDATE_VALUE			BIT(16)
> >  #define CMDQ_THR_SLOT_CYCLES		0x30
> >  #define CMDQ_THR_BASE			0x100
> >  #define CMDQ_THR_SIZE			0x80
> > @@ -83,6 +87,7 @@ struct cmdq {
> >  	struct cmdq_thread	*thread;
> >  	struct clk_bulk_data	clocks[CMDQ_GCE_NUM_MAX];
> >  	bool			suspended;
> > +	spinlock_t		event_lock; /* lock for gce event */
> >  };
> >  
> >  struct gce_plat {
> > @@ -113,6 +118,38 @@ u8 cmdq_get_shift_pa(struct mbox_chan *chan)
> >  }
> >  EXPORT_SYMBOL(cmdq_get_shift_pa);
> >  
> > +void cmdq_set_event(void *chan, u16 event_id)
> 
> struct mbox_chan *chan
> 
OK, I'll change it.

> Is the event_id the hardware event id listed in include/dt-
> bindings/gce 
> ? I mean CPU could trigger the event which should be trigger by
> hardware?
> 
Yes, this can also trigger the hardware event, but CMDQ user should not
do that. Otherwise, it will cause error in other GCE threads that use
this hardware event.

> > +{
> > +	struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)-
> > > mbox,
> > 
> > +		typeof(*cmdq), mbox);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&cmdq->event_lock, flags);
> > +
> > +	writel(CMDQ_TOKEN_UPDATE_VALUE | event_id, cmdq->base +
> > CMDQ_SYNC_TOKEN_UPDATE);
> > +
> > +	spin_unlock_irqrestore(&cmdq->event_lock, flags);
> > +}
> > +EXPORT_SYMBOL(cmdq_set_event);
> > +
> > +u32 cmdq_get_event(void *chan, u16 event_id)
> 
> Does this get the event status? I think event has only two status,
> set
> or cleared. So I would like this to return true for set and false for
> cleared.
Yes, the event status is 1 or 0. I'll change it to boolean.

Regards,
Jason-JH.Lin

> 
> Regards,
> CK
> 

Re: [RESEND, PATCH 2/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_mem_move() function

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Mon, 2024-03-04 at 02:39 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > Add cmdq_pkt_mem_move() function to support CMDQ user making
> > an instruction for moving a value from a source address to a
> > destination address.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c | 26
> > ++++++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-cmdq.h  | 10 ++++++++++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index b0cd071c4719..3a1e47ad8a41 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -299,6 +299,32 @@ int cmdq_pkt_write_s_mask_value(struct
> > cmdq_pkt
> > *pkt, u8 high_addr_reg_idx,
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_write_s_mask_value);
> >  
> > +s32 cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr,
> > dma_addr_t dst_addr)
> 
> return type int.

OK, I'll change it.
> 
> > +{
> > +	s32 err;
> 
> It may not error, so I prefer to use 'ret'.
> Move this after the next two declaration.

OK, Ill change it to 'ret' and move it.
> 
> > +	const u16 tmp_reg_idx = CMDQ_THR_SPR_IDX0;
> > +	const u16 swap_reg_idx = CMDQ_THR_SPR_IDX1;
> 
> I would like tmp_reg_idx to be high_addr_reg_idx and swap_reg_idx to
> be
> value_reg_idx.
> 

OK, I'll rename them.

Regards,
Jason-JH.Lin

> Regards,
> CK
> 

Re: [RESEND, PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > an instruction for polling a specific address of hardware rigster
> > to check the value with or without mask.
> > 
> > POLL is an old operation in GCE, so it does not support SPR and
> > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
> > to move polling register address to GPR to make an instruction.
> > This will be done in cmdq_pkt_poll_addr().
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > ++++++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-cmdq.h  | 16 +++++++++++
> >  2 files changed, 54 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 3a1e47ad8a41..2e9fc9bb1183 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_POLL_HIGH_ADDR_GPR	(14)
> 
> I think there are multiple GPR and you use #14 to store high addr. I
> would like you to list all GPR and do not limit the usage of each
> GPR.
> The question is, why limit #14 to be high addr? If the GPR is shared
> by
> all threads, there should be a mechanism to manage GPR usage for
> client
> driver to allocate/free GPR.

Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are shared by all
threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.

I think user may not know which GPR is available, so I think CMDQ
driver should manage the usage of GPR instead of configure by the user.

Currently, we only use 1 dedicated GPR in poll, so I defined it in CMDQ
driver to make it simpler.

> 
> >  #define CMDQ_EOC_IRQ_EN		BIT(0)
> >  #define CMDQ_REG_TYPE		1
> >  #define CMDQ_JUMP_RELATIVE	1
> > @@ -406,6 +407,43 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt,
> > u8
> > subsys,
> >  }
> >  EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> >  
> > +int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32
> > value, u32 mask)
> > +{
> > +	struct cmdq_instruction inst = { {0} };
> > +	int err;
> > +	u8 use_mask = 0;
> > +
> > +	if (mask != U32_MAX) {
> > +		inst.op = CMDQ_CODE_MASK;
> > +		inst.mask = ~mask;
> > +		err = cmdq_pkt_append_command(pkt, inst);
> > +		if (err != 0)
> > +			return err;
> > +		use_mask = CMDQ_POLL_ENABLE_MASK;
> > +	}
> > +
> > +	/*
> > +	 * POLL is an old operation in GCE and it does not support SPR
> > and CMDQ_CODE_LOGIC,
> > +	 * so it can not use cmdq_pkt_assign to keep polling register
> > address to SPR.
> > +	 * It needs to use GPR and CMDQ_CODE_MASK to move polling
> > register address to GPR.
> > +	 */
> > +	inst.op = CMDQ_CODE_MASK;
> > +	inst.dst_t = CMDQ_REG_TYPE;
> > +	inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
> > +	inst.mask = addr;
> 
> This is full address, not high address. Why name the GPR to
> CMDQ_POLL_HIGH_ADDR_GPR?
> 
My mistake, I'll remove that 'HIGH' word.

> The addr is not mask, so I would like inst.value = addr.

OK, I'll change it.

Regards,
Jason-JH.Lin

> 
> Regards,
> CK
> 

Re: [RESEND, PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function

[CK Hu (胡俊光)]

Hi, Jason:

On Mon, 2024-03-04 at 16:04 +0000, Jason-JH Lin (林睿祥) wrote:
> Hi CK,
> 
> Thanks for the reviews.
> 
> On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> > Hi, Jason:
> > 
> > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > > an instruction for polling a specific address of hardware rigster
> > > to check the value with or without mask.
> > > 
> > > POLL is an old operation in GCE, so it does not support SPR and
> > > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
> > > to move polling register address to GPR to make an instruction.
> > > This will be done in cmdq_pkt_poll_addr().
> > > 
> > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > ---
> > >  drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > > ++++++++++++++++++++++++++
> > >  include/linux/soc/mediatek/mtk-cmdq.h  | 16 +++++++++++
> > >  2 files changed, 54 insertions(+)
> > > 
> > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > index 3a1e47ad8a41..2e9fc9bb1183 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_POLL_HIGH_ADDR_GPR	(14)
> > 
> > I think there are multiple GPR and you use #14 to store high addr.
> > I
> > would like you to list all GPR and do not limit the usage of each
> > GPR.
> > The question is, why limit #14 to be high addr? If the GPR is
> > shared
> > by
> > all threads, there should be a mechanism to manage GPR usage for
> > client
> > driver to allocate/free GPR.
> 
> Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are shared by
> all
> threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.
> 
> I think user may not know which GPR is available, so I think CMDQ
> driver should manage the usage of GPR instead of configure by the
> user.
> 
> Currently, we only use 1 dedicated GPR in poll, so I defined it in
> CMDQ
> driver to make it simpler.

If thread 1 poll addr A first, GPR is set to A. But poll time exceed
GCE_THD_SLOT_CYCLES, change to thread 2 and thread 2 poll addr B, GPR
is set to B. Later switch to thread A and GCE would execute poll
command and GPR is B, so thread 1 would poll addr B, but this is wrong.
How do you solve this problem?

Regards,
CK

> 
> > 
> > >  #define CMDQ_EOC_IRQ_EN		BIT(0)
> > >  #define CMDQ_REG_TYPE		1
> > >  #define CMDQ_JUMP_RELATIVE	1
> > > @@ -406,6 +407,43 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt,
> > > u8
> > > subsys,
> > >  }
> > >  EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> > >  
> > > +int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr,
> > > u32
> > > value, u32 mask)
> > > +{
> > > +	struct cmdq_instruction inst = { {0} };
> > > +	int err;
> > > +	u8 use_mask = 0;
> > > +
> > > +	if (mask != U32_MAX) {
> > > +		inst.op = CMDQ_CODE_MASK;
> > > +		inst.mask = ~mask;
> > > +		err = cmdq_pkt_append_command(pkt, inst);
> > > +		if (err != 0)
> > > +			return err;
> > > +		use_mask = CMDQ_POLL_ENABLE_MASK;
> > > +	}
> > > +
> > > +	/*
> > > +	 * POLL is an old operation in GCE and it does not support SPR
> > > and CMDQ_CODE_LOGIC,
> > > +	 * so it can not use cmdq_pkt_assign to keep polling register
> > > address to SPR.
> > > +	 * It needs to use GPR and CMDQ_CODE_MASK to move polling
> > > register address to GPR.
> > > +	 */
> > > +	inst.op = CMDQ_CODE_MASK;
> > > +	inst.dst_t = CMDQ_REG_TYPE;
> > > +	inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
> > > +	inst.mask = addr;
> > 
> > This is full address, not high address. Why name the GPR to
> > CMDQ_POLL_HIGH_ADDR_GPR?
> > 
> 
> My mistake, I'll remove that 'HIGH' word.
> 
> > The addr is not mask, so I would like inst.value = addr.
> 
> OK, I'll change it.
> 
> Regards,
> Jason-JH.Lin
> 
> > 
> > Regards,
> > CK
> > 

Re: [RESEND, PATCH 5/5] mailbox: mtk-cmdq: Add support runtime get and set GCE event

[CK Hu (胡俊光)]

Hi, Jason:

On Mon, 2024-03-04 at 15:50 +0000, Jason-JH Lin (林睿祥) wrote:
> Hi CK,
> 
> Thanks for the reviews.
> 
> On Mon, 2024-03-04 at 02:30 +0000, CK Hu (胡俊光) wrote:
> > Hi, Jason:
> > 
> > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > ISP drivers need to get and set GCE event in their runtime
> > > contorl
> > > flow.
> > > So add these functions to support get and set GCE by CPU.
> > > 
> > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > ---
> > >  drivers/mailbox/mtk-cmdq-mailbox.c       | 37
> > > ++++++++++++++++++++++++
> > >  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 ++
> > >  2 files changed, 39 insertions(+)
> > > 
> > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > index ead2200f39ba..d7c08249c898 100644
> > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > @@ -25,7 +25,11 @@
> > >  #define CMDQ_GCE_NUM_MAX		(2)
> > >  
> > >  #define CMDQ_CURR_IRQ_STATUS		0x10
> > > +#define CMDQ_SYNC_TOKEN_ID		0x60
> > > +#define CMDQ_SYNC_TOKEN_VALUE		0x64
> > > +#define CMDQ_TOKEN_ID_MASK			GENMASK(9, 0)
> > >  #define CMDQ_SYNC_TOKEN_UPDATE		0x68
> > > +#define CMDQ_TOKEN_UPDATE_VALUE			BIT(16)
> > >  #define CMDQ_THR_SLOT_CYCLES		0x30
> > >  #define CMDQ_THR_BASE			0x100
> > >  #define CMDQ_THR_SIZE			0x80
> > > @@ -83,6 +87,7 @@ struct cmdq {
> > >  	struct cmdq_thread	*thread;
> > >  	struct clk_bulk_data	clocks[CMDQ_GCE_NUM_MAX];
> > >  	bool			suspended;
> > > +	spinlock_t		event_lock; /* lock for gce event */
> > >  };
> > >  
> > >  struct gce_plat {
> > > @@ -113,6 +118,38 @@ u8 cmdq_get_shift_pa(struct mbox_chan *chan)
> > >  }
> > >  EXPORT_SYMBOL(cmdq_get_shift_pa);
> > >  
> > > +void cmdq_set_event(void *chan, u16 event_id)
> > 
> > struct mbox_chan *chan
> > 
> 
> OK, I'll change it.
> 
> > Is the event_id the hardware event id listed in include/dt-
> > bindings/gce 
> > ? I mean CPU could trigger the event which should be trigger by
> > hardware?
> > 
> 
> Yes, this can also trigger the hardware event, but CMDQ user should
> not
> do that. Otherwise, it will cause error in other GCE threads that use
> this hardware event.

So, what event id could client driver use? And how to prevent different
client driver use the same event id?

Regards,
CK

> 
> > > +{
> > > +	struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)-
> > > > mbox,
> > > 
> > > +		typeof(*cmdq), mbox);
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&cmdq->event_lock, flags);
> > > +
> > > +	writel(CMDQ_TOKEN_UPDATE_VALUE | event_id, cmdq->base +
> > > CMDQ_SYNC_TOKEN_UPDATE);
> > > +
> > > +	spin_unlock_irqrestore(&cmdq->event_lock, flags);
> > > +}
> > > +EXPORT_SYMBOL(cmdq_set_event);
> > > +
> > > +u32 cmdq_get_event(void *chan, u16 event_id)
> > 
> > Does this get the event status? I think event has only two status,
> > set
> > or cleared. So I would like this to return true for set and false
> > for
> > cleared.
> 
> Yes, the event status is 1 or 0. I'll change it to boolean.
> 
> Regards,
> Jason-JH.Lin
> 
> > 
> > Regards,
> > CK
> > 

Re: [RESEND, PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function

[Jason-JH Lin (林睿祥)]

On Tue, 2024-03-05 at 03:26 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2024-03-04 at 16:04 +0000, Jason-JH Lin (林睿祥) wrote:
> > Hi CK,
> > 
> > Thanks for the reviews.
> > 
> > On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> > > Hi, Jason:
> > > 
> > > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > > > an instruction for polling a specific address of hardware
> > > > rigster
> > > > to check the value with or without mask.
> > > > 
> > > > POLL is an old operation in GCE, so it does not support SPR and
> > > > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
> > > > to move polling register address to GPR to make an instruction.
> > > > This will be done in cmdq_pkt_poll_addr().
> > > > 
> > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > > ---
> > > >  drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > > > ++++++++++++++++++++++++++
> > > >  include/linux/soc/mediatek/mtk-cmdq.h  | 16 +++++++++++
> > > >  2 files changed, 54 insertions(+)
> > > > 
> > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > index 3a1e47ad8a41..2e9fc9bb1183 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_POLL_HIGH_ADDR_GPR	(14)
> > > 
> > > I think there are multiple GPR and you use #14 to store high
> > > addr.
> > > I
> > > would like you to list all GPR and do not limit the usage of each
> > > GPR.
> > > The question is, why limit #14 to be high addr? If the GPR is
> > > shared
> > > by
> > > all threads, there should be a mechanism to manage GPR usage for
> > > client
> > > driver to allocate/free GPR.
> > 
> > Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are shared by
> > all
> > threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.
> > 
> > I think user may not know which GPR is available, so I think CMDQ
> > driver should manage the usage of GPR instead of configure by the
> > user.
> > 
> > Currently, we only use 1 dedicated GPR in poll, so I defined it in
> > CMDQ
> > driver to make it simpler.
> 
> If thread 1 poll addr A first, GPR is set to A. But poll time exceed
> GCE_THD_SLOT_CYCLES, change to thread 2 and thread 2 poll addr B, GPR
> is set to B. Later switch to thread A and GCE would execute poll
> command and GPR is B, so thread 1 would poll addr B, but this is
> wrong.
> How do you solve this problem?
> 
If POLL instruction has timeout, this may be a problem.

But POLL is a legacy operation, it won't context switch when the
execute time exceed GCE_THR_SLOT_CYCLES. So we add the comment "All GCE
hardware threads will be blocked by this instruction" in the kerneldoc.

And currently, we don't set the GCE hardware timeout in
CMDQ_THR_INST_TIMEOUT_CYCLES, so it'll poll forever until the polling
value is set...

Regards,
Jason-JH.Lin

> Regards,
> CK
> 

Re: [RESEND, PATCH 5/5] mailbox: mtk-cmdq: Add support runtime get and set GCE event

[Jason-JH Lin (林睿祥)]

On Tue, 2024-03-05 at 03:31 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2024-03-04 at 15:50 +0000, Jason-JH Lin (林睿祥) wrote:
> > Hi CK,
> > 
> > Thanks for the reviews.
> > 
> > On Mon, 2024-03-04 at 02:30 +0000, CK Hu (胡俊光) wrote:
> > > Hi, Jason:
> > > 
> > > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > > ISP drivers need to get and set GCE event in their runtime
> > > > contorl
> > > > flow.
> > > > So add these functions to support get and set GCE by CPU.
> > > > 
> > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > > ---
> > > >  drivers/mailbox/mtk-cmdq-mailbox.c       | 37
> > > > ++++++++++++++++++++++++
> > > >  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 ++
> > > >  2 files changed, 39 insertions(+)
> > > > 
> > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > index ead2200f39ba..d7c08249c898 100644
> > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > @@ -25,7 +25,11 @@
> > > >  #define CMDQ_GCE_NUM_MAX		(2)
> > > >  
> > > >  #define CMDQ_CURR_IRQ_STATUS		0x10
> > > > +#define CMDQ_SYNC_TOKEN_ID		0x60
> > > > +#define CMDQ_SYNC_TOKEN_VALUE		0x64
> > > > +#define CMDQ_TOKEN_ID_MASK			GENMASK(9, 0)
> > > >  #define CMDQ_SYNC_TOKEN_UPDATE		0x68
> > > > +#define CMDQ_TOKEN_UPDATE_VALUE			BIT(16)
> > > >  #define CMDQ_THR_SLOT_CYCLES		0x30
> > > >  #define CMDQ_THR_BASE			0x100
> > > >  #define CMDQ_THR_SIZE			0x80
> > > > @@ -83,6 +87,7 @@ struct cmdq {
> > > >  	struct cmdq_thread	*thread;
> > > >  	struct clk_bulk_data	clocks[CMDQ_GCE_NUM_MAX];
> > > >  	bool			suspended;
> > > > +	spinlock_t		event_lock; /* lock for gce
> > > > event */
> > > >  };
> > > >  
> > > >  struct gce_plat {
> > > > @@ -113,6 +118,38 @@ u8 cmdq_get_shift_pa(struct mbox_chan
> > > > *chan)
> > > >  }
> > > >  EXPORT_SYMBOL(cmdq_get_shift_pa);
> > > >  
> > > > +void cmdq_set_event(void *chan, u16 event_id)
> > > 
> > > struct mbox_chan *chan
> > > 
> > 
> > OK, I'll change it.
> > 
> > > Is the event_id the hardware event id listed in include/dt-
> > > bindings/gce 
> > > ? I mean CPU could trigger the event which should be trigger by
> > > hardware?
> > > 
> > 
> > Yes, this can also trigger the hardware event, but CMDQ user should
> > not
> > do that. Otherwise, it will cause error in other GCE threads that
> > use
> > this hardware event.
> 
> So, what event id could client driver use? And how to prevent
> different
> client driver use the same event id?
> 
Yes, this might be a problem.

Client user should use the SW token events defined in dt-binding and
parsing them from dts.
Maybe different user should see if the SW token events has used by
other user.

Anyway, after confirming with ISP owners, they dropped the usage of
these 2 APIs. So I'll drop this patch in the next version.

Regards,
Jason-JH.Lin

> Regards,
> CK
> 

Re: [RESEND, PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function

[CK Hu (胡俊光)]

Hi, Jason:

On Tue, 2024-03-05 at 03:37 +0000, Jason-JH Lin (林睿祥) wrote:
> On Tue, 2024-03-05 at 03:26 +0000, CK Hu (胡俊光) wrote:
> > Hi, Jason:
> > 
> > On Mon, 2024-03-04 at 16:04 +0000, Jason-JH Lin (林睿祥) wrote:
> > > Hi CK,
> > > 
> > > Thanks for the reviews.
> > > 
> > > On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> > > > Hi, Jason:
> > > > 
> > > > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > > > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > > > > an instruction for polling a specific address of hardware
> > > > > rigster
> > > > > to check the value with or without mask.
> > > > > 
> > > > > POLL is an old operation in GCE, so it does not support SPR
> > > > > and
> > > > > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and
> > > > > CMDQ_CODE_MASK
> > > > > to move polling register address to GPR to make an
> > > > > instruction.
> > > > > This will be done in cmdq_pkt_poll_addr().
> > > > > 
> > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > > > ---
> > > > >  drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > > > > ++++++++++++++++++++++++++
> > > > >  include/linux/soc/mediatek/mtk-cmdq.h  | 16 +++++++++++
> > > > >  2 files changed, 54 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > index 3a1e47ad8a41..2e9fc9bb1183 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_POLL_HIGH_ADDR_GPR	(14)
> > > > 
> > > > I think there are multiple GPR and you use #14 to store high
> > > > addr.
> > > > I
> > > > would like you to list all GPR and do not limit the usage of
> > > > each
> > > > GPR.
> > > > The question is, why limit #14 to be high addr? If the GPR is
> > > > shared
> > > > by
> > > > all threads, there should be a mechanism to manage GPR usage
> > > > for
> > > > client
> > > > driver to allocate/free GPR.
> > > 
> > > Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are shared
> > > by
> > > all
> > > threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.
> > > 
> > > I think user may not know which GPR is available, so I think CMDQ
> > > driver should manage the usage of GPR instead of configure by the
> > > user.
> > > 
> > > Currently, we only use 1 dedicated GPR in poll, so I defined it
> > > in
> > > CMDQ
> > > driver to make it simpler.
> > 
> > If thread 1 poll addr A first, GPR is set to A. But poll time
> > exceed
> > GCE_THD_SLOT_CYCLES, change to thread 2 and thread 2 poll addr B,
> > GPR
> > is set to B. Later switch to thread A and GCE would execute poll
> > command and GPR is B, so thread 1 would poll addr B, but this is
> > wrong.
> > How do you solve this problem?
> > 
> 
> If POLL instruction has timeout, this may be a problem.
> 
> But POLL is a legacy operation, it won't context switch when the
> execute time exceed GCE_THR_SLOT_CYCLES. So we add the comment "All
> GCE
> hardware threads will be blocked by this instruction" in the
> kerneldoc.
> 
> And currently, we don't set the GCE hardware timeout in
> CMDQ_THR_INST_TIMEOUT_CYCLES, so it'll poll forever until the polling
> value is set...

If ISP poll time longer than vblank, it may make display disorder. When
I see display disorder, could I find out ISP poll trigger this
disorder?

Regards,
CK

> 
> Regards,
> Jason-JH.Lin
> 
> > Regards,
> > CK
> > 

Re: [RESEND, PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function

[Jason-JH Lin (林睿祥)]

On Tue, 2024-03-05 at 03:51 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Tue, 2024-03-05 at 03:37 +0000, Jason-JH Lin (林睿祥) wrote:
> > On Tue, 2024-03-05 at 03:26 +0000, CK Hu (胡俊光) wrote:
> > > Hi, Jason:
> > > 
> > > On Mon, 2024-03-04 at 16:04 +0000, Jason-JH Lin (林睿祥) wrote:
> > > > Hi CK,
> > > > 
> > > > Thanks for the reviews.
> > > > 
> > > > On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> > > > > Hi, Jason:
> > > > > 
> > > > > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > > > > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > > > > > an instruction for polling a specific address of hardware
> > > > > > rigster
> > > > > > to check the value with or without mask.
> > > > > > 
> > > > > > POLL is an old operation in GCE, so it does not support SPR
> > > > > > and
> > > > > > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and
> > > > > > CMDQ_CODE_MASK
> > > > > > to move polling register address to GPR to make an
> > > > > > instruction.
> > > > > > This will be done in cmdq_pkt_poll_addr().
> > > > > > 
> > > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > > > > ---
> > > > > >  drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > > > > > ++++++++++++++++++++++++++
> > > > > >  include/linux/soc/mediatek/mtk-cmdq.h  | 16 +++++++++++
> > > > > >  2 files changed, 54 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > > index 3a1e47ad8a41..2e9fc9bb1183 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_POLL_HIGH_ADDR_GPR	(14)
> > > > > 
> > > > > I think there are multiple GPR and you use #14 to store high
> > > > > addr.
> > > > > I
> > > > > would like you to list all GPR and do not limit the usage of
> > > > > each
> > > > > GPR.
> > > > > The question is, why limit #14 to be high addr? If the GPR is
> > > > > shared
> > > > > by
> > > > > all threads, there should be a mechanism to manage GPR usage
> > > > > for
> > > > > client
> > > > > driver to allocate/free GPR.
> > > > 
> > > > Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are
> > > > shared
> > > > by
> > > > all
> > > > threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.
> > > > 
> > > > I think user may not know which GPR is available, so I think
> > > > CMDQ
> > > > driver should manage the usage of GPR instead of configure by
> > > > the
> > > > user.
> > > > 
> > > > Currently, we only use 1 dedicated GPR in poll, so I defined it
> > > > in
> > > > CMDQ
> > > > driver to make it simpler.
> > > 
> > > If thread 1 poll addr A first, GPR is set to A. But poll time
> > > exceed
> > > GCE_THD_SLOT_CYCLES, change to thread 2 and thread 2 poll addr B,
> > > GPR
> > > is set to B. Later switch to thread A and GCE would execute poll
> > > command and GPR is B, so thread 1 would poll addr B, but this is
> > > wrong.
> > > How do you solve this problem?
> > > 
> > 
> > If POLL instruction has timeout, this may be a problem.
> > 
> > But POLL is a legacy operation, it won't context switch when the
> > execute time exceed GCE_THR_SLOT_CYCLES. So we add the comment "All
> > GCE
> > hardware threads will be blocked by this instruction" in the
> > kerneldoc.
> > 
> > And currently, we don't set the GCE hardware timeout in
> > CMDQ_THR_INST_TIMEOUT_CYCLES, so it'll poll forever until the
> > polling
> > value is set...
> 
> If ISP poll time longer than vblank, it may make display disorder.
> When
> I see display disorder, could I find out ISP poll trigger this
> disorder?
> 
If we can get mtk_drm_ddp_irq timeout error when display disorder,
we need to dump all the cmd buffers and current PC in every executing
GCE threads by modifying the CMDQ driver code and see which PC
instruction may cause the problem.

If there is no timeout error when display disorder, then we have no
idea who cause that problem. We can only check the frame sequence id in
ISP driver frame done callback is disorder or not.

Because we don't set CMDQ hardware timeout IRQ or use software timer to
handle software timeout in CMDQ driver currently, so we won't know poll
instruction is blocking. ISP client drivers need to add their timeout
handler for each cmdq_pkt sent to the mailbox channels.

Regards,
Jason-JH.Lin

> Regards,
> CK
> 
> > 
> > Regards,
> > Jason-JH.Lin
> > 
> > > Regards,
> > > CK
> > >