[v23,4/4] soc: mediatek: Add Mediatek CMDQ helper

Commit Message

houlong.wei July 25, 2018, 1:26 a.m. UTC

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

Signed-off-by: Houlong Wei <[e-mail]>
Signed-off-by: HS Liao <[e-mail]>
---
 drivers/soc/mediatek/Kconfig           |   12 ++
 drivers/soc/mediatek/Makefile          |    1 +
 drivers/soc/mediatek/mtk-cmdq-helper.c |  291 ++++++++++++++++++++++++++++++++
 include/linux/soc/mediatek/mtk-cmdq.h  |  135 +++++++++++++++
 4 files changed, 439 insertions(+)
 create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
 create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h

Comments

houlong.wei Aug. 15, 2018, 1:46 a.m. UTC | #1

On Wed, 2018-07-25 at 09:26 +0800, Houlong Wei wrote:
> Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> 
> Signed-off-by: Houlong Wei <[e-mail]>
> Signed-off-by: HS Liao <[e-mail]>
> ---
>  drivers/soc/mediatek/Kconfig           |   12 ++
>  drivers/soc/mediatek/Makefile          |    1 +
>  drivers/soc/mediatek/mtk-cmdq-helper.c |  291 ++++++++++++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  |  135 +++++++++++++++
>  4 files changed, 439 insertions(+)
>  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
>  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> 
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index a7d0667..17bd759 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -4,6 +4,18 @@
>  menu "MediaTek SoC drivers"
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
>  
> +config MTK_CMDQ
> +	tristate "MediaTek CMDQ Support"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	select MAILBOX
> +	select MTK_CMDQ_MBOX
> +	select MTK_INFRACFG
> +	help
> +	  Say yes here to add support for the MediaTek Command Queue (CMDQ)
> +	  driver. The CMDQ is used to help read/write registers with critical
> +	  time limitation, such as updating display configuration during the
> +	  vblank.
> +
>  config MTK_INFRACFG
>  	bool "MediaTek INFRACFG Support"
>  	select REGMAP
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index 12998b0..64ce5ee 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
>  obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
>  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
>  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> new file mode 100644
> index 0000000..e4dbb7e
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2018 MediaTek Inc.
> +
> +#include <linux/completion.h>
> +#include <linux/errno.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/soc/mediatek/mtk-cmdq.h>
> +
> +#define CMDQ_ARG_A_WRITE_MASK	0xffff
> +#define CMDQ_WRITE_ENABLE_MASK	BIT(0)
> +#define CMDQ_EOC_IRQ_EN		BIT(0)
> +#define CMDQ_EOC_CMD		((u64)((CMDQ_CODE_EOC << CMDQ_OP_CODE_SHIFT)) \
> +				<< 32 | CMDQ_EOC_IRQ_EN)
> +
> +static void cmdq_client_timeout(struct timer_list *t)
> +{
> +	struct cmdq_client *client = from_timer(client, t, timer);
> +
> +	dev_err(client->client.dev, "cmdq timeout!\n");
> +}
> +
> +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
> +{
> +	struct cmdq_client *client;
> +
> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
> +	if (!client)
> +		return (struct cmdq_client *)-ENOMEM;
> +
> +	client->timeout_ms = timeout;
> +	if (timeout != CMDQ_NO_TIMEOUT) {
> +		spin_lock_init(&client->lock);
> +		timer_setup(&client->timer, cmdq_client_timeout, 0);
> +	}
> +	client->pkt_cnt = 0;
> +	client->client.dev = dev;
> +	client->client.tx_block = false;
> +	client->chan = mbox_request_channel(&client->client, index);
> +
> +	if (IS_ERR(client->chan)) {
> +		long err = 0;
> +
> +		dev_err(dev, "failed to request channel\n");
> +		err = PTR_ERR(client->chan);
> +		kfree(client);
> +
> +		return (struct cmdq_client *)err;
> +	}
> +
> +	return client;
> +}
> +EXPORT_SYMBOL(cmdq_mbox_create);
> +
> +void cmdq_mbox_destroy(struct cmdq_client *client)
> +{
> +	if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> +		spin_lock(&client->lock);
> +		del_timer_sync(&client->timer);
> +		spin_unlock(&client->lock);
> +	}
> +	mbox_free_channel(client->chan);
> +	kfree(client);
> +}
> +EXPORT_SYMBOL(cmdq_mbox_destroy);
> +
> +int cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt **pkt_ptr,
> +		    size_t size)
> +{
> +	struct cmdq_pkt *pkt;
> +	struct device *dev;
> +	dma_addr_t dma_addr;
> +
> +	pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> +	if (!pkt)
> +		return -ENOMEM;
> +	pkt->va_base = kzalloc(size, GFP_KERNEL);
> +	if (!pkt->va_base) {
> +		kfree(pkt);
> +		return -ENOMEM;
> +	}
> +	pkt->buf_size = size;
> +	pkt->cl = (void *)client;
> +
> +	dev = client->chan->mbox->dev;
> +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
> +		DMA_TO_DEVICE);
> +	if (dma_mapping_error(dev, dma_addr)) {
> +		dev_err(dev, "dma map failed, size=%u\n", (u32)(u64)size);
> +		kfree(pkt->va_base);
> +		kfree(pkt);
> +		return -ENOMEM;
> +	}
> +
> +	pkt->pa_base = dma_addr;
> +	*pkt_ptr = pkt;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_create);
> +
> +void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> +
> +	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
> +			 DMA_TO_DEVICE);
> +	kfree(pkt->va_base);
> +	kfree(pkt);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_destroy);
> +
> +static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
> +				   u32 arg_a, u32 arg_b)
> +{
> +	u64 *cmd_ptr;
> +
> +	if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) {
> +		pkt->cmd_buf_size += CMDQ_INST_SIZE;
> +		return -ENOMEM;
> +	}
> +	cmd_ptr = pkt->va_base + pkt->cmd_buf_size;
> +	(*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
> +	pkt->cmd_buf_size += CMDQ_INST_SIZE;
> +
> +	return 0;
> +}
> +
> +int cmdq_pkt_write(struct cmdq_pkt *pkt, u32 value, u32 subsys, u32 offset)
> +{
> +	u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) |
> +		    (subsys << CMDQ_SUBSYS_SHIFT);
> +
> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_write);
> +
> +int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u32 value,
> +			u32 subsys, u32 offset, u32 mask)
> +{
> +	u32 offset_mask = offset;
> +	int err;
> +
> +	if (mask != 0xffffffff) {
> +		err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask);
> +		WARN_ON(err < 0);
> +		offset_mask |= CMDQ_WRITE_ENABLE_MASK;
> +	}
> +
> +	return cmdq_pkt_write(pkt, value, subsys, offset_mask);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_write_mask);
> +
> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event)
> +{
> +	u32 arg_b;
> +
> +	if (event >= CMDQ_MAX_EVENT)
> +		return -EINVAL;
> +
> +	/*
> +	 * WFE arg_b
> +	 * bit 0-11: wait value
> +	 * bit 15: 1 - wait, 0 - no wait
> +	 * bit 16-27: update value
> +	 * bit 31: 1 - update, 0 - no update
> +	 */
> +	arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
> +
> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_wfe);
> +
> +int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event)
> +{
> +	if (event >= CMDQ_MAX_EVENT)
> +		return -EINVAL;
> +
> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event,
> +				       CMDQ_WFE_UPDATE);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_clear_event);
> +
> +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> +{
> +	int err;
> +
> +	/* insert EOC and generate IRQ for each command iteration */
> +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> +	WARN_ON(err < 0);
> +
> +	/* JUMP to end */
> +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> +	WARN_ON(err < 0);
> +
> +	return err;
> +}
> +
> +static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> +{
> +	struct cmdq_pkt *pkt = (struct cmdq_pkt *)data.data;
> +	struct cmdq_task_cb *cb = &pkt->cb;
> +	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> +
> +	if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> +		unsigned long flags = 0;
> +
> +		spin_lock_irqsave(&client->lock, flags);
> +		if (--client->pkt_cnt == 0)
> +			del_timer(&client->timer);
> +		else
> +			mod_timer(&client->timer, jiffies +
> +				  msecs_to_jiffies(client->timeout_ms));
> +		spin_unlock_irqrestore(&client->lock, flags);
> +	}
> +
> +	dma_sync_single_for_cpu(client->chan->mbox->dev, pkt->pa_base,
> +				pkt->cmd_buf_size, DMA_TO_DEVICE);
> +	if (cb->cb) {
> +		data.data = cb->data;
> +		cb->cb(data);
> +	}
> +}
> +
> +int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> +			 void *data)
> +{
> +	int err;
> +	unsigned long flags = 0;
> +	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> +
> +	err = cmdq_pkt_finalize(pkt);
> +	if (err < 0)
> +		return err;
> +
> +	pkt->cb.cb = cb;
> +	pkt->cb.data = data;
> +	pkt->async_cb.cb = cmdq_pkt_flush_async_cb;
> +	pkt->async_cb.data = pkt;
> +
> +	dma_sync_single_for_device(client->chan->mbox->dev, pkt->pa_base,
> +				   pkt->cmd_buf_size, DMA_TO_DEVICE);
> +
> +	if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> +		spin_lock_irqsave(&client->lock, flags);
> +		if (client->pkt_cnt++ == 0)
> +			mod_timer(&client->timer, jiffies +
> +				  msecs_to_jiffies(client->timeout_ms));
> +		spin_unlock_irqrestore(&client->lock, flags);
> +	}
> +
> +	mbox_send_message(client->chan, pkt);
> +	/* We can send next packet immediately, so just call txdone. */
> +	mbox_client_txdone(client->chan, 0);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> +
> +struct cmdq_flush_completion {
> +	struct completion cmplt;
> +	bool err;
> +};
> +
> +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> +{
> +	struct cmdq_flush_completion *cmplt;
> +
> +	cmplt = (struct cmdq_flush_completion *)data.data;
> +	if (data.sta != CMDQ_CB_NORMAL)
> +		cmplt->err = true;
> +	else
> +		cmplt->err = false;
> +	complete(&cmplt->cmplt);
> +}
> +
> +int cmdq_pkt_flush(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_flush_completion cmplt;
> +	int err;
> +
> +	init_completion(&cmplt.cmplt);
> +	err = cmdq_pkt_flush_async(pkt, cmdq_pkt_flush_cb, &cmplt);
> +	if (err < 0)
> +		return err;
> +	wait_for_completion(&cmplt.cmplt);
> +
> +	return cmplt.err ? -EFAULT : 0;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_flush);
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> new file mode 100644
> index 0000000..fc84fe4
> --- /dev/null
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -0,0 +1,135 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + *
> + */
> +
> +#ifndef __MTK_CMDQ_H__
> +#define __MTK_CMDQ_H__
> +
> +#include <linux/mailbox_client.h>
> +#include <linux/mailbox/mtk-cmdq-mailbox.h>
> +#include <linux/timer.h>
> +
> +#define CMDQ_NO_TIMEOUT		0xffffffffu
> +
> +/** cmdq event maximum */
> +#define CMDQ_MAX_EVENT				0x3ff
> +
> +struct cmdq_pkt;
> +
> +struct cmdq_client {
> +	spinlock_t lock;
> +	u32 pkt_cnt;
> +	struct mbox_client client;
> +	struct mbox_chan *chan;
> +	struct timer_list timer;
> +	u32 timeout_ms; /* in unit of microsecond */
> +};
> +
> +/**
> + * cmdq_mbox_create() - create CMDQ mailbox client and channel
> + * @dev:	device of CMDQ mailbox client
> + * @index:	index of CMDQ mailbox channel
> + * @timeout:	timeout of a pkt execution by GCE, in unit of microsecond, set
> + *		CMDQ_NO_TIMEOUT if a timer is not used.
> + *
> + * Return: CMDQ mailbox client pointer
> + */
> +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index,
> +				     u32 timeout);
> +
> +/**
> + * cmdq_mbox_destroy() - destroy CMDQ mailbox client and channel
> + * @client:	the CMDQ mailbox client
> + */
> +void cmdq_mbox_destroy(struct cmdq_client *client);
> +
> +/**
> + * cmdq_pkt_create() - create a CMDQ packet
> + * @client:	the CMDQ mailbox client
> + * @pkt_ptr:	CMDQ packet pointer to retrieve cmdq_pkt
> + * @size:	CMDQ buffer size
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt **pkt_ptr,
> +		    size_t size);
> +
> +/**
> + * cmdq_pkt_destroy() - destroy the CMDQ packet
> + * @pkt:	the CMDQ packet
> + */
> +void cmdq_pkt_destroy(struct cmdq_pkt *pkt);
> +
> +/**
> + * cmdq_pkt_write() - append write command to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @value:	the specified target register value
> + * @subsys:	the CMDQ sub system code
> + * @offset:	register offset from CMDQ sub system
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_write(struct cmdq_pkt *pkt, u32 value, u32 subsys, u32 offset);
> +
> +/**
> + * cmdq_pkt_write_mask() - append write command with mask to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @value:	the specified target register value
> + * @subsys:	the CMDQ sub system code
> + * @offset:	register offset from CMDQ sub system
> + * @mask:	the specified target register mask
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u32 value,
> +			u32 subsys, u32 offset, u32 mask);
> +
> +/**
> + * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @event:	the desired event type to "wait and CLEAR"
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event);
> +
> +/**
> + * cmdq_pkt_clear_event() - append clear event command to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @event:	the desired event to be cleared
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event);
> +
> +/**
> + * cmdq_pkt_flush() - trigger CMDQ to execute the CMDQ packet
> + * @pkt:	the CMDQ packet
> + *
> + * Return: 0 for success; else the error code is returned
> + *
> + * Trigger CMDQ to execute the CMDQ packet. Note that this is a
> + * synchronous flush function. When the function returned, the recorded
> + * commands have been done.
> + */
> +int cmdq_pkt_flush(struct cmdq_pkt *pkt);
> +
> +/**
> + * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
> + *                          packet and call back at the end of done packet
> + * @pkt:	the CMDQ packet
> + * @cb:		called at the end of done packet
> + * @data:	this data will pass back to cb
> + *
> + * Return: 0 for success; else the error code is returned
> + *
> + * Trigger CMDQ to asynchronously execute the CMDQ packet and call back
> + * at the end of done packet. Note that this is an ASYNC function. When the
> + * function returned, it may or may not be finished.
> + */
> +int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> +			 void *data);
> +
> +#endif	/* __MTK_CMDQ_H__ */

Hello Matthias,
  Could you please review this path when you are available? Thanks a
lot.

houlong.wei Sept. 9, 2018, 6:54 a.m. UTC | #2

On Wed, 2018-08-15 at 09:46 +0800, houlong wei wrote:
> On Wed, 2018-07-25 at 09:26 +0800, Houlong Wei wrote:
> > Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> >[...]

Hello Matthias,
   Sorry to disturb you. Could you please review this patch and give
your comment? Thanks a lot.

Regards
Houlong

Matthias Brugger Sept. 26, 2018, 3:53 p.m. UTC | #3

On 25/07/2018 03:26, Houlong Wei wrote:
> Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> 
> Signed-off-by: Houlong Wei <[e-mail]>
> Signed-off-by: HS Liao <[e-mail]>
> ---
>  drivers/soc/mediatek/Kconfig           |   12 ++
>  drivers/soc/mediatek/Makefile          |    1 +
>  drivers/soc/mediatek/mtk-cmdq-helper.c |  291 ++++++++++++++++++++++++++++++++
>  include/linux/soc/mediatek/mtk-cmdq.h  |  135 +++++++++++++++
>  4 files changed, 439 insertions(+)
>  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
>  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> 
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index a7d0667..17bd759 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -4,6 +4,18 @@
>  menu "MediaTek SoC drivers"
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
>  
> +config MTK_CMDQ
> +	tristate "MediaTek CMDQ Support"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	select MAILBOX
> +	select MTK_CMDQ_MBOX
> +	select MTK_INFRACFG
> +	help
> +	  Say yes here to add support for the MediaTek Command Queue (CMDQ)
> +	  driver. The CMDQ is used to help read/write registers with critical
> +	  time limitation, such as updating display configuration during the
> +	  vblank.
> +
>  config MTK_INFRACFG
>  	bool "MediaTek INFRACFG Support"
>  	select REGMAP
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index 12998b0..64ce5ee 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
>  obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
>  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
>  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> new file mode 100644
> index 0000000..e4dbb7e
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2018 MediaTek Inc.
> +
> +#include <linux/completion.h>
> +#include <linux/errno.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/soc/mediatek/mtk-cmdq.h>
> +
> +#define CMDQ_ARG_A_WRITE_MASK	0xffff
> +#define CMDQ_WRITE_ENABLE_MASK	BIT(0)
> +#define CMDQ_EOC_IRQ_EN		BIT(0)
> +#define CMDQ_EOC_CMD		((u64)((CMDQ_CODE_EOC << CMDQ_OP_CODE_SHIFT)) \
> +				<< 32 | CMDQ_EOC_IRQ_EN)
> +
> +static void cmdq_client_timeout(struct timer_list *t)
> +{
> +	struct cmdq_client *client = from_timer(client, t, timer);
> +
> +	dev_err(client->client.dev, "cmdq timeout!\n");
> +}
> +
> +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
> +{
> +	struct cmdq_client *client;
> +
> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
> +	if (!client)
> +		return (struct cmdq_client *)-ENOMEM;
> +
> +	client->timeout_ms = timeout;
> +	if (timeout != CMDQ_NO_TIMEOUT) {
> +		spin_lock_init(&client->lock);
> +		timer_setup(&client->timer, cmdq_client_timeout, 0);
> +	}
> +	client->pkt_cnt = 0;
> +	client->client.dev = dev;
> +	client->client.tx_block = false;
> +	client->chan = mbox_request_channel(&client->client, index);
> +
> +	if (IS_ERR(client->chan)) {
> +		long err = 0;
> +
> +		dev_err(dev, "failed to request channel\n");
> +		err = PTR_ERR(client->chan);
> +		kfree(client);
> +
> +		return (struct cmdq_client *)err;

Can't we use
return ERR_PTR(err);
here?

> +	}
> +
> +	return client;
> +}
> +EXPORT_SYMBOL(cmdq_mbox_create);
> +
> +void cmdq_mbox_destroy(struct cmdq_client *client)
> +{
> +	if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> +		spin_lock(&client->lock);
> +		del_timer_sync(&client->timer);
> +		spin_unlock(&client->lock);
> +	}
> +	mbox_free_channel(client->chan);
> +	kfree(client);
> +}
> +EXPORT_SYMBOL(cmdq_mbox_destroy);
> +
> +int cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt **pkt_ptr,
> +		    size_t size)

I suppose you are not returning the allocated cmdq_pkt to avoid checks for
IS_ERR() logic in the consumer of this API. Am I correct?
Do we really need a pointer to a pointer here? Can't we just use a struct
cmdq_pkt *pkt as argument? That would make things easier.

> +{
> +	struct cmdq_pkt *pkt;
> +	struct device *dev;
> +	dma_addr_t dma_addr;
> +
> +	pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> +	if (!pkt)
> +		return -ENOMEM;
> +	pkt->va_base = kzalloc(size, GFP_KERNEL);
> +	if (!pkt->va_base) {
> +		kfree(pkt);
> +		return -ENOMEM;
> +	}
> +	pkt->buf_size = size;
> +	pkt->cl = (void *)client;
> +
> +	dev = client->chan->mbox->dev;
> +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
> +		DMA_TO_DEVICE);
> +	if (dma_mapping_error(dev, dma_addr)) {
> +		dev_err(dev, "dma map failed, size=%u\n", (u32)(u64)size);
> +		kfree(pkt->va_base);
> +		kfree(pkt);
> +		return -ENOMEM;
> +	}
> +
> +	pkt->pa_base = dma_addr;
> +	*pkt_ptr = pkt;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_create);
> +
> +void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> +
> +	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
> +			 DMA_TO_DEVICE);
> +	kfree(pkt->va_base);
> +	kfree(pkt);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_destroy);
> +
> +static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
> +				   u32 arg_a, u32 arg_b)
> +{
> +	u64 *cmd_ptr;
> +
> +	if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) {
> +		pkt->cmd_buf_size += CMDQ_INST_SIZE;

Why do we update the cmd_buf_size here?

> +		return -ENOMEM;
> +	}
> +	cmd_ptr = pkt->va_base + pkt->cmd_buf_size;
> +	(*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
> +	pkt->cmd_buf_size += CMDQ_INST_SIZE;
> +
> +	return 0;
> +}
> +
> +int cmdq_pkt_write(struct cmdq_pkt *pkt, u32 value, u32 subsys, u32 offset)
> +{
> +	u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) |
> +		    (subsys << CMDQ_SUBSYS_SHIFT);
> +
> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_write);
> +
> +int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u32 value,
> +			u32 subsys, u32 offset, u32 mask)
> +{
> +	u32 offset_mask = offset;
> +	int err;
> +
> +	if (mask != 0xffffffff) {
> +		err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask);
> +		WARN_ON(err < 0);
> +		offset_mask |= CMDQ_WRITE_ENABLE_MASK;
> +	}
> +
> +	return cmdq_pkt_write(pkt, value, subsys, offset_mask);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_write_mask);
> +
> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event)
> +{
> +	u32 arg_b;
> +
> +	if (event >= CMDQ_MAX_EVENT)
> +		return -EINVAL;
> +
> +	/*
> +	 * WFE arg_b
> +	 * bit 0-11: wait value
> +	 * bit 15: 1 - wait, 0 - no wait
> +	 * bit 16-27: update value
> +	 * bit 31: 1 - update, 0 - no update
> +	 */
> +	arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
> +
> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_wfe);
> +
> +int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event)
> +{
> +	if (event >= CMDQ_MAX_EVENT)
> +		return -EINVAL;
> +
> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event,
> +				       CMDQ_WFE_UPDATE);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_clear_event);
> +
> +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> +{
> +	int err;
> +
> +	/* insert EOC and generate IRQ for each command iteration */
> +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> +	WARN_ON(err < 0);
> +
> +	/* JUMP to end */
> +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> +	WARN_ON(err < 0);
> +
> +	return err;
> +}
> +
> +static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> +{
> +	struct cmdq_pkt *pkt = (struct cmdq_pkt *)data.data;
> +	struct cmdq_task_cb *cb = &pkt->cb;
> +	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> +
> +	if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> +		unsigned long flags = 0;
> +
> +		spin_lock_irqsave(&client->lock, flags);
> +		if (--client->pkt_cnt == 0)
> +			del_timer(&client->timer);
> +		else
> +			mod_timer(&client->timer, jiffies +
> +				  msecs_to_jiffies(client->timeout_ms));
> +		spin_unlock_irqrestore(&client->lock, flags);
> +	}> +
> +	dma_sync_single_for_cpu(client->chan->mbox->dev, pkt->pa_base,
> +				pkt->cmd_buf_size, DMA_TO_DEVICE);
> +	if (cb->cb) {
> +		data.data = cb->data;
> +		cb->cb(data);
> +	}
> +}
> +
> +int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> +			 void *data)
> +{
> +	int err;
> +	unsigned long flags = 0;
> +	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> +
> +	err = cmdq_pkt_finalize(pkt);
> +	if (err < 0)
> +		return err;
> +
> +	pkt->cb.cb = cb;
> +	pkt->cb.data = data;
> +	pkt->async_cb.cb = cmdq_pkt_flush_async_cb;
> +	pkt->async_cb.data = pkt;
> +
> +	dma_sync_single_for_device(client->chan->mbox->dev, pkt->pa_base,
> +				   pkt->cmd_buf_size, DMA_TO_DEVICE);
> +
> +	if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> +		spin_lock_irqsave(&client->lock, flags);
> +		if (client->pkt_cnt++ == 0)
> +			mod_timer(&client->timer, jiffies +
> +				  msecs_to_jiffies(client->timeout_ms));
> +		spin_unlock_irqrestore(&client->lock, flags);
> +	}
> +
> +	mbox_send_message(client->chan, pkt);
> +	/* We can send next packet immediately, so just call txdone. */
> +	mbox_client_txdone(client->chan, 0);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> +
> +struct cmdq_flush_completion {
> +	struct completion cmplt;
> +	bool err;
> +};
> +
> +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> +{
> +	struct cmdq_flush_completion *cmplt;
> +
> +	cmplt = (struct cmdq_flush_completion *)data.data;
> +	if (data.sta != CMDQ_CB_NORMAL)
> +		cmplt->err = true;
> +	else
> +		cmplt->err = false;
> +	complete(&cmplt->cmplt);
> +}
> +
> +int cmdq_pkt_flush(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_flush_completion cmplt;
> +	int err;
> +
> +	init_completion(&cmplt.cmplt);
> +	err = cmdq_pkt_flush_async(pkt, cmdq_pkt_flush_cb, &cmplt);
> +	if (err < 0)
> +		return err;
> +	wait_for_completion(&cmplt.cmplt);
> +
> +	return cmplt.err ? -EFAULT : 0;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_flush);
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> new file mode 100644
> index 0000000..fc84fe4
> --- /dev/null
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -0,0 +1,135 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + *
> + */
> +
> +#ifndef __MTK_CMDQ_H__
> +#define __MTK_CMDQ_H__
> +
> +#include <linux/mailbox_client.h>
> +#include <linux/mailbox/mtk-cmdq-mailbox.h>
> +#include <linux/timer.h>
> +
> +#define CMDQ_NO_TIMEOUT		0xffffffffu
> +
> +/** cmdq event maximum */
> +#define CMDQ_MAX_EVENT				0x3ff
> +
> +struct cmdq_pkt;
> +
> +struct cmdq_client {
> +	spinlock_t lock;
> +	u32 pkt_cnt;
> +	struct mbox_client client;
> +	struct mbox_chan *chan;
> +	struct timer_list timer;
> +	u32 timeout_ms; /* in unit of microsecond */
> +};
> +
> +/**
> + * cmdq_mbox_create() - create CMDQ mailbox client and channel
> + * @dev:	device of CMDQ mailbox client
> + * @index:	index of CMDQ mailbox channel
> + * @timeout:	timeout of a pkt execution by GCE, in unit of microsecond, set
> + *		CMDQ_NO_TIMEOUT if a timer is not used.
> + *
> + * Return: CMDQ mailbox client pointer
> + */
> +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index,
> +				     u32 timeout);
> +
> +/**
> + * cmdq_mbox_destroy() - destroy CMDQ mailbox client and channel
> + * @client:	the CMDQ mailbox client
> + */
> +void cmdq_mbox_destroy(struct cmdq_client *client);
> +
> +/**
> + * cmdq_pkt_create() - create a CMDQ packet
> + * @client:	the CMDQ mailbox client
> + * @pkt_ptr:	CMDQ packet pointer to retrieve cmdq_pkt
> + * @size:	CMDQ buffer size
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt **pkt_ptr,
> +		    size_t size);
> +
> +/**
> + * cmdq_pkt_destroy() - destroy the CMDQ packet
> + * @pkt:	the CMDQ packet
> + */
> +void cmdq_pkt_destroy(struct cmdq_pkt *pkt);
> +
> +/**
> + * cmdq_pkt_write() - append write command to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @value:	the specified target register value
> + * @subsys:	the CMDQ sub system code
> + * @offset:	register offset from CMDQ sub system
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_write(struct cmdq_pkt *pkt, u32 value, u32 subsys, u32 offset);
> +
> +/**
> + * cmdq_pkt_write_mask() - append write command with mask to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @value:	the specified target register value
> + * @subsys:	the CMDQ sub system code
> + * @offset:	register offset from CMDQ sub system
> + * @mask:	the specified target register mask
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u32 value,
> +			u32 subsys, u32 offset, u32 mask);
> +
> +/**
> + * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @event:	the desired event type to "wait and CLEAR"
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event);
> +
> +/**
> + * cmdq_pkt_clear_event() - append clear event command to the CMDQ packet
> + * @pkt:	the CMDQ packet
> + * @event:	the desired event to be cleared
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event);
> +
> +/**
> + * cmdq_pkt_flush() - trigger CMDQ to execute the CMDQ packet
> + * @pkt:	the CMDQ packet
> + *
> + * Return: 0 for success; else the error code is returned
> + *
> + * Trigger CMDQ to execute the CMDQ packet. Note that this is a
> + * synchronous flush function. When the function returned, the recorded
> + * commands have been done.
> + */
> +int cmdq_pkt_flush(struct cmdq_pkt *pkt);
> +
> +/**
> + * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
> + *                          packet and call back at the end of done packet
> + * @pkt:	the CMDQ packet
> + * @cb:		called at the end of done packet
> + * @data:	this data will pass back to cb
> + *
> + * Return: 0 for success; else the error code is returned
> + *
> + * Trigger CMDQ to asynchronously execute the CMDQ packet and call back
> + * at the end of done packet. Note that this is an ASYNC function. When the
> + * function returned, it may or may not be finished.
> + */
> +int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> +			 void *data);
> +

bikeshadding alarm:
can we order the functions in the include file the same way we do in the c file?

Apart from the few comments, the driver looks fine to me.

Regards,
Matthias

houlong.wei Sept. 27, 2018, 1:57 a.m. UTC | #4

On Wed, 2018-09-26 at 23:53 +0800, Matthias Brugger wrote:
> 
> On 25/07/2018 03:26, Houlong Wei wrote:
> > Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> > 
> > Signed-off-by: Houlong Wei <[e-mail]>
> > Signed-off-by: HS Liao <[e-mail]>
> > ---
> >  drivers/soc/mediatek/Kconfig           |   12 ++
> >  drivers/soc/mediatek/Makefile          |    1 +
> >  drivers/soc/mediatek/mtk-cmdq-helper.c |  291 ++++++++++++++++++++++++++++++++
> >  include/linux/soc/mediatek/mtk-cmdq.h  |  135 +++++++++++++++
> >  4 files changed, 439 insertions(+)
> >  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> >  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h

[...]

> > +
> > +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
> > +{
> > +	struct cmdq_client *client;
> > +
> > +	client = kzalloc(sizeof(*client), GFP_KERNEL);
> > +	if (!client)
> > +		return (struct cmdq_client *)-ENOMEM;
> > +
> > +	client->timeout_ms = timeout;
> > +	if (timeout != CMDQ_NO_TIMEOUT) {
> > +		spin_lock_init(&client->lock);
> > +		timer_setup(&client->timer, cmdq_client_timeout, 0);
> > +	}
> > +	client->pkt_cnt = 0;
> > +	client->client.dev = dev;
> > +	client->client.tx_block = false;
> > +	client->chan = mbox_request_channel(&client->client, index);
> > +
> > +	if (IS_ERR(client->chan)) {
> > +		long err = 0;
> > +
> > +		dev_err(dev, "failed to request channel\n");
> > +		err = PTR_ERR(client->chan);
> > +		kfree(client);
> > +
> > +		return (struct cmdq_client *)err;
> 
> Can't we use
> return ERR_PTR(err);
> here?

Sure, will fix it.

> 
> > +	}
> > +
> > +	return client;
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_create);
> > +
> > +void cmdq_mbox_destroy(struct cmdq_client *client)
> > +{
> > +	if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> > +		spin_lock(&client->lock);
> > +		del_timer_sync(&client->timer);
> > +		spin_unlock(&client->lock);
> > +	}
> > +	mbox_free_channel(client->chan);
> > +	kfree(client);
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_destroy);
> > +
> > +int cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt **pkt_ptr,
> > +		    size_t size)
> 
> I suppose you are not returning the allocated cmdq_pkt to avoid checks for
> IS_ERR() logic in the consumer of this API. Am I correct?
> Do we really need a pointer to a pointer here? Can't we just use a struct
> cmdq_pkt *pkt as argument? That would make things easier.

Do you mean we change the argument 'struct cmdq_pkt **pkt_ptr' to
'struct cmdq_pkt *pkt', and the consumer allocate a struct cmdq_pkt
*pkt, then pass pkt as argument of this API?
If yes, the consumer still need to check the return value of this API.
For the current implementation, the consumer doesn't need check
IS_ERR(*pkt_ptr) if the return value equals to 0.

Since the consumer has to check the return value of this API once, to
make it simpler, I shall change the prototype to 
'struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t
size)' and change its implementation.

> 
> > +{
> > +	struct cmdq_pkt *pkt;
> > +	struct device *dev;
> > +	dma_addr_t dma_addr;
> > +
> > +	pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> > +	if (!pkt)
> > +		return -ENOMEM;
> > +	pkt->va_base = kzalloc(size, GFP_KERNEL);
> > +	if (!pkt->va_base) {
> > +		kfree(pkt);
> > +		return -ENOMEM;
> > +	}
> > +	pkt->buf_size = size;
> > +	pkt->cl = (void *)client;
> > +
> > +	dev = client->chan->mbox->dev;
> > +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
> > +		DMA_TO_DEVICE);
> > +	if (dma_mapping_error(dev, dma_addr)) {
> > +		dev_err(dev, "dma map failed, size=%u\n", (u32)(u64)size);
> > +		kfree(pkt->va_base);
> > +		kfree(pkt);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	pkt->pa_base = dma_addr;
> > +	*pkt_ptr = pkt;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_create);
> > +
> > +void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
> > +{
> > +	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> > +
> > +	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
> > +			 DMA_TO_DEVICE);
> > +	kfree(pkt->va_base);
> > +	kfree(pkt);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_destroy);
> > +
> > +static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
> > +				   u32 arg_a, u32 arg_b)
> > +{
> > +	u64 *cmd_ptr;
> > +
> > +	if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) {
> > +		pkt->cmd_buf_size += CMDQ_INST_SIZE;
> 
> Why do we update the cmd_buf_size here?

Because in developing phase of consumer driver, the consumer has to know
the real command buffer size after adding command failure. Then the
consumer will increase the size and run the cmdq flow (cmdq_pkt_create,
cmdq_pkt_write/wfe...) again. Finally, the consumer get the real size
and fix it.

> 
> > +		return -ENOMEM;
> > +	}
> > +	cmd_ptr = pkt->va_base + pkt->cmd_buf_size;
> > +	(*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
> > +	pkt->cmd_buf_size += CMDQ_INST_SIZE;
> > +
> > +	return 0;
> > +}

[...]

> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > new file mode 100644
> > index 0000000..fc84fe4

[...]

> > +/**
> > + * cmdq_pkt_flush() - trigger CMDQ to execute the CMDQ packet
> > + * @pkt:	the CMDQ packet
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + *
> > + * Trigger CMDQ to execute the CMDQ packet. Note that this is a
> > + * synchronous flush function. When the function returned, the recorded
> > + * commands have been done.
> > + */
> > +int cmdq_pkt_flush(struct cmdq_pkt *pkt);
> > +
> > +/**
> > + * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
> > + *                          packet and call back at the end of done packet
> > + * @pkt:	the CMDQ packet
> > + * @cb:		called at the end of done packet
> > + * @data:	this data will pass back to cb
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + *
> > + * Trigger CMDQ to asynchronously execute the CMDQ packet and call back
> > + * at the end of done packet. Note that this is an ASYNC function. When the
> > + * function returned, it may or may not be finished.
> > + */
> > +int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> > +			 void *data);
> > +
> 
> bikeshadding alarm:
> can we order the functions in the include file the same way we do in the c file?

Will adjust the functions order in mtk_cmdq.h, thanks.

> Apart from the few comments, the driver looks fine to me.
> 
> Regards,
> Matthias

Matthias Brugger Sept. 27, 2018, 7:50 a.m. UTC | #5

On 27/09/2018 03:57, houlong wei wrote:
[...]
>>> +	}
>>> +
>>> +	return client;
>>> +}
>>> +EXPORT_SYMBOL(cmdq_mbox_create);
>>> +
>>> +void cmdq_mbox_destroy(struct cmdq_client *client)
>>> +{
>>> +	if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
>>> +		spin_lock(&client->lock);
>>> +		del_timer_sync(&client->timer);
>>> +		spin_unlock(&client->lock);
>>> +	}
>>> +	mbox_free_channel(client->chan);
>>> +	kfree(client);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_mbox_destroy);
>>> +
>>> +int cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt **pkt_ptr,
>>> +		    size_t size)
>>
>> I suppose you are not returning the allocated cmdq_pkt to avoid checks for
>> IS_ERR() logic in the consumer of this API. Am I correct?
>> Do we really need a pointer to a pointer here? Can't we just use a struct
>> cmdq_pkt *pkt as argument? That would make things easier.
> 
> Do you mean we change the argument 'struct cmdq_pkt **pkt_ptr' to
> 'struct cmdq_pkt *pkt', and the consumer allocate a struct cmdq_pkt
> *pkt, then pass pkt as argument of this API?
> If yes, the consumer still need to check the return value of this API.
> For the current implementation, the consumer doesn't need check
> IS_ERR(*pkt_ptr) if the return value equals to 0.
> 
> Since the consumer has to check the return value of this API once, to
> make it simpler, I shall change the prototype to 
> 'struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t
> size)' and change its implementation.
> 

Exactly that's what I proposed. Sorry for not explaining myself :)

>>
>>> +{
>>> +	struct cmdq_pkt *pkt;
>>> +	struct device *dev;
>>> +	dma_addr_t dma_addr;
>>> +
>>> +	pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
>>> +	if (!pkt)
>>> +		return -ENOMEM;
>>> +	pkt->va_base = kzalloc(size, GFP_KERNEL);
>>> +	if (!pkt->va_base) {
>>> +		kfree(pkt);
>>> +		return -ENOMEM;
>>> +	}
>>> +	pkt->buf_size = size;
>>> +	pkt->cl = (void *)client;
>>> +
>>> +	dev = client->chan->mbox->dev;
>>> +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
>>> +		DMA_TO_DEVICE);
>>> +	if (dma_mapping_error(dev, dma_addr)) {
>>> +		dev_err(dev, "dma map failed, size=%u\n", (u32)(u64)size);
>>> +		kfree(pkt->va_base);
>>> +		kfree(pkt);
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	pkt->pa_base = dma_addr;
>>> +	*pkt_ptr = pkt;
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_create);
>>> +
>>> +void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
>>> +{
>>> +	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
>>> +
>>> +	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
>>> +			 DMA_TO_DEVICE);
>>> +	kfree(pkt->va_base);
>>> +	kfree(pkt);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_destroy);
>>> +
>>> +static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
>>> +				   u32 arg_a, u32 arg_b)
>>> +{
>>> +	u64 *cmd_ptr;
>>> +
>>> +	if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) {
>>> +		pkt->cmd_buf_size += CMDQ_INST_SIZE;
>>
>> Why do we update the cmd_buf_size here?
> 
> Because in developing phase of consumer driver, the consumer has to know
> the real command buffer size after adding command failure. Then the
> consumer will increase the size and run the cmdq flow (cmdq_pkt_create,
> cmdq_pkt_write/wfe...) again. Finally, the consumer get the real size
> and fix it.
> 

But the consumer should know the size it needs for it's buffer and if not it
should be able to decide on it's own how much space it needs. If he get's a
-ENOMEM he implicitly knows that he has to increase the buf_size. Now the size
depends on how many command he has pending and wasn't able to write to the cmdq_pkt.

Regards,
Matthias

Matthias Brugger Sept. 27, 2018, 7:55 a.m. UTC | #6

On 15/08/2018 03:46, houlong wei wrote:
[...]
>> +
>> +static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
>> +				   u32 arg_a, u32 arg_b)
>> +{
>> +	u64 *cmd_ptr;
>> +
>> +	if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) {
>> +		pkt->cmd_buf_size += CMDQ_INST_SIZE;
>> +		return -ENOMEM;
>> +	}
>> +	cmd_ptr = pkt->va_base + pkt->cmd_buf_size;
>> +	(*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
>> +	pkt->cmd_buf_size += CMDQ_INST_SIZE;
>> +
>> +	return 0;
>> +}
>> +
>> +int cmdq_pkt_write(struct cmdq_pkt *pkt, u32 value, u32 subsys, u32 offset)
>> +{
>> +	u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) |
>> +		    (subsys << CMDQ_SUBSYS_SHIFT);
>> +
>> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value);
>> +}
>> +EXPORT_SYMBOL(cmdq_pkt_write);
>> +
>> +int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u32 value,
>> +			u32 subsys, u32 offset, u32 mask)
>> +{
>> +	u32 offset_mask = offset;
>> +	int err;
>> +
>> +	if (mask != 0xffffffff) {
>> +		err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask);
>> +		WARN_ON(err < 0);

If we want to write out a warning to the kernel log, then we should but that in
the if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) from
cmdq_pkt_append_command to make it consistent between cmdq_pkt_write,
cmdq_pkt_write_mask and cmdq_pkt_finalize

Sorry, just saw this right now.

>> +		offset_mask |= CMDQ_WRITE_ENABLE_MASK;
>> +	}
>> +
>> +	return cmdq_pkt_write(pkt, value, subsys, offset_mask);
>> +}
>> +EXPORT_SYMBOL(cmdq_pkt_write_mask);
>> +
>> +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event)
>> +{
>> +	u32 arg_b;
>> +
>> +	if (event >= CMDQ_MAX_EVENT)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * WFE arg_b
>> +	 * bit 0-11: wait value
>> +	 * bit 15: 1 - wait, 0 - no wait
>> +	 * bit 16-27: update value
>> +	 * bit 31: 1 - update, 0 - no update
>> +	 */
>> +	arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
>> +
>> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b);
>> +}
>> +EXPORT_SYMBOL(cmdq_pkt_wfe);
>> +
>> +int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event)
>> +{
>> +	if (event >= CMDQ_MAX_EVENT)
>> +		return -EINVAL;
>> +
>> +	return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event,
>> +				       CMDQ_WFE_UPDATE);
>> +}
>> +EXPORT_SYMBOL(cmdq_pkt_clear_event);
>> +
>> +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>> +{
>> +	int err;
>> +
>> +	/* insert EOC and generate IRQ for each command iteration */
>> +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
>> +	WARN_ON(err < 0);

Should move into cmdq_pkt_append_command

>> +
>> +	/* JUMP to end */
>> +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
>> +	WARN_ON(err < 0);

same here.

>> +
>> +	return err;
>> +}
>> +
[...]

Regards,
Matthias

houlong.wei Sept. 27, 2018, 10:30 a.m. UTC | #7

On Thu, 2018-09-27 at 15:50 +0800, Matthias Brugger wrote:
> 
> On 27/09/2018 03:57, houlong wei wrote:
[...]
> >>> +
> >>> +static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
> >>> +				   u32 arg_a, u32 arg_b)
> >>> +{
> >>> +	u64 *cmd_ptr;
> >>> +
> >>> +	if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) {
> >>> +		pkt->cmd_buf_size += CMDQ_INST_SIZE;
> >>
> >> Why do we update the cmd_buf_size here?
> > 
> > Because in developing phase of consumer driver, the consumer has to know
> > the real command buffer size after adding command failure. Then the
> > consumer will increase the size and run the cmdq flow (cmdq_pkt_create,
> > cmdq_pkt_write/wfe...) again. Finally, the consumer get the real size
> > and fix it.
> > 
> 
> But the consumer should know the size it needs for it's buffer and if not it
> should be able to decide on it's own how much space it needs. If he get's a
> -ENOMEM he implicitly knows that he has to increase the buf_size. Now the size
> depends on how many command he has pending and wasn't able to write to the cmdq_pkt.
> 
> Regards,
> Matthias

The consumer doesn't know how to calculate the command buffer size that
he needs.
When the consumer driver is developing, he could ignore the return value
of cmdq_pkt_write and other command appending functions.
He can print the pkt->cmdq_buf_size after cmdq_pkt_flush() or
cmdq_pkt_flush_async() failure. Now he can get the buffer size he needs.

I copy your another comment here, so I can reply in one mail.
>>If we want to write out a warning to the kernel log, then we should
>>but that in the if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE
>>pkt->buf_size)) from cmdq_pkt_append_command to make it consistent
>>between cmdq_pkt_write, cmdq_pkt_write_mask and cmdq_pkt_finalize.

Thanks, I will move WARN_ON() into cmdq_pkt_append_command() before
returning -ENOMEM.

After your confirmation of the comments above, I will re-send a new
patch.

Patch

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