[PATCH v14 0/4] Mediatek MT8173 CMDQ support

[PATCH v14 0/4] Mediatek MT8173 CMDQ support

[HS Liao]

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.

These patches have a build dependency on top of v4.8-rc1.

Changes since v13:
 - move to mailbox folder
 - simplify cmdq_thread_is_in_wfe

Best regards,
HS Liao

HS Liao (4):
  dt-bindings: soc: Add documentation for the MediaTek GCE unit
  CMDQ: Mediatek CMDQ driver
  arm64: dts: mt8173: Add GCE node
  CMDQ: save more energy in idle

 .../devicetree/bindings/mailbox/mtk-gce.txt        |  43 +
 arch/arm64/boot/dts/mediatek/mt8173.dtsi           |  10 +
 drivers/mailbox/Kconfig                            |  10 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/mtk-cmdq.c                         | 965 +++++++++++++++++++++
 include/linux/mailbox/mtk-cmdq.h                   | 180 ++++
 6 files changed, 1210 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
 create mode 100644 drivers/mailbox/mtk-cmdq.c
 create mode 100644 include/linux/mailbox/mtk-cmdq.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[HS Liao]

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

Signed-off-by: HS Liao <hs.liao@mediatek.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/mailbox/mtk-gce.txt        | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt

diff --git a/Documentation/devicetree/bindings/mailbox/mtk-gce.txt b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
new file mode 100644
index 0000000..d2d3ccb
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mtk-gce.txt
@@ -0,0 +1,43 @@
+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
+- #mbox-cells: Should be 2
+
+Required properties for a client device:
+- mboxes: client use mailbox to communicate with GCE, it should have this
+  property and list of phandle, mailbox channel specifiers, and atomic
+  execution flag.
+
+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";
+
+		#mbox-cells = <2>;
+	};
+
+Example for a client device:
+
+	mmsys: clock-controller@14000000 {
+		compatible = "mediatek,mt8173-mmsys";
+		mboxes = <&gce 0 1 /* main display with atomic execution */
+			  &gce 1 1>; /* sub display with atomic execution */
+		...
+	};
-- 
1.9.1

[PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[HS Liao]

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: 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.c       | 927 +++++++++++++++++++++++++++++++++++++++
 include/linux/mailbox/mtk-cmdq.h | 180 ++++++++
 4 files changed, 1119 insertions(+)
 create mode 100644 drivers/mailbox/mtk-cmdq.c
 create mode 100644 include/linux/mailbox/mtk-cmdq.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 97c3729..c987382 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -132,4 +132,14 @@ config BCM_PDC_MBOX
 	  Mailbox implementation for the Broadcom PDC ring manager,
 	  which provides access to various offload engines on Broadcom
 	  SoCs. Say Y here if you want to use the Broadcom PDC.
+
+config MTK_CMDQ
+	bool "MediaTek CMDQ Support"
+	depends on ARM64 && ( ARCH_MEDIATEK || COMPILE_TEST )
+	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.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 66c38e3..eb5e04e 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -27,3 +27,5 @@ obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
 obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
+
+obj-$(CONFIG_MTK_CMDQ)		+= mtk-cmdq.o
diff --git a/drivers/mailbox/mtk-cmdq.c b/drivers/mailbox/mtk-cmdq.c
new file mode 100644
index 0000000..daf5561
--- /dev/null
+++ b/drivers/mailbox/mtk-cmdq.c
@@ -0,0 +1,927 @@
+/*
+ * Copyright (c) 2015 MediaTek Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/completion.h>
+#include <linux/dma-mapping.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/kthread.h>
+#include <linux/mailbox/mtk-cmdq.h>
+#include <linux/mutex.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/suspend.h>
+#include <linux/timer.h>
+
+#define CMDQ_THR_MAX_COUNT		3 /* main, sub, general(misc) */
+#define CMDQ_INST_SIZE			8 /* instruction is 64-bit */
+#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_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_ACTIVE_SLOT_CYCLES	0x3200
+#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_OP_CODE_SHIFT		24
+#define CMDQ_SUBSYS_SHIFT		16
+
+#define CMDQ_ARG_A_WRITE_MASK		0xffff
+#define CMDQ_OP_CODE_MASK		(0xff << CMDQ_OP_CODE_SHIFT)
+
+#define CMDQ_WRITE_ENABLE_MASK		BIT(0)
+#define CMDQ_JUMP_BY_OFFSET		0x10000000
+#define CMDQ_JUMP_BY_PA			0x10000001
+#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
+#define CMDQ_EOC_IRQ_EN			BIT(0)
+
+/*
+ * 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_task_cb {
+	cmdq_async_flush_cb	cb;
+	void			*data;
+};
+
+struct cmdq_thread {
+	struct mbox_chan	*chan;
+	void __iomem		*base;
+	struct list_head	task_busy_list;
+	struct timer_list	timeout;
+	bool			atomic_exec;
+};
+
+struct cmdq_task {
+	struct cmdq		*cmdq;
+	struct list_head	list_entry;
+	void			*va_base;
+	dma_addr_t		pa_base;
+	size_t			cmd_buf_size; /* command occupied size */
+	size_t			buf_size; /* real buffer size */
+	bool			finalized;
+	struct cmdq_thread	*thread;
+	struct cmdq_task_cb	cb;
+};
+
+struct cmdq {
+	struct mbox_controller	mbox;
+	void __iomem		*base;
+	u32			irq;
+	struct cmdq_thread	thread[CMDQ_THR_MAX_COUNT];
+	struct mutex		task_mutex;
+	struct clk		*clock;
+	bool			suspended;
+};
+
+struct cmdq_subsys {
+	u32	base;
+	int	id;
+};
+
+static const struct cmdq_subsys gce_subsys[] = {
+	{0x1400, 1},
+	{0x1401, 2},
+	{0x1402, 3},
+};
+
+static int cmdq_subsys_base_to_id(u32 base)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(gce_subsys); i++)
+		if (gce_subsys[i].base == base)
+			return gce_subsys[i].id;
+	return -EFAULT;
+}
+
+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->va_base;
+
+	/* let previous task jump to this task */
+	dma_sync_single_for_cpu(dev, prev_task->pa_base,
+				prev_task->cmd_buf_size, DMA_TO_DEVICE);
+	prev_task_base[CMDQ_NUM_CMD(prev_task) - 1] =
+		(u64)CMDQ_JUMP_BY_PA << 32 | task->pa_base;
+	dma_sync_single_for_device(dev, prev_task->pa_base,
+				   prev_task->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->va_base;
+	int i;
+
+	dma_sync_single_for_cpu(dev, task->pa_base, task->cmd_buf_size,
+				DMA_TO_DEVICE);
+	for (i = 0; i < CMDQ_NUM_CMD(task); 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->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_task *task, struct cmdq_thread *thread)
+{
+	struct cmdq *cmdq = task->cmdq;
+	unsigned long curr_pa, end_pa;
+
+	task->thread = thread;
+	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 + task->cmd_buf_size,
+		       thread->base + CMDQ_THR_END_ADDR);
+		writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
+		writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
+
+		mod_timer(&thread->timeout,
+			  jiffies + msecs_to_jiffies(CMDQ_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 + task->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;
+
+	if (task->cb.cb) {
+		cmdq_cb_data.err = err;
+		cmdq_cb_data.data = task->cb.data;
+		task->cb.cb(cmdq_cb_data);
+	}
+	list_del(&task->list_entry);
+	dma_unmap_single(dev, task->pa_base, task->cmd_buf_size, DMA_TO_DEVICE);
+	kfree(task->va_base);
+}
+
+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->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 {
+		mod_timer(&thread->timeout,
+			  jiffies + msecs_to_jiffies(CMDQ_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(unsigned long data)
+{
+	struct cmdq_thread *thread = (struct cmdq_thread *)data;
+	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_task_realloc_cmd_buffer(struct cmdq_task *task, size_t size)
+{
+	void *new_buf;
+
+	new_buf = krealloc(task->va_base, size, GFP_KERNEL | __GFP_ZERO);
+	if (!new_buf)
+		return -ENOMEM;
+	task->va_base = new_buf;
+	task->buf_size = size;
+	return 0;
+}
+
+struct cmdq_base *cmdq_register_device(struct device *dev)
+{
+	struct cmdq_base *cmdq_base;
+	struct resource res;
+	int subsys;
+	u32 base;
+
+	if (of_address_to_resource(dev->of_node, 0, &res))
+		return NULL;
+	base = (u32)res.start;
+
+	subsys = cmdq_subsys_base_to_id(base >> 16);
+	if (subsys < 0)
+		return NULL;
+
+	cmdq_base = devm_kmalloc(dev, sizeof(*cmdq_base), GFP_KERNEL);
+	if (!cmdq_base)
+		return NULL;
+	cmdq_base->subsys = subsys;
+	cmdq_base->base = base;
+
+	return cmdq_base;
+}
+EXPORT_SYMBOL(cmdq_register_device);
+
+struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
+{
+	struct cmdq_client *client;
+
+	client = kzalloc(sizeof(*client), GFP_KERNEL);
+	client->client.dev = dev;
+	client->client.tx_block = false;
+	client->chan = mbox_request_channel(&client->client, index);
+	return client;
+}
+EXPORT_SYMBOL(cmdq_mbox_create);
+
+int cmdq_task_create(struct device *dev, struct cmdq_task **task_ptr)
+{
+	struct cmdq_task *task;
+	int err;
+
+	task = kzalloc(sizeof(*task), GFP_KERNEL);
+	if (!task)
+		return -ENOMEM;
+	task->cmdq = dev_get_drvdata(dev);
+	err = cmdq_task_realloc_cmd_buffer(task, PAGE_SIZE);
+	if (err < 0) {
+		kfree(task);
+		return err;
+	}
+	*task_ptr = task;
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_task_create);
+
+static int cmdq_task_append_command(struct cmdq_task *task, enum cmdq_code code,
+				    u32 arg_a, u32 arg_b)
+{
+	u64 *cmd_ptr;
+	int err;
+
+	if (WARN_ON(task->finalized))
+		return -EBUSY;
+	if (unlikely(task->cmd_buf_size + CMDQ_INST_SIZE > task->buf_size)) {
+		err = cmdq_task_realloc_cmd_buffer(task, task->buf_size * 2);
+		if (err < 0)
+			return err;
+	}
+	cmd_ptr = task->va_base + task->cmd_buf_size;
+	(*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
+	task->cmd_buf_size += CMDQ_INST_SIZE;
+	return 0;
+}
+
+int cmdq_task_write(struct cmdq_task *task, u32 value, struct cmdq_base *base,
+		    u32 offset)
+{
+	u32 arg_a = ((base->base + offset) & CMDQ_ARG_A_WRITE_MASK) |
+		    (base->subsys << CMDQ_SUBSYS_SHIFT);
+	return cmdq_task_append_command(task, CMDQ_CODE_WRITE, arg_a, value);
+}
+EXPORT_SYMBOL(cmdq_task_write);
+
+int cmdq_task_write_mask(struct cmdq_task *task, u32 value,
+			 struct cmdq_base *base, u32 offset, u32 mask)
+{
+	u32 offset_mask = offset;
+	int err;
+
+	if (mask != 0xffffffff) {
+		err = cmdq_task_append_command(task, CMDQ_CODE_MASK, 0, ~mask);
+		if (err < 0)
+			return err;
+		offset_mask |= CMDQ_WRITE_ENABLE_MASK;
+	}
+	return cmdq_task_write(task, value, base, offset_mask);
+}
+EXPORT_SYMBOL(cmdq_task_write_mask);
+
+static const u32 cmdq_event_value[CMDQ_MAX_EVENT] = {
+	/* Display start of frame(SOF) events */
+	[CMDQ_EVENT_DISP_OVL0_SOF] = 11,
+	[CMDQ_EVENT_DISP_OVL1_SOF] = 12,
+	[CMDQ_EVENT_DISP_RDMA0_SOF] = 13,
+	[CMDQ_EVENT_DISP_RDMA1_SOF] = 14,
+	[CMDQ_EVENT_DISP_RDMA2_SOF] = 15,
+	[CMDQ_EVENT_DISP_WDMA0_SOF] = 16,
+	[CMDQ_EVENT_DISP_WDMA1_SOF] = 17,
+	/* Display end of frame(EOF) events */
+	[CMDQ_EVENT_DISP_OVL0_EOF] = 39,
+	[CMDQ_EVENT_DISP_OVL1_EOF] = 40,
+	[CMDQ_EVENT_DISP_RDMA0_EOF] = 41,
+	[CMDQ_EVENT_DISP_RDMA1_EOF] = 42,
+	[CMDQ_EVENT_DISP_RDMA2_EOF] = 43,
+	[CMDQ_EVENT_DISP_WDMA0_EOF] = 44,
+	[CMDQ_EVENT_DISP_WDMA1_EOF] = 45,
+	/* Mutex end of frame(EOF) events */
+	[CMDQ_EVENT_MUTEX0_STREAM_EOF] = 53,
+	[CMDQ_EVENT_MUTEX1_STREAM_EOF] = 54,
+	[CMDQ_EVENT_MUTEX2_STREAM_EOF] = 55,
+	[CMDQ_EVENT_MUTEX3_STREAM_EOF] = 56,
+	[CMDQ_EVENT_MUTEX4_STREAM_EOF] = 57,
+	/* Display underrun events */
+	[CMDQ_EVENT_DISP_RDMA0_UNDERRUN] = 63,
+	[CMDQ_EVENT_DISP_RDMA1_UNDERRUN] = 64,
+	[CMDQ_EVENT_DISP_RDMA2_UNDERRUN] = 65,
+};
+
+int cmdq_task_wfe(struct cmdq_task *task, enum cmdq_event 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_task_append_command(task, CMDQ_CODE_WFE,
+			cmdq_event_value[event], arg_b);
+}
+EXPORT_SYMBOL(cmdq_task_wfe);
+
+int cmdq_task_clear_event(struct cmdq_task *task, enum cmdq_event event)
+{
+	if (event >= CMDQ_MAX_EVENT || event < 0)
+		return -EINVAL;
+
+	return cmdq_task_append_command(task, CMDQ_CODE_WFE,
+			cmdq_event_value[event], CMDQ_WFE_UPDATE);
+}
+EXPORT_SYMBOL(cmdq_task_clear_event);
+
+static int cmdq_task_finalize(struct cmdq_task *task)
+{
+	int err;
+
+	if (task->finalized)
+		return 0;
+
+	/* insert EOC and generate IRQ for each command iteration */
+	err = cmdq_task_append_command(task, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
+	if (err < 0)
+		return err;
+
+	/* JUMP to end */
+	err = cmdq_task_append_command(task, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
+	if (err < 0)
+		return err;
+
+	task->finalized = true;
+	return 0;
+}
+
+int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task *task,
+			  cmdq_async_flush_cb cb, void *data)
+{
+	struct cmdq *cmdq = task->cmdq;
+	int err;
+
+	mutex_lock(&cmdq->task_mutex);
+	if (cmdq->suspended) {
+		dev_err(cmdq->mbox.dev, "%s is called after suspended\n",
+			__func__);
+		mutex_unlock(&cmdq->task_mutex);
+		return -EPERM;
+	}
+
+	err = cmdq_task_finalize(task);
+	if (err < 0) {
+		mutex_unlock(&cmdq->task_mutex);
+		return err;
+	}
+
+	INIT_LIST_HEAD(&task->list_entry);
+	task->cb.cb = cb;
+	task->cb.data = data;
+	task->pa_base = dma_map_single(cmdq->mbox.dev, task->va_base,
+				       task->cmd_buf_size, DMA_TO_DEVICE);
+
+	mbox_send_message(client->chan, task);
+	/* We can send next task immediately, so just call txdone. */
+	mbox_client_txdone(client->chan, 0);
+	mutex_unlock(&cmdq->task_mutex);
+	return 0;
+}
+EXPORT_SYMBOL(cmdq_task_flush_async);
+
+struct cmdq_flush_completion {
+	struct completion cmplt;
+	bool err;
+};
+
+static void cmdq_task_flush_cb(struct cmdq_cb_data data)
+{
+	struct cmdq_flush_completion *cmplt = data.data;
+
+	cmplt->err = data.err;
+	complete(&cmplt->cmplt);
+}
+
+int cmdq_task_flush(struct cmdq_client *client, struct cmdq_task *task)
+{
+	struct cmdq_flush_completion cmplt;
+	int err;
+
+	init_completion(&cmplt.cmplt);
+	err = cmdq_task_flush_async(client, task, cmdq_task_flush_cb, &cmplt);
+	if (err < 0)
+		return err;
+	wait_for_completion(&cmplt.cmplt);
+	return cmplt.err ? -EFAULT : 0;
+}
+EXPORT_SYMBOL(cmdq_task_flush);
+
+void cmdq_mbox_free(struct cmdq_client *client)
+{
+	mbox_free_channel(client->chan);
+	kfree(client);
+}
+EXPORT_SYMBOL(cmdq_mbox_free);
+
+static int cmdq_suspend(struct device *dev)
+{
+	struct cmdq *cmdq = dev_get_drvdata(dev);
+	struct cmdq_thread *thread;
+	int i;
+	bool task_running = false;
+
+	mutex_lock(&cmdq->task_mutex);
+	cmdq->suspended = true;
+	mutex_unlock(&cmdq->task_mutex);
+
+	for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
+		thread = &cmdq->thread[i];
+		if (!list_empty(&thread->task_busy_list)) {
+			mod_timer(&thread->timeout, jiffies + 1);
+			task_running = true;
+		}
+	}
+
+	if (task_running) {
+		dev_warn(dev, "exist running task(s) in suspend\n");
+		msleep(20);
+	}
+
+	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);
+	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->atomic_exec = (sp->args[1] != 0);
+	thread->chan = &mbox->chans[ind];
+
+	return &mbox->chans[ind];
+}
+
+static int cmdq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	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 = irq_of_parse_and_map(node, 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);
+	}
+
+	mutex_init(&cmdq->task_mutex);
+
+	cmdq->mbox.dev = dev;
+	cmdq->mbox.chans = devm_kcalloc(dev, CMDQ_THR_MAX_COUNT,
+					sizeof(*cmdq->mbox.chans), GFP_KERNEL);
+	if (!cmdq->mbox.chans)
+		return -ENOMEM;
+
+	cmdq->mbox.num_chans = CMDQ_THR_MAX_COUNT;
+	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;
+
+	for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
+		cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
+				CMDQ_THR_SIZE * i;
+		INIT_LIST_HEAD(&cmdq->thread[i].task_busy_list);
+		init_timer(&cmdq->thread[i].timeout);
+		cmdq->thread[i].timeout.function = cmdq_thread_handle_timeout;
+		cmdq->thread[i].timeout.data = (unsigned long)&cmdq->thread[i];
+		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",
+		.owner = THIS_MODULE,
+		.pm = &cmdq_pm_ops,
+		.of_match_table = cmdq_of_ids,
+	}
+};
+
+builtin_platform_driver(cmdq_drv);
diff --git a/include/linux/mailbox/mtk-cmdq.h b/include/linux/mailbox/mtk-cmdq.h
new file mode 100644
index 0000000..c3c924d
--- /dev/null
+++ b/include/linux/mailbox/mtk-cmdq.h
@@ -0,0 +1,180 @@
+/*
+ * Copyright (c) 2015 MediaTek Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __MTK_CMDQ_H__
+#define __MTK_CMDQ_H__
+
+#include <linux/mailbox_client.h>
+#include <linux/mailbox_controller.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+/* display events in command queue(CMDQ) */
+enum cmdq_event {
+	/* Display start of frame(SOF) events */
+	CMDQ_EVENT_DISP_OVL0_SOF,
+	CMDQ_EVENT_DISP_OVL1_SOF,
+	CMDQ_EVENT_DISP_RDMA0_SOF,
+	CMDQ_EVENT_DISP_RDMA1_SOF,
+	CMDQ_EVENT_DISP_RDMA2_SOF,
+	CMDQ_EVENT_DISP_WDMA0_SOF,
+	CMDQ_EVENT_DISP_WDMA1_SOF,
+	/* Display end of frame(EOF) events */
+	CMDQ_EVENT_DISP_OVL0_EOF,
+	CMDQ_EVENT_DISP_OVL1_EOF,
+	CMDQ_EVENT_DISP_RDMA0_EOF,
+	CMDQ_EVENT_DISP_RDMA1_EOF,
+	CMDQ_EVENT_DISP_RDMA2_EOF,
+	CMDQ_EVENT_DISP_WDMA0_EOF,
+	CMDQ_EVENT_DISP_WDMA1_EOF,
+	/* Mutex end of frame(EOF) events */
+	CMDQ_EVENT_MUTEX0_STREAM_EOF,
+	CMDQ_EVENT_MUTEX1_STREAM_EOF,
+	CMDQ_EVENT_MUTEX2_STREAM_EOF,
+	CMDQ_EVENT_MUTEX3_STREAM_EOF,
+	CMDQ_EVENT_MUTEX4_STREAM_EOF,
+	/* Display underrun events */
+	CMDQ_EVENT_DISP_RDMA0_UNDERRUN,
+	CMDQ_EVENT_DISP_RDMA1_UNDERRUN,
+	CMDQ_EVENT_DISP_RDMA2_UNDERRUN,
+	/* Keep this at the end */
+	CMDQ_MAX_EVENT,
+};
+
+struct cmdq_cb_data {
+	bool	err;
+	void	*data;
+};
+
+typedef void (*cmdq_async_flush_cb)(struct cmdq_cb_data data);
+
+struct cmdq_task;
+
+struct cmdq_base {
+	int	subsys;
+	u32	base;
+};
+
+struct cmdq_client {
+	struct mbox_client client;
+	struct mbox_chan *chan;
+};
+
+/**
+ * cmdq_register_device() - register device which needs CMDQ
+ * @dev:	device for CMDQ to access its registers
+ *
+ * Return: cmdq_base pointer or NULL for failed
+ */
+struct cmdq_base *cmdq_register_device(struct device *dev);
+
+/**
+ * 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_task_create() - create CMDQ task
+ * @dev:	CMDQ device
+ * @task_ptr:	CMDQ task pointer to retrieve cmdq_task
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_task_create(struct device *dev, struct cmdq_task **task_ptr);
+
+/**
+ * cmdq_task_write() - append write command to the CMDQ task
+ * @task:	the CMDQ task
+ * @value:	the specified target register value
+ * @base:	the CMDQ base
+ * @offset:	register offset from module base
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_task_write(struct cmdq_task *task, u32 value,
+		    struct cmdq_base *base, u32 offset);
+
+/**
+ * cmdq_task_write_mask() - append write command with mask to the CMDQ task
+ * @task:	the CMDQ task
+ * @value:	the specified target register value
+ * @base:	the CMDQ base
+ * @offset:	register offset from module base
+ * @mask:	the specified target register mask
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_task_write_mask(struct cmdq_task *task, u32 value,
+			 struct cmdq_base *base, u32 offset, u32 mask);
+
+/**
+ * cmdq_task_wfe() - append wait for event command to the CMDQ task
+ * @task:	the CMDQ task
+ * @event:	the desired event type to "wait and CLEAR"
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_task_wfe(struct cmdq_task *task, enum cmdq_event event);
+
+/**
+ * cmdq_task_clear_event() - append clear event command to the CMDQ task
+ * @task:	the CMDQ task
+ * @event:	the desired event to be cleared
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_task_clear_event(struct cmdq_task *task, enum cmdq_event event);
+
+/**
+ * cmdq_task_flush() - trigger CMDQ to execute the recorded commands
+ * @client:	the CMDQ mailbox client
+ * @task:	the CMDQ task
+ *
+ * Return: 0 for success; else the error code is returned
+ *
+ * Trigger CMDQ to execute the recorded commands. Note that this is a
+ * synchronous flush function. When the function returned, the recorded
+ * commands have been done. CMDQ task will be destroy automatically
+ * after CMDQ finish all the recorded commands.
+ */
+int cmdq_task_flush(struct cmdq_client *client, struct cmdq_task *task);
+
+/**
+ * cmdq_task_flush_async() - trigger CMDQ to asynchronously execute the recorded
+ *			     commands and call back at the end of ISR
+ * @client:	the CMDQ mailbox client
+ * @task:	the CMDQ task
+ * @cb:		called at the end of CMDQ ISR
+ * @data:	this data will pass back to cb
+ *
+ * Return: 0 for success; else the error code is returned
+ *
+ * Trigger CMDQ to asynchronously execute the recorded commands and call back
+ * at the end of ISR. Note that this is an ASYNC function. When the function
+ * returned, it may or may not be finished. CMDQ task will be destroy
+ * automatically after CMDQ finish all the recorded commands.
+ */
+int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task *task,
+			  cmdq_async_flush_cb cb, void *data);
+
+/**
+ * cmdq_mbox_free() - destroy CMDQ mailbox client and channel
+ * @client:	the CMDQ mailbox client
+ */
+void cmdq_mbox_free(struct cmdq_client *client);
+
+#endif	/* __MTK_CMDQ_H__ */
-- 
1.9.1

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Jassi Brar]

On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <hs.liao@mediatek.com> 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: 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.c       | 927 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mailbox/mtk-cmdq.h | 180 ++++++++
>  4 files changed, 1119 insertions(+)
>  create mode 100644 drivers/mailbox/mtk-cmdq.c
>  create mode 100644 include/linux/mailbox/mtk-cmdq.h
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 97c3729..c987382 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -132,4 +132,14 @@ config BCM_PDC_MBOX
>           Mailbox implementation for the Broadcom PDC ring manager,
>           which provides access to various offload engines on Broadcom
>           SoCs. Say Y here if you want to use the Broadcom PDC.
> +
> +config MTK_CMDQ
> +       bool "MediaTek CMDQ Support"
> +       depends on ARM64 && ( ARCH_MEDIATEK || COMPILE_TEST )
> +       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.
>  endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 66c38e3..eb5e04e 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -27,3 +27,5 @@ obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>  obj-$(CONFIG_HI6220_MBOX)      += hi6220-mailbox.o
>
>  obj-$(CONFIG_BCM_PDC_MBOX)     += bcm-pdc-mailbox.o
> +
> +obj-$(CONFIG_MTK_CMDQ)         += mtk-cmdq.o
> diff --git a/drivers/mailbox/mtk-cmdq.c b/drivers/mailbox/mtk-cmdq.c
> new file mode 100644
> index 0000000..daf5561
> --- /dev/null
> +++ b/drivers/mailbox/mtk-cmdq.c
> @@ -0,0 +1,927 @@
> +/*
> + * Copyright (c) 2015 MediaTek Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/completion.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/mailbox/mtk-cmdq.h>
> +#include <linux/mutex.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/suspend.h>
> +#include <linux/timer.h>
> +
> +#define CMDQ_THR_MAX_COUNT             3 /* main, sub, general(misc) */
> +#define CMDQ_INST_SIZE                 8 /* instruction is 64-bit */
> +#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_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_ACTIVE_SLOT_CYCLES    0x3200
> +#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_OP_CODE_SHIFT             24
> +#define CMDQ_SUBSYS_SHIFT              16
> +
> +#define CMDQ_ARG_A_WRITE_MASK          0xffff
> +#define CMDQ_OP_CODE_MASK              (0xff << CMDQ_OP_CODE_SHIFT)
> +
> +#define CMDQ_WRITE_ENABLE_MASK         BIT(0)
> +#define CMDQ_JUMP_BY_OFFSET            0x10000000
> +#define CMDQ_JUMP_BY_PA                        0x10000001
> +#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
> +#define CMDQ_EOC_IRQ_EN                        BIT(0)
> +
> +/*
> + * 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_task_cb {
> +       cmdq_async_flush_cb     cb;
> +       void                    *data;
> +};
> +
> +struct cmdq_thread {
> +       struct mbox_chan        *chan;
> +       void __iomem            *base;
> +       struct list_head        task_busy_list;
> +       struct timer_list       timeout;
> +       bool                    atomic_exec;
> +};
> +
> +struct cmdq_task {
> +       struct cmdq             *cmdq;
> +       struct list_head        list_entry;
> +       void                    *va_base;
> +       dma_addr_t              pa_base;
> +       size_t                  cmd_buf_size; /* command occupied size */
> +       size_t                  buf_size; /* real buffer size */
> +       bool                    finalized;
> +       struct cmdq_thread      *thread;
> +       struct cmdq_task_cb     cb;
> +};
> +
> +struct cmdq {
> +       struct mbox_controller  mbox;
> +       void __iomem            *base;
> +       u32                     irq;
> +       struct cmdq_thread      thread[CMDQ_THR_MAX_COUNT];
> +       struct mutex            task_mutex;
> +       struct clk              *clock;
> +       bool                    suspended;
> +};
> +
> +struct cmdq_subsys {
> +       u32     base;
> +       int     id;
> +};
> +
> +static const struct cmdq_subsys gce_subsys[] = {
> +       {0x1400, 1},
> +       {0x1401, 2},
> +       {0x1402, 3},
> +};
> +
> +static int cmdq_subsys_base_to_id(u32 base)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(gce_subsys); i++)
> +               if (gce_subsys[i].base == base)
> +                       return gce_subsys[i].id;
> +       return -EFAULT;
> +}
> +
> +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->va_base;
> +
> +       /* let previous task jump to this task */
> +       dma_sync_single_for_cpu(dev, prev_task->pa_base,
> +                               prev_task->cmd_buf_size, DMA_TO_DEVICE);
> +       prev_task_base[CMDQ_NUM_CMD(prev_task) - 1] =
> +               (u64)CMDQ_JUMP_BY_PA << 32 | task->pa_base;
> +       dma_sync_single_for_device(dev, prev_task->pa_base,
> +                                  prev_task->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->va_base;
> +       int i;
> +
> +       dma_sync_single_for_cpu(dev, task->pa_base, task->cmd_buf_size,
> +                               DMA_TO_DEVICE);
> +       for (i = 0; i < CMDQ_NUM_CMD(task); 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->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_task *task, struct cmdq_thread *thread)
> +{
> +       struct cmdq *cmdq = task->cmdq;
> +       unsigned long curr_pa, end_pa;
> +
> +       task->thread = thread;
> +       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 + task->cmd_buf_size,
> +                      thread->base + CMDQ_THR_END_ADDR);
> +               writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
> +               writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
> +
> +               mod_timer(&thread->timeout,
> +                         jiffies + msecs_to_jiffies(CMDQ_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 + task->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;
> +
> +       if (task->cb.cb) {
> +               cmdq_cb_data.err = err;
> +               cmdq_cb_data.data = task->cb.data;
> +               task->cb.cb(cmdq_cb_data);
> +       }
> +       list_del(&task->list_entry);
> +       dma_unmap_single(dev, task->pa_base, task->cmd_buf_size, DMA_TO_DEVICE);
> +       kfree(task->va_base);
> +}
> +
> +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->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 {
> +               mod_timer(&thread->timeout,
> +                         jiffies + msecs_to_jiffies(CMDQ_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(unsigned long data)
> +{
> +       struct cmdq_thread *thread = (struct cmdq_thread *)data;
> +       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_task_realloc_cmd_buffer(struct cmdq_task *task, size_t size)
> +{
> +       void *new_buf;
> +
> +       new_buf = krealloc(task->va_base, size, GFP_KERNEL | __GFP_ZERO);
> +       if (!new_buf)
> +               return -ENOMEM;
> +       task->va_base = new_buf;
> +       task->buf_size = size;
> +       return 0;
> +}
> +
> +struct cmdq_base *cmdq_register_device(struct device *dev)
> +{
> +       struct cmdq_base *cmdq_base;
> +       struct resource res;
> +       int subsys;
> +       u32 base;
> +
> +       if (of_address_to_resource(dev->of_node, 0, &res))
> +               return NULL;
> +       base = (u32)res.start;
> +
> +       subsys = cmdq_subsys_base_to_id(base >> 16);
> +       if (subsys < 0)
> +               return NULL;
> +
> +       cmdq_base = devm_kmalloc(dev, sizeof(*cmdq_base), GFP_KERNEL);
> +       if (!cmdq_base)
> +               return NULL;
> +       cmdq_base->subsys = subsys;
> +       cmdq_base->base = base;
> +
> +       return cmdq_base;
> +}
> +EXPORT_SYMBOL(cmdq_register_device);
> +
> +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
> +{
> +       struct cmdq_client *client;
> +
> +       client = kzalloc(sizeof(*client), GFP_KERNEL);
> +       client->client.dev = dev;
> +       client->client.tx_block = false;
> +       client->chan = mbox_request_channel(&client->client, index);
> +       return client;
> +}
> +EXPORT_SYMBOL(cmdq_mbox_create);
> +
> +int cmdq_task_create(struct device *dev, struct cmdq_task **task_ptr)
> +{
> +       struct cmdq_task *task;
> +       int err;
> +
> +       task = kzalloc(sizeof(*task), GFP_KERNEL);
> +       if (!task)
> +               return -ENOMEM;
> +       task->cmdq = dev_get_drvdata(dev);
> +       err = cmdq_task_realloc_cmd_buffer(task, PAGE_SIZE);
> +       if (err < 0) {
> +               kfree(task);
> +               return err;
> +       }
> +       *task_ptr = task;
> +       return 0;
> +}
> +EXPORT_SYMBOL(cmdq_task_create);
> +
> +static int cmdq_task_append_command(struct cmdq_task *task, enum cmdq_code code,
> +                                   u32 arg_a, u32 arg_b)
> +{
> +       u64 *cmd_ptr;
> +       int err;
> +
> +       if (WARN_ON(task->finalized))
> +               return -EBUSY;
> +       if (unlikely(task->cmd_buf_size + CMDQ_INST_SIZE > task->buf_size)) {
> +               err = cmdq_task_realloc_cmd_buffer(task, task->buf_size * 2);
> +               if (err < 0)
> +                       return err;
> +       }
> +       cmd_ptr = task->va_base + task->cmd_buf_size;
> +       (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
> +       task->cmd_buf_size += CMDQ_INST_SIZE;
> +       return 0;
> +}
> +
> +int cmdq_task_write(struct cmdq_task *task, u32 value, struct cmdq_base *base,
> +                   u32 offset)
> +{
> +       u32 arg_a = ((base->base + offset) & CMDQ_ARG_A_WRITE_MASK) |
> +                   (base->subsys << CMDQ_SUBSYS_SHIFT);
> +       return cmdq_task_append_command(task, CMDQ_CODE_WRITE, arg_a, value);
> +}
> +EXPORT_SYMBOL(cmdq_task_write);
> +
> +int cmdq_task_write_mask(struct cmdq_task *task, u32 value,
> +                        struct cmdq_base *base, u32 offset, u32 mask)
> +{
> +       u32 offset_mask = offset;
> +       int err;
> +
> +       if (mask != 0xffffffff) {
> +               err = cmdq_task_append_command(task, CMDQ_CODE_MASK, 0, ~mask);
> +               if (err < 0)
> +                       return err;
> +               offset_mask |= CMDQ_WRITE_ENABLE_MASK;
> +       }
> +       return cmdq_task_write(task, value, base, offset_mask);
> +}
> +EXPORT_SYMBOL(cmdq_task_write_mask);
> +
> +static const u32 cmdq_event_value[CMDQ_MAX_EVENT] = {
> +       /* Display start of frame(SOF) events */
> +       [CMDQ_EVENT_DISP_OVL0_SOF] = 11,
> +       [CMDQ_EVENT_DISP_OVL1_SOF] = 12,
> +       [CMDQ_EVENT_DISP_RDMA0_SOF] = 13,
> +       [CMDQ_EVENT_DISP_RDMA1_SOF] = 14,
> +       [CMDQ_EVENT_DISP_RDMA2_SOF] = 15,
> +       [CMDQ_EVENT_DISP_WDMA0_SOF] = 16,
> +       [CMDQ_EVENT_DISP_WDMA1_SOF] = 17,
> +       /* Display end of frame(EOF) events */
> +       [CMDQ_EVENT_DISP_OVL0_EOF] = 39,
> +       [CMDQ_EVENT_DISP_OVL1_EOF] = 40,
> +       [CMDQ_EVENT_DISP_RDMA0_EOF] = 41,
> +       [CMDQ_EVENT_DISP_RDMA1_EOF] = 42,
> +       [CMDQ_EVENT_DISP_RDMA2_EOF] = 43,
> +       [CMDQ_EVENT_DISP_WDMA0_EOF] = 44,
> +       [CMDQ_EVENT_DISP_WDMA1_EOF] = 45,
> +       /* Mutex end of frame(EOF) events */
> +       [CMDQ_EVENT_MUTEX0_STREAM_EOF] = 53,
> +       [CMDQ_EVENT_MUTEX1_STREAM_EOF] = 54,
> +       [CMDQ_EVENT_MUTEX2_STREAM_EOF] = 55,
> +       [CMDQ_EVENT_MUTEX3_STREAM_EOF] = 56,
> +       [CMDQ_EVENT_MUTEX4_STREAM_EOF] = 57,
> +       /* Display underrun events */
> +       [CMDQ_EVENT_DISP_RDMA0_UNDERRUN] = 63,
> +       [CMDQ_EVENT_DISP_RDMA1_UNDERRUN] = 64,
> +       [CMDQ_EVENT_DISP_RDMA2_UNDERRUN] = 65,
> +};
> +
> +int cmdq_task_wfe(struct cmdq_task *task, enum cmdq_event 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_task_append_command(task, CMDQ_CODE_WFE,
> +                       cmdq_event_value[event], arg_b);
> +}
> +EXPORT_SYMBOL(cmdq_task_wfe);
> +
> +int cmdq_task_clear_event(struct cmdq_task *task, enum cmdq_event event)
> +{
> +       if (event >= CMDQ_MAX_EVENT || event < 0)
> +               return -EINVAL;
> +
> +       return cmdq_task_append_command(task, CMDQ_CODE_WFE,
> +                       cmdq_event_value[event], CMDQ_WFE_UPDATE);
> +}
> +EXPORT_SYMBOL(cmdq_task_clear_event);
> +
> +static int cmdq_task_finalize(struct cmdq_task *task)
> +{
> +       int err;
> +
> +       if (task->finalized)
> +               return 0;
> +
> +       /* insert EOC and generate IRQ for each command iteration */
> +       err = cmdq_task_append_command(task, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> +       if (err < 0)
> +               return err;
> +
> +       /* JUMP to end */
> +       err = cmdq_task_append_command(task, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> +       if (err < 0)
> +               return err;
> +
> +       task->finalized = true;
> +       return 0;
> +}
> +
> +int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task *task,
> +                         cmdq_async_flush_cb cb, void *data)
> +{
> +       struct cmdq *cmdq = task->cmdq;
> +       int err;
> +
> +       mutex_lock(&cmdq->task_mutex);
> +       if (cmdq->suspended) {
> +               dev_err(cmdq->mbox.dev, "%s is called after suspended\n",
> +                       __func__);
> +               mutex_unlock(&cmdq->task_mutex);
> +               return -EPERM;
> +       }
> +
> +       err = cmdq_task_finalize(task);
> +       if (err < 0) {
> +               mutex_unlock(&cmdq->task_mutex);
> +               return err;
> +       }
> +
> +       INIT_LIST_HEAD(&task->list_entry);
> +       task->cb.cb = cb;
> +       task->cb.data = data;
> +       task->pa_base = dma_map_single(cmdq->mbox.dev, task->va_base,
> +                                      task->cmd_buf_size, DMA_TO_DEVICE);
> +
> +       mbox_send_message(client->chan, task);
> +       /* We can send next task immediately, so just call txdone. */
> +       mbox_client_txdone(client->chan, 0);
> +       mutex_unlock(&cmdq->task_mutex);
> +       return 0;
> +}
> +EXPORT_SYMBOL(cmdq_task_flush_async);
> +
> +struct cmdq_flush_completion {
> +       struct completion cmplt;
> +       bool err;
> +};
> +
> +static void cmdq_task_flush_cb(struct cmdq_cb_data data)
> +{
> +       struct cmdq_flush_completion *cmplt = data.data;
> +
> +       cmplt->err = data.err;
> +       complete(&cmplt->cmplt);
> +}
> +
> +int cmdq_task_flush(struct cmdq_client *client, struct cmdq_task *task)
> +{
> +       struct cmdq_flush_completion cmplt;
> +       int err;
> +
> +       init_completion(&cmplt.cmplt);
> +       err = cmdq_task_flush_async(client, task, cmdq_task_flush_cb, &cmplt);
> +       if (err < 0)
> +               return err;
> +       wait_for_completion(&cmplt.cmplt);
> +       return cmplt.err ? -EFAULT : 0;
> +}
> +EXPORT_SYMBOL(cmdq_task_flush);
> +
> +void cmdq_mbox_free(struct cmdq_client *client)
> +{
> +       mbox_free_channel(client->chan);
> +       kfree(client);
> +}
> +EXPORT_SYMBOL(cmdq_mbox_free);
> +
All these exported functions implement the protocol, so should not be
a part of this controller driver. That should go into
drivers/soc/mediatek/

The controller driver (mtk-cmdq.c) should implement mainly the
mbox_chan_ops and mbox.of_xlate.


> +static int cmdq_suspend(struct device *dev)
> +{
> +       struct cmdq *cmdq = dev_get_drvdata(dev);
> +       struct cmdq_thread *thread;
> +       int i;
> +       bool task_running = false;
> +
> +       mutex_lock(&cmdq->task_mutex);
> +       cmdq->suspended = true;
> +       mutex_unlock(&cmdq->task_mutex);
> +
> +       for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> +               thread = &cmdq->thread[i];
> +               if (!list_empty(&thread->task_busy_list)) {
> +                       mod_timer(&thread->timeout, jiffies + 1);
> +                       task_running = true;
> +               }
> +       }
> +
> +       if (task_running) {
> +               dev_warn(dev, "exist running task(s) in suspend\n");
> +               msleep(20);
> +       }
> +
> +       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);
> +       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->atomic_exec = (sp->args[1] != 0);
> +       thread->chan = &mbox->chans[ind];
> +
> +       return &mbox->chans[ind];
> +}
> +
> +static int cmdq_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *node = dev->of_node;
> +       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 = irq_of_parse_and_map(node, 0);
>
why not,  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);
> +       }
> +
> +       mutex_init(&cmdq->task_mutex);
> +
> +       cmdq->mbox.dev = dev;
> +       cmdq->mbox.chans = devm_kcalloc(dev, CMDQ_THR_MAX_COUNT,
> +                                       sizeof(*cmdq->mbox.chans), GFP_KERNEL);
> +       if (!cmdq->mbox.chans)
> +               return -ENOMEM;
> +
> +       cmdq->mbox.num_chans = CMDQ_THR_MAX_COUNT;
> +       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;
> +
> +       for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> +               cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
> +                               CMDQ_THR_SIZE * i;
> +               INIT_LIST_HEAD(&cmdq->thread[i].task_busy_list);
> +               init_timer(&cmdq->thread[i].timeout);
> +               cmdq->thread[i].timeout.function = cmdq_thread_handle_timeout;
> +               cmdq->thread[i].timeout.data = (unsigned long)&cmdq->thread[i];
> +               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",
> +               .owner = THIS_MODULE,
>
please remove the unnecessary .owner field.

> +               .pm = &cmdq_pm_ops,
> +               .of_match_table = cmdq_of_ids,
> +       }
> +};
> +
> +builtin_platform_driver(cmdq_drv);
> diff --git a/include/linux/mailbox/mtk-cmdq.h b/include/linux/mailbox/mtk-cmdq.h
> new file mode 100644
> index 0000000..c3c924d
> --- /dev/null
> +++ b/include/linux/mailbox/mtk-cmdq.h
>
The api implemented is Mediateck proprietary, so I think it should be
include/linux/soc/mediatek/cmdq.h


> @@ -0,0 +1,180 @@
> +/*
> + * Copyright (c) 2015 MediaTek Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __MTK_CMDQ_H__
> +#define __MTK_CMDQ_H__
> +
> +#include <linux/mailbox_client.h>
> +#include <linux/mailbox_controller.h>
>
Clients should not need to include mailbox_controller.h

> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +/* display events in command queue(CMDQ) */
> +enum cmdq_event {
> +       /* Display start of frame(SOF) events */
> +       CMDQ_EVENT_DISP_OVL0_SOF,
>
you may want to explicitly initialise the first element.

> +       CMDQ_EVENT_DISP_OVL1_SOF,
> +       CMDQ_EVENT_DISP_RDMA0_SOF,
> +       CMDQ_EVENT_DISP_RDMA1_SOF,
> +       CMDQ_EVENT_DISP_RDMA2_SOF,
> +       CMDQ_EVENT_DISP_WDMA0_SOF,
> +       CMDQ_EVENT_DISP_WDMA1_SOF,
> +       /* Display end of frame(EOF) events */
> +       CMDQ_EVENT_DISP_OVL0_EOF,
> +       CMDQ_EVENT_DISP_OVL1_EOF,
> +       CMDQ_EVENT_DISP_RDMA0_EOF,
> +       CMDQ_EVENT_DISP_RDMA1_EOF,
> +       CMDQ_EVENT_DISP_RDMA2_EOF,
> +       CMDQ_EVENT_DISP_WDMA0_EOF,
> +       CMDQ_EVENT_DISP_WDMA1_EOF,
> +       /* Mutex end of frame(EOF) events */
> +       CMDQ_EVENT_MUTEX0_STREAM_EOF,
> +       CMDQ_EVENT_MUTEX1_STREAM_EOF,
> +       CMDQ_EVENT_MUTEX2_STREAM_EOF,
> +       CMDQ_EVENT_MUTEX3_STREAM_EOF,
> +       CMDQ_EVENT_MUTEX4_STREAM_EOF,
> +       /* Display underrun events */
> +       CMDQ_EVENT_DISP_RDMA0_UNDERRUN,
> +       CMDQ_EVENT_DISP_RDMA1_UNDERRUN,
> +       CMDQ_EVENT_DISP_RDMA2_UNDERRUN,
> +       /* Keep this at the end */
> +       CMDQ_MAX_EVENT,
> +};
> +

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

Hi Jassi,

Please see my inline reply.

On Thu, 2016-09-22 at 13:47 +0530, Jassi Brar wrote:
> On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <hs.liao@mediatek.com> wrote:
[...]
> > +struct cmdq_base *cmdq_register_device(struct device *dev)
> > +{
> > +       struct cmdq_base *cmdq_base;
> > +       struct resource res;
> > +       int subsys;
> > +       u32 base;
> > +
> > +       if (of_address_to_resource(dev->of_node, 0, &res))
> > +               return NULL;
> > +       base = (u32)res.start;
> > +
> > +       subsys = cmdq_subsys_base_to_id(base >> 16);
> > +       if (subsys < 0)
> > +               return NULL;
> > +
> > +       cmdq_base = devm_kmalloc(dev, sizeof(*cmdq_base), GFP_KERNEL);
> > +       if (!cmdq_base)
> > +               return NULL;
> > +       cmdq_base->subsys = subsys;
> > +       cmdq_base->base = base;
> > +
> > +       return cmdq_base;
> > +}
> > +EXPORT_SYMBOL(cmdq_register_device);
> > +
> > +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
> > +{
> > +       struct cmdq_client *client;
> > +
> > +       client = kzalloc(sizeof(*client), GFP_KERNEL);
> > +       client->client.dev = dev;
> > +       client->client.tx_block = false;
> > +       client->chan = mbox_request_channel(&client->client, index);
> > +       return client;
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_create);
> > +
> > +int cmdq_task_create(struct device *dev, struct cmdq_task **task_ptr)
> > +{
> > +       struct cmdq_task *task;
> > +       int err;
> > +
> > +       task = kzalloc(sizeof(*task), GFP_KERNEL);
> > +       if (!task)
> > +               return -ENOMEM;
> > +       task->cmdq = dev_get_drvdata(dev);
> > +       err = cmdq_task_realloc_cmd_buffer(task, PAGE_SIZE);
> > +       if (err < 0) {
> > +               kfree(task);
> > +               return err;
> > +       }
> > +       *task_ptr = task;
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_task_create);
> > +
> > +static int cmdq_task_append_command(struct cmdq_task *task, enum cmdq_code code,
> > +                                   u32 arg_a, u32 arg_b)
> > +{
> > +       u64 *cmd_ptr;
> > +       int err;
> > +
> > +       if (WARN_ON(task->finalized))
> > +               return -EBUSY;
> > +       if (unlikely(task->cmd_buf_size + CMDQ_INST_SIZE > task->buf_size)) {
> > +               err = cmdq_task_realloc_cmd_buffer(task, task->buf_size * 2);
> > +               if (err < 0)
> > +                       return err;
> > +       }
> > +       cmd_ptr = task->va_base + task->cmd_buf_size;
> > +       (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
> > +       task->cmd_buf_size += CMDQ_INST_SIZE;
> > +       return 0;
> > +}
> > +
> > +int cmdq_task_write(struct cmdq_task *task, u32 value, struct cmdq_base *base,
> > +                   u32 offset)
> > +{
> > +       u32 arg_a = ((base->base + offset) & CMDQ_ARG_A_WRITE_MASK) |
> > +                   (base->subsys << CMDQ_SUBSYS_SHIFT);
> > +       return cmdq_task_append_command(task, CMDQ_CODE_WRITE, arg_a, value);
> > +}
> > +EXPORT_SYMBOL(cmdq_task_write);
> > +
> > +int cmdq_task_write_mask(struct cmdq_task *task, u32 value,
> > +                        struct cmdq_base *base, u32 offset, u32 mask)
> > +{
> > +       u32 offset_mask = offset;
> > +       int err;
> > +
> > +       if (mask != 0xffffffff) {
> > +               err = cmdq_task_append_command(task, CMDQ_CODE_MASK, 0, ~mask);
> > +               if (err < 0)
> > +                       return err;
> > +               offset_mask |= CMDQ_WRITE_ENABLE_MASK;
> > +       }
> > +       return cmdq_task_write(task, value, base, offset_mask);
> > +}
> > +EXPORT_SYMBOL(cmdq_task_write_mask);
> > +
> > +static const u32 cmdq_event_value[CMDQ_MAX_EVENT] = {
> > +       /* Display start of frame(SOF) events */
> > +       [CMDQ_EVENT_DISP_OVL0_SOF] = 11,
> > +       [CMDQ_EVENT_DISP_OVL1_SOF] = 12,
> > +       [CMDQ_EVENT_DISP_RDMA0_SOF] = 13,
> > +       [CMDQ_EVENT_DISP_RDMA1_SOF] = 14,
> > +       [CMDQ_EVENT_DISP_RDMA2_SOF] = 15,
> > +       [CMDQ_EVENT_DISP_WDMA0_SOF] = 16,
> > +       [CMDQ_EVENT_DISP_WDMA1_SOF] = 17,
> > +       /* Display end of frame(EOF) events */
> > +       [CMDQ_EVENT_DISP_OVL0_EOF] = 39,
> > +       [CMDQ_EVENT_DISP_OVL1_EOF] = 40,
> > +       [CMDQ_EVENT_DISP_RDMA0_EOF] = 41,
> > +       [CMDQ_EVENT_DISP_RDMA1_EOF] = 42,
> > +       [CMDQ_EVENT_DISP_RDMA2_EOF] = 43,
> > +       [CMDQ_EVENT_DISP_WDMA0_EOF] = 44,
> > +       [CMDQ_EVENT_DISP_WDMA1_EOF] = 45,
> > +       /* Mutex end of frame(EOF) events */
> > +       [CMDQ_EVENT_MUTEX0_STREAM_EOF] = 53,
> > +       [CMDQ_EVENT_MUTEX1_STREAM_EOF] = 54,
> > +       [CMDQ_EVENT_MUTEX2_STREAM_EOF] = 55,
> > +       [CMDQ_EVENT_MUTEX3_STREAM_EOF] = 56,
> > +       [CMDQ_EVENT_MUTEX4_STREAM_EOF] = 57,
> > +       /* Display underrun events */
> > +       [CMDQ_EVENT_DISP_RDMA0_UNDERRUN] = 63,
> > +       [CMDQ_EVENT_DISP_RDMA1_UNDERRUN] = 64,
> > +       [CMDQ_EVENT_DISP_RDMA2_UNDERRUN] = 65,
> > +};
> > +
> > +int cmdq_task_wfe(struct cmdq_task *task, enum cmdq_event 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_task_append_command(task, CMDQ_CODE_WFE,
> > +                       cmdq_event_value[event], arg_b);
> > +}
> > +EXPORT_SYMBOL(cmdq_task_wfe);
> > +
> > +int cmdq_task_clear_event(struct cmdq_task *task, enum cmdq_event event)
> > +{
> > +       if (event >= CMDQ_MAX_EVENT || event < 0)
> > +               return -EINVAL;
> > +
> > +       return cmdq_task_append_command(task, CMDQ_CODE_WFE,
> > +                       cmdq_event_value[event], CMDQ_WFE_UPDATE);
> > +}
> > +EXPORT_SYMBOL(cmdq_task_clear_event);
> > +
> > +static int cmdq_task_finalize(struct cmdq_task *task)
> > +{
> > +       int err;
> > +
> > +       if (task->finalized)
> > +               return 0;
> > +
> > +       /* insert EOC and generate IRQ for each command iteration */
> > +       err = cmdq_task_append_command(task, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       /* JUMP to end */
> > +       err = cmdq_task_append_command(task, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       task->finalized = true;
> > +       return 0;
> > +}
> > +
> > +int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task *task,
> > +                         cmdq_async_flush_cb cb, void *data)
> > +{
> > +       struct cmdq *cmdq = task->cmdq;
> > +       int err;
> > +
> > +       mutex_lock(&cmdq->task_mutex);
> > +       if (cmdq->suspended) {
> > +               dev_err(cmdq->mbox.dev, "%s is called after suspended\n",
> > +                       __func__);
> > +               mutex_unlock(&cmdq->task_mutex);
> > +               return -EPERM;
> > +       }
> > +
> > +       err = cmdq_task_finalize(task);
> > +       if (err < 0) {
> > +               mutex_unlock(&cmdq->task_mutex);
> > +               return err;
> > +       }
> > +
> > +       INIT_LIST_HEAD(&task->list_entry);
> > +       task->cb.cb = cb;
> > +       task->cb.data = data;
> > +       task->pa_base = dma_map_single(cmdq->mbox.dev, task->va_base,
> > +                                      task->cmd_buf_size, DMA_TO_DEVICE);
> > +
> > +       mbox_send_message(client->chan, task);
> > +       /* We can send next task immediately, so just call txdone. */
> > +       mbox_client_txdone(client->chan, 0);
> > +       mutex_unlock(&cmdq->task_mutex);
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_task_flush_async);
> > +
> > +struct cmdq_flush_completion {
> > +       struct completion cmplt;
> > +       bool err;
> > +};
> > +
> > +static void cmdq_task_flush_cb(struct cmdq_cb_data data)
> > +{
> > +       struct cmdq_flush_completion *cmplt = data.data;
> > +
> > +       cmplt->err = data.err;
> > +       complete(&cmplt->cmplt);
> > +}
> > +
> > +int cmdq_task_flush(struct cmdq_client *client, struct cmdq_task *task)
> > +{
> > +       struct cmdq_flush_completion cmplt;
> > +       int err;
> > +
> > +       init_completion(&cmplt.cmplt);
> > +       err = cmdq_task_flush_async(client, task, cmdq_task_flush_cb, &cmplt);
> > +       if (err < 0)
> > +               return err;
> > +       wait_for_completion(&cmplt.cmplt);
> > +       return cmplt.err ? -EFAULT : 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_task_flush);
> > +
> > +void cmdq_mbox_free(struct cmdq_client *client)
> > +{
> > +       mbox_free_channel(client->chan);
> > +       kfree(client);
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_free);
> > +
> All these exported functions implement the protocol, so should not be
> a part of this controller driver. That should go into
> drivers/soc/mediatek/
> 
> The controller driver (mtk-cmdq.c) should implement mainly the
> mbox_chan_ops and mbox.of_xlate.
> 

I can do that, but I would like to confirm with Matthias in advance.

[...]
> > +       cmdq->irq = irq_of_parse_and_map(node, 0);
> >
> why not,  cmdq->irq = platform_get_irq(pdev, 0);

Will do

[...]
> > +static struct platform_driver cmdq_drv = {
> > +       .probe = cmdq_probe,
> > +       .remove = cmdq_remove,
> > +       .driver = {
> > +               .name = "mtk_cmdq",
> > +               .owner = THIS_MODULE,
> >
> please remove the unnecessary .owner field.

Will do

> > +               .pm = &cmdq_pm_ops,
> > +               .of_match_table = cmdq_of_ids,
> > +       }
> > +};
> > +
> > +builtin_platform_driver(cmdq_drv);
> > diff --git a/include/linux/mailbox/mtk-cmdq.h b/include/linux/mailbox/mtk-cmdq.h
> > new file mode 100644
> > index 0000000..c3c924d
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-cmdq.h
> >
> The api implemented is Mediateck proprietary, so I think it should be
> include/linux/soc/mediatek/cmdq.h
> 
> 
> > @@ -0,0 +1,180 @@
> > +/*
> > + * Copyright (c) 2015 MediaTek Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#ifndef __MTK_CMDQ_H__
> > +#define __MTK_CMDQ_H__
> > +
> > +#include <linux/mailbox_client.h>
> > +#include <linux/mailbox_controller.h>
> >
> Clients should not need to include mailbox_controller.h

This is because client needs to know controller's dev.

Please see my CMDQ v13.
http://www.spinics.net/lists/kernel/msg2327867.html
I add mailbox_controller.h for client to get controller's dev,
so I can remove a node reference in device tree.

Should I revert the modification of CMDQ v13?

> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
> > +
> > +/* display events in command queue(CMDQ) */
> > +enum cmdq_event {
> > +       /* Display start of frame(SOF) events */
> > +       CMDQ_EVENT_DISP_OVL0_SOF,
> >
> you may want to explicitly initialise the first element.

Will do

> > +       CMDQ_EVENT_DISP_OVL1_SOF,
> > +       CMDQ_EVENT_DISP_RDMA0_SOF,
> > +       CMDQ_EVENT_DISP_RDMA1_SOF,
> > +       CMDQ_EVENT_DISP_RDMA2_SOF,
> > +       CMDQ_EVENT_DISP_WDMA0_SOF,
> > +       CMDQ_EVENT_DISP_WDMA1_SOF,
> > +       /* Display end of frame(EOF) events */
> > +       CMDQ_EVENT_DISP_OVL0_EOF,
> > +       CMDQ_EVENT_DISP_OVL1_EOF,
> > +       CMDQ_EVENT_DISP_RDMA0_EOF,
> > +       CMDQ_EVENT_DISP_RDMA1_EOF,
> > +       CMDQ_EVENT_DISP_RDMA2_EOF,
> > +       CMDQ_EVENT_DISP_WDMA0_EOF,
> > +       CMDQ_EVENT_DISP_WDMA1_EOF,
> > +       /* Mutex end of frame(EOF) events */
> > +       CMDQ_EVENT_MUTEX0_STREAM_EOF,
> > +       CMDQ_EVENT_MUTEX1_STREAM_EOF,
> > +       CMDQ_EVENT_MUTEX2_STREAM_EOF,
> > +       CMDQ_EVENT_MUTEX3_STREAM_EOF,
> > +       CMDQ_EVENT_MUTEX4_STREAM_EOF,
> > +       /* Display underrun events */
> > +       CMDQ_EVENT_DISP_RDMA0_UNDERRUN,
> > +       CMDQ_EVENT_DISP_RDMA1_UNDERRUN,
> > +       CMDQ_EVENT_DISP_RDMA2_UNDERRUN,
> > +       /* Keep this at the end */
> > +       CMDQ_MAX_EVENT,
> > +};
> > +

Thanks,
HS


Hi Matthias,

Do you agree with Jassi's comments about moving parts of code back to
soc/mediatek/ ?
If I do it, the part in soc/mediatek/ will be similar to a library.
Could you tell me a good way to handle this situation?

Thanks,
HS

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

On Fri, 2016-09-23 at 17:28 +0800, Horng-Shyang Liao wrote:
> Hi Jassi,
> 
> Please see my inline reply.
> 
> On Thu, 2016-09-22 at 13:47 +0530, Jassi Brar wrote:
> > On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> [...]
> > > +struct cmdq_base *cmdq_register_device(struct device *dev)
> > > +{
> > > +       struct cmdq_base *cmdq_base;
> > > +       struct resource res;
> > > +       int subsys;
> > > +       u32 base;
> > > +
> > > +       if (of_address_to_resource(dev->of_node, 0, &res))
> > > +               return NULL;
> > > +       base = (u32)res.start;
> > > +
> > > +       subsys = cmdq_subsys_base_to_id(base >> 16);
> > > +       if (subsys < 0)
> > > +               return NULL;
> > > +
> > > +       cmdq_base = devm_kmalloc(dev, sizeof(*cmdq_base), GFP_KERNEL);
> > > +       if (!cmdq_base)
> > > +               return NULL;
> > > +       cmdq_base->subsys = subsys;
> > > +       cmdq_base->base = base;
> > > +
> > > +       return cmdq_base;
> > > +}
> > > +EXPORT_SYMBOL(cmdq_register_device);
> > > +
> > > +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
> > > +{
> > > +       struct cmdq_client *client;
> > > +
> > > +       client = kzalloc(sizeof(*client), GFP_KERNEL);
> > > +       client->client.dev = dev;
> > > +       client->client.tx_block = false;
> > > +       client->chan = mbox_request_channel(&client->client, index);
> > > +       return client;
> > > +}
> > > +EXPORT_SYMBOL(cmdq_mbox_create);
> > > +
> > > +int cmdq_task_create(struct device *dev, struct cmdq_task **task_ptr)
> > > +{
> > > +       struct cmdq_task *task;
> > > +       int err;
> > > +
> > > +       task = kzalloc(sizeof(*task), GFP_KERNEL);
> > > +       if (!task)
> > > +               return -ENOMEM;
> > > +       task->cmdq = dev_get_drvdata(dev);
> > > +       err = cmdq_task_realloc_cmd_buffer(task, PAGE_SIZE);
> > > +       if (err < 0) {
> > > +               kfree(task);
> > > +               return err;
> > > +       }
> > > +       *task_ptr = task;
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL(cmdq_task_create);
> > > +
> > > +static int cmdq_task_append_command(struct cmdq_task *task, enum cmdq_code code,
> > > +                                   u32 arg_a, u32 arg_b)
> > > +{
> > > +       u64 *cmd_ptr;
> > > +       int err;
> > > +
> > > +       if (WARN_ON(task->finalized))
> > > +               return -EBUSY;
> > > +       if (unlikely(task->cmd_buf_size + CMDQ_INST_SIZE > task->buf_size)) {
> > > +               err = cmdq_task_realloc_cmd_buffer(task, task->buf_size * 2);
> > > +               if (err < 0)
> > > +                       return err;
> > > +       }
> > > +       cmd_ptr = task->va_base + task->cmd_buf_size;
> > > +       (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
> > > +       task->cmd_buf_size += CMDQ_INST_SIZE;
> > > +       return 0;
> > > +}
> > > +
> > > +int cmdq_task_write(struct cmdq_task *task, u32 value, struct cmdq_base *base,
> > > +                   u32 offset)
> > > +{
> > > +       u32 arg_a = ((base->base + offset) & CMDQ_ARG_A_WRITE_MASK) |
> > > +                   (base->subsys << CMDQ_SUBSYS_SHIFT);
> > > +       return cmdq_task_append_command(task, CMDQ_CODE_WRITE, arg_a, value);
> > > +}
> > > +EXPORT_SYMBOL(cmdq_task_write);
> > > +
> > > +int cmdq_task_write_mask(struct cmdq_task *task, u32 value,
> > > +                        struct cmdq_base *base, u32 offset, u32 mask)
> > > +{
> > > +       u32 offset_mask = offset;
> > > +       int err;
> > > +
> > > +       if (mask != 0xffffffff) {
> > > +               err = cmdq_task_append_command(task, CMDQ_CODE_MASK, 0, ~mask);
> > > +               if (err < 0)
> > > +                       return err;
> > > +               offset_mask |= CMDQ_WRITE_ENABLE_MASK;
> > > +       }
> > > +       return cmdq_task_write(task, value, base, offset_mask);
> > > +}
> > > +EXPORT_SYMBOL(cmdq_task_write_mask);
> > > +
> > > +static const u32 cmdq_event_value[CMDQ_MAX_EVENT] = {
> > > +       /* Display start of frame(SOF) events */
> > > +       [CMDQ_EVENT_DISP_OVL0_SOF] = 11,
> > > +       [CMDQ_EVENT_DISP_OVL1_SOF] = 12,
> > > +       [CMDQ_EVENT_DISP_RDMA0_SOF] = 13,
> > > +       [CMDQ_EVENT_DISP_RDMA1_SOF] = 14,
> > > +       [CMDQ_EVENT_DISP_RDMA2_SOF] = 15,
> > > +       [CMDQ_EVENT_DISP_WDMA0_SOF] = 16,
> > > +       [CMDQ_EVENT_DISP_WDMA1_SOF] = 17,
> > > +       /* Display end of frame(EOF) events */
> > > +       [CMDQ_EVENT_DISP_OVL0_EOF] = 39,
> > > +       [CMDQ_EVENT_DISP_OVL1_EOF] = 40,
> > > +       [CMDQ_EVENT_DISP_RDMA0_EOF] = 41,
> > > +       [CMDQ_EVENT_DISP_RDMA1_EOF] = 42,
> > > +       [CMDQ_EVENT_DISP_RDMA2_EOF] = 43,
> > > +       [CMDQ_EVENT_DISP_WDMA0_EOF] = 44,
> > > +       [CMDQ_EVENT_DISP_WDMA1_EOF] = 45,
> > > +       /* Mutex end of frame(EOF) events */
> > > +       [CMDQ_EVENT_MUTEX0_STREAM_EOF] = 53,
> > > +       [CMDQ_EVENT_MUTEX1_STREAM_EOF] = 54,
> > > +       [CMDQ_EVENT_MUTEX2_STREAM_EOF] = 55,
> > > +       [CMDQ_EVENT_MUTEX3_STREAM_EOF] = 56,
> > > +       [CMDQ_EVENT_MUTEX4_STREAM_EOF] = 57,
> > > +       /* Display underrun events */
> > > +       [CMDQ_EVENT_DISP_RDMA0_UNDERRUN] = 63,
> > > +       [CMDQ_EVENT_DISP_RDMA1_UNDERRUN] = 64,
> > > +       [CMDQ_EVENT_DISP_RDMA2_UNDERRUN] = 65,
> > > +};
> > > +
> > > +int cmdq_task_wfe(struct cmdq_task *task, enum cmdq_event 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_task_append_command(task, CMDQ_CODE_WFE,
> > > +                       cmdq_event_value[event], arg_b);
> > > +}
> > > +EXPORT_SYMBOL(cmdq_task_wfe);
> > > +
> > > +int cmdq_task_clear_event(struct cmdq_task *task, enum cmdq_event event)
> > > +{
> > > +       if (event >= CMDQ_MAX_EVENT || event < 0)
> > > +               return -EINVAL;
> > > +
> > > +       return cmdq_task_append_command(task, CMDQ_CODE_WFE,
> > > +                       cmdq_event_value[event], CMDQ_WFE_UPDATE);
> > > +}
> > > +EXPORT_SYMBOL(cmdq_task_clear_event);
> > > +
> > > +static int cmdq_task_finalize(struct cmdq_task *task)
> > > +{
> > > +       int err;
> > > +
> > > +       if (task->finalized)
> > > +               return 0;
> > > +
> > > +       /* insert EOC and generate IRQ for each command iteration */
> > > +       err = cmdq_task_append_command(task, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > > +       if (err < 0)
> > > +               return err;
> > > +
> > > +       /* JUMP to end */
> > > +       err = cmdq_task_append_command(task, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > > +       if (err < 0)
> > > +               return err;
> > > +
> > > +       task->finalized = true;
> > > +       return 0;
> > > +}
> > > +
> > > +int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task *task,
> > > +                         cmdq_async_flush_cb cb, void *data)
> > > +{
> > > +       struct cmdq *cmdq = task->cmdq;
> > > +       int err;
> > > +
> > > +       mutex_lock(&cmdq->task_mutex);
> > > +       if (cmdq->suspended) {
> > > +               dev_err(cmdq->mbox.dev, "%s is called after suspended\n",
> > > +                       __func__);
> > > +               mutex_unlock(&cmdq->task_mutex);
> > > +               return -EPERM;
> > > +       }
> > > +
> > > +       err = cmdq_task_finalize(task);
> > > +       if (err < 0) {
> > > +               mutex_unlock(&cmdq->task_mutex);
> > > +               return err;
> > > +       }
> > > +
> > > +       INIT_LIST_HEAD(&task->list_entry);
> > > +       task->cb.cb = cb;
> > > +       task->cb.data = data;
> > > +       task->pa_base = dma_map_single(cmdq->mbox.dev, task->va_base,
> > > +                                      task->cmd_buf_size, DMA_TO_DEVICE);
> > > +
> > > +       mbox_send_message(client->chan, task);
> > > +       /* We can send next task immediately, so just call txdone. */
> > > +       mbox_client_txdone(client->chan, 0);
> > > +       mutex_unlock(&cmdq->task_mutex);
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL(cmdq_task_flush_async);
> > > +
> > > +struct cmdq_flush_completion {
> > > +       struct completion cmplt;
> > > +       bool err;
> > > +};
> > > +
> > > +static void cmdq_task_flush_cb(struct cmdq_cb_data data)
> > > +{
> > > +       struct cmdq_flush_completion *cmplt = data.data;
> > > +
> > > +       cmplt->err = data.err;
> > > +       complete(&cmplt->cmplt);
> > > +}
> > > +
> > > +int cmdq_task_flush(struct cmdq_client *client, struct cmdq_task *task)
> > > +{
> > > +       struct cmdq_flush_completion cmplt;
> > > +       int err;
> > > +
> > > +       init_completion(&cmplt.cmplt);
> > > +       err = cmdq_task_flush_async(client, task, cmdq_task_flush_cb, &cmplt);
> > > +       if (err < 0)
> > > +               return err;
> > > +       wait_for_completion(&cmplt.cmplt);
> > > +       return cmplt.err ? -EFAULT : 0;
> > > +}
> > > +EXPORT_SYMBOL(cmdq_task_flush);
> > > +
> > > +void cmdq_mbox_free(struct cmdq_client *client)
> > > +{
> > > +       mbox_free_channel(client->chan);
> > > +       kfree(client);
> > > +}
> > > +EXPORT_SYMBOL(cmdq_mbox_free);
> > > +
> > All these exported functions implement the protocol, so should not be
> > a part of this controller driver. That should go into
> > drivers/soc/mediatek/
> > 
> > The controller driver (mtk-cmdq.c) should implement mainly the
> > mbox_chan_ops and mbox.of_xlate.
> > 
> 
> I can do that, but I would like to confirm with Matthias in advance.
> 
> [...]
> > > +       cmdq->irq = irq_of_parse_and_map(node, 0);
> > >
> > why not,  cmdq->irq = platform_get_irq(pdev, 0);
> 
> Will do
> 
> [...]
> > > +static struct platform_driver cmdq_drv = {
> > > +       .probe = cmdq_probe,
> > > +       .remove = cmdq_remove,
> > > +       .driver = {
> > > +               .name = "mtk_cmdq",
> > > +               .owner = THIS_MODULE,
> > >
> > please remove the unnecessary .owner field.
> 
> Will do
> 
> > > +               .pm = &cmdq_pm_ops,
> > > +               .of_match_table = cmdq_of_ids,
> > > +       }
> > > +};
> > > +
> > > +builtin_platform_driver(cmdq_drv);
> > > diff --git a/include/linux/mailbox/mtk-cmdq.h b/include/linux/mailbox/mtk-cmdq.h
> > > new file mode 100644
> > > index 0000000..c3c924d
> > > --- /dev/null
> > > +++ b/include/linux/mailbox/mtk-cmdq.h
> > >
> > The api implemented is Mediateck proprietary, so I think it should be
> > include/linux/soc/mediatek/cmdq.h
> > 
> > 
> > > @@ -0,0 +1,180 @@
> > > +/*
> > > + * Copyright (c) 2015 MediaTek Inc.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#ifndef __MTK_CMDQ_H__
> > > +#define __MTK_CMDQ_H__
> > > +
> > > +#include <linux/mailbox_client.h>
> > > +#include <linux/mailbox_controller.h>
> > >
> > Clients should not need to include mailbox_controller.h
> 
> This is because client needs to know controller's dev.
> 
> Please see my CMDQ v13.
> http://www.spinics.net/lists/kernel/msg2327867.html
> I add mailbox_controller.h for client to get controller's dev,
> so I can remove a node reference in device tree.
> 
> Should I revert the modification of CMDQ v13?


Hi Jassi,

CMDQ clients don't need to know controller device before flush,
and CMDQ driver can get controller device by itself in flushing flow.
So, I think mailbox_controller.h can be removed from here,
and CMDQ v13 doesn't need to be reverted, either.
I will update this part in CMDQ v15.

Thanks,
HS

> > > +#include <linux/platform_device.h>
> > > +#include <linux/types.h>
> > > +
> > > +/* display events in command queue(CMDQ) */
> > > +enum cmdq_event {
> > > +       /* Display start of frame(SOF) events */
> > > +       CMDQ_EVENT_DISP_OVL0_SOF,
> > >
> > you may want to explicitly initialise the first element.
> 
> Will do
> 
> > > +       CMDQ_EVENT_DISP_OVL1_SOF,
> > > +       CMDQ_EVENT_DISP_RDMA0_SOF,
> > > +       CMDQ_EVENT_DISP_RDMA1_SOF,
> > > +       CMDQ_EVENT_DISP_RDMA2_SOF,
> > > +       CMDQ_EVENT_DISP_WDMA0_SOF,
> > > +       CMDQ_EVENT_DISP_WDMA1_SOF,
> > > +       /* Display end of frame(EOF) events */
> > > +       CMDQ_EVENT_DISP_OVL0_EOF,
> > > +       CMDQ_EVENT_DISP_OVL1_EOF,
> > > +       CMDQ_EVENT_DISP_RDMA0_EOF,
> > > +       CMDQ_EVENT_DISP_RDMA1_EOF,
> > > +       CMDQ_EVENT_DISP_RDMA2_EOF,
> > > +       CMDQ_EVENT_DISP_WDMA0_EOF,
> > > +       CMDQ_EVENT_DISP_WDMA1_EOF,
> > > +       /* Mutex end of frame(EOF) events */
> > > +       CMDQ_EVENT_MUTEX0_STREAM_EOF,
> > > +       CMDQ_EVENT_MUTEX1_STREAM_EOF,
> > > +       CMDQ_EVENT_MUTEX2_STREAM_EOF,
> > > +       CMDQ_EVENT_MUTEX3_STREAM_EOF,
> > > +       CMDQ_EVENT_MUTEX4_STREAM_EOF,
> > > +       /* Display underrun events */
> > > +       CMDQ_EVENT_DISP_RDMA0_UNDERRUN,
> > > +       CMDQ_EVENT_DISP_RDMA1_UNDERRUN,
> > > +       CMDQ_EVENT_DISP_RDMA2_UNDERRUN,
> > > +       /* Keep this at the end */
> > > +       CMDQ_MAX_EVENT,
> > > +};
> > > +
> 
> Thanks,
> HS
> 
> 
> Hi Matthias,
> 
> Do you agree with Jassi's comments about moving parts of code back to
> soc/mediatek/ ?
> If I do it, the part in soc/mediatek/ will be similar to a library.
> Could you tell me a good way to handle this situation?
> 
> Thanks,
> HS


Hi Matthias,

Do you have any suggestion about moving parts of code back to
soc/mediatek/ ?

Thanks,
HS


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[CK Hu]

Hi, HS:

On Mon, 2016-09-05 at 09:44 +0800, HS Liao 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: HS Liao <hs.liao@mediatek.com>
> Signed-off-by: CK Hu <ck.hu@mediatek.com>
> ---

[snip...]

> +
> +struct cmdq_task {
> +	struct cmdq		*cmdq;
> +	struct list_head	list_entry;
> +	void			*va_base;
> +	dma_addr_t		pa_base;
> +	size_t			cmd_buf_size; /* command occupied size */
> +	size_t			buf_size; /* real buffer size */
> +	bool			finalized;
> +	struct cmdq_thread	*thread;

I think thread info could be removed from cmdq_task. Only
cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
task->thread and caller of both function has the thread info. So you
could just pass thread info into these two function and remove thread
info in cmdq_task.

> +	struct cmdq_task_cb	cb;

I think this callback function is equal to mailbox client tx_done
callback. It's better to use already-defined interface rather than
creating your own.

> +};
> +

[snip...]

> +
> +static int cmdq_suspend(struct device *dev)
> +{
> +	struct cmdq *cmdq = dev_get_drvdata(dev);
> +	struct cmdq_thread *thread;
> +	int i;
> +	bool task_running = false;
> +
> +	mutex_lock(&cmdq->task_mutex);
> +	cmdq->suspended = true;
> +	mutex_unlock(&cmdq->task_mutex);
> +
> +	for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> +		thread = &cmdq->thread[i];
> +		if (!list_empty(&thread->task_busy_list)) {
> +			mod_timer(&thread->timeout, jiffies + 1);
> +			task_running = true;
> +		}
> +	}
> +
> +	if (task_running) {
> +		dev_warn(dev, "exist running task(s) in suspend\n");
> +		msleep(20);

Why sleep here? It looks like a recovery but could 20ms recovery
something? I think warning message is enough because you see the warning
message, and you fix the bug, so no need to recovery anything.

> +	}
> +
> +	clk_unprepare(cmdq->clock);
> +	return 0;
> +}
> +

Regards,
CK

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

Hi CK,

Please see my inline reply.

On Fri, 2016-09-30 at 11:06 +0800, CK Hu wrote:
> Hi, HS:
> 
> On Mon, 2016-09-05 at 09:44 +0800, HS Liao 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: HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: CK Hu <ck.hu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> 
> [snip...]
> 
> > +
> > +struct cmdq_task {
> > +	struct cmdq		*cmdq;
> > +	struct list_head	list_entry;
> > +	void			*va_base;
> > +	dma_addr_t		pa_base;
> > +	size_t			cmd_buf_size; /* command occupied size */
> > +	size_t			buf_size; /* real buffer size */
> > +	bool			finalized;
> > +	struct cmdq_thread	*thread;
> 
> I think thread info could be removed from cmdq_task. Only
> cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
> task->thread and caller of both function has the thread info. So you
> could just pass thread info into these two function and remove thread
> info in cmdq_task.

This modification will remove 1 pointer but add 2 pointers. Moreover,
more pointers will need to be delivered between functions for future
extension. IMHO, it would be better to keep thread pointer inside
cmdq_task.

> > +	struct cmdq_task_cb	cb;
> 
> I think this callback function is equal to mailbox client tx_done
> callback. It's better to use already-defined interface rather than
> creating your own.

This is because CMDQ driver allows different callback functions for
different tasks, but mailbox only allows one callback function per
channel. But, I think I can add a wrapper for tx_done to call CMDQ
callback functions. So, I will use tx_done in CMDQ v15.

> > +};
> > +
> 
> [snip...]
> 
> > +
> > +static int cmdq_suspend(struct device *dev)
> > +{
> > +	struct cmdq *cmdq = dev_get_drvdata(dev);
> > +	struct cmdq_thread *thread;
> > +	int i;
> > +	bool task_running = false;
> > +
> > +	mutex_lock(&cmdq->task_mutex);
> > +	cmdq->suspended = true;
> > +	mutex_unlock(&cmdq->task_mutex);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> > +		thread = &cmdq->thread[i];
> > +		if (!list_empty(&thread->task_busy_list)) {
> > +			mod_timer(&thread->timeout, jiffies + 1);
> > +			task_running = true;
> > +		}
> > +	}
> > +
> > +	if (task_running) {
> > +		dev_warn(dev, "exist running task(s) in suspend\n");
> > +		msleep(20);
> 
> Why sleep here? It looks like a recovery but could 20ms recovery
> something? I think warning message is enough because you see the warning
> message, and you fix the bug, so no need to recovery anything.

My purpose is context switch to finish timer's work.
I will replace it by schedule().

> > +	}
> > +
> > +	clk_unprepare(cmdq->clock);
> > +	return 0;
> > +}
> > +
> 
> Regards,
> CK

Thanks,
HS

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[CK Hu]

Hi, HS:

One comment inline

On Fri, 2016-09-30 at 16:56 +0800, Horng-Shyang Liao wrote:
> Hi CK,
> 
> Please see my inline reply.
> 
> On Fri, 2016-09-30 at 11:06 +0800, CK Hu wrote:
> > Hi, HS:
> > 
> > On Mon, 2016-09-05 at 09:44 +0800, HS Liao 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: HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > Signed-off-by: CK Hu <ck.hu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > ---
> > 
> > [snip...]
> > 
> > > +
> > > +struct cmdq_task {
> > > +	struct cmdq		*cmdq;
> > > +	struct list_head	list_entry;
> > > +	void			*va_base;
> > > +	dma_addr_t		pa_base;
> > > +	size_t			cmd_buf_size; /* command occupied size */
> > > +	size_t			buf_size; /* real buffer size */
> > > +	bool			finalized;
> > > +	struct cmdq_thread	*thread;
> > 
> > I think thread info could be removed from cmdq_task. Only
> > cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
> > task->thread and caller of both function has the thread info. So you
> > could just pass thread info into these two function and remove thread
> > info in cmdq_task.
> 
> This modification will remove 1 pointer but add 2 pointers. Moreover,
> more pointers will need to be delivered between functions for future
> extension. IMHO, it would be better to keep thread pointer inside
> cmdq_task.
> 
> > > +	struct cmdq_task_cb	cb;
> > 
> > I think this callback function is equal to mailbox client tx_done
> > callback. It's better to use already-defined interface rather than
> > creating your own.
> 
> This is because CMDQ driver allows different callback functions for
> different tasks, but mailbox only allows one callback function per
> channel. But, I think I can add a wrapper for tx_done to call CMDQ
> callback functions. So, I will use tx_done in CMDQ v15.

Up to now, one callback function for one channel is enough for DRM. So
'different callback function for different sent-message' looks like an
advanced function. Maybe you should not include it in first patch. 

Regards,
CK

> 
> > > +};
> > > +
> > 
> > [snip...]
> > 
> > > +
> > > +static int cmdq_suspend(struct device *dev)
> > > +{
> > > +	struct cmdq *cmdq = dev_get_drvdata(dev);
> > > +	struct cmdq_thread *thread;
> > > +	int i;
> > > +	bool task_running = false;
> > > +
> > > +	mutex_lock(&cmdq->task_mutex);
> > > +	cmdq->suspended = true;
> > > +	mutex_unlock(&cmdq->task_mutex);
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> > > +		thread = &cmdq->thread[i];
> > > +		if (!list_empty(&thread->task_busy_list)) {
> > > +			mod_timer(&thread->timeout, jiffies + 1);
> > > +			task_running = true;
> > > +		}
> > > +	}
> > > +
> > > +	if (task_running) {
> > > +		dev_warn(dev, "exist running task(s) in suspend\n");
> > > +		msleep(20);
> > 
> > Why sleep here? It looks like a recovery but could 20ms recovery
> > something? I think warning message is enough because you see the warning
> > message, and you fix the bug, so no need to recovery anything.
> 
> My purpose is context switch to finish timer's work.
> I will replace it by schedule().
> 
> > > +	}
> > > +
> > > +	clk_unprepare(cmdq->clock);
> > > +	return 0;
> > > +}
> > > +
> > 
> > Regards,
> > CK
> 
> Thanks,
> HS
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

On Fri, 2016-09-30 at 17:11 +0800, CK Hu wrote:
> Hi, HS:
> 
> One comment inline
> 
> On Fri, 2016-09-30 at 16:56 +0800, Horng-Shyang Liao wrote:
> > Hi CK,
> > 
> > Please see my inline reply.
> > 
> > On Fri, 2016-09-30 at 11:06 +0800, CK Hu wrote:
> > > Hi, HS:
> > > 
> > > On Mon, 2016-09-05 at 09:44 +0800, HS Liao 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: HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > > Signed-off-by: CK Hu <ck.hu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > > ---
> > > 
> > > [snip...]
> > > 
> > > > +
> > > > +struct cmdq_task {
> > > > +	struct cmdq		*cmdq;
> > > > +	struct list_head	list_entry;
> > > > +	void			*va_base;
> > > > +	dma_addr_t		pa_base;
> > > > +	size_t			cmd_buf_size; /* command occupied size */
> > > > +	size_t			buf_size; /* real buffer size */
> > > > +	bool			finalized;
> > > > +	struct cmdq_thread	*thread;
> > > 
> > > I think thread info could be removed from cmdq_task. Only
> > > cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
> > > task->thread and caller of both function has the thread info. So you
> > > could just pass thread info into these two function and remove thread
> > > info in cmdq_task.
> > 
> > This modification will remove 1 pointer but add 2 pointers. Moreover,
> > more pointers will need to be delivered between functions for future
> > extension. IMHO, it would be better to keep thread pointer inside
> > cmdq_task.
> > 
> > > > +	struct cmdq_task_cb	cb;
> > > 
> > > I think this callback function is equal to mailbox client tx_done
> > > callback. It's better to use already-defined interface rather than
> > > creating your own.
> > 
> > This is because CMDQ driver allows different callback functions for
> > different tasks, but mailbox only allows one callback function per
> > channel. But, I think I can add a wrapper for tx_done to call CMDQ
> > callback functions. So, I will use tx_done in CMDQ v15.
> 
> Up to now, one callback function for one channel is enough for DRM. So
> 'different callback function for different sent-message' looks like an
> advanced function. Maybe you should not include it in first patch. 
> 
> Regards,
> CK

Hi CK,

OK. I will do it.

Thanks,
HS

[snip...]


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

On Fri, 2016-09-30 at 17:47 +0800, Horng-Shyang Liao wrote:
> On Fri, 2016-09-30 at 17:11 +0800, CK Hu wrote:
> > Hi, HS:
> > 
> > One comment inline
> > 
> > On Fri, 2016-09-30 at 16:56 +0800, Horng-Shyang Liao wrote:
> > > Hi CK,
> > > 
> > > Please see my inline reply.
> > > 
> > > On Fri, 2016-09-30 at 11:06 +0800, CK Hu wrote:
> > > > Hi, HS:
> > > > 
> > > > On Mon, 2016-09-05 at 09:44 +0800, HS Liao 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: HS Liao <hs.liao@mediatek.com>
> > > > > Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > > > > ---
> > > > 
> > > > [snip...]
> > > > 
> > > > > +
> > > > > +struct cmdq_task {
> > > > > +	struct cmdq		*cmdq;
> > > > > +	struct list_head	list_entry;
> > > > > +	void			*va_base;
> > > > > +	dma_addr_t		pa_base;
> > > > > +	size_t			cmd_buf_size; /* command occupied size */
> > > > > +	size_t			buf_size; /* real buffer size */
> > > > > +	bool			finalized;
> > > > > +	struct cmdq_thread	*thread;
> > > > 
> > > > I think thread info could be removed from cmdq_task. Only
> > > > cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
> > > > task->thread and caller of both function has the thread info. So you
> > > > could just pass thread info into these two function and remove thread
> > > > info in cmdq_task.
> > > 
> > > This modification will remove 1 pointer but add 2 pointers. Moreover,
> > > more pointers will need to be delivered between functions for future
> > > extension. IMHO, it would be better to keep thread pointer inside
> > > cmdq_task.
> > > 
> > > > > +	struct cmdq_task_cb	cb;
> > > > 
> > > > I think this callback function is equal to mailbox client tx_done
> > > > callback. It's better to use already-defined interface rather than
> > > > creating your own.
> > > 
> > > This is because CMDQ driver allows different callback functions for
> > > different tasks, but mailbox only allows one callback function per
> > > channel. But, I think I can add a wrapper for tx_done to call CMDQ
> > > callback functions. So, I will use tx_done in CMDQ v15.
> > 
> > Up to now, one callback function for one channel is enough for DRM. So
> > 'different callback function for different sent-message' looks like an
> > advanced function. Maybe you should not include it in first patch. 
> > 
> > Regards,
> > CK
> 
> Hi CK,
> 
> OK. I will do it.
> 
> Thanks,
> HS
> 
> [snip...]


Hi CK,

After I trace mailbox driver, I realize that CMDQ driver cannot use
tx_done.

CMDQ clients will flush many tasks into CMDQ driver, and then CMDQ
driver will apply these tasks into GCE HW "immediately". These tasks,
which are queued in GCE HW, may not execute immediately since they
may need to wait event(s), e.g. vsync.

However, in mailbox driver, mailbox uses a software buffer to queue
sent messages. It only sends next message until previous message is
done. This cannot fulfill CMDQ's requirement.

Quote some code from mailbox driver. Please notice "active_req" part.

static void msg_submit(struct mbox_chan *chan)
{
	...
	if (!chan->msg_count || chan->active_req)
		goto exit;
	...
	err = chan->mbox->ops->send_data(chan, data);
	if (!err) {
		chan->active_req = data;
		chan->msg_count--;
	}
	...
}

static void tx_tick(struct mbox_chan *chan, int r)
{
	...
	spin_lock_irqsave(&chan->lock, flags);
	mssg = chan->active_req;
	chan->active_req = NULL;
	spin_unlock_irqrestore(&chan->lock, flags);
	...
}

Current workable CMDQ driver uses mbox_client_txdone() to prevent
this issue, and then uses self callback functions to handle done tasks.

int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task
*task, cmdq_async_flush_cb cb, void *data)
{
	...
	mbox_send_message(client->chan, task);
	/* We can send next task immediately, so just call txdone. */
	mbox_client_txdone(client->chan, 0);
	...
}

Another solution is to use rx_callback; i.e. CMDQ mailbox controller
call mbox_chan_received_data() when CMDQ task is done. But, this may
violate the design of mailbox. What do you think?

Thanks,
HS


Hi Jassi,

Do you have any suggestion about previous situation?

Thanks,
HS

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Jassi Brar]

On 5 October 2016 at 08:24, Horng-Shyang Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> On Fri, 2016-09-30 at 17:47 +0800, Horng-Shyang Liao wrote:
>> On Fri, 2016-09-30 at 17:11 +0800, CK Hu wrote:

>
> After I trace mailbox driver, I realize that CMDQ driver cannot use
> tx_done.
>
> CMDQ clients will flush many tasks into CMDQ driver, and then CMDQ
> driver will apply these tasks into GCE HW "immediately". These tasks,
> which are queued in GCE HW, may not execute immediately since they
> may need to wait event(s), e.g. vsync.
>
> However, in mailbox driver, mailbox uses a software buffer to queue
> sent messages. It only sends next message until previous message is
> done. This cannot fulfill CMDQ's requirement.
>
I understand
 a) GCE HW can internally queue many tasks in some 'FIFO'
 b) Execution of some task may have to wait until some external event
occurs (like vsync)
 c) GCE does not generate irq/flag for each task executed (?)

If so, may be your tx_done should return 'true' so long as the GCE HW
can accept tasks in its 'FIFO'. For mailbox api, any task that is
queued on GCE, is assumed to be transmitted.

> Quote some code from mailbox driver. Please notice "active_req" part.
>
> static void msg_submit(struct mbox_chan *chan)
> {
>         ...
>         if (!chan->msg_count || chan->active_req)
>                 goto exit;
>         ...
>         err = chan->mbox->ops->send_data(chan, data);
>         if (!err) {
>                 chan->active_req = data;
>                 chan->msg_count--;
>         }
>         ...
> }
>
> static void tx_tick(struct mbox_chan *chan, int r)
> {
>         ...
>         spin_lock_irqsave(&chan->lock, flags);
>         mssg = chan->active_req;
>         chan->active_req = NULL;
>         spin_unlock_irqrestore(&chan->lock, flags);
>         ...
> }
>
> Current workable CMDQ driver uses mbox_client_txdone() to prevent
> this issue, and then uses self callback functions to handle done tasks.
>
> int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task
> *task, cmdq_async_flush_cb cb, void *data)
> {
>         ...
>         mbox_send_message(client->chan, task);
>         /* We can send next task immediately, so just call txdone. */
>         mbox_client_txdone(client->chan, 0);
>         ...
> }
>
> Another solution is to use rx_callback; i.e. CMDQ mailbox controller
> call mbox_chan_received_data() when CMDQ task is done. But, this may
> violate the design of mailbox. What do you think?
>
If my point (c) above does not hold, maybe look at implementing
tx_done() callback and submit next task from the callback of last
done.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

On Wed, 2016-10-05 at 09:07 +0530, Jassi Brar wrote:
> On 5 October 2016 at 08:24, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > On Fri, 2016-09-30 at 17:47 +0800, Horng-Shyang Liao wrote:
> >> On Fri, 2016-09-30 at 17:11 +0800, CK Hu wrote:
> 
> >
> > After I trace mailbox driver, I realize that CMDQ driver cannot use
> > tx_done.
> >
> > CMDQ clients will flush many tasks into CMDQ driver, and then CMDQ
> > driver will apply these tasks into GCE HW "immediately". These tasks,
> > which are queued in GCE HW, may not execute immediately since they
> > may need to wait event(s), e.g. vsync.
> >
> > However, in mailbox driver, mailbox uses a software buffer to queue
> > sent messages. It only sends next message until previous message is
> > done. This cannot fulfill CMDQ's requirement.
> >
> I understand
>  a) GCE HW can internally queue many tasks in some 'FIFO'
>  b) Execution of some task may have to wait until some external event
> occurs (like vsync)
>  c) GCE does not generate irq/flag for each task executed (?)
> 
> If so, may be your tx_done should return 'true' so long as the GCE HW
> can accept tasks in its 'FIFO'. For mailbox api, any task that is
> queued on GCE, is assumed to be transmitted.
> 
> > Quote some code from mailbox driver. Please notice "active_req" part.
> >
> > static void msg_submit(struct mbox_chan *chan)
> > {
> >         ...
> >         if (!chan->msg_count || chan->active_req)
> >                 goto exit;
> >         ...
> >         err = chan->mbox->ops->send_data(chan, data);
> >         if (!err) {
> >                 chan->active_req = data;
> >                 chan->msg_count--;
> >         }
> >         ...
> > }
> >
> > static void tx_tick(struct mbox_chan *chan, int r)
> > {
> >         ...
> >         spin_lock_irqsave(&chan->lock, flags);
> >         mssg = chan->active_req;
> >         chan->active_req = NULL;
> >         spin_unlock_irqrestore(&chan->lock, flags);
> >         ...
> > }
> >
> > Current workable CMDQ driver uses mbox_client_txdone() to prevent
> > this issue, and then uses self callback functions to handle done tasks.
> >
> > int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task
> > *task, cmdq_async_flush_cb cb, void *data)
> > {
> >         ...
> >         mbox_send_message(client->chan, task);
> >         /* We can send next task immediately, so just call txdone. */
> >         mbox_client_txdone(client->chan, 0);
> >         ...
> > }
> >
> > Another solution is to use rx_callback; i.e. CMDQ mailbox controller
> > call mbox_chan_received_data() when CMDQ task is done. But, this may
> > violate the design of mailbox. What do you think?
> >
> If my point (c) above does not hold, maybe look at implementing
> tx_done() callback and submit next task from the callback of last
> done.


Hi Jassi,

For point (c), GCE irq means 1~n tasks done or
0~n tasks done + 1 task error.
In irq, we can know which tasks are done by register and GCE pc.

As I mentioned before, we cannot submit next task after previous task
call tx_done. We need to submit multiple tasks to GCE HW immediately
and queue them in GCE HW. Let me explain this requirement by mouse
cursor example. User may move mouse quickly between two vsync, so DRM
may update display registers frequently. For CMDQ, that means many tasks
are flushed into CMDQ driver, and CMDQ driver needs to process all of
them in next vblank. Therefore, we cannot block any CMDQ task in SW
buffer.

CMDQ needs to call callback function to notice clients which tasks are
done. In my previous e-mail, I mentioned that rx_callback may be an
alternative solution. However, it seems to violate the design of
mailbox. Therefore, I think mailbox may not have a good solution for
CMDQ callback currently. IMHO, the better way is to use CMDQ self
callback for now.

Thanks,
HS

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Jassi Brar]

On 5 October 2016 at 18:01, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> On Wed, 2016-10-05 at 09:07 +0530, Jassi Brar wrote:
>> On 5 October 2016 at 08:24, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
>> > On Fri, 2016-09-30 at 17:47 +0800, Horng-Shyang Liao wrote:
>> >> On Fri, 2016-09-30 at 17:11 +0800, CK Hu wrote:
>>
>> >
>> > After I trace mailbox driver, I realize that CMDQ driver cannot use
>> > tx_done.
>> >
>> > CMDQ clients will flush many tasks into CMDQ driver, and then CMDQ
>> > driver will apply these tasks into GCE HW "immediately". These tasks,
>> > which are queued in GCE HW, may not execute immediately since they
>> > may need to wait event(s), e.g. vsync.
>> >
>> > However, in mailbox driver, mailbox uses a software buffer to queue
>> > sent messages. It only sends next message until previous message is
>> > done. This cannot fulfill CMDQ's requirement.
>> >
>> I understand
>>  a) GCE HW can internally queue many tasks in some 'FIFO'
>>  b) Execution of some task may have to wait until some external event
>> occurs (like vsync)
>>  c) GCE does not generate irq/flag for each task executed (?)
>>
>> If so, may be your tx_done should return 'true' so long as the GCE HW
>> can accept tasks in its 'FIFO'. For mailbox api, any task that is
>> queued on GCE, is assumed to be transmitted.
>>
>> > Quote some code from mailbox driver. Please notice "active_req" part.
>> >
>> > static void msg_submit(struct mbox_chan *chan)
>> > {
>> >         ...
>> >         if (!chan->msg_count || chan->active_req)
>> >                 goto exit;
>> >         ...
>> >         err = chan->mbox->ops->send_data(chan, data);
>> >         if (!err) {
>> >                 chan->active_req = data;
>> >                 chan->msg_count--;
>> >         }
>> >         ...
>> > }
>> >
>> > static void tx_tick(struct mbox_chan *chan, int r)
>> > {
>> >         ...
>> >         spin_lock_irqsave(&chan->lock, flags);
>> >         mssg = chan->active_req;
>> >         chan->active_req = NULL;
>> >         spin_unlock_irqrestore(&chan->lock, flags);
>> >         ...
>> > }
>> >
>> > Current workable CMDQ driver uses mbox_client_txdone() to prevent
>> > this issue, and then uses self callback functions to handle done tasks.
>> >
>> > int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task
>> > *task, cmdq_async_flush_cb cb, void *data)
>> > {
>> >         ...
>> >         mbox_send_message(client->chan, task);
>> >         /* We can send next task immediately, so just call txdone. */
>> >         mbox_client_txdone(client->chan, 0);
>> >         ...
>> > }
>> >
>> > Another solution is to use rx_callback; i.e. CMDQ mailbox controller
>> > call mbox_chan_received_data() when CMDQ task is done. But, this may
>> > violate the design of mailbox. What do you think?
>> >
>> If my point (c) above does not hold, maybe look at implementing
>> tx_done() callback and submit next task from the callback of last
>> done.
>
>
> Hi Jassi,
>
> For point (c), GCE irq means 1~n tasks done or
> 0~n tasks done + 1 task error.
> In irq, we can know which tasks are done by register and GCE pc.
>
> As I mentioned before, we cannot submit next task after previous task
> call tx_done. We need to submit multiple tasks to GCE HW immediately
> and queue them in GCE HW.

> Let me explain this requirement by mouse
> cursor example. User may move mouse quickly between two vsync, so DRM
> may update display registers frequently. For CMDQ, that means many tasks
> are flushed into CMDQ driver, and CMDQ driver needs to process all of
> them in next vblank. Therefore, we cannot block any CMDQ task in SW
> buffer.
>
We are interested only in the current position of cursor and not its
trail. Also the current position should be updated at next vsync (and
not the one after it).
Going by this example, if the GCE HW can take in 'N' tasks at a time,
then the N+1th submission should shift out (drop) the 1st task queued.
So that at any time GCE HW has only the latest N tasks. Right?

 If yes, maybe you don't need to care about tx-done and simply keep
shoving tasks as you generate them.

 If no, maybe your client driver need to emulate such a circular
buffer where oldest task is overwritten by newest submission. And you
submit the circular buffer (most relevant tasks) at one go to the GCE
HW.

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

On Wed, 2016-10-05 at 20:13 +0530, Jassi Brar wrote:
> On 5 October 2016 at 18:01, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > On Wed, 2016-10-05 at 09:07 +0530, Jassi Brar wrote:
> >> On 5 October 2016 at 08:24, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> >> > On Fri, 2016-09-30 at 17:47 +0800, Horng-Shyang Liao wrote:
> >> >> On Fri, 2016-09-30 at 17:11 +0800, CK Hu wrote:
> >>
> >> >
> >> > After I trace mailbox driver, I realize that CMDQ driver cannot use
> >> > tx_done.
> >> >
> >> > CMDQ clients will flush many tasks into CMDQ driver, and then CMDQ
> >> > driver will apply these tasks into GCE HW "immediately". These tasks,
> >> > which are queued in GCE HW, may not execute immediately since they
> >> > may need to wait event(s), e.g. vsync.
> >> >
> >> > However, in mailbox driver, mailbox uses a software buffer to queue
> >> > sent messages. It only sends next message until previous message is
> >> > done. This cannot fulfill CMDQ's requirement.
> >> >
> >> I understand
> >>  a) GCE HW can internally queue many tasks in some 'FIFO'
> >>  b) Execution of some task may have to wait until some external event
> >> occurs (like vsync)
> >>  c) GCE does not generate irq/flag for each task executed (?)
> >>
> >> If so, may be your tx_done should return 'true' so long as the GCE HW
> >> can accept tasks in its 'FIFO'. For mailbox api, any task that is
> >> queued on GCE, is assumed to be transmitted.
> >>
> >> > Quote some code from mailbox driver. Please notice "active_req" part.
> >> >
> >> > static void msg_submit(struct mbox_chan *chan)
> >> > {
> >> >         ...
> >> >         if (!chan->msg_count || chan->active_req)
> >> >                 goto exit;
> >> >         ...
> >> >         err = chan->mbox->ops->send_data(chan, data);
> >> >         if (!err) {
> >> >                 chan->active_req = data;
> >> >                 chan->msg_count--;
> >> >         }
> >> >         ...
> >> > }
> >> >
> >> > static void tx_tick(struct mbox_chan *chan, int r)
> >> > {
> >> >         ...
> >> >         spin_lock_irqsave(&chan->lock, flags);
> >> >         mssg = chan->active_req;
> >> >         chan->active_req = NULL;
> >> >         spin_unlock_irqrestore(&chan->lock, flags);
> >> >         ...
> >> > }
> >> >
> >> > Current workable CMDQ driver uses mbox_client_txdone() to prevent
> >> > this issue, and then uses self callback functions to handle done tasks.
> >> >
> >> > int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task
> >> > *task, cmdq_async_flush_cb cb, void *data)
> >> > {
> >> >         ...
> >> >         mbox_send_message(client->chan, task);
> >> >         /* We can send next task immediately, so just call txdone. */
> >> >         mbox_client_txdone(client->chan, 0);
> >> >         ...
> >> > }
> >> >
> >> > Another solution is to use rx_callback; i.e. CMDQ mailbox controller
> >> > call mbox_chan_received_data() when CMDQ task is done. But, this may
> >> > violate the design of mailbox. What do you think?
> >> >
> >> If my point (c) above does not hold, maybe look at implementing
> >> tx_done() callback and submit next task from the callback of last
> >> done.
> >
> >
> > Hi Jassi,
> >
> > For point (c), GCE irq means 1~n tasks done or
> > 0~n tasks done + 1 task error.
> > In irq, we can know which tasks are done by register and GCE pc.
> >
> > As I mentioned before, we cannot submit next task after previous task
> > call tx_done. We need to submit multiple tasks to GCE HW immediately
> > and queue them in GCE HW.
> 
> > Let me explain this requirement by mouse
> > cursor example. User may move mouse quickly between two vsync, so DRM
> > may update display registers frequently. For CMDQ, that means many tasks
> > are flushed into CMDQ driver, and CMDQ driver needs to process all of
> > them in next vblank. Therefore, we cannot block any CMDQ task in SW
> > buffer.
> >
> We are interested only in the current position of cursor and not its
> trail. Also the current position should be updated at next vsync (and
> not the one after it).
> Going by this example, if the GCE HW can take in 'N' tasks at a time,
> then the N+1th submission should shift out (drop) the 1st task queued.
> So that at any time GCE HW has only the latest N tasks. Right?
> 
>  If yes, maybe you don't need to care about tx-done and simply keep
> shoving tasks as you generate them.
> 
>  If no, maybe your client driver need to emulate such a circular
> buffer where oldest task is overwritten by newest submission. And you
> submit the circular buffer (most relevant tasks) at one go to the GCE
> HW.


Hi Jassi,

CMDQ driver doesn't know the task type, so CMDQ cannot decide which
tasks can be dropped. So, I think the answer is "no".

Client driver is also hard to emulate a circular buffer where oldest
task is overwritten by newest submission. Let me illustrate with DRM
client (display driver). For DRM, the only way to know the latest
mouse cursor task before vsync is vsync interrupt. However, if we
keep latest mouse cursor task and start to send it to CMDQ after
vsync, the task may be executed outside the vblank and generate garbage
on screen in this case. (since we allow nested interrupt) We can wait
one more vsync to prevent garbage, but we will lose mouse cursor
performance. Therefore, our current solution is to flush all DRM tasks
into CMDQ, and queue all tasks in GCE HW. When GCE get vsync event, it
can finish all tasks within vblank to fulfill DRM's requirement.

Back to our original statement, we need to flush all tasks to queue
in GCE HW; i.e. we need to use mbox_client_txdone after
mbox_send_message, or send tx_done once mailbox controller receive
message (task). However, we still need a way to notice done tasks to
clients. Currently, we don't have a good way to call callback in mailbox
framework. Therefore, CMDQ driver has its owner callback functions.

Thanks,
HS

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Jassi Brar]

On 6 October 2016 at 18:31, Horng-Shyang Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:

> Back to our original statement, we need to flush all tasks to queue
> in GCE HW; i.e. we need to use mbox_client_txdone after
> mbox_send_message, or send tx_done once mailbox controller receive
> message (task). However, we still need a way to notice done tasks to
> clients. Currently, we don't have a good way to call callback in mailbox
> framework. Therefore, CMDQ driver has its owner callback functions.
>
mbox_client_txdone() is called by the client driver when only it knows
the messages has been transmitted (i.e your submitted tasks are done).
Obviously the client driver should do any callbacks to its users
upstream.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

On Thu, 2016-10-06 at 18:40 +0530, Jassi Brar wrote:
> On 6 October 2016 at 18:31, Horng-Shyang Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> 
> > Back to our original statement, we need to flush all tasks to queue
> > in GCE HW; i.e. we need to use mbox_client_txdone after
> > mbox_send_message, or send tx_done once mailbox controller receive
> > message (task). However, we still need a way to notice done tasks to
> > clients. Currently, we don't have a good way to call callback in mailbox
> > framework. Therefore, CMDQ driver has its owner callback functions.
> >
> mbox_client_txdone() is called by the client driver when only it knows
> the messages has been transmitted (i.e your submitted tasks are done).
> Obviously the client driver should do any callbacks to its users
> upstream.

Hi Jassi,

In current CMDQ driver, mbox_client_txdone() is called to prevent the
blocking of chan->active_req. It is not the real point of CMDQ task
done, so the client driver cannot do any callbacks to its user upstream.

(1) If we don't use mbox_client_txdone(), could you tell us an
    alternative way to prevent the blocking of chan->active_req?
    And then we can use tx_done when CMDQ task is relly done.
(2) If we use mbox_client_txdone() to prevent the blocking of
    chan->active_req, could CMDQ driver just uses self-defined callback
    function to notice client driver CMDQ task done?

Thanks,
HS


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver

[Jassi Brar]

On 11 October 2016 at 08:10, Horng-Shyang Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> On Thu, 2016-10-06 at 18:40 +0530, Jassi Brar wrote:
>> On 6 October 2016 at 18:31, Horng-Shyang Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>>
>> > Back to our original statement, we need to flush all tasks to queue
>> > in GCE HW; i.e. we need to use mbox_client_txdone after
>> > mbox_send_message, or send tx_done once mailbox controller receive
>> > message (task). However, we still need a way to notice done tasks to
>> > clients. Currently, we don't have a good way to call callback in mailbox
>> > framework. Therefore, CMDQ driver has its owner callback functions.
>> >
>> mbox_client_txdone() is called by the client driver when only it knows
>> the messages has been transmitted (i.e your submitted tasks are done).
>> Obviously the client driver should do any callbacks to its users
>> upstream.
>
> Hi Jassi,
>
> In current CMDQ driver, mbox_client_txdone() is called to prevent the
> blocking of chan->active_req. It is not the real point of CMDQ task
> done, so the client driver cannot do any callbacks to its user upstream.
>
> (1) If we don't use mbox_client_txdone(), could you tell us an
>     alternative way to prevent the blocking of chan->active_req?
>     And then we can use tx_done when CMDQ task is relly done.
>
mbox_client_txdone() should be used only when the mailbox controller
driver can't figure when the TX is done. Client driver (by like some
reply packet) realises the TX is done (for the reply to have arrived).

 If your hardware does flag/irq when tx is done, you should prefer
that over mbox_client_txdone().


> (2) If we use mbox_client_txdone() to prevent the blocking of
>     chan->active_req, could CMDQ driver just uses self-defined callback
>     function to notice client driver CMDQ task done?
>
Anything above the mailbox api, is none of its business. Your platform
specific 'server' driver can implement its own callbacks to notify its
users.

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

[HS Liao]

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

Signed-off-by: HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 10f638f..00d9d92 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -383,6 +383,16 @@
 			#clock-cells = <1>;
 		};
 
+		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";
+
+			#mbox-cells = <2>;
+		};
+
 		mipi_tx0: mipi-dphy@10215000 {
 			compatible = "mediatek,mt8173-mipi-tx";
 			reg = <0 0x10215000 0 0x1000>;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v14 4/4] CMDQ: save more energy in idle

[HS Liao]

Use clk_disable_unprepare instead of clk_disable to save more energy
when CMDQ is idle.

Signed-off-by: HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 8 deletions(-)

diff --git a/drivers/mailbox/mtk-cmdq.c b/drivers/mailbox/mtk-cmdq.c
index daf5561..0bf30cb 100644
--- a/drivers/mailbox/mtk-cmdq.c
+++ b/drivers/mailbox/mtk-cmdq.c
@@ -29,6 +29,7 @@
 #include <linux/spinlock.h>
 #include <linux/suspend.h>
 #include <linux/timer.h>
+#include <linux/workqueue.h>
 
 #define CMDQ_THR_MAX_COUNT		3 /* main, sub, general(misc) */
 #define CMDQ_INST_SIZE			8 /* instruction is 64-bit */
@@ -130,10 +131,16 @@ struct cmdq_task {
 	struct cmdq_task_cb	cb;
 };
 
+struct cmdq_clk_release {
+	struct cmdq		*cmdq;
+	struct work_struct	release_work;
+};
+
 struct cmdq {
 	struct mbox_controller	mbox;
 	void __iomem		*base;
 	u32			irq;
+	struct workqueue_struct	*clk_release_wq;
 	struct cmdq_thread	thread[CMDQ_THR_MAX_COUNT];
 	struct mutex		task_mutex;
 	struct clk		*clock;
@@ -279,11 +286,19 @@ static void cmdq_thread_wait_end(struct cmdq_thread *thread,
 static void cmdq_task_exec(struct cmdq_task *task, struct cmdq_thread *thread)
 {
 	struct cmdq *cmdq = task->cmdq;
-	unsigned long curr_pa, end_pa;
+	unsigned long curr_pa, end_pa, flags;
 
 	task->thread = thread;
 	if (list_empty(&thread->task_busy_list)) {
-		WARN_ON(clk_enable(cmdq->clock) < 0);
+		/*
+		 * Unlock for clk prepare (sleeping function).
+		 * We are safe to do that since we have task_mutex and
+		 * only flush will add task.
+		 */
+		spin_unlock_irqrestore(&thread->chan->lock, flags);
+		WARN_ON(clk_prepare_enable(cmdq->clock) < 0);
+		spin_lock_irqsave(&thread->chan->lock, flags);
+
 		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
 
 		writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
@@ -365,6 +380,26 @@ static void cmdq_task_handle_error(struct cmdq_task *task)
 	cmdq_thread_resume(thread);
 }
 
+static void cmdq_clk_release_work(struct work_struct *work_item)
+{
+	struct cmdq_clk_release *clk_release = container_of(work_item,
+			struct cmdq_clk_release, release_work);
+	struct cmdq *cmdq = clk_release->cmdq;
+
+	clk_disable_unprepare(cmdq->clock);
+	kfree(clk_release);
+}
+
+static void cmdq_clk_release_schedule(struct cmdq *cmdq)
+{
+	struct cmdq_clk_release *clk_release;
+
+	clk_release = kmalloc(sizeof(*clk_release), GFP_ATOMIC);
+	clk_release->cmdq = cmdq;
+	INIT_WORK(&clk_release->release_work, cmdq_clk_release_work);
+	queue_work(cmdq->clk_release_wq, &clk_release->release_work);
+}
+
 static void cmdq_thread_irq_handler(struct cmdq *cmdq,
 				    struct cmdq_thread *thread)
 {
@@ -414,7 +449,7 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq,
 
 	if (list_empty(&thread->task_busy_list)) {
 		cmdq_thread_disable(cmdq, thread);
-		clk_disable(cmdq->clock);
+		cmdq_clk_release_schedule(cmdq);
 	} else {
 		mod_timer(&thread->timeout,
 			  jiffies + msecs_to_jiffies(CMDQ_TIMEOUT_MS));
@@ -473,7 +508,7 @@ static void cmdq_thread_handle_timeout(unsigned long data)
 
 	cmdq_thread_resume(thread);
 	cmdq_thread_disable(cmdq, thread);
-	clk_disable(cmdq->clock);
+	cmdq_clk_release_schedule(cmdq);
 	spin_unlock_irqrestore(&thread->chan->lock, flags);
 }
 
@@ -761,7 +796,7 @@ static int cmdq_suspend(struct device *dev)
 		msleep(20);
 	}
 
-	clk_unprepare(cmdq->clock);
+	flush_workqueue(cmdq->clk_release_wq);
 	return 0;
 }
 
@@ -769,7 +804,6 @@ 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;
 }
@@ -778,8 +812,8 @@ static int cmdq_remove(struct platform_device *pdev)
 {
 	struct cmdq *cmdq = platform_get_drvdata(pdev);
 
+	destroy_workqueue(cmdq->clk_release_wq);
 	mbox_controller_unregister(&cmdq->mbox);
-	clk_unprepare(cmdq->clock);
 	return 0;
 }
 
@@ -898,8 +932,12 @@ static int cmdq_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	cmdq->clk_release_wq = alloc_ordered_workqueue(
+			"%s", WQ_MEM_RECLAIM | WQ_HIGHPRI,
+			"cmdq_clk_release");
+
 	platform_set_drvdata(pdev, cmdq);
-	WARN_ON(clk_prepare(cmdq->clock) < 0);
+
 	return 0;
 }
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 4/4] CMDQ: save more energy in idle

[Jassi Brar]

On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> Use clk_disable_unprepare instead of clk_disable to save more energy
> when CMDQ is idle.
>
> Signed-off-by: HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------

The driver is introduced by second patch of the set, so it makes sense
to merge this patch into patch 2/4.

Re: [PATCH v14 4/4] CMDQ: save more energy in idle

[Horng-Shyang Liao]

On Thu, 2016-09-22 at 13:22 +0530, Jassi Brar wrote:
> On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <hs.liao@mediatek.com> wrote:
> > Use clk_disable_unprepare instead of clk_disable to save more energy
> > when CMDQ is idle.
> >
> > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------
> 
> The driver is introduced by second patch of the set, so it makes sense
> to merge this patch into patch 2/4.

Hi Jassi,

Could you take a look at previous discussion between Matthias and me?
http://lkml.iu.edu/hypermail/linux/kernel/1606.2/05239.html
His basic idea is to simplify first working version.
Therefore, I move some code to this patch.

Thanks,
HS

Re: [PATCH v14 4/4] CMDQ: save more energy in idle

[Horng-Shyang Liao]

On Fri, 2016-09-23 at 17:28 +0800, Horng-Shyang Liao wrote:
> On Thu, 2016-09-22 at 13:22 +0530, Jassi Brar wrote:
> > On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > > Use clk_disable_unprepare instead of clk_disable to save more energy
> > > when CMDQ is idle.
> > >
> > > Signed-off-by: HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > ---
> > >  drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------
> > 
> > The driver is introduced by second patch of the set, so it makes sense
> > to merge this patch into patch 2/4.
> 
> Hi Jassi,
> 
> Could you take a look at previous discussion between Matthias and me?
> http://lkml.iu.edu/hypermail/linux/kernel/1606.2/05239.html
> His basic idea is to simplify first working version.
> Therefore, I move some code to this patch.
> 
> Thanks,
> HS
> 

Hi Jassi,

What do you think about our previous discussion?

Thanks,
HS



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 4/4] CMDQ: save more energy in idle

[Matthias Brugger]

On 09/30/2016 10:56 AM, Horng-Shyang Liao wrote:
> On Fri, 2016-09-23 at 17:28 +0800, Horng-Shyang Liao wrote:
>> On Thu, 2016-09-22 at 13:22 +0530, Jassi Brar wrote:
>>> On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>>>> Use clk_disable_unprepare instead of clk_disable to save more energy
>>>> when CMDQ is idle.
>>>>
>>>> Signed-off-by: HS Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>>  drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------
>>>
>>> The driver is introduced by second patch of the set, so it makes sense
>>> to merge this patch into patch 2/4.
>>
>> Hi Jassi,
>>
>> Could you take a look at previous discussion between Matthias and me?
>> http://lkml.iu.edu/hypermail/linux/kernel/1606.2/05239.html
>> His basic idea is to simplify first working version.
>> Therefore, I move some code to this patch.
>>

Well what I wanted to say is, that right now this driver is quite a big 
beast and this makes it difficult to review. So my idea was to just 
submit the most basic version of this driver.
Any improvements on the driver should be sent in follow-up patches after 
the basic driver got merged. That was my idea.

Regards,
Matthias
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 0/4] Mediatek MT8173 CMDQ support

[Horng-Shyang Liao]

On Mon, 2016-09-05 at 09:44 +0800, HS Liao wrote:
> 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.
> 
> These patches have a build dependency on top of v4.8-rc1.
> 
> Changes since v13:
>  - move to mailbox folder
>  - simplify cmdq_thread_is_in_wfe
> 
> Best regards,
> HS Liao
> 
> HS Liao (4):
>   dt-bindings: soc: Add documentation for the MediaTek GCE unit
>   CMDQ: Mediatek CMDQ driver
>   arm64: dts: mt8173: Add GCE node
>   CMDQ: save more energy in idle
> 
>  .../devicetree/bindings/mailbox/mtk-gce.txt        |  43 +
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi           |  10 +
>  drivers/mailbox/Kconfig                            |  10 +
>  drivers/mailbox/Makefile                           |   2 +
>  drivers/mailbox/mtk-cmdq.c                         | 965 +++++++++++++++++++++
>  include/linux/mailbox/mtk-cmdq.h                   | 180 ++++
>  6 files changed, 1210 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
>  create mode 100644 drivers/mailbox/mtk-cmdq.c
>  create mode 100644 include/linux/mailbox/mtk-cmdq.h
> 

Hi Jassi,

Sorry to disturb you.
Do you have any further comment on CMDQ v14?

Thanks.
HS


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html