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 [off-list ref] wrote:

[...]

quoted

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

[...]

quoted

+       cmdq->irq = irq_of_parse_and_map(node, 0);

why not,  cmdq->irq = platform_get_irq(pdev, 0);

Will do

[...]

quoted

+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

quoted

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

quoted

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

quoted

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

quoted

+       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