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

From: Horng-Shyang Liao
Date: Fri Sep 30 2016 - 04:56:56 EST

Next message: Greg KH: "Re: Linux 4.4.23"
Previous message: Horng-Shyang Liao: "Re: [PATCH v14 4/4] CMDQ: save more energy in idle"
In reply to: Horng-Shyang Liao: "Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Greg KH: "Re: Linux 4.4.23"
Previous message: Horng-Shyang Liao: "Re: [PATCH v14 4/4] CMDQ: save more energy in idle"
In reply to: Horng-Shyang Liao: "Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]