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

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

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

[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>
Change-Id: Ie72dd86d6ccfb60761461ad128b02d32adaacbc0
---
 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

[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>
Change-Id: I0128db6a3412303fc6da61f8a57a0c08e0c0067e
---
 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

[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>
Change-Id: I91ff98e06570dc4501187eb29de64aaa65b1cf13
---
 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

[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>
Change-Id: Icdae6b60345c7ec1d7541ac76d1f06da56959cde
---
 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

[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>
Change-Id: I494c34ebc5ec26c82213f2bc03d2033d60652523
---
 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: [PATCH 2/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_mem_move() function

[AngeloGioacchino Del Regno]

Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
> 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>
> Change-Id: I0128db6a3412303fc6da61f8a57a0c08e0c0067e

Drop Change-id please

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

s32 err at the end please

> +
> +	/* 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;

In the documentation, you say "0 for success", so...

return 0; here :-)

> +}
> +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
> + *

  * @dst_addr: destination address
  *
  * Appends a CMDQ command to copy the value found in `src_addr` to `dst_addr`
  *
  * Return ....

> + * 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: [PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function

[AngeloGioacchino Del Regno]

Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
> 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

POLL is a legacy operation in GCE, so it does not support .....

> 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>
> Change-Id: I91ff98e06570dc4501187eb29de64aaa65b1cf13
> ---
>   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;

/* Describe what you're doing here as a comment please */
if (mask < GENMASK(31, 0)) {
	inst.mask = ~mask;
	err = cmdq_pkt_append_command ...
	use_mask ...
}

/* POLL is a legacy operation in GCE and ... etc etc */
inst.mask = addr;
inst.dst_t = ...
inst.sop = ...
err = cmdq_pkt_append... etc etc etc


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

This is a reassignment of the same value. You can do it like

/* dst_t and sop must be CMDQ_REG_TYPE, CMDQ_POLL_HIGH_ADDR_GPR */
inst.op = CMDQ_CODE_POLL;
inst.offset ...
inst.value ....

(but you're also inheriting the same `inst.mask`, was that intended?)

err = cmdq_pkt_append ....
if (err < 0)
	return err;

return 0
}

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

/**
  * cmdq_pkt_poll_addr() - Append blocking POLL command to CMDQ packet
  * @pkt:	the CMDQ packet
  * @addr:	the hardware register address
  * @value:	the specified target register value
  * @mask:	the specified target register mask
  *
  * Appends a polling (POLL) command to the CMDQ packet and asks the GCE
  * to execute an instruction that checks for the specified `value` (with
  * or without `mask`) to appear in the specified hardware register `addr`.
  * All GCE threads will be blocked by this instruction.
  *
  * Return: 0 for success or negative error code
  */

That's better, isn't it? :-)

Cheers,
Angelo

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

[AngeloGioacchino Del Regno]

Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
> 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>
> Change-Id: Icdae6b60345c7ec1d7541ac76d1f06da56959cde

Drop Change-Id please.

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

What you wrote in the commit message is good documentation for this function,
please also put it here in kerneldoc format as a documentation paragraph.

> + *
> + * 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: [PATCH 5/5] mailbox: mtk-cmdq: Add support runtime get and set GCE event

[AngeloGioacchino Del Regno]

Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
> 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>
> Change-Id: I494c34ebc5ec26c82213f2bc03d2033d60652523

Change-Id makes no sense upstream. Please drop.

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

struct mbox_chan *mbc = chan;
struct cmdq *cmdq = container_of(mbc->mbox, ... etc); (and this fits in one line)

> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cmdq->event_lock, flags);

Why do you need irqsave/irqrestore? I think I know, but please explain.

> +
> +	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__ */

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

[Jason-JH Lin (林睿祥)]

Hi Angelo, 

Thanks for the reviews.

On Mon, 2024-03-04 at 11:05 +0100, AngeloGioacchino Del Regno wrote:
> Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
> > 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
> 
> POLL is a legacy operation in GCE, so it does not support .....
> 
OK, I'll change that.

> > 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>
> > Change-Id: I91ff98e06570dc4501187eb29de64aaa65b1cf13
> > ---
> >   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;
> 
> /* Describe what you're doing here as a comment please */
> if (mask < GENMASK(31, 0)) {
> 	inst.mask = ~mask;
> 	err = cmdq_pkt_append_command ...
> 	use_mask ...
> }
OK, I'll change the U32_MAX to GENMASK(31, 0).

> 
> /* POLL is a legacy operation in GCE and ... etc etc */
> inst.mask = addr;
> inst.dst_t = ...
> inst.sop = ...
> err = cmdq_pkt_append... etc etc etc
> 
OK, I'll change like this and I'll use inst.value = addr;
because addr is not mask.

> 
> > +		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;
> 
> This is a reassignment of the same value. You can do it like
> 
> /* dst_t and sop must be CMDQ_REG_TYPE, CMDQ_POLL_HIGH_ADDR_GPR */
> inst.op = CMDQ_CODE_POLL;
> inst.offset ...
> inst.value ....
> 
inst is used to generate an instruction for GCE, so it will be
reassigned every time after calling cmdq_pkt_append_command(pkt, inst);
to append a generated inst into the cmd buffer.

> (but you're also inheriting the same `inst.mask`, was that intended?)
> 
The definition of cmdq_instruction is:

struct cmdq_instruction {
	union {
		u32 value;
		u32 mask;
		struct {
			u16 arg_c;
			u16 src_reg;
		};
	};
	union {
		u16 offset;
		u16 event;
		u16 reg_dst;
	};
	...
};

so inst.mask and inst.value are the same u32 in inst.
That means inst.mask has been reassigned as the inst.value here.

> err = cmdq_pkt_append ....
> if (err < 0)
> 	return err;
> 
> return 0
> }

OK, I'll change it to return 0.

> 
> > +	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.
> 
> /**
>   * cmdq_pkt_poll_addr() - Append blocking POLL command to CMDQ
> packet
>   * @pkt:	the CMDQ packet
>   * @addr:	the hardware register address
>   * @value:	the specified target register value
>   * @mask:	the specified target register mask
>   *
>   * Appends a polling (POLL) command to the CMDQ packet and asks the
> GCE
>   * to execute an instruction that checks for the specified `value`
> (with
>   * or without `mask`) to appear in the specified hardware register
> `addr`.
>   * All GCE threads will be blocked by this instruction.
>   *
>   * Return: 0 for success or negative error code
>   */
> 
> That's better, isn't it? :-)
> 
OK, I'll change that. Thanks for making the entire sample.

Regards,
Jason-JH.Lin

> Cheers,
> Angelo
> 
> 

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

[Jason-JH Lin (林睿祥)]

Hi Angelo,

Thanks for the reviews.

On Mon, 2024-03-04 at 11:05 +0100, AngeloGioacchino Del Regno wrote:
> Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
> > 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>
> > Change-Id: I0128db6a3412303fc6da61f8a57a0c08e0c0067e
> 
> Drop Change-id please
> 
OK, I'll drop this.

> > ---
> >   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;
> 
> s32 err at the end please

OK, I'll move it here and I'll change 's32' to 'int' to align with
other functions.

> > +
> > +	/* 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;
> 
> In the documentation, you say "0 for success", so...
> 
> return 0; here :-)

OK, I'll change it.

> 
> > +}
> > +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
> > + *
> 
>   * @dst_addr: destination address
>   *
I'll fix that typo. Thanks!

>   * Appends a CMDQ command to copy the value found in `src_addr` to
> `dst_addr`
>   *
>   * Return ....
> 
and also change the doc format like this.

Regards,
Jason-JH.Lin

> > + * 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: [PATCH 4/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_acquire_event() function

[Jason-JH Lin (林睿祥)]

Hi Angelo,

Thanks for the reviews.

On Mon, 2024-03-04 at 11:05 +0100, AngeloGioacchino Del Regno wrote:
> Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
> > 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>
> > Change-Id: Icdae6b60345c7ec1d7541ac76d1f06da56959cde
> 
> Drop Change-Id please.

OK, I'll drop it.

> 
> > ---
> >   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
> 
> What you wrote in the commit message is good documentation for this
> function,
> please also put it here in kerneldoc format as a documentation
> paragraph.
> 
OK, I'll put it in kerneldoc, too.

Regards,
Jason-JH.Lin

> > + *
> > + * 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: [PATCH 5/5] mailbox: mtk-cmdq: Add support runtime get and set GCE event

[Jason-JH Lin (林睿祥)]

Hi Angelo,

Thanks for the reviews.

On Mon, 2024-03-04 at 11:06 +0100, AngeloGioacchino Del Regno wrote:
> Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
> > 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>
> > Change-Id: I494c34ebc5ec26c82213f2bc03d2033d60652523
> 
> Change-Id makes no sense upstream. Please drop.

OK, I'll drop it.

> 
> > ---
> >   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);
> 
> struct mbox_chan *mbc = chan;
> struct cmdq *cmdq = container_of(mbc->mbox, ... etc); (and this fits
> in one line)
> 
OK, I'll change it.

> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&cmdq->event_lock, flags);
> 
> Why do you need irqsave/irqrestore? I think I know, but please
> explain.
> 
Because ISP driver may call cmdq_get_event() first than use
cmdq_set_event() to update the event status in one
mtk_imgsys_setevent() function frequently.

And mtk_imgsys_setevent() will be called in SW multi-thread after cmdq
callback from cmdq_irq_handler, so we use the spin_lock_irqsave to
avoid the race condition.

But after discussing with ISP owners, they dropped
mtk_imgsys_setevent() recently, so I'll drop this patch in the next version.

Regards,
Jason-JH.Lin

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

[AngeloGioacchino Del Regno]

Il 05/03/24 04:09, Jason-JH Lin (林睿祥) ha scritto:
> Hi Angelo,
> 
> Thanks for the reviews.
> 
> On Mon, 2024-03-04 at 11:06 +0100, AngeloGioacchino Del Regno wrote:
>> Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
>>> 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>
>>> Change-Id: I494c34ebc5ec26c82213f2bc03d2033d60652523
>>
>> Change-Id makes no sense upstream. Please drop.
> 
> OK, I'll drop it.
> 
>>
>>> ---
>>>    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);
>>
>> struct mbox_chan *mbc = chan;
>> struct cmdq *cmdq = container_of(mbc->mbox, ... etc); (and this fits
>> in one line)
>>
> OK, I'll change it.
> 
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&cmdq->event_lock, flags);
>>
>> Why do you need irqsave/irqrestore? I think I know, but please
>> explain.
>>
> Because ISP driver may call cmdq_get_event() first than use
> cmdq_set_event() to update the event status in one
> mtk_imgsys_setevent() function frequently.
> 
> And mtk_imgsys_setevent() will be called in SW multi-thread after cmdq
> callback from cmdq_irq_handler, so we use the spin_lock_irqsave to
> avoid the race condition.

I was imagining something like that, yes - thank you for explaining.

Cheers,
Angelo