[PATCH v22 0/4] MediaTek MT8173 CMDQ support

[PATCH v22 0/4] MediaTek MT8173 CMDQ support

[Houlong Wei]

Hi,

This is Mediatek MT8173 Command Queue(CMDQ) driver. The CMDQ is used
to help write registers with critical time limitation, such as
updating display configuration during the vblank. It controls Global
Command Engine (GCE) hardware to achieve this requirement.

Changes since v21:
 -rebase on v4.18-rc1
 -remove subsys code and event id definition from mtk-cmdq-helper.c
 -add mt8173-gce.h to define the subsys code and envent id
Changes since v20:
 -rebase on v4.15-rc1
 -move dma_map_single outside of spinlock
Changes since v19:
- rebase to v4.10-rc2

Houlong Wei (4):
  dt-bindings: soc: Add documentation for the MediaTek GCE unit
  mailbox: mediatek: Add Mediatek CMDQ driver
  arm64: dts: mt8173: Add GCE node
  soc: mediatek: Add Mediatek CMDQ helper

 .../devicetree/bindings/mailbox/mtk-gce.txt        |   65 ++
 arch/arm64/boot/dts/mediatek/mt8173.dtsi           |   11 +
 drivers/mailbox/Kconfig                            |   10 +
 drivers/mailbox/Makefile                           |    2 +
 drivers/mailbox/mtk-cmdq-mailbox.c                 |  633 ++++++++++++++++++++
 drivers/soc/mediatek/Kconfig                       |   12 +
 drivers/soc/mediatek/Makefile                      |    1 +
 drivers/soc/mediatek/mtk-cmdq-helper.c             |  258 ++++++++
 include/dt-bindings/gce/mt8173-gce.h               |   48 ++
 include/linux/mailbox/mtk-cmdq-mailbox.h           |   70 +++
 include/linux/soc/mediatek/mtk-cmdq.h              |  132 ++++
 11 files changed, 1242 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
 create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
 create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
 create mode 100644 include/dt-bindings/gce/mt8173-gce.h
 create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h
 create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h

-- 
1.7.9.5 

[PATCH v22 1/4] dt-bindings: soc: Add documentation for the MediaTek GCE unit

[Houlong Wei]

This adds documentation for the MediaTek Global Command Engine (GCE) unit
found in MT8173 SoCs.

Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
Signed-off-by: HS Liao <hs.liao@mediatek.com>
---
Hi Rob,
  I don't add your ACK in this version since the dt-binding description
has been changed. Thanks.
---
 .../devicetree/bindings/mailbox/mtk-gce.txt        |   65 ++++++++++++++++++++
 include/dt-bindings/gce/mt8173-gce.h               |   48 +++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
 create mode 100644 include/dt-bindings/gce/mt8173-gce.h

diff --git a/Documentation/devicetree/bindings/mailbox/mtk-gce.txt b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
new file mode 100644
index 0000000..26f65a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
@@ -0,0 +1,65 @@
+MediaTek GCE
+===============
+
+The Global Command Engine (GCE) is used to help read/write registers with
+critical time limitation, such as updating display configuration during the
+vblank. The GCE can be used to implement the Command Queue (CMDQ) driver.
+
+CMDQ driver uses mailbox framework for communication. Please refer to
+mailbox.txt for generic information about mailbox device-tree bindings.
+
+Required properties:
+- compatible: Must be "mediatek,mt8173-gce"
+- reg: Address range of the GCE unit
+- interrupts: The interrupt signal from the GCE block
+- clock: Clocks according to the common clock binding
+- clock-names: Must be "gce" to stand for GCE clock
+- thread-num: Maximum threads count of GCE.
+- #mbox-cells: Should be 4.
+	<&phandle channel timeout priority atomic_exec>
+	phandle: Label name of a gce node.
+	channel: Channel of mailbox. Be equal to the thread id of GCE.
+	timeout: Maximum time of software waiting GCE processing done, in unit
+		of millisecond.
+	priority: Priority of GCE thread.
+	atomic_exec: GCE processing continuous packets of commands in atomic
+		way.
+
+Required properties for a client device:
+- mboxes: Client use mailbox to communicate with GCE, it should have this
+  property and list of phandle, mailbox specifiers.
+- gce-subsys: Specify the sub-system id which is corresponding to the register
+  address.
+
+Optional properties for a client device:
+- gce-event: Specify the event if the client has any. Because the event is
+  parsed by client, so client can replace 'gce-event' with other meaningful
+  name.
+
+Some vaules of properties are defined in 'dt-bindings/gce/mt8173-gce.h'. Such as
+thread number, sub-system ids, thread priority, event ids.
+
+Example:
+
+	gce: gce@10212000 {
+		compatible = "mediatek,mt8173-gce";
+		reg = <0 0x10212000 0 0x1000>;
+		interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
+		clocks = <&infracfg CLK_INFRA_GCE>;
+		clock-names = "gce";
+		thread-num = CMDQ_THR_MAX_COUNT;
+		#mbox-cells = <4>;
+	};
+
+Example for a client device:
+
+	mmsys: clock-controller@14000000 {
+		compatible = "mediatek,mt8173-mmsys";
+		mboxes = <&gce 0 2000 CMDQ_THR_PRIO_LOWEST 1>,
+			 <&gce 1 2000 CMDQ_THR_PRIO_LOWEST 1>;
+		gce-subsys = <SUBSYS_1400XXXX>;
+		mutex-event-eof = <CMDQ_EVENT_MUTEX0_STREAM_EOF
+				CMDQ_EVENT_MUTEX1_STREAM_EOF>;
+
+		...
+	};
diff --git a/include/dt-bindings/gce/mt8173-gce.h b/include/dt-bindings/gce/mt8173-gce.h
new file mode 100644
index 0000000..89eb3b8
--- /dev/null
+++ b/include/dt-bindings/gce/mt8173-gce.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Houlong Wei <houlong.wei@mediatek.com>
+ *
+ */
+
+#ifndef _DT_BINDINGS_GCE_MT8173_H
+#define _DT_BINDINGS_GCE_MT8173_H
+
+#define CMDQ_NO_TIMEOUT		0xffffffff
+
+#define CMDQ_THR_MAX_COUNT	16
+
+/* GCE HW thread priority */
+#define CMDQ_THR_PRIO_LOWEST	0
+#define CMDQ_THR_PRIO_HIGHEST	1
+
+/* GCE SUBSYS */
+#define SUBSYS_1400XXXX		1
+#define SUBSYS_1401XXXX		2
+#define SUBSYS_1402XXXX		3
+
+/* GCE HW EVENT */
+#define CMDQ_EVENT_DISP_OVL0_SOF		11
+#define CMDQ_EVENT_DISP_OVL1_SOF		12
+#define CMDQ_EVENT_DISP_RDMA0_SOF		13
+#define CMDQ_EVENT_DISP_RDMA1_SOF		14
+#define CMDQ_EVENT_DISP_RDMA2_SOF		15
+#define CMDQ_EVENT_DISP_WDMA0_SOF		16
+#define CMDQ_EVENT_DISP_WDMA1_SOF		17
+#define CMDQ_EVENT_DISP_OVL0_EOF		39
+#define CMDQ_EVENT_DISP_OVL1_EOF		40
+#define CMDQ_EVENT_DISP_RDMA0_EOF		41
+#define CMDQ_EVENT_DISP_RDMA1_EOF		42
+#define CMDQ_EVENT_DISP_RDMA2_EOF		43
+#define CMDQ_EVENT_DISP_WDMA0_EOF		44
+#define CMDQ_EVENT_DISP_WDMA1_EOF		45
+#define CMDQ_EVENT_MUTEX0_STREAM_EOF		53
+#define CMDQ_EVENT_MUTEX1_STREAM_EOF		54
+#define CMDQ_EVENT_MUTEX2_STREAM_EOF		55
+#define CMDQ_EVENT_MUTEX3_STREAM_EOF		56
+#define CMDQ_EVENT_MUTEX4_STREAM_EOF		57
+#define CMDQ_EVENT_DISP_RDMA0_UNDERRUN		63
+#define CMDQ_EVENT_DISP_RDMA1_UNDERRUN		64
+#define CMDQ_EVENT_DISP_RDMA2_UNDERRUN		65
+
+#endif
-- 
1.7.9.5

[PATCH v22 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

[Houlong Wei]

This patch is first version of Mediatek Command Queue(CMDQ) driver. The
CMDQ is used to help write registers with critical time limitation,
such as updating display configuration during the vblank. It controls
Global Command Engine (GCE) hardware to achieve this requirement.
Currently, CMDQ only supports display related hardwares, but we expect
it can be extended to other hardwares for future requirements.

Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
Signed-off-by: HS Liao <hs.liao@mediatek.com>
Signed-off-by: CK Hu <ck.hu@mediatek.com>
---
 drivers/mailbox/Kconfig                  |   10 +
 drivers/mailbox/Makefile                 |    2 +
 drivers/mailbox/mtk-cmdq-mailbox.c       |  634 ++++++++++++++++++++++++++++++
 include/linux/mailbox/mtk-cmdq-mailbox.h |   70 ++++
 4 files changed, 716 insertions(+)
 create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
 create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index e63d29a..2bbabc9 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -189,4 +189,14 @@ config STM32_IPCC
 	  Mailbox implementation for STMicroelectonics STM32 family chips
 	  with hardware for Inter-Processor Communication Controller (IPCC)
 	  between processors. Say Y here if you want to have this support.
+
+config MTK_CMDQ_MBOX
+	tristate "MediaTek CMDQ Mailbox Support"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	select MTK_INFRACFG
+	help
+	  Say yes here to add support for the MediaTek Command Queue (CMDQ)
+	  mailbox driver. The CMDQ is used to help read/write registers with
+	  critical time limitation, such as updating display configuration
+	  during the vblank.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 4d501be..4b00804 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -40,3 +40,5 @@ obj-$(CONFIG_QCOM_APCS_IPC)	+= qcom-apcs-ipc-mailbox.o
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
 
 obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
+
+obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
new file mode 100644
index 0000000..93d87cb
--- /dev/null
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -0,0 +1,634 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/dma-mapping.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/mtk-cmdq-mailbox.h>
+#include <linux/timer.h>
+
+#define CMDQ_OP_CODE_MASK		(0xff << CMDQ_OP_CODE_SHIFT)
+#define CMDQ_TIMEOUT_MS			1000
+#define CMDQ_IRQ_MASK			0xffff
+#define CMDQ_NUM_CMD(t)			(t->cmd_buf_size / CMDQ_INST_SIZE)
+
+#define CMDQ_CURR_IRQ_STATUS		0x10
+#define CMDQ_THR_SLOT_CYCLES		0x30
+#define CMDQ_THR_BASE			0x100
+#define CMDQ_THR_SIZE			0x80
+#define CMDQ_THR_WARM_RESET		0x00
+#define CMDQ_THR_ENABLE_TASK		0x04
+#define CMDQ_THR_SUSPEND_TASK		0x08
+#define CMDQ_THR_CURR_STATUS		0x0c
+#define CMDQ_THR_IRQ_STATUS		0x10
+#define CMDQ_THR_IRQ_ENABLE		0x14
+#define CMDQ_THR_CURR_ADDR		0x20
+#define CMDQ_THR_END_ADDR		0x24
+#define CMDQ_THR_WAIT_TOKEN		0x30
+#define CMDQ_THR_PRIORITY		0x40
+
+#define CMDQ_NO_TIMEOUT			0xffffffffu
+#define CMDQ_THR_ACTIVE_SLOT_CYCLES	0x3200
+#define CMDQ_THR_ENABLED		0x1
+#define CMDQ_THR_DISABLED		0x0
+#define CMDQ_THR_SUSPEND		0x1
+#define CMDQ_THR_RESUME			0x0
+#define CMDQ_THR_STATUS_SUSPENDED	BIT(1)
+#define CMDQ_THR_DO_WARM_RESET		BIT(0)
+#define CMDQ_THR_IRQ_DONE		0x1
+#define CMDQ_THR_IRQ_ERROR		0x12
+#define CMDQ_THR_IRQ_EN			(CMDQ_THR_IRQ_ERROR | CMDQ_THR_IRQ_DONE)
+#define CMDQ_THR_IS_WAITING		BIT(31)
+
+#define CMDQ_JUMP_BY_OFFSET		0x10000000
+#define CMDQ_JUMP_BY_PA			0x10000001
+
+struct cmdq_thread {
+	struct mbox_chan	*chan;
+	void __iomem		*base;
+	struct list_head	task_busy_list;
+	struct timer_list	timeout;
+	u32			timeout_ms;
+	u32			priority;
+	bool			atomic_exec;
+};
+
+struct cmdq_task {
+	struct cmdq		*cmdq;
+	struct list_head	list_entry;
+	dma_addr_t		pa_base;
+	struct cmdq_thread	*thread;
+	struct cmdq_pkt		*pkt; /* the packet sent from mailbox client */
+};
+
+struct cmdq {
+	struct mbox_controller	mbox;
+	void __iomem		*base;
+	u32			irq;
+	u32			thread_nr;
+	struct cmdq_thread	*thread;
+	struct clk		*clock;
+	bool			suspended;
+};
+
+static int cmdq_thread_suspend(struct cmdq *cmdq, struct cmdq_thread *thread)
+{
+	u32 status;
+
+	writel(CMDQ_THR_SUSPEND, thread->base + CMDQ_THR_SUSPEND_TASK);
+
+	/* If already disabled, treat as suspended successful. */
+	if (!(readl(thread->base + CMDQ_THR_ENABLE_TASK) & CMDQ_THR_ENABLED))
+		return 0;
+
+	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_CURR_STATUS,
+			status, status & CMDQ_THR_STATUS_SUSPENDED, 0, 10)) {
+		dev_err(cmdq->mbox.dev, "suspend GCE thread 0x%x failed\n",
+			(u32)(thread->base - cmdq->base));
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
+static void cmdq_thread_resume(struct cmdq_thread *thread)
+{
+	writel(CMDQ_THR_RESUME, thread->base + CMDQ_THR_SUSPEND_TASK);
+}
+
+static int cmdq_thread_reset(struct cmdq *cmdq, struct cmdq_thread *thread)
+{
+	u32 warm_reset;
+
+	writel(CMDQ_THR_DO_WARM_RESET, thread->base + CMDQ_THR_WARM_RESET);
+	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_WARM_RESET,
+			warm_reset, !(warm_reset & CMDQ_THR_DO_WARM_RESET),
+			0, 10)) {
+		dev_err(cmdq->mbox.dev, "reset GCE thread 0x%x failed\n",
+			(u32)(thread->base - cmdq->base));
+		return -EFAULT;
+	}
+	writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base + CMDQ_THR_SLOT_CYCLES);
+
+	return 0;
+}
+
+static void cmdq_thread_disable(struct cmdq *cmdq, struct cmdq_thread *thread)
+{
+	cmdq_thread_reset(cmdq, thread);
+	writel(CMDQ_THR_DISABLED, thread->base + CMDQ_THR_ENABLE_TASK);
+}
+
+/* notify GCE to re-fetch commands by setting GCE thread PC */
+static void cmdq_thread_invalidate_fetched_data(struct cmdq_thread *thread)
+{
+	writel(readl(thread->base + CMDQ_THR_CURR_ADDR),
+	       thread->base + CMDQ_THR_CURR_ADDR);
+}
+
+static void cmdq_task_insert_into_thread(struct cmdq_task *task)
+{
+	struct device *dev = task->cmdq->mbox.dev;
+	struct cmdq_thread *thread = task->thread;
+	struct cmdq_task *prev_task = list_last_entry(
+			&thread->task_busy_list, typeof(*task), list_entry);
+	u64 *prev_task_base = prev_task->pkt->va_base;
+
+	/* let previous task jump to this task */
+	dma_sync_single_for_cpu(dev, prev_task->pa_base,
+				prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
+	prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] =
+		(u64)CMDQ_JUMP_BY_PA << 32 | task->pa_base;
+	dma_sync_single_for_device(dev, prev_task->pa_base,
+				   prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
+
+	cmdq_thread_invalidate_fetched_data(thread);
+}
+
+static bool cmdq_command_is_wfe(u64 cmd)
+{
+	u64 wfe_option = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
+	u64 wfe_op = (u64)(CMDQ_CODE_WFE << CMDQ_OP_CODE_SHIFT) << 32;
+	u64 wfe_mask = (u64)CMDQ_OP_CODE_MASK << 32 | 0xffffffff;
+
+	return ((cmd & wfe_mask) == (wfe_op | wfe_option));
+}
+
+/* we assume tasks in the same display GCE thread are waiting the same event. */
+static void cmdq_task_remove_wfe(struct cmdq_task *task)
+{
+	struct device *dev = task->cmdq->mbox.dev;
+	u64 *base = task->pkt->va_base;
+	int i;
+
+	dma_sync_single_for_cpu(dev, task->pa_base, task->pkt->cmd_buf_size,
+				DMA_TO_DEVICE);
+	for (i = 0; i < CMDQ_NUM_CMD(task->pkt); i++)
+		if (cmdq_command_is_wfe(base[i]))
+			base[i] = (u64)CMDQ_JUMP_BY_OFFSET << 32 |
+				  CMDQ_JUMP_PASS;
+	dma_sync_single_for_device(dev, task->pa_base, task->pkt->cmd_buf_size,
+				   DMA_TO_DEVICE);
+}
+
+static bool cmdq_thread_is_in_wfe(struct cmdq_thread *thread)
+{
+	return readl(thread->base + CMDQ_THR_WAIT_TOKEN) & CMDQ_THR_IS_WAITING;
+}
+
+static void cmdq_thread_wait_end(struct cmdq_thread *thread,
+				 unsigned long end_pa)
+{
+	struct device *dev = thread->chan->mbox->dev;
+	unsigned long curr_pa;
+
+	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_CURR_ADDR,
+			curr_pa, curr_pa == end_pa, 1, 20))
+		dev_err(dev, "GCE thread cannot run to end.\n");
+}
+
+static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
+{
+	struct cmdq *cmdq;
+	struct cmdq_task *task;
+	unsigned long curr_pa, end_pa;
+
+	cmdq = dev_get_drvdata(thread->chan->mbox->dev);
+
+	/* Client should not flush new tasks if suspended. */
+	WARN_ON(cmdq->suspended);
+
+	task = kzalloc(sizeof(*task), GFP_ATOMIC);
+	task->cmdq = cmdq;
+	INIT_LIST_HEAD(&task->list_entry);
+	task->pa_base = pkt->pa_base;
+	task->thread = thread;
+	task->pkt = pkt;
+
+	if (list_empty(&thread->task_busy_list)) {
+		WARN_ON(clk_enable(cmdq->clock) < 0);
+		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
+
+		writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
+		writel(task->pa_base + pkt->cmd_buf_size,
+		       thread->base + CMDQ_THR_END_ADDR);
+		writel(thread->priority, thread->base + CMDQ_THR_PRIORITY);
+		writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
+		writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
+
+		if (thread->timeout_ms != CMDQ_NO_TIMEOUT)
+			mod_timer(&thread->timeout,
+				jiffies + msecs_to_jiffies(thread->timeout_ms));
+	} else {
+		WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
+		curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR);
+		end_pa = readl(thread->base + CMDQ_THR_END_ADDR);
+
+		/*
+		 * Atomic execution should remove the following wfe, i.e. only
+		 * wait event at first task, and prevent to pause when running.
+		 */
+		if (thread->atomic_exec) {
+			/* GCE is executing if command is not WFE */
+			if (!cmdq_thread_is_in_wfe(thread)) {
+				cmdq_thread_resume(thread);
+				cmdq_thread_wait_end(thread, end_pa);
+				WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
+				/* set to this task directly */
+				writel(task->pa_base,
+				       thread->base + CMDQ_THR_CURR_ADDR);
+			} else {
+				cmdq_task_insert_into_thread(task);
+				cmdq_task_remove_wfe(task);
+				smp_mb(); /* modify jump before enable thread */
+			}
+		} else {
+			/* check boundary */
+			if (curr_pa == end_pa - CMDQ_INST_SIZE ||
+			    curr_pa == end_pa) {
+				/* set to this task directly */
+				writel(task->pa_base,
+				       thread->base + CMDQ_THR_CURR_ADDR);
+			} else {
+				cmdq_task_insert_into_thread(task);
+				smp_mb(); /* modify jump before enable thread */
+			}
+		}
+		writel(task->pa_base + pkt->cmd_buf_size,
+		       thread->base + CMDQ_THR_END_ADDR);
+		cmdq_thread_resume(thread);
+	}
+	list_move_tail(&task->list_entry, &thread->task_busy_list);
+}
+
+static void cmdq_task_exec_done(struct cmdq_task *task, bool err)
+{
+	struct device *dev = task->cmdq->mbox.dev;
+	struct cmdq_cb_data cmdq_cb_data;
+
+	dma_unmap_single(dev, task->pa_base, task->pkt->cmd_buf_size,
+			 DMA_TO_DEVICE);
+	if (task->pkt->cb.cb) {
+		cmdq_cb_data.err = err;
+		cmdq_cb_data.data = task->pkt->cb.data;
+		task->pkt->cb.cb(cmdq_cb_data);
+	}
+	list_del(&task->list_entry);
+}
+
+static void cmdq_task_handle_error(struct cmdq_task *task)
+{
+	struct cmdq_thread *thread = task->thread;
+	struct cmdq_task *next_task;
+
+	dev_err(task->cmdq->mbox.dev, "task 0x%p error\n", task);
+	WARN_ON(cmdq_thread_suspend(task->cmdq, thread) < 0);
+	next_task = list_first_entry_or_null(&thread->task_busy_list,
+			struct cmdq_task, list_entry);
+	if (next_task)
+		writel(next_task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
+	cmdq_thread_resume(thread);
+}
+
+static void cmdq_thread_irq_handler(struct cmdq *cmdq,
+				    struct cmdq_thread *thread)
+{
+	struct cmdq_task *task, *tmp, *curr_task = NULL;
+	u32 curr_pa, irq_flag, task_end_pa;
+	bool err;
+
+	irq_flag = readl(thread->base + CMDQ_THR_IRQ_STATUS);
+	writel(~irq_flag, thread->base + CMDQ_THR_IRQ_STATUS);
+
+	/*
+	 * When ISR call this function, another CPU core could run
+	 * "release task" right before we acquire the spin lock, and thus
+	 * reset / disable this GCE thread, so we need to check the enable
+	 * bit of this GCE thread.
+	 */
+	if (!(readl(thread->base + CMDQ_THR_ENABLE_TASK) & CMDQ_THR_ENABLED))
+		return;
+
+	if (irq_flag & CMDQ_THR_IRQ_ERROR)
+		err = true;
+	else if (irq_flag & CMDQ_THR_IRQ_DONE)
+		err = false;
+	else
+		return;
+
+	curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR);
+
+	list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
+				 list_entry) {
+		task_end_pa = task->pa_base + task->pkt->cmd_buf_size;
+		if (curr_pa >= task->pa_base && curr_pa < task_end_pa)
+			curr_task = task;
+
+		if (!curr_task || curr_pa == task_end_pa - CMDQ_INST_SIZE) {
+			cmdq_task_exec_done(task, false);
+			kfree(task);
+		} else if (err) {
+			cmdq_task_exec_done(task, true);
+			cmdq_task_handle_error(curr_task);
+			kfree(task);
+		}
+
+		if (curr_task)
+			break;
+	}
+
+	if (list_empty(&thread->task_busy_list)) {
+		cmdq_thread_disable(cmdq, thread);
+		clk_disable(cmdq->clock);
+	} else {
+		if (thread->timeout_ms != CMDQ_NO_TIMEOUT)
+			mod_timer(&thread->timeout,
+				jiffies + msecs_to_jiffies(thread->timeout_ms));
+	}
+}
+
+static irqreturn_t cmdq_irq_handler(int irq, void *dev)
+{
+	struct cmdq *cmdq = dev;
+	unsigned long irq_status, flags = 0L;
+	int bit;
+
+	irq_status = readl(cmdq->base + CMDQ_CURR_IRQ_STATUS) & CMDQ_IRQ_MASK;
+	if (!(irq_status ^ CMDQ_IRQ_MASK))
+		return IRQ_NONE;
+
+	for_each_clear_bit(bit, &irq_status, fls(CMDQ_IRQ_MASK)) {
+		struct cmdq_thread *thread = &cmdq->thread[bit];
+
+		spin_lock_irqsave(&thread->chan->lock, flags);
+		cmdq_thread_irq_handler(cmdq, thread);
+		spin_unlock_irqrestore(&thread->chan->lock, flags);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void cmdq_thread_handle_timeout(struct timer_list *t)
+{
+	struct cmdq_thread *thread = from_timer(thread, t, timeout);
+	struct cmdq *cmdq = container_of(thread->chan->mbox, struct cmdq, mbox);
+	struct cmdq_task *task, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&thread->chan->lock, flags);
+	WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
+
+	/*
+	 * Although IRQ is disabled, GCE continues to execute.
+	 * It may have pending IRQ before GCE thread is suspended,
+	 * so check this condition again.
+	 */
+	cmdq_thread_irq_handler(cmdq, thread);
+
+	if (list_empty(&thread->task_busy_list)) {
+		cmdq_thread_resume(thread);
+		spin_unlock_irqrestore(&thread->chan->lock, flags);
+		return;
+	}
+
+	dev_err(cmdq->mbox.dev, "timeout\n");
+	list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
+				 list_entry) {
+		cmdq_task_exec_done(task, true);
+		kfree(task);
+	}
+
+	cmdq_thread_resume(thread);
+	cmdq_thread_disable(cmdq, thread);
+	clk_disable(cmdq->clock);
+	spin_unlock_irqrestore(&thread->chan->lock, flags);
+}
+
+static int cmdq_suspend(struct device *dev)
+{
+	struct cmdq *cmdq = dev_get_drvdata(dev);
+	struct cmdq_thread *thread;
+	int i;
+	bool task_running = false;
+
+	cmdq->suspended = true;
+
+	for (i = 0; i < cmdq->thread_nr; i++) {
+		thread = &cmdq->thread[i];
+		if (!list_empty(&thread->task_busy_list)) {
+			task_running = true;
+			break;
+		}
+	}
+
+	if (task_running)
+		dev_warn(dev, "exist running task(s) in suspend\n");
+
+	clk_unprepare(cmdq->clock);
+
+	return 0;
+}
+
+static int cmdq_resume(struct device *dev)
+{
+	struct cmdq *cmdq = dev_get_drvdata(dev);
+
+	WARN_ON(clk_prepare(cmdq->clock) < 0);
+	cmdq->suspended = false;
+	return 0;
+}
+
+static int cmdq_remove(struct platform_device *pdev)
+{
+	struct cmdq *cmdq = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&cmdq->mbox);
+	clk_unprepare(cmdq->clock);
+
+	if (cmdq->mbox.chans)
+		devm_kfree(&pdev->dev, cmdq->mbox.chans);
+
+	if (cmdq->thread)
+		devm_kfree(&pdev->dev, cmdq->thread);
+
+	devm_kfree(&pdev->dev, cmdq);
+
+	return 0;
+}
+
+static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	cmdq_task_exec(data, chan->con_priv);
+
+	return 0;
+}
+
+static int cmdq_mbox_startup(struct mbox_chan *chan)
+{
+	return 0;
+}
+
+static void cmdq_mbox_shutdown(struct mbox_chan *chan)
+{
+}
+
+static bool cmdq_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	return true;
+}
+
+static const struct mbox_chan_ops cmdq_mbox_chan_ops = {
+	.send_data = cmdq_mbox_send_data,
+	.startup = cmdq_mbox_startup,
+	.shutdown = cmdq_mbox_shutdown,
+	.last_tx_done = cmdq_mbox_last_tx_done,
+};
+
+static struct mbox_chan *cmdq_xlate(struct mbox_controller *mbox,
+		const struct of_phandle_args *sp)
+{
+	int ind = sp->args[0];
+	struct cmdq_thread *thread;
+
+	if (ind >= mbox->num_chans)
+		return ERR_PTR(-EINVAL);
+
+	thread = mbox->chans[ind].con_priv;
+	thread->timeout_ms = sp->args[1];
+	thread->priority = sp->args[2];
+	thread->atomic_exec = (sp->args[3] != 0);
+	thread->chan = &mbox->chans[ind];
+
+	return &mbox->chans[ind];
+}
+
+static int cmdq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct cmdq *cmdq;
+	int err, i;
+
+	cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
+	if (!cmdq)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cmdq->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(cmdq->base)) {
+		dev_err(dev, "failed to ioremap gce\n");
+		return PTR_ERR(cmdq->base);
+	}
+
+	cmdq->irq = platform_get_irq(pdev, 0);
+	if (!cmdq->irq) {
+		dev_err(dev, "failed to get irq\n");
+		return -EINVAL;
+	}
+	err = devm_request_irq(dev, cmdq->irq, cmdq_irq_handler, IRQF_SHARED,
+			       "mtk_cmdq", cmdq);
+	if (err < 0) {
+		dev_err(dev, "failed to register ISR (%d)\n", err);
+		return err;
+	}
+
+	dev_dbg(dev, "cmdq device: addr:0x%p, va:0x%p, irq:%d\n",
+		dev, cmdq->base, cmdq->irq);
+
+	cmdq->clock = devm_clk_get(dev, "gce");
+	if (IS_ERR(cmdq->clock)) {
+		dev_err(dev, "failed to get gce clk\n");
+		return PTR_ERR(cmdq->clock);
+	}
+
+	err = of_property_read_u32(dev->of_node, "thread-num",
+				   &cmdq->thread_nr);
+	if (err < 0) {
+		dev_err(dev, "failed to read thread number.\n");
+		return err;
+	}
+
+	cmdq->mbox.dev = dev;
+	cmdq->mbox.chans = devm_kcalloc(dev, cmdq->thread_nr,
+					sizeof(*cmdq->mbox.chans), GFP_KERNEL);
+	if (!cmdq->mbox.chans)
+		return -ENOMEM;
+
+	cmdq->mbox.num_chans = cmdq->thread_nr;
+	cmdq->mbox.ops = &cmdq_mbox_chan_ops;
+	cmdq->mbox.of_xlate = cmdq_xlate;
+
+	/* make use of TXDONE_BY_ACK */
+	cmdq->mbox.txdone_irq = false;
+	cmdq->mbox.txdone_poll = false;
+
+	cmdq->thread = devm_kcalloc(dev, cmdq->thread_nr,
+					sizeof(*cmdq->thread), GFP_KERNEL);
+	if (!cmdq->thread)
+		return -ENOMEM;
+
+	for (i = 0; i < cmdq->thread_nr; i++) {
+		cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
+				CMDQ_THR_SIZE * i;
+		INIT_LIST_HEAD(&cmdq->thread[i].task_busy_list);
+		if (cmdq->thread[i].timeout_ms != CMDQ_NO_TIMEOUT)
+			timer_setup(&cmdq->thread[i].timeout,
+				cmdq_thread_handle_timeout, 0);
+		cmdq->mbox.chans[i].con_priv = &cmdq->thread[i];
+	}
+
+	err = mbox_controller_register(&cmdq->mbox);
+	if (err < 0) {
+		dev_err(dev, "failed to register mailbox: %d\n", err);
+		return err;
+	}
+
+	platform_set_drvdata(pdev, cmdq);
+	WARN_ON(clk_prepare(cmdq->clock) < 0);
+
+	return 0;
+}
+
+static const struct dev_pm_ops cmdq_pm_ops = {
+	.suspend = cmdq_suspend,
+	.resume = cmdq_resume,
+};
+
+static const struct of_device_id cmdq_of_ids[] = {
+	{.compatible = "mediatek,mt8173-gce"},
+	{}
+};
+
+static struct platform_driver cmdq_drv = {
+	.probe = cmdq_probe,
+	.remove = cmdq_remove,
+	.driver = {
+		.name = "mtk_cmdq",
+		.pm = &cmdq_pm_ops,
+		.of_match_table = cmdq_of_ids,
+	}
+};
+
+static int __init cmdq_drv_init(void)
+{
+	return platform_driver_register(&cmdq_drv);
+}
+
+static void __exit cmdq_drv_exit(void)
+{
+	platform_driver_unregister(&cmdq_drv);
+}
+
+subsys_initcall(cmdq_drv_init);
+module_exit(cmdq_drv_exit);
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
new file mode 100644
index 0000000..1e4a891
--- /dev/null
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -0,0 +1,70 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ *
+ */
+
+#ifndef __MTK_CMDQ_MAILBOX_H__
+#define __MTK_CMDQ_MAILBOX_H__
+
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define CMDQ_INST_SIZE			8 /* instruction is 64-bit */
+#define CMDQ_SUBSYS_SHIFT		16
+#define CMDQ_OP_CODE_SHIFT		24
+#define CMDQ_JUMP_PASS			CMDQ_INST_SIZE
+
+#define CMDQ_WFE_UPDATE			BIT(31)
+#define CMDQ_WFE_WAIT			BIT(15)
+#define CMDQ_WFE_WAIT_VALUE		0x1
+
+/*
+ * CMDQ_CODE_MASK:
+ *   set write mask
+ *   format: op mask
+ * CMDQ_CODE_WRITE:
+ *   write value into target register
+ *   format: op subsys address value
+ * CMDQ_CODE_JUMP:
+ *   jump by offset
+ *   format: op offset
+ * CMDQ_CODE_WFE:
+ *   wait for event and clear
+ *   it is just clear if no wait
+ *   format: [wait]  op event update:1 to_wait:1 wait:1
+ *           [clear] op event update:1 to_wait:0 wait:0
+ * CMDQ_CODE_EOC:
+ *   end of command
+ *   format: op irq_flag
+ */
+enum cmdq_code {
+	CMDQ_CODE_MASK = 0x02,
+	CMDQ_CODE_WRITE = 0x04,
+	CMDQ_CODE_JUMP = 0x10,
+	CMDQ_CODE_WFE = 0x20,
+	CMDQ_CODE_EOC = 0x40,
+};
+
+struct cmdq_cb_data {
+	bool	err;
+	void	*data;
+};
+
+typedef void (*cmdq_async_flush_cb)(struct cmdq_cb_data data);
+
+struct cmdq_task_cb {
+	cmdq_async_flush_cb	cb;
+	void			*data;
+};
+
+struct cmdq_pkt {
+	void			*va_base;
+	dma_addr_t		pa_base;
+	size_t			cmd_buf_size; /* command occupied size */
+	size_t			buf_size; /* real buffer size */
+	struct cmdq_task_cb	cb;
+};
+
+#endif /* __MTK_CMDQ_MAILBOX_H__ */
-- 
1.7.9.5

[PATCH v22 3/4] arm64: dts: mt8173: Add GCE node

[Houlong Wei]

This patch adds the device node of the GCE hardware for CMDQ module.

Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
Signed-off-by: HS Liao <hs.liao@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 94597e3..d180a6d 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -18,6 +18,7 @@
 #include <dt-bindings/phy/phy.h>
 #include <dt-bindings/power/mt8173-power.h>
 #include <dt-bindings/reset/mt8173-resets.h>
+#include <dt-bindings/gce/mt8173-gce.h>
 #include "mt8173-pinfunc.h"
 
 / {
@@ -519,6 +520,16 @@
 			status = "disabled";
 		};
 
+		gce: gce@10212000 {
+			compatible = "mediatek,mt8173-gce";
+			reg = <0 0x10212000 0 0x1000>;
+			interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
+			clocks = <&infracfg CLK_INFRA_GCE>;
+			clock-names = "gce";
+			thread-num = <CMDQ_THR_MAX_COUNT>;
+			#mbox-cells = <4>;
+		};
+
 		mipi_tx0: mipi-dphy@10215000 {
 			compatible = "mediatek,mt8173-mipi-tx";
 			reg = <0 0x10215000 0 0x1000>;
-- 
1.7.9.5

[PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper

[Houlong Wei]

Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.

Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
Signed-off-by: HS Liao <hs.liao@mediatek.com>
---
 drivers/soc/mediatek/Kconfig           |   12 ++
 drivers/soc/mediatek/Makefile          |    1 +
 drivers/soc/mediatek/mtk-cmdq-helper.c |  258 ++++++++++++++++++++++++++++++++
 include/linux/soc/mediatek/mtk-cmdq.h  |  132 ++++++++++++++++
 4 files changed, 403 insertions(+)
 create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
 create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h

diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
index a7d0667..17bd759 100644
--- a/drivers/soc/mediatek/Kconfig
+++ b/drivers/soc/mediatek/Kconfig
@@ -4,6 +4,18 @@
 menu "MediaTek SoC drivers"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
 
+config MTK_CMDQ
+	tristate "MediaTek CMDQ Support"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	select MAILBOX
+	select MTK_CMDQ_MBOX
+	select MTK_INFRACFG
+	help
+	  Say yes here to add support for the MediaTek Command Queue (CMDQ)
+	  driver. The CMDQ is used to help read/write registers with critical
+	  time limitation, such as updating display configuration during the
+	  vblank.
+
 config MTK_INFRACFG
 	bool "MediaTek INFRACFG Support"
 	select REGMAP
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index 12998b0..64ce5ee 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
 obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
 obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
 obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
new file mode 100644
index 0000000..6c66091
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -0,0 +1,258 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ *
+ */
+
+#include <linux/completion.h>
+#include <linux/errno.h>
+#include <linux/dma-mapping.h>
+#include <linux/mailbox_controller.h>
+#include <linux/soc/mediatek/mtk-cmdq.h>
+
+#define CMDQ_ARG_A_WRITE_MASK	0xffff
+#define CMDQ_WRITE_ENABLE_MASK	BIT(0)
+#define CMDQ_EOC_IRQ_EN		BIT(0)
+#define CMDQ_EOC_CMD		((u64)((CMDQ_CODE_EOC << CMDQ_OP_CODE_SHIFT)) \
+				<< 32 | CMDQ_EOC_IRQ_EN)
+
+int cmdq_pkt_realloc_cmd_buffer(struct cmdq_pkt *pkt, size_t size)
+{
+	void *new_buf;
+
+	new_buf = krealloc(pkt->va_base, size, GFP_KERNEL | __GFP_ZERO);
+	if (!new_buf)
+		return -ENOMEM;
+	pkt->va_base = new_buf;
+	pkt->buf_size = size;
+
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_pkt_realloc_cmd_buffer);
+
+struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
+{
+	struct cmdq_client *client;
+	long err = 0;
+
+	client = kzalloc(sizeof(*client), GFP_KERNEL);
+	if (!client)
+		return (struct cmdq_client *)-ENOMEM;
+
+	client->client.dev = dev;
+	client->client.tx_block = false;
+	client->chan = mbox_request_channel(&client->client, index);
+
+	if (IS_ERR(client->chan)) {
+		dev_err(dev, "failed to request channel\n");
+		err = PTR_ERR(client->chan);
+		kfree(client);
+
+		return (struct cmdq_client *)err;
+	}
+
+	return client;
+}
+EXPORT_SYMBOL(cmdq_mbox_create);
+
+void cmdq_mbox_destroy(struct cmdq_client *client)
+{
+	mbox_free_channel(client->chan);
+	kfree(client);
+}
+EXPORT_SYMBOL(cmdq_mbox_destroy);
+
+int cmdq_pkt_create(struct cmdq_pkt **pkt_ptr)
+{
+	struct cmdq_pkt *pkt;
+	int err;
+
+	pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
+	if (!pkt)
+		return -ENOMEM;
+	err = cmdq_pkt_realloc_cmd_buffer(pkt, PAGE_SIZE);
+	if (err < 0) {
+		kfree(pkt);
+		return err;
+	}
+	*pkt_ptr = pkt;
+
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_pkt_create);
+
+void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
+{
+	kfree(pkt->va_base);
+	kfree(pkt);
+}
+EXPORT_SYMBOL(cmdq_pkt_destroy);
+
+static bool cmdq_pkt_is_finalized(struct cmdq_pkt *pkt)
+{
+	u64 *expect_eoc;
+
+	if (pkt->cmd_buf_size < CMDQ_INST_SIZE << 1)
+		return false;
+
+	expect_eoc = pkt->va_base + pkt->cmd_buf_size - (CMDQ_INST_SIZE << 1);
+	if (*expect_eoc == CMDQ_EOC_CMD)
+		return true;
+
+	return false;
+}
+
+static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
+				   u32 arg_a, u32 arg_b)
+{
+	u64 *cmd_ptr;
+	int err;
+
+	if (WARN_ON(cmdq_pkt_is_finalized(pkt)))
+		return -EBUSY;
+	if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) {
+		err = cmdq_pkt_realloc_cmd_buffer(pkt, pkt->buf_size << 1);
+		if (err < 0)
+			return err;
+	}
+	cmd_ptr = pkt->va_base + pkt->cmd_buf_size;
+	(*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
+	pkt->cmd_buf_size += CMDQ_INST_SIZE;
+
+	return 0;
+}
+
+int cmdq_pkt_write(struct cmdq_pkt *pkt, u32 value, u32 subsys, u32 offset)
+{
+	u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) |
+		    (subsys << CMDQ_SUBSYS_SHIFT);
+
+	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value);
+}
+EXPORT_SYMBOL(cmdq_pkt_write);
+
+int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u32 value,
+			u32 subsys, u32 offset, u32 mask)
+{
+	u32 offset_mask = offset;
+	int err;
+
+	if (mask != 0xffffffff) {
+		err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask);
+		if (err < 0)
+			return err;
+		offset_mask |= CMDQ_WRITE_ENABLE_MASK;
+	}
+
+	return cmdq_pkt_write(pkt, value, subsys, offset_mask);
+}
+EXPORT_SYMBOL(cmdq_pkt_write_mask);
+
+int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event)
+{
+	u32 arg_b;
+
+	if (event >= CMDQ_MAX_EVENT || event < 0)
+		return -EINVAL;
+
+	/*
+	 * WFE arg_b
+	 * bit 0-11: wait value
+	 * bit 15: 1 - wait, 0 - no wait
+	 * bit 16-27: update value
+	 * bit 31: 1 - update, 0 - no update
+	 */
+	arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
+
+	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b);
+}
+EXPORT_SYMBOL(cmdq_pkt_wfe);
+
+int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event)
+{
+	if (event >= CMDQ_MAX_EVENT || event < 0)
+		return -EINVAL;
+
+	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event,
+				       CMDQ_WFE_UPDATE);
+}
+EXPORT_SYMBOL(cmdq_pkt_clear_event);
+
+static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
+{
+	int err;
+
+	if (cmdq_pkt_is_finalized(pkt))
+		return 0;
+
+	/* insert EOC and generate IRQ for each command iteration */
+	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
+	if (err < 0)
+		return err;
+
+	/* JUMP to end */
+	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
+			 cmdq_async_flush_cb cb, void *data)
+{
+	int err;
+	struct device *dev;
+	dma_addr_t dma_addr;
+
+	err = cmdq_pkt_finalize(pkt);
+	if (err < 0)
+		return err;
+
+	dev = client->chan->mbox->dev;
+	dma_addr = dma_map_single(dev, pkt->va_base, pkt->cmd_buf_size,
+		DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, dma_addr)) {
+		dev_err(client->chan->mbox->dev, "dma map failed\n");
+		return -ENOMEM;
+	}
+
+	pkt->pa_base = dma_addr;
+	pkt->cb.cb = cb;
+	pkt->cb.data = data;
+
+	mbox_send_message(client->chan, pkt);
+	/* We can send next packet immediately, so just call txdone. */
+	mbox_client_txdone(client->chan, 0);
+
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_pkt_flush_async);
+
+struct cmdq_flush_completion {
+	struct completion cmplt;
+	bool err;
+};
+
+static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
+{
+	struct cmdq_flush_completion *cmplt = data.data;
+
+	cmplt->err = data.err;
+	complete(&cmplt->cmplt);
+}
+
+int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt)
+{
+	struct cmdq_flush_completion cmplt;
+	int err;
+
+	init_completion(&cmplt.cmplt);
+	err = cmdq_pkt_flush_async(client, pkt, cmdq_pkt_flush_cb, &cmplt);
+	if (err < 0)
+		return err;
+	wait_for_completion(&cmplt.cmplt);
+
+	return cmplt.err ? -EFAULT : 0;
+}
+EXPORT_SYMBOL(cmdq_pkt_flush);
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
new file mode 100644
index 0000000..d7c8ef2
--- /dev/null
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -0,0 +1,132 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ *
+ */
+
+#ifndef __MTK_CMDQ_H__
+#define __MTK_CMDQ_H__
+
+#include <linux/mailbox_client.h>
+#include <linux/mailbox/mtk-cmdq-mailbox.h>
+
+/** cmdq event maximum */
+#define CMDQ_MAX_EVENT				0x3ff
+
+struct cmdq_pkt;
+
+struct cmdq_client {
+	struct mbox_client client;
+	struct mbox_chan *chan;
+};
+
+/**
+ * cmdq_mbox_create() - create CMDQ mailbox client and channel
+ * @dev:	device of CMDQ mailbox client
+ * @index:	index of CMDQ mailbox channel
+ *
+ * Return: CMDQ mailbox client pointer
+ */
+struct cmdq_client *cmdq_mbox_create(struct device *dev, int index);
+
+/**
+ * cmdq_mbox_destroy() - destroy CMDQ mailbox client and channel
+ * @client:	the CMDQ mailbox client
+ */
+void cmdq_mbox_destroy(struct cmdq_client *client);
+
+/**
+ * cmdq_pkt_create() - create a CMDQ packet
+ * @pkt_ptr:	CMDQ packet pointer to retrieve cmdq_pkt
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_create(struct cmdq_pkt **pkt_ptr);
+
+/**
+ * cmdq_pkt_destroy() - destroy the CMDQ packet
+ * @pkt:	the CMDQ packet
+ */
+void cmdq_pkt_destroy(struct cmdq_pkt *pkt);
+
+/**
+ * cmdq_pkt_realloc_cmd_buffer() - reallocate command buffer for CMDQ packet
+ * @pkt:	the CMDQ packet
+ * @size:	the request size
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_realloc_cmd_buffer(struct cmdq_pkt *pkt, size_t size);
+
+/**
+ * cmdq_pkt_write() - append write command to the CMDQ packet
+ * @pkt:	the CMDQ packet
+ * @value:	the specified target register value
+ * @subsys:	the CMDQ sub system code
+ * @offset:	register offset from CMDQ sub system
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_write(struct cmdq_pkt *pkt, u32 value, u32 subsys, u32 offset);
+
+/**
+ * cmdq_pkt_write_mask() - append write command with mask to the CMDQ packet
+ * @pkt:	the CMDQ packet
+ * @value:	the specified target register value
+ * @subsys:	the CMDQ sub system code
+ * @offset:	register offset from CMDQ sub system
+ * @mask:	the specified target register mask
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u32 value,
+			u32 subsys, u32 offset, u32 mask);
+
+/**
+ * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
+ * @pkt:	the CMDQ packet
+ * @event:	the desired event type to "wait and CLEAR"
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event);
+
+/**
+ * cmdq_pkt_clear_event() - append clear event command to the CMDQ packet
+ * @pkt:	the CMDQ packet
+ * @event:	the desired event to be cleared
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event);
+
+/**
+ * cmdq_pkt_flush() - trigger CMDQ to execute the CMDQ packet
+ * @client:	the CMDQ mailbox client
+ * @pkt:	the CMDQ packet
+ *
+ * Return: 0 for success; else the error code is returned
+ *
+ * Trigger CMDQ to execute the CMDQ packet. Note that this is a
+ * synchronous flush function. When the function returned, the recorded
+ * commands have been done.
+ */
+int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt);
+
+/**
+ * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
+ *                          packet and call back at the end of done packet
+ * @client:	the CMDQ mailbox client
+ * @pkt:	the CMDQ packet
+ * @cb:		called at the end of done packet
+ * @data:	this data will pass back to cb
+ *
+ * Return: 0 for success; else the error code is returned
+ *
+ * Trigger CMDQ to asynchronously execute the CMDQ packet and call back
+ * at the end of done packet. Note that this is an ASYNC function. When the
+ * function returned, it may or may not be finished.
+ */
+int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
+			 cmdq_async_flush_cb cb, void *data);
+
+#endif	/* __MTK_CMDQ_H__ */
-- 
1.7.9.5

Re: [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper

[CK Hu]

Hi, Houlong:

Some inline comment.

On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> 
> Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> Signed-off-by: HS Liao <hs.liao@mediatek.com>
> ---
>  drivers/soc/mediatek/Kconfig           |   12 ++
>  drivers/soc/mediatek/Makefile          |    1 +
>  drivers/soc/mediatek/mtk-cmdq-helper.c |  258 ++++++++++++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  |  132 ++++++++++++++++
>  4 files changed, 403 insertions(+)
>  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
>  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> 
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index a7d0667..17bd759 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -4,6 +4,18 @@
>  menu "MediaTek SoC drivers"
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
>  

[...]

> +
> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event)
> +{
> +	u32 arg_b;
> +
> +	if (event >= CMDQ_MAX_EVENT || event < 0)

The type of event is 'u32', so checking 'event < 0' is redundant.

> +		return -EINVAL;
> +
> +	/*
> +	 * WFE arg_b
> +	 * bit 0-11: wait value
> +	 * bit 15: 1 - wait, 0 - no wait
> +	 * bit 16-27: update value
> +	 * bit 31: 1 - update, 0 - no update
> +	 */
> +	arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
> +
> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_wfe);
> +
> +int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event)
> +{
> +	if (event >= CMDQ_MAX_EVENT || event < 0)

The type of event is 'u32', so checking 'event < 0' is redundant.

> +		return -EINVAL;
> +
> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event,
> +				       CMDQ_WFE_UPDATE);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_clear_event);
> +
> +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> +{
> +	int err;
> +
> +	if (cmdq_pkt_is_finalized(pkt))
> +		return 0;
> +
> +	/* insert EOC and generate IRQ for each command iteration */
> +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> +	if (err < 0)
> +		return err;
> +
> +	/* JUMP to end */
> +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +
> +int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
> +			 cmdq_async_flush_cb cb, void *data)
> +{
> +	int err;
> +	struct device *dev;
> +	dma_addr_t dma_addr;
> +
> +	err = cmdq_pkt_finalize(pkt);
> +	if (err < 0)
> +		return err;
> +
> +	dev = client->chan->mbox->dev;
> +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->cmd_buf_size,
> +		DMA_TO_DEVICE);

You map here, but I could not find unmap, so the unmap should be done in
client driver. I would prefer a symmetric map/unmap which means that
both map and unmap are done in client driver. I think you put map here
because you should map after finalize. Therefore, export
cmdq_pkt_finalize() to client driver and let client do finalize, so
there is no finalize in flush function. This method have a benefit that
if client reuse command buffer, it need not to map/unmap frequently.

Regards,
CK

> +	if (dma_mapping_error(dev, dma_addr)) {
> +		dev_err(client->chan->mbox->dev, "dma map failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	pkt->pa_base = dma_addr;
> +	pkt->cb.cb = cb;
> +	pkt->cb.data = data;
> +
> +	mbox_send_message(client->chan, pkt);
> +	/* We can send next packet immediately, so just call txdone. */
> +	mbox_client_txdone(client->chan, 0);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> +
> +struct cmdq_flush_completion {
> +	struct completion cmplt;
> +	bool err;
> +};
> +
> +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> +{
> +	struct cmdq_flush_completion *cmplt = data.data;
> +
> +	cmplt->err = data.err;
> +	complete(&cmplt->cmplt);
> +}
> +
> +int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_flush_completion cmplt;
> +	int err;
> +
> +	init_completion(&cmplt.cmplt);
> +	err = cmdq_pkt_flush_async(client, pkt, cmdq_pkt_flush_cb, &cmplt);
> +	if (err < 0)
> +		return err;
> +	wait_for_completion(&cmplt.cmplt);
> +
> +	return cmplt.err ? -EFAULT : 0;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_flush);

[...]

Re: [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper

[houlong wei]

On Thu, 2018-06-28 at 18:41 +0800, CK Hu wrote:
> Hi, Houlong:
> 
> Some inline comment.
> 
> On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> > 
> > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > ---
> >  drivers/soc/mediatek/Kconfig           |   12 ++
> >  drivers/soc/mediatek/Makefile          |    1 +
> >  drivers/soc/mediatek/mtk-cmdq-helper.c |  258 ++++++++++++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-cmdq.h  |  132 ++++++++++++++++
> >  4 files changed, 403 insertions(+)
> >  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> >  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> > 
> > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > index a7d0667..17bd759 100644
> > --- a/drivers/soc/mediatek/Kconfig
> > +++ b/drivers/soc/mediatek/Kconfig
> > @@ -4,6 +4,18 @@
> >  menu "MediaTek SoC drivers"
> >  	depends on ARCH_MEDIATEK || COMPILE_TEST
> >  
> 
> [...]
> 
> > +
> > +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event)
> > +{
> > +	u32 arg_b;
> > +
> > +	if (event >= CMDQ_MAX_EVENT || event < 0)
> 
> The type of event is 'u32', so checking 'event < 0' is redundant.

will fix.

> 
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * WFE arg_b
> > +	 * bit 0-11: wait value
> > +	 * bit 15: 1 - wait, 0 - no wait
> > +	 * bit 16-27: update value
> > +	 * bit 31: 1 - update, 0 - no update
> > +	 */
> > +	arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
> > +
> > +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_wfe);
> > +
> > +int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event)
> > +{
> > +	if (event >= CMDQ_MAX_EVENT || event < 0)
> 
> The type of event is 'u32', so checking 'event < 0' is redundant.

Will fix.

> 
> > +		return -EINVAL;
> > +
> > +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event,
> > +				       CMDQ_WFE_UPDATE);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_clear_event);
> > +
> > +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > +{
> > +	int err;
> > +
> > +	if (cmdq_pkt_is_finalized(pkt))
> > +		return 0;
> > +
> > +	/* insert EOC and generate IRQ for each command iteration */
> > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	/* JUMP to end */
> > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
> > +			 cmdq_async_flush_cb cb, void *data)
> > +{
> > +	int err;
> > +	struct device *dev;
> > +	dma_addr_t dma_addr;
> > +
> > +	err = cmdq_pkt_finalize(pkt);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	dev = client->chan->mbox->dev;
> > +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->cmd_buf_size,
> > +		DMA_TO_DEVICE);
> 
> You map here, but I could not find unmap, so the unmap should be done in
> client driver. I would prefer a symmetric map/unmap which means that
> both map and unmap are done in client driver. I think you put map here
> because you should map after finalize. 

The unmap is done before callback to client, in function
cmdq_task_exec_done, mtk-cmdq-mailbox.c.

> Therefore, export
> cmdq_pkt_finalize() to client driver and let client do finalize, so
> there is no finalize in flush function. This method have a benefit that
> if client reuse command buffer, it need not to map/unmap frequently.

If client reuse command buffer or cmdq_pkt(command queue packet), client
will add new commands to the cmdq_pkt, so map/unmap are necessary for
each cmdq_pkt flush.

> 
> Regards,
> CK
> 
> > +	if (dma_mapping_error(dev, dma_addr)) {
> > +		dev_err(client->chan->mbox->dev, "dma map failed\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	pkt->pa_base = dma_addr;
> > +	pkt->cb.cb = cb;
> > +	pkt->cb.data = data;
> > +
> > +	mbox_send_message(client->chan, pkt);
> > +	/* We can send next packet immediately, so just call txdone. */
> > +	mbox_client_txdone(client->chan, 0);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> > +
> > +struct cmdq_flush_completion {
> > +	struct completion cmplt;
> > +	bool err;
> > +};
> > +
> > +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> > +{
> > +	struct cmdq_flush_completion *cmplt = data.data;
> > +
> > +	cmplt->err = data.err;
> > +	complete(&cmplt->cmplt);
> > +}
> > +
> > +int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt)
> > +{
> > +	struct cmdq_flush_completion cmplt;
> > +	int err;
> > +
> > +	init_completion(&cmplt.cmplt);
> > +	err = cmdq_pkt_flush_async(client, pkt, cmdq_pkt_flush_cb, &cmplt);
> > +	if (err < 0)
> > +		return err;
> > +	wait_for_completion(&cmplt.cmplt);
> > +
> > +	return cmplt.err ? -EFAULT : 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_flush);
> 
> [...]
> 
> 

Re: [PATCH v22 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

[CK Hu]

Hi, Houlong:

Some inline comment.

On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> CMDQ is used to help write registers with critical time limitation,
> such as updating display configuration during the vblank. It controls
> Global Command Engine (GCE) hardware to achieve this requirement.
> Currently, CMDQ only supports display related hardwares, but we expect
> it can be extended to other hardwares for future requirements.
> 
> Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> Signed-off-by: HS Liao <hs.liao@mediatek.com>
> Signed-off-by: CK Hu <ck.hu@mediatek.com>
> ---
>  drivers/mailbox/Kconfig                  |   10 +
>  drivers/mailbox/Makefile                 |    2 +
>  drivers/mailbox/mtk-cmdq-mailbox.c       |  634 ++++++++++++++++++++++++++++++
>  include/linux/mailbox/mtk-cmdq-mailbox.h |   70 ++++
>  4 files changed, 716 insertions(+)
>  create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
>  create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h
> 

[...]

> +
> +static int cmdq_thread_reset(struct cmdq *cmdq, struct cmdq_thread *thread)
> +{
> +	u32 warm_reset;
> +
> +	writel(CMDQ_THR_DO_WARM_RESET, thread->base + CMDQ_THR_WARM_RESET);
> +	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_WARM_RESET,
> +			warm_reset, !(warm_reset & CMDQ_THR_DO_WARM_RESET),
> +			0, 10)) {
> +		dev_err(cmdq->mbox.dev, "reset GCE thread 0x%x failed\n",
> +			(u32)(thread->base - cmdq->base));
> +		return -EFAULT;
> +	}
> +	writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base + CMDQ_THR_SLOT_CYCLES);

The CMDQ_THR_SLOT_CYCLES looks like not relevant to reset. Maybe you
just need to set this value when startup.

> +
> +	return 0;
> +}
> +

[...]

> +
> +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> +{
> +	struct cmdq *cmdq;
> +	struct cmdq_task *task;
> +	unsigned long curr_pa, end_pa;
> +
> +	cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> +
> +	/* Client should not flush new tasks if suspended. */
> +	WARN_ON(cmdq->suspended);
> +
> +	task = kzalloc(sizeof(*task), GFP_ATOMIC);
> +	task->cmdq = cmdq;
> +	INIT_LIST_HEAD(&task->list_entry);
> +	task->pa_base = pkt->pa_base;
> +	task->thread = thread;
> +	task->pkt = pkt;
> +
> +	if (list_empty(&thread->task_busy_list)) {
> +		WARN_ON(clk_enable(cmdq->clock) < 0);
> +		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
> +
> +		writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
> +		writel(task->pa_base + pkt->cmd_buf_size,
> +		       thread->base + CMDQ_THR_END_ADDR);
> +		writel(thread->priority, thread->base + CMDQ_THR_PRIORITY);
> +		writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
> +		writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
> +
> +		if (thread->timeout_ms != CMDQ_NO_TIMEOUT)
> +			mod_timer(&thread->timeout,
> +				jiffies + msecs_to_jiffies(thread->timeout_ms));

I think the timeout processing should be done by client driver. The
total time to execute a command buffer does not depend on GCE HW speed
because the WFE (wait for event) command would wait for client HW event,
so the total time depend on how long a client HW send this event to GCE
and the timeout processing should be client driver's job. Each client
may have different timeout processing mechanism, for example, if display
could dynamic change panel frame rate between 120Hz and 60Hz, and the
timeout time is 2 frame, so it may dynamically change timeout time
between 17ms and 33ms. Another reason is that display have interrupt
every vblank, and it could check timeout in that interrupt, so the timer
in cmdq driver looks redundant. Because each client would define its own
timeout processing mechanism, so it's not wise to put timeout processing
in cmdq driver.

> +	} else {
> +		WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> +		curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR);
> +		end_pa = readl(thread->base + CMDQ_THR_END_ADDR);
> +
> +		/*
> +		 * Atomic execution should remove the following wfe, i.e. only
> +		 * wait event at first task, and prevent to pause when running.
> +		 */
> +		if (thread->atomic_exec) {
> +			/* GCE is executing if command is not WFE */
> +			if (!cmdq_thread_is_in_wfe(thread)) {
> +				cmdq_thread_resume(thread);
> +				cmdq_thread_wait_end(thread, end_pa);
> +				WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> +				/* set to this task directly */
> +				writel(task->pa_base,
> +				       thread->base + CMDQ_THR_CURR_ADDR);
> +			} else {
> +				cmdq_task_insert_into_thread(task);
> +				cmdq_task_remove_wfe(task);
> +				smp_mb(); /* modify jump before enable thread */
> +			}
> +		} else {
> +			/* check boundary */
> +			if (curr_pa == end_pa - CMDQ_INST_SIZE ||
> +			    curr_pa == end_pa) {
> +				/* set to this task directly */
> +				writel(task->pa_base,
> +				       thread->base + CMDQ_THR_CURR_ADDR);
> +			} else {
> +				cmdq_task_insert_into_thread(task);
> +				smp_mb(); /* modify jump before enable thread */
> +			}
> +		}
> +		writel(task->pa_base + pkt->cmd_buf_size,
> +		       thread->base + CMDQ_THR_END_ADDR);
> +		cmdq_thread_resume(thread);
> +	}
> +	list_move_tail(&task->list_entry, &thread->task_busy_list);
> +}
> +
> +static void cmdq_task_exec_done(struct cmdq_task *task, bool err)
> +{
> +	struct device *dev = task->cmdq->mbox.dev;
> +	struct cmdq_cb_data cmdq_cb_data;
> +
> +	dma_unmap_single(dev, task->pa_base, task->pkt->cmd_buf_size,
> +			 DMA_TO_DEVICE);

Move this to client driver.

> +	if (task->pkt->cb.cb) {
> +		cmdq_cb_data.err = err;
> +		cmdq_cb_data.data = task->pkt->cb.data;
> +		task->pkt->cb.cb(cmdq_cb_data);
> +	}
> +	list_del(&task->list_entry);
> +}
> +

[...]

> +
> +static bool cmdq_mbox_last_tx_done(struct mbox_chan *chan)
> +{
> +	return true;
> +}
> +
> +static const struct mbox_chan_ops cmdq_mbox_chan_ops = {
> +	.send_data = cmdq_mbox_send_data,
> +	.startup = cmdq_mbox_startup,
> +	.shutdown = cmdq_mbox_shutdown,
> +	.last_tx_done = cmdq_mbox_last_tx_done,

Because mbox->txdone_poll is false, so you need not to implement
last_tx_done.

Regards,
CK

> +};
> +
> +static struct mbox_chan *cmdq_xlate(struct mbox_controller *mbox,
> +		const struct of_phandle_args *sp)
> +{
> +	int ind = sp->args[0];
> +	struct cmdq_thread *thread;
> +
> +	if (ind >= mbox->num_chans)
> +		return ERR_PTR(-EINVAL);
> +
> +	thread = mbox->chans[ind].con_priv;
> +	thread->timeout_ms = sp->args[1];
> +	thread->priority = sp->args[2];
> +	thread->atomic_exec = (sp->args[3] != 0);
> +	thread->chan = &mbox->chans[ind];
> +
> +	return &mbox->chans[ind];
> +}
> +
[...]

Re: [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper

[CK Hu]

Hi, Houlong:

On Fri, 2018-06-29 at 07:32 +0800, houlong wei wrote:
> On Thu, 2018-06-28 at 18:41 +0800, CK Hu wrote:
> > Hi, Houlong:
> > 
> > Some inline comment.
> > 
> > On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > > Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> > > 
> > > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > ---
> > >  drivers/soc/mediatek/Kconfig           |   12 ++
> > >  drivers/soc/mediatek/Makefile          |    1 +
> > >  drivers/soc/mediatek/mtk-cmdq-helper.c |  258 ++++++++++++++++++++++++++++++++
> > >  include/linux/soc/mediatek/mtk-cmdq.h  |  132 ++++++++++++++++
> > >  4 files changed, 403 insertions(+)
> > >  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> > >  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> > > 
> > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > > index a7d0667..17bd759 100644
> > > --- a/drivers/soc/mediatek/Kconfig
> > > +++ b/drivers/soc/mediatek/Kconfig
> > > @@ -4,6 +4,18 @@
> > >  menu "MediaTek SoC drivers"
> > >  	depends on ARCH_MEDIATEK || COMPILE_TEST
> > >  
> > 

[...]

> > > +
> > > +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > > +{
> > > +	int err;
> > > +
> > > +	if (cmdq_pkt_is_finalized(pkt))
> > > +		return 0;
> > > +
> > > +	/* insert EOC and generate IRQ for each command iteration */
> > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	/* JUMP to end */
> > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
> > > +			 cmdq_async_flush_cb cb, void *data)
> > > +{
> > > +	int err;
> > > +	struct device *dev;
> > > +	dma_addr_t dma_addr;
> > > +
> > > +	err = cmdq_pkt_finalize(pkt);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	dev = client->chan->mbox->dev;
> > > +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->cmd_buf_size,
> > > +		DMA_TO_DEVICE);
> > 
> > You map here, but I could not find unmap, so the unmap should be done in
> > client driver. I would prefer a symmetric map/unmap which means that
> > both map and unmap are done in client driver. I think you put map here
> > because you should map after finalize. 
> 
> The unmap is done before callback to client, in function
> cmdq_task_exec_done, mtk-cmdq-mailbox.c.

You put unmap in mailbox controller, and map here (here would be mailbox
client), so this is not symmetric. If the code is asymmetric, it's easy
to cause bug and not easy to maintain. So I would like move both
map/unmap to client driver.

> 
> > Therefore, export
> > cmdq_pkt_finalize() to client driver and let client do finalize, so
> > there is no finalize in flush function. This method have a benefit that
> > if client reuse command buffer, it need not to map/unmap frequently.
> 
> If client reuse command buffer or cmdq_pkt(command queue packet), client
> will add new commands to the cmdq_pkt, so map/unmap are necessary for
> each cmdq_pkt flush.

If the buffer size is 4K bytes, client driver could map the whole 4K at
initialization. Before it write new command, it call
dma_sync_single_for_cpu(), after it write new command, it call
dma_sync_single_for_device(). And then it could flush this buffer to
mailbox controller. So client driver just call dma sync function when it
reuse the command buffer. dma sync function is much lightweight then dma
map because map would search the memory area which could be mapped.

Regards,
CK

> 
> > 
> > Regards,
> > CK
> > 
> > > +	if (dma_mapping_error(dev, dma_addr)) {
> > > +		dev_err(client->chan->mbox->dev, "dma map failed\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	pkt->pa_base = dma_addr;
> > > +	pkt->cb.cb = cb;
> > > +	pkt->cb.data = data;
> > > +
> > > +	mbox_send_message(client->chan, pkt);
> > > +	/* We can send next packet immediately, so just call txdone. */
> > > +	mbox_client_txdone(client->chan, 0);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> > > +
> > > +struct cmdq_flush_completion {
> > > +	struct completion cmplt;
> > > +	bool err;
> > > +};
> > > +
> > > +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> > > +{
> > > +	struct cmdq_flush_completion *cmplt = data.data;
> > > +
> > > +	cmplt->err = data.err;
> > > +	complete(&cmplt->cmplt);
> > > +}
> > > +
> > > +int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt)
> > > +{
> > > +	struct cmdq_flush_completion cmplt;
> > > +	int err;
> > > +
> > > +	init_completion(&cmplt.cmplt);
> > > +	err = cmdq_pkt_flush_async(client, pkt, cmdq_pkt_flush_cb, &cmplt);
> > > +	if (err < 0)
> > > +		return err;
> > > +	wait_for_completion(&cmplt.cmplt);
> > > +
> > > +	return cmplt.err ? -EFAULT : 0;
> > > +}
> > > +EXPORT_SYMBOL(cmdq_pkt_flush);
> > 
> > [...]
> > 
> > 
> 
> 

Re: [PATCH v22 1/4] dt-bindings: soc: Add documentation for the MediaTek GCE unit

[Rob Herring]

On Wed, Jun 27, 2018 at 07:16:09PM +0800, Houlong Wei wrote:
> This adds documentation for the MediaTek Global Command Engine (GCE) unit
> found in MT8173 SoCs.
> 
> Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> Signed-off-by: HS Liao <hs.liao@mediatek.com>
> ---
> Hi Rob,
>   I don't add your ACK in this version since the dt-binding description
> has been changed. Thanks.
> ---
>  .../devicetree/bindings/mailbox/mtk-gce.txt        |   65 ++++++++++++++++++++
>  include/dt-bindings/gce/mt8173-gce.h               |   48 +++++++++++++++
>  2 files changed, 113 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
>  create mode 100644 include/dt-bindings/gce/mt8173-gce.h
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/mtk-gce.txt b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> new file mode 100644
> index 0000000..26f65a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> @@ -0,0 +1,65 @@
> +MediaTek GCE
> +===============
> +
> +The Global Command Engine (GCE) is used to help read/write registers with
> +critical time limitation, such as updating display configuration during the
> +vblank. The GCE can be used to implement the Command Queue (CMDQ) driver.
> +
> +CMDQ driver uses mailbox framework for communication. Please refer to
> +mailbox.txt for generic information about mailbox device-tree bindings.
> +
> +Required properties:
> +- compatible: Must be "mediatek,mt8173-gce"
> +- reg: Address range of the GCE unit
> +- interrupts: The interrupt signal from the GCE block
> +- clock: Clocks according to the common clock binding
> +- clock-names: Must be "gce" to stand for GCE clock
> +- thread-num: Maximum threads count of GCE.

mediatek,thread-num

Is this needed for anything other than error checking the thread id in 
the mbox cells? if that's it, then drop it. 

> +- #mbox-cells: Should be 4.
> +	<&phandle channel timeout priority atomic_exec>
> +	phandle: Label name of a gce node.
> +	channel: Channel of mailbox. Be equal to the thread id of GCE.
> +	timeout: Maximum time of software waiting GCE processing done, in unit
> +		of millisecond.
> +	priority: Priority of GCE thread.
> +	atomic_exec: GCE processing continuous packets of commands in atomic
> +		way.
> +
> +Required properties for a client device:
> +- mboxes: Client use mailbox to communicate with GCE, it should have this
> +  property and list of phandle, mailbox specifiers.
> +- gce-subsys: Specify the sub-system id which is corresponding to the register
> +  address.

What is this for?

> +
> +Optional properties for a client device:
> +- gce-event: Specify the event if the client has any. Because the event is
> +  parsed by client, so client can replace 'gce-event' with other meaningful
> +  name.

If the client sets the name, then no point having here. It must be 
documented in the client binding. But then, what is this for in the 
first place?

> +
> +Some vaules of properties are defined in 'dt-bindings/gce/mt8173-gce.h'. Such as
> +thread number, sub-system ids, thread priority, event ids.
> +
> +Example:
> +
> +	gce: gce@10212000 {

mailbox@...

> +		compatible = "mediatek,mt8173-gce";
> +		reg = <0 0x10212000 0 0x1000>;
> +		interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
> +		clocks = <&infracfg CLK_INFRA_GCE>;
> +		clock-names = "gce";
> +		thread-num = CMDQ_THR_MAX_COUNT;
> +		#mbox-cells = <4>;
> +	};
> +
> +Example for a client device:
> +
> +	mmsys: clock-controller@14000000 {
> +		compatible = "mediatek,mt8173-mmsys";
> +		mboxes = <&gce 0 2000 CMDQ_THR_PRIO_LOWEST 1>,
> +			 <&gce 1 2000 CMDQ_THR_PRIO_LOWEST 1>;
> +		gce-subsys = <SUBSYS_1400XXXX>;
> +		mutex-event-eof = <CMDQ_EVENT_MUTEX0_STREAM_EOF
> +				CMDQ_EVENT_MUTEX1_STREAM_EOF>;
> +
> +		...
> +	};

Re: [PATCH v22 1/4] dt-bindings: soc: Add documentation for the MediaTek GCE unit

[houlong wei]

On Tue, 2018-07-03 at 10:30 +0800, Rob Herring wrote:
> On Wed, Jun 27, 2018 at 07:16:09PM +0800, Houlong Wei wrote:
> > This adds documentation for the MediaTek Global Command Engine (GCE) unit
> > found in MT8173 SoCs.
> > 
> > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > ---
> > Hi Rob,
> >   I don't add your ACK in this version since the dt-binding description
> > has been changed. Thanks.
> > ---
> >  .../devicetree/bindings/mailbox/mtk-gce.txt        |   65 ++++++++++++++++++++
> >  include/dt-bindings/gce/mt8173-gce.h               |   48 +++++++++++++++
> >  2 files changed, 113 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> >  create mode 100644 include/dt-bindings/gce/mt8173-gce.h
> > 
> > diff --git a/Documentation/devicetree/bindings/mailbox/mtk-gce.txt b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> > new file mode 100644
> > index 0000000..26f65a4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> > @@ -0,0 +1,65 @@
> > +MediaTek GCE
> > +===============
> > +
> > +The Global Command Engine (GCE) is used to help read/write registers with
> > +critical time limitation, such as updating display configuration during the
> > +vblank. The GCE can be used to implement the Command Queue (CMDQ) driver.
> > +
> > +CMDQ driver uses mailbox framework for communication. Please refer to
> > +mailbox.txt for generic information about mailbox device-tree bindings.
> > +
> > +Required properties:
> > +- compatible: Must be "mediatek,mt8173-gce"
> > +- reg: Address range of the GCE unit
> > +- interrupts: The interrupt signal from the GCE block
> > +- clock: Clocks according to the common clock binding
> > +- clock-names: Must be "gce" to stand for GCE clock
> > +- thread-num: Maximum threads count of GCE.
> 
> mediatek,thread-num
> 
> Is this needed for anything other than error checking the thread id in 
> the mbox cells? if that's it, then drop it. 
> 

'thread-num' is used to configure the GCE thread number, which is the
channel number of gce mailbox. This property is read in
cmdq_probe()/mtk-cmdq-mailbox.c and a mailbox's channel array is
allocated according to the number.
Since the thread number may be different on different SoC, we wish it
could be configured in device tree.

> > +- #mbox-cells: Should be 4.
> > +	<&phandle channel timeout priority atomic_exec>
> > +	phandle: Label name of a gce node.
> > +	channel: Channel of mailbox. Be equal to the thread id of GCE.
> > +	timeout: Maximum time of software waiting GCE processing done, in unit
> > +		of millisecond.
> > +	priority: Priority of GCE thread.
> > +	atomic_exec: GCE processing continuous packets of commands in atomic
> > +		way.
> > +
> > +Required properties for a client device:
> > +- mboxes: Client use mailbox to communicate with GCE, it should have this
> > +  property and list of phandle, mailbox specifiers.
> > +- gce-subsys: Specify the sub-system id which is corresponding to the register
> > +  address.
> 
> What is this for?

You mean the new added property 'gce-subsys'?  
It is used for GCE to distinguish the high 16-bit of a hardware register
address. When a client driver packets a register setting into a GCE
instruction, it uses a sub-system code and register offset instead of
the 32-bit register address.
Since sub-system code may be different on different SoC, we wish it
could be configured in device tree.

> 
> > +
> > +Optional properties for a client device:
> > +- gce-event: Specify the event if the client has any. Because the event is
> > +  parsed by client, so client can replace 'gce-event' with other meaningful
> > +  name.
> 
> If the client sets the name, then no point having here. It must be 
> documented in the client binding. But then, what is this for in the 
> first place?

Since display driver will use GCE firstly, so we will move the
description to
'Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt'
when display driver start using the GCE driver.
Is that ok?

> 
> > +
> > +Some vaules of properties are defined in 'dt-bindings/gce/mt8173-gce.h'. Such as
> > +thread number, sub-system ids, thread priority, event ids.
> > +
> > +Example:
> > +
> > +	gce: gce@10212000 {
> 
> mailbox@...

Will do.

> 
> > +		compatible = "mediatek,mt8173-gce";
> > +		reg = <0 0x10212000 0 0x1000>;
> > +		interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
> > +		clocks = <&infracfg CLK_INFRA_GCE>;
> > +		clock-names = "gce";
> > +		thread-num = CMDQ_THR_MAX_COUNT;
> > +		#mbox-cells = <4>;
> > +	};
> > +
> > +Example for a client device:
> > +
> > +	mmsys: clock-controller@14000000 {
> > +		compatible = "mediatek,mt8173-mmsys";
> > +		mboxes = <&gce 0 2000 CMDQ_THR_PRIO_LOWEST 1>,
> > +			 <&gce 1 2000 CMDQ_THR_PRIO_LOWEST 1>;
> > +		gce-subsys = <SUBSYS_1400XXXX>;
> > +		mutex-event-eof = <CMDQ_EVENT_MUTEX0_STREAM_EOF
> > +				CMDQ_EVENT_MUTEX1_STREAM_EOF>;
> > +
> > +		...
> > +	};

Re: [PATCH v22 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

[houlong wei]

On Fri, 2018-06-29 at 15:08 +0800, CK Hu wrote:
> Hi, Houlong:
> 
> Some inline comment.
> 
> On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> > CMDQ is used to help write registers with critical time limitation,
> > such as updating display configuration during the vblank. It controls
> > Global Command Engine (GCE) hardware to achieve this requirement.
> > Currently, CMDQ only supports display related hardwares, but we expect
> > it can be extended to other hardwares for future requirements.
> > 
> > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >  drivers/mailbox/Kconfig                  |   10 +
> >  drivers/mailbox/Makefile                 |    2 +
> >  drivers/mailbox/mtk-cmdq-mailbox.c       |  634 ++++++++++++++++++++++++++++++
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |   70 ++++
> >  4 files changed, 716 insertions(+)
> >  create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
> >  create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h
> > 
> 
> [...]
> 
> > +
> > +static int cmdq_thread_reset(struct cmdq *cmdq, struct cmdq_thread *thread)
> > +{
> > +	u32 warm_reset;
> > +
> > +	writel(CMDQ_THR_DO_WARM_RESET, thread->base + CMDQ_THR_WARM_RESET);
> > +	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_WARM_RESET,
> > +			warm_reset, !(warm_reset & CMDQ_THR_DO_WARM_RESET),
> > +			0, 10)) {
> > +		dev_err(cmdq->mbox.dev, "reset GCE thread 0x%x failed\n",
> > +			(u32)(thread->base - cmdq->base));
> > +		return -EFAULT;
> > +	}
> > +	writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base + CMDQ_THR_SLOT_CYCLES);
> 
> The CMDQ_THR_SLOT_CYCLES looks like not relevant to reset. Maybe you
> just need to set this value when startup.

Will move configuration of CMDQ_THR_SLOT_CYCLES to cmdq_xlate() where is
startup of a GCE thread.

> 
> > +
> > +	return 0;
> > +}
> > +
> 
> [...]
> 
> > +
> > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> > +{
> > +	struct cmdq *cmdq;
> > +	struct cmdq_task *task;
> > +	unsigned long curr_pa, end_pa;
> > +
> > +	cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> > +
> > +	/* Client should not flush new tasks if suspended. */
> > +	WARN_ON(cmdq->suspended);
> > +
> > +	task = kzalloc(sizeof(*task), GFP_ATOMIC);
> > +	task->cmdq = cmdq;
> > +	INIT_LIST_HEAD(&task->list_entry);
> > +	task->pa_base = pkt->pa_base;
> > +	task->thread = thread;
> > +	task->pkt = pkt;
> > +
> > +	if (list_empty(&thread->task_busy_list)) {
> > +		WARN_ON(clk_enable(cmdq->clock) < 0);
> > +		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
> > +
> > +		writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
> > +		writel(task->pa_base + pkt->cmd_buf_size,
> > +		       thread->base + CMDQ_THR_END_ADDR);
> > +		writel(thread->priority, thread->base + CMDQ_THR_PRIORITY);
> > +		writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
> > +		writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
> > +
> > +		if (thread->timeout_ms != CMDQ_NO_TIMEOUT)
> > +			mod_timer(&thread->timeout,
> > +				jiffies + msecs_to_jiffies(thread->timeout_ms));
> 
> I think the timeout processing should be done by client driver. The
> total time to execute a command buffer does not depend on GCE HW speed
> because the WFE (wait for event) command would wait for client HW event,
> so the total time depend on how long a client HW send this event to GCE
> and the timeout processing should be client driver's job. Each client
> may have different timeout processing mechanism, for example, if display
> could dynamic change panel frame rate between 120Hz and 60Hz, and the
> timeout time is 2 frame, so it may dynamically change timeout time
> between 17ms and 33ms. Another reason is that display have interrupt
> every vblank, and it could check timeout in that interrupt, so the timer
> in cmdq driver looks redundant. Because each client would define its own
> timeout processing mechanism, so it's not wise to put timeout processing
> in cmdq driver.

The client drivers' owners strongly hope to keep the current timeout
mechanism, the reasons are below.
1. If remove, all clients should add timeout mechanism and the code will
be redundant.
2. If timeout happens, only GCE driver can do reset and continue to
execute next packet.

> 
> > +	} else {
> > +		WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> > +		curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR);
> > +		end_pa = readl(thread->base + CMDQ_THR_END_ADDR);
> > +
> > +		/*
> > +		 * Atomic execution should remove the following wfe, i.e. only
> > +		 * wait event at first task, and prevent to pause when running.
> > +		 */
> > +		if (thread->atomic_exec) {
> > +			/* GCE is executing if command is not WFE */
> > +			if (!cmdq_thread_is_in_wfe(thread)) {
> > +				cmdq_thread_resume(thread);
> > +				cmdq_thread_wait_end(thread, end_pa);
> > +				WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> > +				/* set to this task directly */
> > +				writel(task->pa_base,
> > +				       thread->base + CMDQ_THR_CURR_ADDR);
> > +			} else {
> > +				cmdq_task_insert_into_thread(task);
> > +				cmdq_task_remove_wfe(task);
> > +				smp_mb(); /* modify jump before enable thread */
> > +			}
> > +		} else {
> > +			/* check boundary */
> > +			if (curr_pa == end_pa - CMDQ_INST_SIZE ||
> > +			    curr_pa == end_pa) {
> > +				/* set to this task directly */
> > +				writel(task->pa_base,
> > +				       thread->base + CMDQ_THR_CURR_ADDR);
> > +			} else {
> > +				cmdq_task_insert_into_thread(task);
> > +				smp_mb(); /* modify jump before enable thread */
> > +			}
> > +		}
> > +		writel(task->pa_base + pkt->cmd_buf_size,
> > +		       thread->base + CMDQ_THR_END_ADDR);
> > +		cmdq_thread_resume(thread);
> > +	}
> > +	list_move_tail(&task->list_entry, &thread->task_busy_list);
> > +}
> > +
> > +static void cmdq_task_exec_done(struct cmdq_task *task, bool err)
> > +{
> > +	struct device *dev = task->cmdq->mbox.dev;
> > +	struct cmdq_cb_data cmdq_cb_data;
> > +
> > +	dma_unmap_single(dev, task->pa_base, task->pkt->cmd_buf_size,
> > +			 DMA_TO_DEVICE);
> 
> Move this to client driver.

map/unmap are common code for clients driver, could we move it to cmdq
helper?

> 
> > +	if (task->pkt->cb.cb) {
> > +		cmdq_cb_data.err = err;
> > +		cmdq_cb_data.data = task->pkt->cb.data;
> > +		task->pkt->cb.cb(cmdq_cb_data);
> > +	}
> > +	list_del(&task->list_entry);
> > +}
> > +
> 
> [...]
> 
> > +
> > +static bool cmdq_mbox_last_tx_done(struct mbox_chan *chan)
> > +{
> > +	return true;
> > +}
> > +
> > +static const struct mbox_chan_ops cmdq_mbox_chan_ops = {
> > +	.send_data = cmdq_mbox_send_data,
> > +	.startup = cmdq_mbox_startup,
> > +	.shutdown = cmdq_mbox_shutdown,
> > +	.last_tx_done = cmdq_mbox_last_tx_done,
> 
> Because mbox->txdone_poll is false, so you need not to implement
> last_tx_done.
> 
> Regards,
> CK

Will remove cmdq_mbox_last_tx_done().

> 
> > +};
> > +
> > +static struct mbox_chan *cmdq_xlate(struct mbox_controller *mbox,
> > +		const struct of_phandle_args *sp)
> > +{
> > +	int ind = sp->args[0];
> > +	struct cmdq_thread *thread;
> > +
> > +	if (ind >= mbox->num_chans)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	thread = mbox->chans[ind].con_priv;
> > +	thread->timeout_ms = sp->args[1];
> > +	thread->priority = sp->args[2];
> > +	thread->atomic_exec = (sp->args[3] != 0);
> > +	thread->chan = &mbox->chans[ind];
> > +
> > +	return &mbox->chans[ind];
> > +}
> > +
> [...]
> 
> 

Re: [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper

[houlong wei]

On Fri, 2018-06-29 at 17:22 +0800, CK Hu wrote:
> Hi, Houlong:
> 
> On Fri, 2018-06-29 at 07:32 +0800, houlong wei wrote:
> > On Thu, 2018-06-28 at 18:41 +0800, CK Hu wrote:
> > > Hi, Houlong:
> > > 
> > > Some inline comment.
> > > 
> > > On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > > > Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> > > > 
> > > > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > > ---
> > > >  drivers/soc/mediatek/Kconfig           |   12 ++
> > > >  drivers/soc/mediatek/Makefile          |    1 +
> > > >  drivers/soc/mediatek/mtk-cmdq-helper.c |  258 ++++++++++++++++++++++++++++++++
> > > >  include/linux/soc/mediatek/mtk-cmdq.h  |  132 ++++++++++++++++
> > > >  4 files changed, 403 insertions(+)
> > > >  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> > > >  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> > > > 
> > > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > > > index a7d0667..17bd759 100644
> > > > --- a/drivers/soc/mediatek/Kconfig
> > > > +++ b/drivers/soc/mediatek/Kconfig
> > > > @@ -4,6 +4,18 @@
> > > >  menu "MediaTek SoC drivers"
> > > >  	depends on ARCH_MEDIATEK || COMPILE_TEST
> > > >  
> > > 
> 
> [...]
> 
> > > > +
> > > > +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > > > +{
> > > > +	int err;
> > > > +
> > > > +	if (cmdq_pkt_is_finalized(pkt))
> > > > +		return 0;
> > > > +
> > > > +	/* insert EOC and generate IRQ for each command iteration */
> > > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +
> > > > +	/* JUMP to end */
> > > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
> > > > +			 cmdq_async_flush_cb cb, void *data)
> > > > +{
> > > > +	int err;
> > > > +	struct device *dev;
> > > > +	dma_addr_t dma_addr;
> > > > +
> > > > +	err = cmdq_pkt_finalize(pkt);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +
> > > > +	dev = client->chan->mbox->dev;
> > > > +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->cmd_buf_size,
> > > > +		DMA_TO_DEVICE);
> > > 
> > > You map here, but I could not find unmap, so the unmap should be done in
> > > client driver. I would prefer a symmetric map/unmap which means that
> > > both map and unmap are done in client driver. I think you put map here
> > > because you should map after finalize. 
> > 
> > The unmap is done before callback to client, in function
> > cmdq_task_exec_done, mtk-cmdq-mailbox.c.
> 
> You put unmap in mailbox controller, and map here (here would be mailbox
> client), so this is not symmetric. If the code is asymmetric, it's easy
> to cause bug and not easy to maintain. So I would like move both
> map/unmap to client driver.
> 

Since map/unmap is common code for client drivers, can we move unmap to
CMDQ helper and do not put in client driver?

> > 
> > > Therefore, export
> > > cmdq_pkt_finalize() to client driver and let client do finalize, so
> > > there is no finalize in flush function. This method have a benefit that
> > > if client reuse command buffer, it need not to map/unmap frequently.
> > 
> > If client reuse command buffer or cmdq_pkt(command queue packet), client
> > will add new commands to the cmdq_pkt, so map/unmap are necessary for
> > each cmdq_pkt flush.
> 
> If the buffer size is 4K bytes, client driver could map the whole 4K at
> initialization. Before it write new command, it call
> dma_sync_single_for_cpu(), after it write new command, it call
> dma_sync_single_for_device(). And then it could flush this buffer to
> mailbox controller. So client driver just call dma sync function when it
> reuse the command buffer. dma sync function is much lightweight then dma
> map because map would search the memory area which could be mapped.
> 
> Regards,
> CK

Maybe we can do dma map/unmap/sync in cmdq helper, and make client
driver simple.

> 
> > 
> > > 
> > > Regards,
> > > CK
> > > 
> > > > +	if (dma_mapping_error(dev, dma_addr)) {
> > > > +		dev_err(client->chan->mbox->dev, "dma map failed\n");
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	pkt->pa_base = dma_addr;
> > > > +	pkt->cb.cb = cb;
> > > > +	pkt->cb.data = data;
> > > > +
> > > > +	mbox_send_message(client->chan, pkt);
> > > > +	/* We can send next packet immediately, so just call txdone. */
> > > > +	mbox_client_txdone(client->chan, 0);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> > > > +
> > > > +struct cmdq_flush_completion {
> > > > +	struct completion cmplt;
> > > > +	bool err;
> > > > +};
> > > > +
> > > > +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> > > > +{
> > > > +	struct cmdq_flush_completion *cmplt = data.data;
> > > > +
> > > > +	cmplt->err = data.err;
> > > > +	complete(&cmplt->cmplt);
> > > > +}
> > > > +
> > > > +int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt)
> > > > +{
> > > > +	struct cmdq_flush_completion cmplt;
> > > > +	int err;
> > > > +
> > > > +	init_completion(&cmplt.cmplt);
> > > > +	err = cmdq_pkt_flush_async(client, pkt, cmdq_pkt_flush_cb, &cmplt);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +	wait_for_completion(&cmplt.cmplt);
> > > > +
> > > > +	return cmplt.err ? -EFAULT : 0;
> > > > +}
> > > > +EXPORT_SYMBOL(cmdq_pkt_flush);
> > > 
> > > [...]
> > > 
> > > 
> > 
> > 
> 
> 

Re: [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper

[CK Hu]

Hi, Houlong:

On Wed, 2018-07-04 at 08:47 +0800, houlong wei wrote:
> On Fri, 2018-06-29 at 17:22 +0800, CK Hu wrote:
> > Hi, Houlong:
> > 
> > On Fri, 2018-06-29 at 07:32 +0800, houlong wei wrote:
> > > On Thu, 2018-06-28 at 18:41 +0800, CK Hu wrote:
> > > > Hi, Houlong:
> > > > 
> > > > Some inline comment.
> > > > 
> > > > On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > > > > Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> > > > > 
> > > > > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > > > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > > > ---
> > > > >  drivers/soc/mediatek/Kconfig           |   12 ++
> > > > >  drivers/soc/mediatek/Makefile          |    1 +
> > > > >  drivers/soc/mediatek/mtk-cmdq-helper.c |  258 ++++++++++++++++++++++++++++++++
> > > > >  include/linux/soc/mediatek/mtk-cmdq.h  |  132 ++++++++++++++++
> > > > >  4 files changed, 403 insertions(+)
> > > > >  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > >  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> > > > > 
> > > > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > > > > index a7d0667..17bd759 100644
> > > > > --- a/drivers/soc/mediatek/Kconfig
> > > > > +++ b/drivers/soc/mediatek/Kconfig
> > > > > @@ -4,6 +4,18 @@
> > > > >  menu "MediaTek SoC drivers"
> > > > >  	depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > >  
> > > > 
> > 
> > [...]
> > 
> > > > > +
> > > > > +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > > > > +{
> > > > > +	int err;
> > > > > +
> > > > > +	if (cmdq_pkt_is_finalized(pkt))
> > > > > +		return 0;
> > > > > +
> > > > > +	/* insert EOC and generate IRQ for each command iteration */
> > > > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > > > > +	if (err < 0)
> > > > > +		return err;
> > > > > +
> > > > > +	/* JUMP to end */
> > > > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > > > > +	if (err < 0)
> > > > > +		return err;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
> > > > > +			 cmdq_async_flush_cb cb, void *data)
> > > > > +{
> > > > > +	int err;
> > > > > +	struct device *dev;
> > > > > +	dma_addr_t dma_addr;
> > > > > +
> > > > > +	err = cmdq_pkt_finalize(pkt);
> > > > > +	if (err < 0)
> > > > > +		return err;
> > > > > +
> > > > > +	dev = client->chan->mbox->dev;
> > > > > +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->cmd_buf_size,
> > > > > +		DMA_TO_DEVICE);
> > > > 
> > > > You map here, but I could not find unmap, so the unmap should be done in
> > > > client driver. I would prefer a symmetric map/unmap which means that
> > > > both map and unmap are done in client driver. I think you put map here
> > > > because you should map after finalize. 
> > > 
> > > The unmap is done before callback to client, in function
> > > cmdq_task_exec_done, mtk-cmdq-mailbox.c.
> > 
> > You put unmap in mailbox controller, and map here (here would be mailbox
> > client), so this is not symmetric. If the code is asymmetric, it's easy
> > to cause bug and not easy to maintain. So I would like move both
> > map/unmap to client driver.
> > 
> 
> Since map/unmap is common code for client drivers, can we move unmap to
> CMDQ helper and do not put in client driver?
> 
> > > 
> > > > Therefore, export
> > > > cmdq_pkt_finalize() to client driver and let client do finalize, so
> > > > there is no finalize in flush function. This method have a benefit that
> > > > if client reuse command buffer, it need not to map/unmap frequently.
> > > 
> > > If client reuse command buffer or cmdq_pkt(command queue packet), client
> > > will add new commands to the cmdq_pkt, so map/unmap are necessary for
> > > each cmdq_pkt flush.
> > 
> > If the buffer size is 4K bytes, client driver could map the whole 4K at
> > initialization. Before it write new command, it call
> > dma_sync_single_for_cpu(), after it write new command, it call
> > dma_sync_single_for_device(). And then it could flush this buffer to
> > mailbox controller. So client driver just call dma sync function when it
> > reuse the command buffer. dma sync function is much lightweight then dma
> > map because map would search the memory area which could be mapped.
> > 
> > Regards,
> > CK
> 
> Maybe we can do dma map/unmap/sync in cmdq helper, and make client
> driver simple.
> 

Why are map/unmap common code for client drivers? I've mentioned that
some client driver may just call dma sync function before flush, so it
does not map for every flush. Frequently map/unmap has some drawback:

1. Waste CPU resource: this also waste power.
2. Risk of mapping fail: to reduce this risk, client driver could map at
initialization.

I think reducing these drawback is more important than making client
driver simple.

Regards,
CK

> > 
> > > 
> > > > 
> > > > Regards,
> > > > CK
> > > > 
> > > > > +	if (dma_mapping_error(dev, dma_addr)) {
> > > > > +		dev_err(client->chan->mbox->dev, "dma map failed\n");
> > > > > +		return -ENOMEM;
> > > > > +	}
> > > > > +
> > > > > +	pkt->pa_base = dma_addr;
> > > > > +	pkt->cb.cb = cb;
> > > > > +	pkt->cb.data = data;
> > > > > +
> > > > > +	mbox_send_message(client->chan, pkt);
> > > > > +	/* We can send next packet immediately, so just call txdone. */
> > > > > +	mbox_client_txdone(client->chan, 0);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> > > > > +
> > > > > +struct cmdq_flush_completion {
> > > > > +	struct completion cmplt;
> > > > > +	bool err;
> > > > > +};
> > > > > +
> > > > > +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> > > > > +{
> > > > > +	struct cmdq_flush_completion *cmplt = data.data;
> > > > > +
> > > > > +	cmplt->err = data.err;
> > > > > +	complete(&cmplt->cmplt);
> > > > > +}
> > > > > +
> > > > > +int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt)
> > > > > +{
> > > > > +	struct cmdq_flush_completion cmplt;
> > > > > +	int err;
> > > > > +
> > > > > +	init_completion(&cmplt.cmplt);
> > > > > +	err = cmdq_pkt_flush_async(client, pkt, cmdq_pkt_flush_cb, &cmplt);
> > > > > +	if (err < 0)
> > > > > +		return err;
> > > > > +	wait_for_completion(&cmplt.cmplt);
> > > > > +
> > > > > +	return cmplt.err ? -EFAULT : 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL(cmdq_pkt_flush);
> > > > 
> > > > [...]
> > > > 
> > > > 
> > > 
> > > 
> > 
> > 
> 
> 

Re: [PATCH v22 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

[CK Hu]

Hi, Houlong:

On Wed, 2018-07-04 at 08:10 +0800, houlong wei wrote:
> On Fri, 2018-06-29 at 15:08 +0800, CK Hu wrote:
> > Hi, Houlong:
> > 
> > Some inline comment.
> > 
> > On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > > This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> > > CMDQ is used to help write registers with critical time limitation,
> > > such as updating display configuration during the vblank. It controls
> > > Global Command Engine (GCE) hardware to achieve this requirement.
> > > Currently, CMDQ only supports display related hardwares, but we expect
> > > it can be extended to other hardwares for future requirements.
> > > 
> > > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > > ---
> > >  drivers/mailbox/Kconfig                  |   10 +
> > >  drivers/mailbox/Makefile                 |    2 +
> > >  drivers/mailbox/mtk-cmdq-mailbox.c       |  634 ++++++++++++++++++++++++++++++
> > >  include/linux/mailbox/mtk-cmdq-mailbox.h |   70 ++++
> > >  4 files changed, 716 insertions(+)
> > >  create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
> > >  create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h
> > > 
> > 
> > [...]
> > 
> > > +
> > > +static int cmdq_thread_reset(struct cmdq *cmdq, struct cmdq_thread *thread)
> > > +{
> > > +	u32 warm_reset;
> > > +
> > > +	writel(CMDQ_THR_DO_WARM_RESET, thread->base + CMDQ_THR_WARM_RESET);
> > > +	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_WARM_RESET,
> > > +			warm_reset, !(warm_reset & CMDQ_THR_DO_WARM_RESET),
> > > +			0, 10)) {
> > > +		dev_err(cmdq->mbox.dev, "reset GCE thread 0x%x failed\n",
> > > +			(u32)(thread->base - cmdq->base));
> > > +		return -EFAULT;
> > > +	}
> > > +	writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base + CMDQ_THR_SLOT_CYCLES);
> > 
> > The CMDQ_THR_SLOT_CYCLES looks like not relevant to reset. Maybe you
> > just need to set this value when startup.
> 
> Will move configuration of CMDQ_THR_SLOT_CYCLES to cmdq_xlate() where is
> startup of a GCE thread.
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > 
> > [...]
> > 
> > > +
> > > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> > > +{
> > > +	struct cmdq *cmdq;
> > > +	struct cmdq_task *task;
> > > +	unsigned long curr_pa, end_pa;
> > > +
> > > +	cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> > > +
> > > +	/* Client should not flush new tasks if suspended. */
> > > +	WARN_ON(cmdq->suspended);
> > > +
> > > +	task = kzalloc(sizeof(*task), GFP_ATOMIC);
> > > +	task->cmdq = cmdq;
> > > +	INIT_LIST_HEAD(&task->list_entry);
> > > +	task->pa_base = pkt->pa_base;
> > > +	task->thread = thread;
> > > +	task->pkt = pkt;
> > > +
> > > +	if (list_empty(&thread->task_busy_list)) {
> > > +		WARN_ON(clk_enable(cmdq->clock) < 0);
> > > +		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
> > > +
> > > +		writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
> > > +		writel(task->pa_base + pkt->cmd_buf_size,
> > > +		       thread->base + CMDQ_THR_END_ADDR);
> > > +		writel(thread->priority, thread->base + CMDQ_THR_PRIORITY);
> > > +		writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
> > > +		writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
> > > +
> > > +		if (thread->timeout_ms != CMDQ_NO_TIMEOUT)
> > > +			mod_timer(&thread->timeout,
> > > +				jiffies + msecs_to_jiffies(thread->timeout_ms));
> > 
> > I think the timeout processing should be done by client driver. The
> > total time to execute a command buffer does not depend on GCE HW speed
> > because the WFE (wait for event) command would wait for client HW event,
> > so the total time depend on how long a client HW send this event to GCE
> > and the timeout processing should be client driver's job. Each client
> > may have different timeout processing mechanism, for example, if display
> > could dynamic change panel frame rate between 120Hz and 60Hz, and the
> > timeout time is 2 frame, so it may dynamically change timeout time
> > between 17ms and 33ms. Another reason is that display have interrupt
> > every vblank, and it could check timeout in that interrupt, so the timer
> > in cmdq driver looks redundant. Because each client would define its own
> > timeout processing mechanism, so it's not wise to put timeout processing
> > in cmdq driver.
> 
> The client drivers' owners strongly hope to keep the current timeout
> mechanism, the reasons are below.
> 1. If remove, all clients should add timeout mechanism and the code will
> be redundant.
> 2. If timeout happens, only GCE driver can do reset and continue to
> execute next packet.

For the reason 2, GCE should not continue execute next packet because
the packets may have dependency. So GCE driver could only drop all
packet (this is what you do in cmdq_thread_handle_timeout()). For reason
1, you have a assumption that all client have the same request for
timeout: constant timeout value or no timeout. If it's so, that's ok to
put timeout mechanism in cmdq driver. But if one day, a new request for
timeout, for example, dynamic timeout value, that means not all client
driver implement timeout in the same way, putting timeout mechanism in
cmdq driver does not reduce any thing but just move multiple client
driver's code into cmdq driver. I would accept to put here only if all
client driver use the same timeout mechanism.

Regards,
CK

> 
> > 
> > > +	} else {
> > > +		WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> > > +		curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR);
> > > +		end_pa = readl(thread->base + CMDQ_THR_END_ADDR);
> > > +
> > > +		/*
> > > +		 * Atomic execution should remove the following wfe, i.e. only
> > > +		 * wait event at first task, and prevent to pause when running.
> > > +		 */
> > > +		if (thread->atomic_exec) {
> > > +			/* GCE is executing if command is not WFE */
> > > +			if (!cmdq_thread_is_in_wfe(thread)) {
> > > +				cmdq_thread_resume(thread);
> > > +				cmdq_thread_wait_end(thread, end_pa);
> > > +				WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> > > +				/* set to this task directly */
> > > +				writel(task->pa_base,
> > > +				       thread->base + CMDQ_THR_CURR_ADDR);
> > > +			} else {
> > > +				cmdq_task_insert_into_thread(task);
> > > +				cmdq_task_remove_wfe(task);
> > > +				smp_mb(); /* modify jump before enable thread */
> > > +			}
> > > +		} else {
> > > +			/* check boundary */
> > > +			if (curr_pa == end_pa - CMDQ_INST_SIZE ||
> > > +			    curr_pa == end_pa) {
> > > +				/* set to this task directly */
> > > +				writel(task->pa_base,
> > > +				       thread->base + CMDQ_THR_CURR_ADDR);
> > > +			} else {
> > > +				cmdq_task_insert_into_thread(task);
> > > +				smp_mb(); /* modify jump before enable thread */
> > > +			}
> > > +		}
> > > +		writel(task->pa_base + pkt->cmd_buf_size,
> > > +		       thread->base + CMDQ_THR_END_ADDR);
> > > +		cmdq_thread_resume(thread);
> > > +	}
> > > +	list_move_tail(&task->list_entry, &thread->task_busy_list);
> > > +}
> > > +
> > > +static void cmdq_task_exec_done(struct cmdq_task *task, bool err)
> > > +{
> > > +	struct device *dev = task->cmdq->mbox.dev;
> > > +	struct cmdq_cb_data cmdq_cb_data;
> > > +
> > > +	dma_unmap_single(dev, task->pa_base, task->pkt->cmd_buf_size,
> > > +			 DMA_TO_DEVICE);
> > 
> > Move this to client driver.
> 
> map/unmap are common code for clients driver, could we move it to cmdq
> helper?
> 
> > 
> > > +	if (task->pkt->cb.cb) {
> > > +		cmdq_cb_data.err = err;
> > > +		cmdq_cb_data.data = task->pkt->cb.data;
> > > +		task->pkt->cb.cb(cmdq_cb_data);
> > > +	}
> > > +	list_del(&task->list_entry);
> > > +}
> > > +
> > 
> > [...]
> > 
> > > +
> > > +static bool cmdq_mbox_last_tx_done(struct mbox_chan *chan)
> > > +{
> > > +	return true;
> > > +}
> > > +
> > > +static const struct mbox_chan_ops cmdq_mbox_chan_ops = {
> > > +	.send_data = cmdq_mbox_send_data,
> > > +	.startup = cmdq_mbox_startup,
> > > +	.shutdown = cmdq_mbox_shutdown,
> > > +	.last_tx_done = cmdq_mbox_last_tx_done,
> > 
> > Because mbox->txdone_poll is false, so you need not to implement
> > last_tx_done.
> > 
> > Regards,
> > CK
> 
> Will remove cmdq_mbox_last_tx_done().
> 
> > 
> > > +};
> > > +
> > > +static struct mbox_chan *cmdq_xlate(struct mbox_controller *mbox,
> > > +		const struct of_phandle_args *sp)
> > > +{
> > > +	int ind = sp->args[0];
> > > +	struct cmdq_thread *thread;
> > > +
> > > +	if (ind >= mbox->num_chans)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	thread = mbox->chans[ind].con_priv;
> > > +	thread->timeout_ms = sp->args[1];
> > > +	thread->priority = sp->args[2];
> > > +	thread->atomic_exec = (sp->args[3] != 0);
> > > +	thread->chan = &mbox->chans[ind];
> > > +
> > > +	return &mbox->chans[ind];
> > > +}
> > > +
> > [...]
> > 
> > 
> 
> 

Re: [PATCH v22 1/4] dt-bindings: soc: Add documentation for the MediaTek GCE unit

[Rob Herring]

On Tue, Jul 3, 2018 at 5:39 PM houlong wei <houlong.wei@mediatek.com> wrote:
>
> On Tue, 2018-07-03 at 10:30 +0800, Rob Herring wrote:
> > On Wed, Jun 27, 2018 at 07:16:09PM +0800, Houlong Wei wrote:
> > > This adds documentation for the MediaTek Global Command Engine (GCE) unit
> > > found in MT8173 SoCs.
> > >
> > > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > ---
> > > Hi Rob,
> > >   I don't add your ACK in this version since the dt-binding description
> > > has been changed. Thanks.
> > > ---
> > >  .../devicetree/bindings/mailbox/mtk-gce.txt        |   65 ++++++++++++++++++++
> > >  include/dt-bindings/gce/mt8173-gce.h               |   48 +++++++++++++++
> > >  2 files changed, 113 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> > >  create mode 100644 include/dt-bindings/gce/mt8173-gce.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/mtk-gce.txt b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> > > new file mode 100644
> > > index 0000000..26f65a4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> > > @@ -0,0 +1,65 @@
> > > +MediaTek GCE
> > > +===============
> > > +
> > > +The Global Command Engine (GCE) is used to help read/write registers with
> > > +critical time limitation, such as updating display configuration during the
> > > +vblank. The GCE can be used to implement the Command Queue (CMDQ) driver.
> > > +
> > > +CMDQ driver uses mailbox framework for communication. Please refer to
> > > +mailbox.txt for generic information about mailbox device-tree bindings.
> > > +
> > > +Required properties:
> > > +- compatible: Must be "mediatek,mt8173-gce"
> > > +- reg: Address range of the GCE unit
> > > +- interrupts: The interrupt signal from the GCE block
> > > +- clock: Clocks according to the common clock binding
> > > +- clock-names: Must be "gce" to stand for GCE clock
> > > +- thread-num: Maximum threads count of GCE.
> >
> > mediatek,thread-num
> >
> > Is this needed for anything other than error checking the thread id in
> > the mbox cells? if that's it, then drop it.
> >
>
> 'thread-num' is used to configure the GCE thread number, which is the
> channel number of gce mailbox. This property is read in
> cmdq_probe()/mtk-cmdq-mailbox.c and a mailbox's channel array is
> allocated according to the number.
> Since the thread number may be different on different SoC, we wish it
> could be configured in device tree.

You should have different compatible strings for different SoCs and
can imply the number of threads from that.

Or if the number of threads doesn't vary greatly, just allocate the
max # of channels. Or allocate the per thread data when a thread is
actually in use.

>
> > > +- #mbox-cells: Should be 4.
> > > +   <&phandle channel timeout priority atomic_exec>
> > > +   phandle: Label name of a gce node.
> > > +   channel: Channel of mailbox. Be equal to the thread id of GCE.
> > > +   timeout: Maximum time of software waiting GCE processing done, in unit
> > > +           of millisecond.
> > > +   priority: Priority of GCE thread.
> > > +   atomic_exec: GCE processing continuous packets of commands in atomic
> > > +           way.
> > > +
> > > +Required properties for a client device:
> > > +- mboxes: Client use mailbox to communicate with GCE, it should have this
> > > +  property and list of phandle, mailbox specifiers.
> > > +- gce-subsys: Specify the sub-system id which is corresponding to the register
> > > +  address.
> >
> > What is this for?
>
> You mean the new added property 'gce-subsys'?
> It is used for GCE to distinguish the high 16-bit of a hardware register
> address. When a client driver packets a register setting into a GCE
> instruction, it uses a sub-system code and register offset instead of
> the 32-bit register address.
> Since sub-system code may be different on different SoC, we wish it
> could be configured in device tree.

Okay. It needs a vendor prefix and to specify the size and type of the value.

>
> >
> > > +
> > > +Optional properties for a client device:
> > > +- gce-event: Specify the event if the client has any. Because the event is
> > > +  parsed by client, so client can replace 'gce-event' with other meaningful
> > > +  name.
> >
> > If the client sets the name, then no point having here. It must be
> > documented in the client binding. But then, what is this for in the
> > first place?
>
> Since display driver will use GCE firstly, so we will move the
> description to
> 'Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt'
> when display driver start using the GCE driver.
> Is that ok?

Not sure. I don't understand how it is used.

> > > +
> > > +Some vaules of properties are defined in 'dt-bindings/gce/mt8173-gce.h'. Such as
> > > +thread number, sub-system ids, thread priority, event ids.
> > > +
> > > +Example:
> > > +
> > > +   gce: gce@10212000 {
> >
> > mailbox@...
>
> Will do.
>
> >
> > > +           compatible = "mediatek,mt8173-gce";
> > > +           reg = <0 0x10212000 0 0x1000>;
> > > +           interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
> > > +           clocks = <&infracfg CLK_INFRA_GCE>;
> > > +           clock-names = "gce";
> > > +           thread-num = CMDQ_THR_MAX_COUNT;
> > > +           #mbox-cells = <4>;
> > > +   };
> > > +
> > > +Example for a client device:
> > > +
> > > +   mmsys: clock-controller@14000000 {
> > > +           compatible = "mediatek,mt8173-mmsys";
> > > +           mboxes = <&gce 0 2000 CMDQ_THR_PRIO_LOWEST 1>,
> > > +                    <&gce 1 2000 CMDQ_THR_PRIO_LOWEST 1>;
> > > +           gce-subsys = <SUBSYS_1400XXXX>;
> > > +           mutex-event-eof = <CMDQ_EVENT_MUTEX0_STREAM_EOF
> > > +                           CMDQ_EVENT_MUTEX1_STREAM_EOF>;
> > > +
> > > +           ...
> > > +   };
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper

[houlong wei]

On Wed, 2018-07-04 at 10:39 +0800, CK Hu wrote:
> Hi, Houlong:
> 
> On Wed, 2018-07-04 at 08:47 +0800, houlong wei wrote:
> > On Fri, 2018-06-29 at 17:22 +0800, CK Hu wrote:
> > > Hi, Houlong:
> > > 
> > > On Fri, 2018-06-29 at 07:32 +0800, houlong wei wrote:
> > > > On Thu, 2018-06-28 at 18:41 +0800, CK Hu wrote:
> > > > > Hi, Houlong:
> > > > > 
> > > > > Some inline comment.
> > > > > 
> > > > > On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > > > > > Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> > > > > > 
> > > > > > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > > > > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > > > > ---
> > > > > >  drivers/soc/mediatek/Kconfig           |   12 ++
> > > > > >  drivers/soc/mediatek/Makefile          |    1 +
> > > > > >  drivers/soc/mediatek/mtk-cmdq-helper.c |  258 ++++++++++++++++++++++++++++++++
> > > > > >  include/linux/soc/mediatek/mtk-cmdq.h  |  132 ++++++++++++++++
> > > > > >  4 files changed, 403 insertions(+)
> > > > > >  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > >  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> > > > > > 
> > > > > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > > > > > index a7d0667..17bd759 100644
> > > > > > --- a/drivers/soc/mediatek/Kconfig
> > > > > > +++ b/drivers/soc/mediatek/Kconfig
> > > > > > @@ -4,6 +4,18 @@
> > > > > >  menu "MediaTek SoC drivers"
> > > > > >  	depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > > >  
> > > > > 
> > > 
> > > [...]
> > > 
> > > > > > +
> > > > > > +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > > > > > +{
> > > > > > +	int err;
> > > > > > +
> > > > > > +	if (cmdq_pkt_is_finalized(pkt))
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	/* insert EOC and generate IRQ for each command iteration */
> > > > > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > > > > > +	if (err < 0)
> > > > > > +		return err;
> > > > > > +
> > > > > > +	/* JUMP to end */
> > > > > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > > > > > +	if (err < 0)
> > > > > > +		return err;
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
> > > > > > +			 cmdq_async_flush_cb cb, void *data)
> > > > > > +{
> > > > > > +	int err;
> > > > > > +	struct device *dev;
> > > > > > +	dma_addr_t dma_addr;
> > > > > > +
> > > > > > +	err = cmdq_pkt_finalize(pkt);
> > > > > > +	if (err < 0)
> > > > > > +		return err;
> > > > > > +
> > > > > > +	dev = client->chan->mbox->dev;
> > > > > > +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->cmd_buf_size,
> > > > > > +		DMA_TO_DEVICE);
> > > > > 
> > > > > You map here, but I could not find unmap, so the unmap should be done in
> > > > > client driver. I would prefer a symmetric map/unmap which means that
> > > > > both map and unmap are done in client driver. I think you put map here
> > > > > because you should map after finalize. 
> > > > 
> > > > The unmap is done before callback to client, in function
> > > > cmdq_task_exec_done, mtk-cmdq-mailbox.c.
> > > 
> > > You put unmap in mailbox controller, and map here (here would be mailbox
> > > client), so this is not symmetric. If the code is asymmetric, it's easy
> > > to cause bug and not easy to maintain. So I would like move both
> > > map/unmap to client driver.
> > > 
> > 
> > Since map/unmap is common code for client drivers, can we move unmap to
> > CMDQ helper and do not put in client driver?
> > 
> > > > 
> > > > > Therefore, export
> > > > > cmdq_pkt_finalize() to client driver and let client do finalize, so
> > > > > there is no finalize in flush function. This method have a benefit that
> > > > > if client reuse command buffer, it need not to map/unmap frequently.
> > > > 
> > > > If client reuse command buffer or cmdq_pkt(command queue packet), client
> > > > will add new commands to the cmdq_pkt, so map/unmap are necessary for
> > > > each cmdq_pkt flush.
> > > 
> > > If the buffer size is 4K bytes, client driver could map the whole 4K at
> > > initialization. Before it write new command, it call
> > > dma_sync_single_for_cpu(), after it write new command, it call
> > > dma_sync_single_for_device(). And then it could flush this buffer to
> > > mailbox controller. So client driver just call dma sync function when it
> > > reuse the command buffer. dma sync function is much lightweight then dma
> > > map because map would search the memory area which could be mapped.
> > > 
> > > Regards,
> > > CK
> > 
> > Maybe we can do dma map/unmap/sync in cmdq helper, and make client
> > driver simple.
> > 
> 
> Why are map/unmap common code for client drivers? I've mentioned that
> some client driver may just call dma sync function before flush, so it
> does not map for every flush. Frequently map/unmap has some drawback:
> 
> 1. Waste CPU resource: this also waste power.
> 2. Risk of mapping fail: to reduce this risk, client driver could map at
> initialization.
> 

Thanks for your advice. One way to achieve this is as follows.
1. do dma map immediately after the cmdq buffer is allocated in
cmdq_pkt_create(), and do dma unmap in cmdq_pkt_destroy.
2. do dma sync for device before a cmdq packet is sent to mailbox in
cmdq_pkt_flush_async().
3. do dma sync for cpu after a cmdq packet is executed by GCE HW in
callback function which will be revised and moved to cmdq helper.

Thus, a cmdq packet will be flushed more efficiently if client driver
would reuse the cmdq packet.


> I think reducing these drawback is more important than making client
> driver simple.
> 
> Regards,
> CK
> 
> > > 
> > > > 
> > > > > 
> > > > > Regards,
> > > > > CK
> > > > > 
> > > > > > +	if (dma_mapping_error(dev, dma_addr)) {
> > > > > > +		dev_err(client->chan->mbox->dev, "dma map failed\n");
> > > > > > +		return -ENOMEM;
> > > > > > +	}
> > > > > > +
> > > > > > +	pkt->pa_base = dma_addr;
> > > > > > +	pkt->cb.cb = cb;
> > > > > > +	pkt->cb.data = data;
> > > > > > +
> > > > > > +	mbox_send_message(client->chan, pkt);
> > > > > > +	/* We can send next packet immediately, so just call txdone. */
> > > > > > +	mbox_client_txdone(client->chan, 0);
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> > > > > > +
> > > > > > +struct cmdq_flush_completion {
> > > > > > +	struct completion cmplt;
> > > > > > +	bool err;
> > > > > > +};
> > > > > > +
> > > > > > +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> > > > > > +{
> > > > > > +	struct cmdq_flush_completion *cmplt = data.data;
> > > > > > +
> > > > > > +	cmplt->err = data.err;
> > > > > > +	complete(&cmplt->cmplt);
> > > > > > +}
> > > > > > +
> > > > > > +int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt)
> > > > > > +{
> > > > > > +	struct cmdq_flush_completion cmplt;
> > > > > > +	int err;
> > > > > > +
> > > > > > +	init_completion(&cmplt.cmplt);
> > > > > > +	err = cmdq_pkt_flush_async(client, pkt, cmdq_pkt_flush_cb, &cmplt);
> > > > > > +	if (err < 0)
> > > > > > +		return err;
> > > > > > +	wait_for_completion(&cmplt.cmplt);
> > > > > > +
> > > > > > +	return cmplt.err ? -EFAULT : 0;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(cmdq_pkt_flush);
> > > > > 
> > > > > [...]
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> > 
> 
> 

Re: [PATCH v22 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

[houlong wei]

On Wed, 2018-07-04 at 17:03 +0800, CK Hu wrote:
> Hi, Houlong:
> 
> On Wed, 2018-07-04 at 08:10 +0800, houlong wei wrote:
> > On Fri, 2018-06-29 at 15:08 +0800, CK Hu wrote:
> > > Hi, Houlong:
> > > 
> > > Some inline comment.
> > > 
> > > On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > > > This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> > > > CMDQ is used to help write registers with critical time limitation,
> > > > such as updating display configuration during the vblank. It controls
> > > > Global Command Engine (GCE) hardware to achieve this requirement.
> > > > Currently, CMDQ only supports display related hardwares, but we expect
> > > > it can be extended to other hardwares for future requirements.
> > > > 
> > > > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > > > ---
> > > >  drivers/mailbox/Kconfig                  |   10 +
> > > >  drivers/mailbox/Makefile                 |    2 +
> > > >  drivers/mailbox/mtk-cmdq-mailbox.c       |  634 ++++++++++++++++++++++++++++++
> > > >  include/linux/mailbox/mtk-cmdq-mailbox.h |   70 ++++
> > > >  4 files changed, 716 insertions(+)
> > > >  create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
> > > >  create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h
> > > > 
> > > 
> > > [...]
> > > 
> > > > +
> > > > +static int cmdq_thread_reset(struct cmdq *cmdq, struct cmdq_thread *thread)
> > > > +{
> > > > +	u32 warm_reset;
> > > > +
> > > > +	writel(CMDQ_THR_DO_WARM_RESET, thread->base + CMDQ_THR_WARM_RESET);
> > > > +	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_WARM_RESET,
> > > > +			warm_reset, !(warm_reset & CMDQ_THR_DO_WARM_RESET),
> > > > +			0, 10)) {
> > > > +		dev_err(cmdq->mbox.dev, "reset GCE thread 0x%x failed\n",
> > > > +			(u32)(thread->base - cmdq->base));
> > > > +		return -EFAULT;
> > > > +	}
> > > > +	writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base + CMDQ_THR_SLOT_CYCLES);
> > > 
> > > The CMDQ_THR_SLOT_CYCLES looks like not relevant to reset. Maybe you
> > > just need to set this value when startup.
> > 
> > Will move configuration of CMDQ_THR_SLOT_CYCLES to cmdq_xlate() where is
> > startup of a GCE thread.
> > 

Since cmdq_xlate() is called when a client requests a channel, it may be
called more than once. Will move it to cmdq_probe().

> > > 
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > 
> > > [...]
> > > 
> > > > +
> > > > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> > > > +{
> > > > +	struct cmdq *cmdq;
> > > > +	struct cmdq_task *task;
> > > > +	unsigned long curr_pa, end_pa;
> > > > +
> > > > +	cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> > > > +
> > > > +	/* Client should not flush new tasks if suspended. */
> > > > +	WARN_ON(cmdq->suspended);
> > > > +
> > > > +	task = kzalloc(sizeof(*task), GFP_ATOMIC);
> > > > +	task->cmdq = cmdq;
> > > > +	INIT_LIST_HEAD(&task->list_entry);
> > > > +	task->pa_base = pkt->pa_base;
> > > > +	task->thread = thread;
> > > > +	task->pkt = pkt;
> > > > +
> > > > +	if (list_empty(&thread->task_busy_list)) {
> > > > +		WARN_ON(clk_enable(cmdq->clock) < 0);
> > > > +		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
> > > > +
> > > > +		writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
> > > > +		writel(task->pa_base + pkt->cmd_buf_size,
> > > > +		       thread->base + CMDQ_THR_END_ADDR);
> > > > +		writel(thread->priority, thread->base + CMDQ_THR_PRIORITY);
> > > > +		writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
> > > > +		writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
> > > > +
> > > > +		if (thread->timeout_ms != CMDQ_NO_TIMEOUT)
> > > > +			mod_timer(&thread->timeout,
> > > > +				jiffies + msecs_to_jiffies(thread->timeout_ms));
> > > 
> > > I think the timeout processing should be done by client driver. The
> > > total time to execute a command buffer does not depend on GCE HW speed
> > > because the WFE (wait for event) command would wait for client HW event,
> > > so the total time depend on how long a client HW send this event to GCE
> > > and the timeout processing should be client driver's job. Each client
> > > may have different timeout processing mechanism, for example, if display
> > > could dynamic change panel frame rate between 120Hz and 60Hz, and the
> > > timeout time is 2 frame, so it may dynamically change timeout time
> > > between 17ms and 33ms. Another reason is that display have interrupt
> > > every vblank, and it could check timeout in that interrupt, so the timer
> > > in cmdq driver looks redundant. Because each client would define its own
> > > timeout processing mechanism, so it's not wise to put timeout processing
> > > in cmdq driver.
> > 
> > The client drivers' owners strongly hope to keep the current timeout
> > mechanism, the reasons are below.
> > 1. If remove, all clients should add timeout mechanism and the code will
> > be redundant.
> > 2. If timeout happens, only GCE driver can do reset and continue to
> > execute next packet.
> 
> For the reason 2, GCE should not continue execute next packet because
> the packets may have dependency. So GCE driver could only drop all
> packet (this is what you do in cmdq_thread_handle_timeout()). For reason
> 1, you have a assumption that all client have the same request for
> timeout: constant timeout value or no timeout. If it's so, that's ok to
> put timeout mechanism in cmdq driver. But if one day, a new request for
> timeout, for example, dynamic timeout value, that means not all client
> driver implement timeout in the same way, putting timeout mechanism in
> cmdq driver does not reduce any thing but just move multiple client
> driver's code into cmdq driver. I would accept to put here only if all
> client driver use the same timeout mechanism.

The client drivers configure their timeout values via 'mboxes' properity
in device tree. So far, they handle timeout in same way.

> 
> Regards,
> CK
> 
> > 
> > > 
> > > > +	} else {
> > > > +		WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> > > > +		curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR);
> > > > +		end_pa = readl(thread->base + CMDQ_THR_END_ADDR);
> > > > +
> > > > +		/*
> > > > +		 * Atomic execution should remove the following wfe, i.e. only
> > > > +		 * wait event at first task, and prevent to pause when running.
> > > > +		 */
> > > > +		if (thread->atomic_exec) {
> > > > +			/* GCE is executing if command is not WFE */
> > > > +			if (!cmdq_thread_is_in_wfe(thread)) {
> > > > +				cmdq_thread_resume(thread);
> > > > +				cmdq_thread_wait_end(thread, end_pa);
> > > > +				WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> > > > +				/* set to this task directly */
> > > > +				writel(task->pa_base,
> > > > +				       thread->base + CMDQ_THR_CURR_ADDR);
> > > > +			} else {
> > > > +				cmdq_task_insert_into_thread(task);
> > > > +				cmdq_task_remove_wfe(task);
> > > > +				smp_mb(); /* modify jump before enable thread */
> > > > +			}
> > > > +		} else {
> > > > +			/* check boundary */
> > > > +			if (curr_pa == end_pa - CMDQ_INST_SIZE ||
> > > > +			    curr_pa == end_pa) {
> > > > +				/* set to this task directly */
> > > > +				writel(task->pa_base,
> > > > +				       thread->base + CMDQ_THR_CURR_ADDR);
> > > > +			} else {
> > > > +				cmdq_task_insert_into_thread(task);
> > > > +				smp_mb(); /* modify jump before enable thread */
> > > > +			}
> > > > +		}
> > > > +		writel(task->pa_base + pkt->cmd_buf_size,
> > > > +		       thread->base + CMDQ_THR_END_ADDR);
> > > > +		cmdq_thread_resume(thread);
> > > > +	}
> > > > +	list_move_tail(&task->list_entry, &thread->task_busy_list);
> > > > +}
> > > > +
> > > > +static void cmdq_task_exec_done(struct cmdq_task *task, bool err)
> > > > +{
> > > > +	struct device *dev = task->cmdq->mbox.dev;
> > > > +	struct cmdq_cb_data cmdq_cb_data;
> > > > +
> > > > +	dma_unmap_single(dev, task->pa_base, task->pkt->cmd_buf_size,
> > > > +			 DMA_TO_DEVICE);
> > > 
> > > Move this to client driver.
> > 
> > map/unmap are common code for clients driver, could we move it to cmdq
> > helper?
> > 
> > > 
> > > > +	if (task->pkt->cb.cb) {
> > > > +		cmdq_cb_data.err = err;
> > > > +		cmdq_cb_data.data = task->pkt->cb.data;
> > > > +		task->pkt->cb.cb(cmdq_cb_data);
> > > > +	}
> > > > +	list_del(&task->list_entry);
> > > > +}
> > > > +
> > > 
> > > [...]
> > > 
> > > > +
> > > > +static bool cmdq_mbox_last_tx_done(struct mbox_chan *chan)
> > > > +{
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static const struct mbox_chan_ops cmdq_mbox_chan_ops = {
> > > > +	.send_data = cmdq_mbox_send_data,
> > > > +	.startup = cmdq_mbox_startup,
> > > > +	.shutdown = cmdq_mbox_shutdown,
> > > > +	.last_tx_done = cmdq_mbox_last_tx_done,
> > > 
> > > Because mbox->txdone_poll is false, so you need not to implement
> > > last_tx_done.
> > > 
> > > Regards,
> > > CK
> > 
> > Will remove cmdq_mbox_last_tx_done().
> > 
> > > 
> > > > +};
> > > > +
> > > > +static struct mbox_chan *cmdq_xlate(struct mbox_controller *mbox,
> > > > +		const struct of_phandle_args *sp)
> > > > +{
> > > > +	int ind = sp->args[0];
> > > > +	struct cmdq_thread *thread;
> > > > +
> > > > +	if (ind >= mbox->num_chans)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	thread = mbox->chans[ind].con_priv;
> > > > +	thread->timeout_ms = sp->args[1];
> > > > +	thread->priority = sp->args[2];
> > > > +	thread->atomic_exec = (sp->args[3] != 0);
> > > > +	thread->chan = &mbox->chans[ind];
> > > > +
> > > > +	return &mbox->chans[ind];
> > > > +}
> > > > +
> > > [...]
> > > 
> > > 
> > 
> > 
> 
> 

Re: [PATCH v22 1/4] dt-bindings: soc: Add documentation for the MediaTek GCE unit

[houlong wei]

On Fri, 2018-07-06 at 04:08 +0800, Rob Herring wrote:
> On Tue, Jul 3, 2018 at 5:39 PM houlong wei <houlong.wei@mediatek.com> wrote:
> >
> > On Tue, 2018-07-03 at 10:30 +0800, Rob Herring wrote:
> > > On Wed, Jun 27, 2018 at 07:16:09PM +0800, Houlong Wei wrote:
> > > > This adds documentation for the MediaTek Global Command Engine (GCE) unit
> > > > found in MT8173 SoCs.
> > > >
> > > > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > > ---
> > > > Hi Rob,
> > > >   I don't add your ACK in this version since the dt-binding description
> > > > has been changed. Thanks.
> > > > ---
> > > >  .../devicetree/bindings/mailbox/mtk-gce.txt        |   65 ++++++++++++++++++++
> > > >  include/dt-bindings/gce/mt8173-gce.h               |   48 +++++++++++++++
> > > >  2 files changed, 113 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> > > >  create mode 100644 include/dt-bindings/gce/mt8173-gce.h
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mailbox/mtk-gce.txt b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> > > > new file mode 100644
> > > > index 0000000..26f65a4
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> > > > @@ -0,0 +1,65 @@
> > > > +MediaTek GCE
> > > > +===============
> > > > +
> > > > +The Global Command Engine (GCE) is used to help read/write registers with
> > > > +critical time limitation, such as updating display configuration during the
> > > > +vblank. The GCE can be used to implement the Command Queue (CMDQ) driver.
> > > > +
> > > > +CMDQ driver uses mailbox framework for communication. Please refer to
> > > > +mailbox.txt for generic information about mailbox device-tree bindings.
> > > > +
> > > > +Required properties:
> > > > +- compatible: Must be "mediatek,mt8173-gce"
> > > > +- reg: Address range of the GCE unit
> > > > +- interrupts: The interrupt signal from the GCE block
> > > > +- clock: Clocks according to the common clock binding
> > > > +- clock-names: Must be "gce" to stand for GCE clock
> > > > +- thread-num: Maximum threads count of GCE.
> > >
> > > mediatek,thread-num
> > >
> > > Is this needed for anything other than error checking the thread id in
> > > the mbox cells? if that's it, then drop it.
> > >
> >
> > 'thread-num' is used to configure the GCE thread number, which is the
> > channel number of gce mailbox. This property is read in
> > cmdq_probe()/mtk-cmdq-mailbox.c and a mailbox's channel array is
> > allocated according to the number.
> > Since the thread number may be different on different SoC, we wish it
> > could be configured in device tree.
> 
> You should have different compatible strings for different SoCs and
> can imply the number of threads from that.
> 

Thanks. Will remove 'thread-num' form device tree and put it in match
table of cmdq dirver.

> Or if the number of threads doesn't vary greatly, just allocate the
> max # of channels. Or allocate the per thread data when a thread is
> actually in use.
> 
> >
> > > > +- #mbox-cells: Should be 4.
> > > > +   <&phandle channel timeout priority atomic_exec>
> > > > +   phandle: Label name of a gce node.
> > > > +   channel: Channel of mailbox. Be equal to the thread id of GCE.
> > > > +   timeout: Maximum time of software waiting GCE processing done, in unit
> > > > +           of millisecond.
> > > > +   priority: Priority of GCE thread.
> > > > +   atomic_exec: GCE processing continuous packets of commands in atomic
> > > > +           way.
> > > > +
> > > > +Required properties for a client device:
> > > > +- mboxes: Client use mailbox to communicate with GCE, it should have this
> > > > +  property and list of phandle, mailbox specifiers.
> > > > +- gce-subsys: Specify the sub-system id which is corresponding to the register
> > > > +  address.
> > >
> > > What is this for?
> >
> > You mean the new added property 'gce-subsys'?
> > It is used for GCE to distinguish the high 16-bit of a hardware register
> > address. When a client driver packets a register setting into a GCE
> > instruction, it uses a sub-system code and register offset instead of
> > the 32-bit register address.
> > Since sub-system code may be different on different SoC, we wish it
> > could be configured in device tree.
> 
> Okay. It needs a vendor prefix and to specify the size and type of the value.
> 

Will do.

> >
> > >
> > > > +
> > > > +Optional properties for a client device:
> > > > +- gce-event: Specify the event if the client has any. Because the event is
> > > > +  parsed by client, so client can replace 'gce-event' with other meaningful
> > > > +  name.
> > >
> > > If the client sets the name, then no point having here. It must be
> > > documented in the client binding. But then, what is this for in the
> > > first place?
> >
> > Since display driver will use GCE firstly, so we will move the
> > description to
> > 'Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt'
> > when display driver start using the GCE driver.
> > Is that ok?
> 
> Not sure. I don't understand how it is used.
> 
> > > > +
> > > > +Some vaules of properties are defined in 'dt-bindings/gce/mt8173-gce.h'. Such as
> > > > +thread number, sub-system ids, thread priority, event ids.
> > > > +
> > > > +Example:
> > > > +
> > > > +   gce: gce@10212000 {
> > >
> > > mailbox@...
> >
> > Will do.
> >
> > >
> > > > +           compatible = "mediatek,mt8173-gce";
> > > > +           reg = <0 0x10212000 0 0x1000>;
> > > > +           interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>;
> > > > +           clocks = <&infracfg CLK_INFRA_GCE>;
> > > > +           clock-names = "gce";
> > > > +           thread-num = CMDQ_THR_MAX_COUNT;
> > > > +           #mbox-cells = <4>;
> > > > +   };
> > > > +
> > > > +Example for a client device:
> > > > +
> > > > +   mmsys: clock-controller@14000000 {
> > > > +           compatible = "mediatek,mt8173-mmsys";
> > > > +           mboxes = <&gce 0 2000 CMDQ_THR_PRIO_LOWEST 1>,
> > > > +                    <&gce 1 2000 CMDQ_THR_PRIO_LOWEST 1>;
> > > > +           gce-subsys = <SUBSYS_1400XXXX>;
> > > > +           mutex-event-eof = <CMDQ_EVENT_MUTEX0_STREAM_EOF
> > > > +                           CMDQ_EVENT_MUTEX1_STREAM_EOF>;
> > > > +
> > > > +           ...
> > > > +   };
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html