diff mbox

[v20,2/4] mailbox: mediatek: Add Mediatek CMDQ driver

Message ID 1483499169-16329-3-git-send-email-hs.liao@mediatek.com (mailing list archive)
State New, archived
Headers show

Commit Message

hs.liao@mediatek.com Jan. 4, 2017, 3:06 a.m. UTC
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-mailbox.c       | 596 +++++++++++++++++++++++++++++++
 include/linux/mailbox/mtk-cmdq-mailbox.h |  75 ++++
 4 files changed, 683 insertions(+)
 create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
 create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h

Comments

Jassi Brar Jan. 26, 2017, 4:38 a.m. UTC | #1
On Wed, Jan 4, 2017 at 8:36 AM, HS Liao <hs.liao@mediatek.com> wrote:

> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> new file mode 100644
> index 0000000..747bcd3
> --- /dev/null
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c

...

> +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> +{
> +       struct cmdq *cmdq;
> +       struct cmdq_task *task;
> +       unsigned long curr_pa, end_pa;
> +
> +       cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> +
> +       /* Client should not flush new tasks if suspended. */
> +       WARN_ON(cmdq->suspended);
> +
> +       task = kzalloc(sizeof(*task), GFP_ATOMIC);
> +       task->cmdq = cmdq;
> +       INIT_LIST_HEAD(&task->list_entry);
> +       task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
> +                                      pkt->cmd_buf_size, DMA_TO_DEVICE);
>
You seem to parse the requests and responses, that should ideally be
done in client driver.
Also, we are here in atomic context, can you move it in client driver
(before the spin_lock)?
Maybe by adding a new 'pa_base' member as well in 'cmdq_pkt'.

....
> +
> +       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++) {
>
You mean  i < CMDQ_THR_MAX_COUNT

> +               cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
> +                               CMDQ_THR_SIZE * i;
> +               INIT_LIST_HEAD(&cmdq->thread[i].task_busy_list);
>
You seem the queue mailbox requests in this controller driver? why not
use the mailbox api for that?

> +               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];
>
Here again... you seem to ignore the polling mechanism provided by the
mailbox api, and implement your own.


> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> new file mode 100644
> index 0000000..3433c64
> --- /dev/null
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
....
> +
> +struct cmdq_pkt {
> +       void                    *va_base;
>
maybe add 'pa_base' here so you don't have to do dma_map_single() in send_data

> +       size_t                  cmd_buf_size; /* command occupied size */
> +       size_t                  buf_size; /* real buffer size */
> +       struct cmdq_task_cb     cb;
> +};
> +
> +#endif /* __MTK_CMDQ_MAILBOX_H__ */
> --
> 1.9.1
>
hs.liao@mediatek.com Jan. 26, 2017, 8:37 a.m. UTC | #2
Hi Jassi,

On Thu, 2017-01-26 at 10:08 +0530, Jassi Brar wrote:
> On Wed, Jan 4, 2017 at 8:36 AM, HS Liao <hs.liao@mediatek.com> wrote:
> 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> > new file mode 100644
> > index 0000000..747bcd3
> > --- /dev/null
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> 
> ...
> 
> > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> > +{
> > +       struct cmdq *cmdq;
> > +       struct cmdq_task *task;
> > +       unsigned long curr_pa, end_pa;
> > +
> > +       cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> > +
> > +       /* Client should not flush new tasks if suspended. */
> > +       WARN_ON(cmdq->suspended);
> > +
> > +       task = kzalloc(sizeof(*task), GFP_ATOMIC);
> > +       task->cmdq = cmdq;
> > +       INIT_LIST_HEAD(&task->list_entry);
> > +       task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
> > +                                      pkt->cmd_buf_size, DMA_TO_DEVICE);
> >
> You seem to parse the requests and responses, that should ideally be
> done in client driver.
> Also, we are here in atomic context, can you move it in client driver
> (before the spin_lock)?
> Maybe by adding a new 'pa_base' member as well in 'cmdq_pkt'.

will do

> ....
> > +
> > +       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++) {
> >
> You mean  i < CMDQ_THR_MAX_COUNT

will do

> > +               cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
> > +                               CMDQ_THR_SIZE * i;
> > +               INIT_LIST_HEAD(&cmdq->thread[i].task_busy_list);
> >
> You seem the queue mailbox requests in this controller driver? why not
> use the mailbox api for that?
> 
> > +               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];
> >
> Here again... you seem to ignore the polling mechanism provided by the
> mailbox api, and implement your own.

The queue is used to record the tasks which are flushed into CMDQ
hardware (GCE). We are handling time critical tasks, so we have to
queue them in GCE rather than a software queue (e.g. mailbox buffer).
Let me use display as an example. Many display tasks are flushed into
CMDQ to wait next vsync event. When vsync event is triggered by display
hardware, GCE needs to process all flushed tasks "within vblank" to
prevent garbage on screen. This is all done by GCE (without CPU)
to fulfill time critical requirement. After GCE finish its work,
it will generate interrupts, and then CMDQ driver will let clients know
which tasks are done.

We have discussed the similar thing before.
Please take a look at the following link,
especially from 2016/10/5 to.2016/10/11 about tx_done.
https://patchwork.kernel.org/patch/9312953/

> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > new file mode 100644
> > index 0000000..3433c64
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> ....
> > +
> > +struct cmdq_pkt {
> > +       void                    *va_base;
> >
> maybe add 'pa_base' here so you don't have to do dma_map_single() in send_data

will do

> > +       size_t                  cmd_buf_size; /* command occupied size */
> > +       size_t                  buf_size; /* real buffer size */
> > +       struct cmdq_task_cb     cb;
> > +};
> > +
> > +#endif /* __MTK_CMDQ_MAILBOX_H__ */
> > --
> > 1.9.1
> >

Thanks,
HS
Jassi Brar Feb. 1, 2017, 5:22 a.m. UTC | #3
On Thu, Jan 26, 2017 at 2:07 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> Hi Jassi,
>
> On Thu, 2017-01-26 at 10:08 +0530, Jassi Brar wrote:
>> On Wed, Jan 4, 2017 at 8:36 AM, HS Liao <hs.liao@mediatek.com> wrote:
>>
>> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
>> > new file mode 100644
>> > index 0000000..747bcd3
>> > --- /dev/null
>> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
>>
>> ...
>>
>> > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
>> > +{
>> > +       struct cmdq *cmdq;
>> > +       struct cmdq_task *task;
>> > +       unsigned long curr_pa, end_pa;
>> > +
>> > +       cmdq = dev_get_drvdata(thread->chan->mbox->dev);
>> > +
>> > +       /* Client should not flush new tasks if suspended. */
>> > +       WARN_ON(cmdq->suspended);
>> > +
>> > +       task = kzalloc(sizeof(*task), GFP_ATOMIC);
>> > +       task->cmdq = cmdq;
>> > +       INIT_LIST_HEAD(&task->list_entry);
>> > +       task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
>> > +                                      pkt->cmd_buf_size, DMA_TO_DEVICE);
>> >
>> You seem to parse the requests and responses, that should ideally be
>> done in client driver.
>> Also, we are here in atomic context, can you move it in client driver
>> (before the spin_lock)?
>> Maybe by adding a new 'pa_base' member as well in 'cmdq_pkt'.
>
> will do
>
>> ....
>> > +
>> > +       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++) {
>> >
>> You mean  i < CMDQ_THR_MAX_COUNT
>
> will do
>
>> > +               cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
>> > +                               CMDQ_THR_SIZE * i;
>> > +               INIT_LIST_HEAD(&cmdq->thread[i].task_busy_list);
>> >
>> You seem the queue mailbox requests in this controller driver? why not
>> use the mailbox api for that?
>>
>> > +               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];
>> >
>> Here again... you seem to ignore the polling mechanism provided by the
>> mailbox api, and implement your own.
>
> The queue is used to record the tasks which are flushed into CMDQ
> hardware (GCE). We are handling time critical tasks, so we have to
> queue them in GCE rather than a software queue (e.g. mailbox buffer).
> Let me use display as an example. Many display tasks are flushed into
> CMDQ to wait next vsync event. When vsync event is triggered by display
> hardware, GCE needs to process all flushed tasks "within vblank" to
> prevent garbage on screen. This is all done by GCE (without CPU)
> to fulfill time critical requirement. After GCE finish its work,
> it will generate interrupts, and then CMDQ driver will let clients know
> which tasks are done.
>
Does the GCE provide any 'lock' to prevent modifying (by adding tasks
to) the GCE h/w buffer when it is processing it at vsync?  Otherwise
there maybe race/error. If there is such a 'lock' flag/irq, that could
help here. However, you are supposed to know your h/w better, so I
will accept this implementation assuming it can't be done any better.

Please address other comments and resubmit.

Thanks
hs.liao@mediatek.com Feb. 6, 2017, 5:37 a.m. UTC | #4
Hi Jassi,

On Wed, 2017-02-01 at 10:52 +0530, Jassi Brar wrote:
> On Thu, Jan 26, 2017 at 2:07 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > Hi Jassi,
> >
> > On Thu, 2017-01-26 at 10:08 +0530, Jassi Brar wrote:
> >> On Wed, Jan 4, 2017 at 8:36 AM, HS Liao <hs.liao@mediatek.com> wrote:
> >>
> >> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> >> > new file mode 100644
> >> > index 0000000..747bcd3
> >> > --- /dev/null
> >> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> >>
> >> ...
> >>
> >> > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> >> > +{
> >> > +       struct cmdq *cmdq;
> >> > +       struct cmdq_task *task;
> >> > +       unsigned long curr_pa, end_pa;
> >> > +
> >> > +       cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> >> > +
> >> > +       /* Client should not flush new tasks if suspended. */
> >> > +       WARN_ON(cmdq->suspended);
> >> > +
> >> > +       task = kzalloc(sizeof(*task), GFP_ATOMIC);
> >> > +       task->cmdq = cmdq;
> >> > +       INIT_LIST_HEAD(&task->list_entry);
> >> > +       task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
> >> > +                                      pkt->cmd_buf_size, DMA_TO_DEVICE);
> >> >
> >> You seem to parse the requests and responses, that should ideally be
> >> done in client driver.
> >> Also, we are here in atomic context, can you move it in client driver
> >> (before the spin_lock)?
> >> Maybe by adding a new 'pa_base' member as well in 'cmdq_pkt'.
> >
> > will do

I agree with moving dma_map_single out from spin_lock.

However, mailbox clients cannot map virtual memory to mailbox
controller's device for DMA. In our previous discussion, we decided to
remove mailbox_controller.h from clients to restrict their capabilities.

Please take a look at following link from 2016/9/22 to 2016/9/30 about
mailbox_controller.h.
https://patchwork.kernel.org/patch/9312953/

Is there any better place to do dma_map_single?

> >> ....
> >> > +
> >> > +       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++) {
> >> >
> >> You mean  i < CMDQ_THR_MAX_COUNT
> >
> > will do
> >
> >> > +               cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
> >> > +                               CMDQ_THR_SIZE * i;
> >> > +               INIT_LIST_HEAD(&cmdq->thread[i].task_busy_list);
> >> >
> >> You seem the queue mailbox requests in this controller driver? why not
> >> use the mailbox api for that?
> >>
> >> > +               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];
> >> >
> >> Here again... you seem to ignore the polling mechanism provided by the
> >> mailbox api, and implement your own.
> >
> > The queue is used to record the tasks which are flushed into CMDQ
> > hardware (GCE). We are handling time critical tasks, so we have to
> > queue them in GCE rather than a software queue (e.g. mailbox buffer).
> > Let me use display as an example. Many display tasks are flushed into
> > CMDQ to wait next vsync event. When vsync event is triggered by display
> > hardware, GCE needs to process all flushed tasks "within vblank" to
> > prevent garbage on screen. This is all done by GCE (without CPU)
> > to fulfill time critical requirement. After GCE finish its work,
> > it will generate interrupts, and then CMDQ driver will let clients know
> > which tasks are done.
> >
> Does the GCE provide any 'lock' to prevent modifying (by adding tasks
> to) the GCE h/w buffer when it is processing it at vsync?  Otherwise

CPU will suspend GCE when adding a task (cmdq_thread_suspend),
and resume GCE after adding task is done (cmdq_thread_resume).
If GCE is processing task(s) at vsync and CPU wants to add a new task
at the same time, CPU will detect this situation
(by cmdq_thread_is_in_wfe), resume GCE immediately, and then add
following task(s) to wait for next vsync event.
All the above logic is implemented at cmdq_task_exec.

> there maybe race/error. If there is such a 'lock' flag/irq, that could
> help here. However, you are supposed to know your h/w better, so I
> will accept this implementation assuming it can't be done any better.
> 
> Please address other comments and resubmit.
> 
> Thanks

After we figure out a better solution for dma_map_single issue, I will
resubmit a new version.

Thanks,
HS
hs.liao@mediatek.com Feb. 9, 2017, 12:03 p.m. UTC | #5
On Mon, 2017-02-06 at 13:37 +0800, Horng-Shyang Liao wrote:
> Hi Jassi,
> 
> On Wed, 2017-02-01 at 10:52 +0530, Jassi Brar wrote:
> > On Thu, Jan 26, 2017 at 2:07 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > > Hi Jassi,
> > >
> > > On Thu, 2017-01-26 at 10:08 +0530, Jassi Brar wrote:
> > >> On Wed, Jan 4, 2017 at 8:36 AM, HS Liao <hs.liao@mediatek.com> wrote:
> > >>
> > >> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> > >> > new file mode 100644
> > >> > index 0000000..747bcd3
> > >> > --- /dev/null
> > >> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > >>
> > >> ...
> > >>
> > >> > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> > >> > +{
> > >> > +       struct cmdq *cmdq;
> > >> > +       struct cmdq_task *task;
> > >> > +       unsigned long curr_pa, end_pa;
> > >> > +
> > >> > +       cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> > >> > +
> > >> > +       /* Client should not flush new tasks if suspended. */
> > >> > +       WARN_ON(cmdq->suspended);
> > >> > +
> > >> > +       task = kzalloc(sizeof(*task), GFP_ATOMIC);
> > >> > +       task->cmdq = cmdq;
> > >> > +       INIT_LIST_HEAD(&task->list_entry);
> > >> > +       task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
> > >> > +                                      pkt->cmd_buf_size, DMA_TO_DEVICE);
> > >> >
> > >> You seem to parse the requests and responses, that should ideally be
> > >> done in client driver.
> > >> Also, we are here in atomic context, can you move it in client driver
> > >> (before the spin_lock)?
> > >> Maybe by adding a new 'pa_base' member as well in 'cmdq_pkt'.
> > >
> > > will do
> 
> I agree with moving dma_map_single out from spin_lock.
> 
> However, mailbox clients cannot map virtual memory to mailbox
> controller's device for DMA. In our previous discussion, we decided to
> remove mailbox_controller.h from clients to restrict their capabilities.
> 
> Please take a look at following link from 2016/9/22 to 2016/9/30 about
> mailbox_controller.h.
> https://patchwork.kernel.org/patch/9312953/
> 
> Is there any better place to do dma_map_single?

Hi Jassi,

According to previous discussion, we have two requirements:
(1) CMDQ clients should not access mailbox_controller.h;
(2) dma_map_single should not put inside spin_lock.

I think a trade-off solution is to put in mtk-cmdq-helper.c.
Although it is a mailbox client, it is not a CMDQ client.
We can include mailbox_controller.h in mtk-cmdq-helper.c
(instead of mtk-cmdq.h), and then map dma at cmdq_pkt_flush_async
before mbox_send_message.

pkt->pa_base = dma_map_single(client->chan->mbox->dev, pkt->va_base,
                              pkt->cmd_buf_size, DMA_TO_DEVICE);

What do you think?

Thanks,
HS

> > >> ....
> > >> > +
> > >> > +       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++) {
> > >> >
> > >> You mean  i < CMDQ_THR_MAX_COUNT
> > >
> > > will do
> > >
> > >> > +               cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
> > >> > +                               CMDQ_THR_SIZE * i;
> > >> > +               INIT_LIST_HEAD(&cmdq->thread[i].task_busy_list);
> > >> >
> > >> You seem the queue mailbox requests in this controller driver? why not
> > >> use the mailbox api for that?
> > >>
> > >> > +               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];
> > >> >
> > >> Here again... you seem to ignore the polling mechanism provided by the
> > >> mailbox api, and implement your own.
> > >
> > > The queue is used to record the tasks which are flushed into CMDQ
> > > hardware (GCE). We are handling time critical tasks, so we have to
> > > queue them in GCE rather than a software queue (e.g. mailbox buffer).
> > > Let me use display as an example. Many display tasks are flushed into
> > > CMDQ to wait next vsync event. When vsync event is triggered by display
> > > hardware, GCE needs to process all flushed tasks "within vblank" to
> > > prevent garbage on screen. This is all done by GCE (without CPU)
> > > to fulfill time critical requirement. After GCE finish its work,
> > > it will generate interrupts, and then CMDQ driver will let clients know
> > > which tasks are done.
> > >
> > Does the GCE provide any 'lock' to prevent modifying (by adding tasks
> > to) the GCE h/w buffer when it is processing it at vsync?  Otherwise
> 
> CPU will suspend GCE when adding a task (cmdq_thread_suspend),
> and resume GCE after adding task is done (cmdq_thread_resume).
> If GCE is processing task(s) at vsync and CPU wants to add a new task
> at the same time, CPU will detect this situation
> (by cmdq_thread_is_in_wfe), resume GCE immediately, and then add
> following task(s) to wait for next vsync event.
> All the above logic is implemented at cmdq_task_exec.
> 
> > there maybe race/error. If there is such a 'lock' flag/irq, that could
> > help here. However, you are supposed to know your h/w better, so I
> > will accept this implementation assuming it can't be done any better.
> > 
> > Please address other comments and resubmit.
> > 
> > Thanks
> 
> After we figure out a better solution for dma_map_single issue, I will
> resubmit a new version.
> 
> Thanks,
> HS
Jassi Brar Feb. 16, 2017, 3:32 p.m. UTC | #6
On Mon, Feb 6, 2017 at 11:07 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> Hi Jassi,
>
> On Wed, 2017-02-01 at 10:52 +0530, Jassi Brar wrote:
>> On Thu, Jan 26, 2017 at 2:07 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
>> > Hi Jassi,
>> >
>> > On Thu, 2017-01-26 at 10:08 +0530, Jassi Brar wrote:
>> >> On Wed, Jan 4, 2017 at 8:36 AM, HS Liao <hs.liao@mediatek.com> wrote:
>> >>
>> >> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
>> >> > new file mode 100644
>> >> > index 0000000..747bcd3
>> >> > --- /dev/null
>> >> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
>> >>
>> >> ...
>> >>
>> >> > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
>> >> > +{
>> >> > +       struct cmdq *cmdq;
>> >> > +       struct cmdq_task *task;
>> >> > +       unsigned long curr_pa, end_pa;
>> >> > +
>> >> > +       cmdq = dev_get_drvdata(thread->chan->mbox->dev);
>> >> > +
>> >> > +       /* Client should not flush new tasks if suspended. */
>> >> > +       WARN_ON(cmdq->suspended);
>> >> > +
>> >> > +       task = kzalloc(sizeof(*task), GFP_ATOMIC);
>> >> > +       task->cmdq = cmdq;
>> >> > +       INIT_LIST_HEAD(&task->list_entry);
>> >> > +       task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
>> >> > +                                      pkt->cmd_buf_size, DMA_TO_DEVICE);
>> >> >
>> >> You seem to parse the requests and responses, that should ideally be
>> >> done in client driver.
>> >> Also, we are here in atomic context, can you move it in client driver
>> >> (before the spin_lock)?
>> >> Maybe by adding a new 'pa_base' member as well in 'cmdq_pkt'.
>> >
>> > will do
>
> I agree with moving dma_map_single out from spin_lock.
>
> However, mailbox clients cannot map virtual memory to mailbox
> controller's device for DMA.
>
If DMA is a resource used by MBox to transfer data, then yes the
mapping needs to be done in the Mbox controller driver. To map memory
outside of spinlock, you could schedule a tasklet in send_data() ?
hs.liao@mediatek.com Feb. 22, 2017, 3:12 a.m. UTC | #7
On Thu, 2017-02-16 at 21:02 +0530, Jassi Brar wrote:
> On Mon, Feb 6, 2017 at 11:07 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > Hi Jassi,
> >
> > On Wed, 2017-02-01 at 10:52 +0530, Jassi Brar wrote:
> >> On Thu, Jan 26, 2017 at 2:07 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> >> > Hi Jassi,
> >> >
> >> > On Thu, 2017-01-26 at 10:08 +0530, Jassi Brar wrote:
> >> >> On Wed, Jan 4, 2017 at 8:36 AM, HS Liao <hs.liao@mediatek.com> wrote:
> >> >>
> >> >> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> >> >> > new file mode 100644
> >> >> > index 0000000..747bcd3
> >> >> > --- /dev/null
> >> >> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> >> >>
> >> >> ...
> >> >>
> >> >> > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> >> >> > +{
> >> >> > +       struct cmdq *cmdq;
> >> >> > +       struct cmdq_task *task;
> >> >> > +       unsigned long curr_pa, end_pa;
> >> >> > +
> >> >> > +       cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> >> >> > +
> >> >> > +       /* Client should not flush new tasks if suspended. */
> >> >> > +       WARN_ON(cmdq->suspended);
> >> >> > +
> >> >> > +       task = kzalloc(sizeof(*task), GFP_ATOMIC);
> >> >> > +       task->cmdq = cmdq;
> >> >> > +       INIT_LIST_HEAD(&task->list_entry);
> >> >> > +       task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
> >> >> > +                                      pkt->cmd_buf_size, DMA_TO_DEVICE);
> >> >> >
> >> >> You seem to parse the requests and responses, that should ideally be
> >> >> done in client driver.
> >> >> Also, we are here in atomic context, can you move it in client driver
> >> >> (before the spin_lock)?
> >> >> Maybe by adding a new 'pa_base' member as well in 'cmdq_pkt'.
> >> >
> >> > will do
> >
> > I agree with moving dma_map_single out from spin_lock.
> >
> > However, mailbox clients cannot map virtual memory to mailbox
> > controller's device for DMA.
> >
> If DMA is a resource used by MBox to transfer data, then yes the
> mapping needs to be done in the Mbox controller driver. To map memory
> outside of spinlock, you could schedule a tasklet in send_data() ?

Hi Jassi,

For CMDQ, the order of CMDQ tasks should be guaranteed.
However, it seems tasklet cannot ensure this requirement.

Quote from Linux Device Drivers 3rd edition ch7.
"void tasklet_schedule(struct tasklet_struct *t);
  Schedule the tasklet for execution. If a tasklet is scheduled again
before it has a chance to run, it runs only once...."

May I use workqueue instead of tasklet?

Thanks,
HS
Jassi Brar Feb. 23, 2017, 4:10 a.m. UTC | #8
On Wed, Feb 22, 2017 at 8:42 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> On Thu, 2017-02-16 at 21:02 +0530, Jassi Brar wrote:
>> On Mon, Feb 6, 2017 at 11:07 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
>> > Hi Jassi,
>> >
>> > On Wed, 2017-02-01 at 10:52 +0530, Jassi Brar wrote:
>> >> On Thu, Jan 26, 2017 at 2:07 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
>> >> > Hi Jassi,
>> >> >
>> >> > On Thu, 2017-01-26 at 10:08 +0530, Jassi Brar wrote:
>> >> >> On Wed, Jan 4, 2017 at 8:36 AM, HS Liao <hs.liao@mediatek.com> wrote:
>> >> >>
>> >> >> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
>> >> >> > new file mode 100644
>> >> >> > index 0000000..747bcd3
>> >> >> > --- /dev/null
>> >> >> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
>> >> >>
>> >> >> ...
>> >> >>
>> >> >> > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
>> >> >> > +{
>> >> >> > +       struct cmdq *cmdq;
>> >> >> > +       struct cmdq_task *task;
>> >> >> > +       unsigned long curr_pa, end_pa;
>> >> >> > +
>> >> >> > +       cmdq = dev_get_drvdata(thread->chan->mbox->dev);
>> >> >> > +
>> >> >> > +       /* Client should not flush new tasks if suspended. */
>> >> >> > +       WARN_ON(cmdq->suspended);
>> >> >> > +
>> >> >> > +       task = kzalloc(sizeof(*task), GFP_ATOMIC);
>> >> >> > +       task->cmdq = cmdq;
>> >> >> > +       INIT_LIST_HEAD(&task->list_entry);
>> >> >> > +       task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
>> >> >> > +                                      pkt->cmd_buf_size, DMA_TO_DEVICE);
>> >> >> >
>> >> >> You seem to parse the requests and responses, that should ideally be
>> >> >> done in client driver.
>> >> >> Also, we are here in atomic context, can you move it in client driver
>> >> >> (before the spin_lock)?
>> >> >> Maybe by adding a new 'pa_base' member as well in 'cmdq_pkt'.
>> >> >
>> >> > will do
>> >
>> > I agree with moving dma_map_single out from spin_lock.
>> >
>> > However, mailbox clients cannot map virtual memory to mailbox
>> > controller's device for DMA.
>> >
>> If DMA is a resource used by MBox to transfer data, then yes the
>> mapping needs to be done in the Mbox controller driver. To map memory
>> outside of spinlock, you could schedule a tasklet in send_data() ?
>
> Hi Jassi,
>
> For CMDQ, the order of CMDQ tasks should be guaranteed.
> However, it seems tasklet cannot ensure this requirement.
>
> Quote from Linux Device Drivers 3rd edition ch7.
> "void tasklet_schedule(struct tasklet_struct *t);
>   Schedule the tasklet for execution. If a tasklet is scheduled again
> before it has a chance to run, it runs only once...."
>
Not sure what bothers you.
If you only add requests to a list, protected by some spinlock, during
send_datam you could always iterate over (submit) requests in the
order you queued them.
hs.liao@mediatek.com Feb. 23, 2017, 12:48 p.m. UTC | #9
On Thu, 2017-02-23 at 09:40 +0530, Jassi Brar wrote:
> On Wed, Feb 22, 2017 at 8:42 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > On Thu, 2017-02-16 at 21:02 +0530, Jassi Brar wrote:
> >> On Mon, Feb 6, 2017 at 11:07 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> >> > Hi Jassi,
> >> >
> >> > On Wed, 2017-02-01 at 10:52 +0530, Jassi Brar wrote:
> >> >> On Thu, Jan 26, 2017 at 2:07 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> >> >> > Hi Jassi,
> >> >> >
> >> >> > On Thu, 2017-01-26 at 10:08 +0530, Jassi Brar wrote:
> >> >> >> On Wed, Jan 4, 2017 at 8:36 AM, HS Liao <hs.liao@mediatek.com> wrote:
> >> >> >>
> >> >> >> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> >> >> >> > new file mode 100644
> >> >> >> > index 0000000..747bcd3
> >> >> >> > --- /dev/null
> >> >> >> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> >> >> >>
> >> >> >> ...
> >> >> >>
> >> >> >> > +static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
> >> >> >> > +{
> >> >> >> > +       struct cmdq *cmdq;
> >> >> >> > +       struct cmdq_task *task;
> >> >> >> > +       unsigned long curr_pa, end_pa;
> >> >> >> > +
> >> >> >> > +       cmdq = dev_get_drvdata(thread->chan->mbox->dev);
> >> >> >> > +
> >> >> >> > +       /* Client should not flush new tasks if suspended. */
> >> >> >> > +       WARN_ON(cmdq->suspended);
> >> >> >> > +
> >> >> >> > +       task = kzalloc(sizeof(*task), GFP_ATOMIC);
> >> >> >> > +       task->cmdq = cmdq;
> >> >> >> > +       INIT_LIST_HEAD(&task->list_entry);
> >> >> >> > +       task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
> >> >> >> > +                                      pkt->cmd_buf_size, DMA_TO_DEVICE);
> >> >> >> >
> >> >> >> You seem to parse the requests and responses, that should ideally be
> >> >> >> done in client driver.
> >> >> >> Also, we are here in atomic context, can you move it in client driver
> >> >> >> (before the spin_lock)?
> >> >> >> Maybe by adding a new 'pa_base' member as well in 'cmdq_pkt'.
> >> >> >
> >> >> > will do
> >> >
> >> > I agree with moving dma_map_single out from spin_lock.
> >> >
> >> > However, mailbox clients cannot map virtual memory to mailbox
> >> > controller's device for DMA.
> >> >
> >> If DMA is a resource used by MBox to transfer data, then yes the
> >> mapping needs to be done in the Mbox controller driver. To map memory
> >> outside of spinlock, you could schedule a tasklet in send_data() ?
> >
> > Hi Jassi,
> >
> > For CMDQ, the order of CMDQ tasks should be guaranteed.
> > However, it seems tasklet cannot ensure this requirement.
> >
> > Quote from Linux Device Drivers 3rd edition ch7.
> > "void tasklet_schedule(struct tasklet_struct *t);
> >   Schedule the tasklet for execution. If a tasklet is scheduled again
> > before it has a chance to run, it runs only once...."
> >
> Not sure what bothers you.
> If you only add requests to a list, protected by some spinlock, during
> send_datam you could always iterate over (submit) requests in the
> order you queued them.

Hi Jassi,

OK. I will do it.

Thanks,
HS
houlong.wei Jan. 18, 2018, 8:31 a.m. UTC | #10
Hi Jassi,

There is one request for one GCE h/w buffer which contains a list of
registers operation.
I will resubmit a version and please review again.

Thanks,
Houlong

On Thu, 2018-01-18 at 16:01 +0800, Jassi Brar wrote:
> On Mon, Jan 8, 2018 at 2:08 PM, houlong wei <houlong.wei@mediatek.com> wrote:
> > Hi Jassi,
> >
> > Sorry for reply so late.
> > According to previous discussion, there are two methods to move
> > dma_map_single() outside of spin_lock.
> > (1) put in mtk-cmdq-helper.c, as described by HS on 2017-02-09.
> >   > I think a trade-off solution is to put in mtk-cmdq-helper.c.
> >   > Although it is a mailbox client, it is not a CMDQ client.
> >   > We can include mailbox_controller.h in mtk-cmdq-helper.c (instead of
> > mtk-cmdq.h), and then map dma at cmdq_pkt_flush_async before
> > mbox_send_message.
> >
> >   > pkt->pa_base = dma_map_single(client->chan->mbox->dev, pkt->va_base,
> >   >                              pkt->cmd_buf_size, DMA_TO_DEVICE);
> > (2) schedule a tasklet in send_data().
> >
> > After internal discussion with HS and other experts, now we prefer
> > method (1).
> > How do you think about it?
> >
> I don't exactly see how you mean but please remember send_data()
> callback is supposed to be atomic ... it is protected by
> spin_lock_irqsave/restore in drivers/mailbox/mailbox.c:msg_submit()
> 
> BTW, how many requests max can be queued in the GCE h/w buffer?
> And since it's been over a year now, could you please resubmit after
> checking for checkpatch with the --strict option?
> 
> Thanks.
diff mbox

Patch

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ceff415..9108dd4 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -152,4 +152,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_MBOX
+	bool "MediaTek CMDQ Mailbox Support"
+	depends on ARM64 && ( ARCH_MEDIATEK || COMPILE_TEST )
+	select MTK_INFRACFG
+	help
+	  Say yes here to add support for the MediaTek Command Queue (CMDQ)
+	  mailbox driver. The CMDQ is used to help read/write registers with
+	  critical time limitation, such as updating display configuration
+	  during the vblank.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 7dde4f6..fad8965 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -31,3 +31,5 @@  obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
+
+obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
new file mode 100644
index 0000000..747bcd3
--- /dev/null
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -0,0 +1,596 @@ 
+/*
+ * 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/dma-mapping.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/mtk-cmdq-mailbox.h>
+#include <linux/timer.h>
+
+#define CMDQ_THR_MAX_COUNT		3 /* main, sub, general(misc) */
+#define CMDQ_OP_CODE_MASK		(0xff << CMDQ_OP_CODE_SHIFT)
+#define CMDQ_TIMEOUT_MS			1000
+#define CMDQ_IRQ_MASK			0xffff
+#define CMDQ_NUM_CMD(t)			(t->cmd_buf_size / CMDQ_INST_SIZE)
+
+#define CMDQ_CURR_IRQ_STATUS		0x10
+#define CMDQ_THR_SLOT_CYCLES		0x30
+
+#define CMDQ_THR_BASE			0x100
+#define CMDQ_THR_SIZE			0x80
+#define CMDQ_THR_WARM_RESET		0x00
+#define CMDQ_THR_ENABLE_TASK		0x04
+#define CMDQ_THR_SUSPEND_TASK		0x08
+#define CMDQ_THR_CURR_STATUS		0x0c
+#define CMDQ_THR_IRQ_STATUS		0x10
+#define CMDQ_THR_IRQ_ENABLE		0x14
+#define CMDQ_THR_CURR_ADDR		0x20
+#define CMDQ_THR_END_ADDR		0x24
+#define CMDQ_THR_WAIT_TOKEN		0x30
+
+#define CMDQ_THR_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_JUMP_BY_OFFSET		0x10000000
+#define CMDQ_JUMP_BY_PA			0x10000001
+
+struct cmdq_thread {
+	struct mbox_chan	*chan;
+	void __iomem		*base;
+	struct list_head	task_busy_list;
+	struct timer_list	timeout;
+	bool			atomic_exec;
+};
+
+struct cmdq_task {
+	struct cmdq		*cmdq;
+	struct list_head	list_entry;
+	dma_addr_t		pa_base;
+	struct cmdq_thread	*thread;
+	struct cmdq_pkt		*pkt; /* the packet sent from mailbox client */
+};
+
+struct cmdq {
+	struct mbox_controller	mbox;
+	void __iomem		*base;
+	u32			irq;
+	struct cmdq_thread	thread[CMDQ_THR_MAX_COUNT];
+	struct clk		*clock;
+	bool			suspended;
+};
+
+static int cmdq_thread_suspend(struct cmdq *cmdq, struct cmdq_thread *thread)
+{
+	u32 status;
+
+	writel(CMDQ_THR_SUSPEND, thread->base + CMDQ_THR_SUSPEND_TASK);
+
+	/* If already disabled, treat as suspended successful. */
+	if (!(readl(thread->base + CMDQ_THR_ENABLE_TASK) & CMDQ_THR_ENABLED))
+		return 0;
+
+	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_CURR_STATUS,
+			status, status & CMDQ_THR_STATUS_SUSPENDED, 0, 10)) {
+		dev_err(cmdq->mbox.dev, "suspend GCE thread 0x%x failed\n",
+			(u32)(thread->base - cmdq->base));
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
+static void cmdq_thread_resume(struct cmdq_thread *thread)
+{
+	writel(CMDQ_THR_RESUME, thread->base + CMDQ_THR_SUSPEND_TASK);
+}
+
+static int cmdq_thread_reset(struct cmdq *cmdq, struct cmdq_thread *thread)
+{
+	u32 warm_reset;
+
+	writel(CMDQ_THR_DO_WARM_RESET, thread->base + CMDQ_THR_WARM_RESET);
+	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_WARM_RESET,
+			warm_reset, !(warm_reset & CMDQ_THR_DO_WARM_RESET),
+			0, 10)) {
+		dev_err(cmdq->mbox.dev, "reset GCE thread 0x%x failed\n",
+			(u32)(thread->base - cmdq->base));
+		return -EFAULT;
+	}
+	writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base + CMDQ_THR_SLOT_CYCLES);
+	return 0;
+}
+
+static void cmdq_thread_disable(struct cmdq *cmdq, struct cmdq_thread *thread)
+{
+	cmdq_thread_reset(cmdq, thread);
+	writel(CMDQ_THR_DISABLED, thread->base + CMDQ_THR_ENABLE_TASK);
+}
+
+/* notify GCE to re-fetch commands by setting GCE thread PC */
+static void cmdq_thread_invalidate_fetched_data(struct cmdq_thread *thread)
+{
+	writel(readl(thread->base + CMDQ_THR_CURR_ADDR),
+	       thread->base + CMDQ_THR_CURR_ADDR);
+}
+
+static void cmdq_task_insert_into_thread(struct cmdq_task *task)
+{
+	struct device *dev = task->cmdq->mbox.dev;
+	struct cmdq_thread *thread = task->thread;
+	struct cmdq_task *prev_task = list_last_entry(
+			&thread->task_busy_list, typeof(*task), list_entry);
+	u64 *prev_task_base = prev_task->pkt->va_base;
+
+	/* let previous task jump to this task */
+	dma_sync_single_for_cpu(dev, prev_task->pa_base,
+				prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
+	prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] =
+		(u64)CMDQ_JUMP_BY_PA << 32 | task->pa_base;
+	dma_sync_single_for_device(dev, prev_task->pa_base,
+				   prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
+
+	cmdq_thread_invalidate_fetched_data(thread);
+}
+
+static bool cmdq_command_is_wfe(u64 cmd)
+{
+	u64 wfe_option = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
+	u64 wfe_op = (u64)(CMDQ_CODE_WFE << CMDQ_OP_CODE_SHIFT) << 32;
+	u64 wfe_mask = (u64)CMDQ_OP_CODE_MASK << 32 | 0xffffffff;
+
+	return ((cmd & wfe_mask) == (wfe_op | wfe_option));
+}
+
+/* we assume tasks in the same display GCE thread are waiting the same event. */
+static void cmdq_task_remove_wfe(struct cmdq_task *task)
+{
+	struct device *dev = task->cmdq->mbox.dev;
+	u64 *base = task->pkt->va_base;
+	int i;
+
+	dma_sync_single_for_cpu(dev, task->pa_base, task->pkt->cmd_buf_size,
+				DMA_TO_DEVICE);
+	for (i = 0; i < CMDQ_NUM_CMD(task->pkt); i++)
+		if (cmdq_command_is_wfe(base[i]))
+			base[i] = (u64)CMDQ_JUMP_BY_OFFSET << 32 |
+				  CMDQ_JUMP_PASS;
+	dma_sync_single_for_device(dev, task->pa_base, task->pkt->cmd_buf_size,
+				   DMA_TO_DEVICE);
+}
+
+static bool cmdq_thread_is_in_wfe(struct cmdq_thread *thread)
+{
+	return readl(thread->base + CMDQ_THR_WAIT_TOKEN) & CMDQ_THR_IS_WAITING;
+}
+
+static void cmdq_thread_wait_end(struct cmdq_thread *thread,
+				 unsigned long end_pa)
+{
+	struct device *dev = thread->chan->mbox->dev;
+	unsigned long curr_pa;
+
+	if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_CURR_ADDR,
+			curr_pa, curr_pa == end_pa, 1, 20))
+		dev_err(dev, "GCE thread cannot run to end.\n");
+}
+
+static void cmdq_task_exec(struct cmdq_pkt *pkt, struct cmdq_thread *thread)
+{
+	struct cmdq *cmdq;
+	struct cmdq_task *task;
+	unsigned long curr_pa, end_pa;
+
+	cmdq = dev_get_drvdata(thread->chan->mbox->dev);
+
+	/* Client should not flush new tasks if suspended. */
+	WARN_ON(cmdq->suspended);
+
+	task = kzalloc(sizeof(*task), GFP_ATOMIC);
+	task->cmdq = cmdq;
+	INIT_LIST_HEAD(&task->list_entry);
+	task->pa_base = dma_map_single(cmdq->mbox.dev, pkt->va_base,
+				       pkt->cmd_buf_size, DMA_TO_DEVICE);
+	task->thread = thread;
+	task->pkt = pkt;
+
+	if (list_empty(&thread->task_busy_list)) {
+		WARN_ON(clk_enable(cmdq->clock) < 0);
+		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
+
+		writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
+		writel(task->pa_base + pkt->cmd_buf_size,
+		       thread->base + CMDQ_THR_END_ADDR);
+		writel(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 + pkt->cmd_buf_size,
+		       thread->base + CMDQ_THR_END_ADDR);
+		cmdq_thread_resume(thread);
+	}
+	list_move_tail(&task->list_entry, &thread->task_busy_list);
+}
+
+static void cmdq_task_exec_done(struct cmdq_task *task, bool err)
+{
+	struct device *dev = task->cmdq->mbox.dev;
+	struct cmdq_cb_data cmdq_cb_data;
+
+	dma_unmap_single(dev, task->pa_base, task->pkt->cmd_buf_size,
+			 DMA_TO_DEVICE);
+	if (task->pkt->cb.cb) {
+		cmdq_cb_data.err = err;
+		cmdq_cb_data.data = task->pkt->cb.data;
+		task->pkt->cb.cb(cmdq_cb_data);
+	}
+	list_del(&task->list_entry);
+}
+
+static void cmdq_task_handle_error(struct cmdq_task *task)
+{
+	struct cmdq_thread *thread = task->thread;
+	struct cmdq_task *next_task;
+
+	dev_err(task->cmdq->mbox.dev, "task 0x%p error\n", task);
+	WARN_ON(cmdq_thread_suspend(task->cmdq, thread) < 0);
+	next_task = list_first_entry_or_null(&thread->task_busy_list,
+			struct cmdq_task, list_entry);
+	if (next_task)
+		writel(next_task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
+	cmdq_thread_resume(thread);
+}
+
+static void cmdq_thread_irq_handler(struct cmdq *cmdq,
+				    struct cmdq_thread *thread)
+{
+	struct cmdq_task *task, *tmp, *curr_task = NULL;
+	u32 curr_pa, irq_flag, task_end_pa;
+	bool err;
+
+	irq_flag = readl(thread->base + CMDQ_THR_IRQ_STATUS);
+	writel(~irq_flag, thread->base + CMDQ_THR_IRQ_STATUS);
+
+	/*
+	 * When ISR call this function, another CPU core could run
+	 * "release task" right before we acquire the spin lock, and thus
+	 * reset / disable this GCE thread, so we need to check the enable
+	 * bit of this GCE thread.
+	 */
+	if (!(readl(thread->base + CMDQ_THR_ENABLE_TASK) & CMDQ_THR_ENABLED))
+		return;
+
+	if (irq_flag & CMDQ_THR_IRQ_ERROR)
+		err = true;
+	else if (irq_flag & CMDQ_THR_IRQ_DONE)
+		err = false;
+	else
+		return;
+
+	curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR);
+
+	list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
+				 list_entry) {
+		task_end_pa = task->pa_base + task->pkt->cmd_buf_size;
+		if (curr_pa >= task->pa_base && curr_pa < task_end_pa)
+			curr_task = task;
+
+		if (!curr_task || curr_pa == task_end_pa - CMDQ_INST_SIZE) {
+			cmdq_task_exec_done(task, false);
+			kfree(task);
+		} else if (err) {
+			cmdq_task_exec_done(task, true);
+			cmdq_task_handle_error(curr_task);
+			kfree(task);
+		}
+
+		if (curr_task)
+			break;
+	}
+
+	if (list_empty(&thread->task_busy_list)) {
+		cmdq_thread_disable(cmdq, thread);
+		clk_disable(cmdq->clock);
+	} else {
+		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_suspend(struct device *dev)
+{
+	struct cmdq *cmdq = dev_get_drvdata(dev);
+	struct cmdq_thread *thread;
+	int i;
+	bool task_running = false;
+
+	cmdq->suspended = true;
+
+	for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
+		thread = &cmdq->thread[i];
+		if (!list_empty(&thread->task_busy_list)) {
+			task_running = true;
+			break;
+		}
+	}
+
+	if (task_running)
+		dev_warn(dev, "exist running task(s) in suspend\n");
+
+	clk_unprepare(cmdq->clock);
+	return 0;
+}
+
+static int cmdq_resume(struct device *dev)
+{
+	struct cmdq *cmdq = dev_get_drvdata(dev);
+
+	WARN_ON(clk_prepare(cmdq->clock) < 0);
+	cmdq->suspended = false;
+	return 0;
+}
+
+static int cmdq_remove(struct platform_device *pdev)
+{
+	struct cmdq *cmdq = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&cmdq->mbox);
+	clk_unprepare(cmdq->clock);
+	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 resource *res;
+	struct cmdq *cmdq;
+	int err, i;
+
+	cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
+	if (!cmdq)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cmdq->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(cmdq->base)) {
+		dev_err(dev, "failed to ioremap gce\n");
+		return PTR_ERR(cmdq->base);
+	}
+
+	cmdq->irq = platform_get_irq(pdev, 0);
+	if (!cmdq->irq) {
+		dev_err(dev, "failed to get irq\n");
+		return -EINVAL;
+	}
+	err = devm_request_irq(dev, cmdq->irq, cmdq_irq_handler, IRQF_SHARED,
+			       "mtk_cmdq", cmdq);
+	if (err < 0) {
+		dev_err(dev, "failed to register ISR (%d)\n", err);
+		return err;
+	}
+
+	dev_dbg(dev, "cmdq device: addr:0x%p, va:0x%p, irq:%d\n",
+		dev, cmdq->base, cmdq->irq);
+
+	cmdq->clock = devm_clk_get(dev, "gce");
+	if (IS_ERR(cmdq->clock)) {
+		dev_err(dev, "failed to get gce clk\n");
+		return PTR_ERR(cmdq->clock);
+	}
+
+	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",
+		.pm = &cmdq_pm_ops,
+		.of_match_table = cmdq_of_ids,
+	}
+};
+
+builtin_platform_driver(cmdq_drv);
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
new file mode 100644
index 0000000..3433c64
--- /dev/null
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -0,0 +1,75 @@ 
+/*
+ * 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_MAILBOX_H__
+#define __MTK_CMDQ_MAILBOX_H__
+
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define CMDQ_INST_SIZE			8 /* instruction is 64-bit */
+#define CMDQ_OP_CODE_SHIFT		24
+#define CMDQ_JUMP_PASS			CMDQ_INST_SIZE
+
+#define CMDQ_WFE_UPDATE			BIT(31)
+#define CMDQ_WFE_WAIT			BIT(15)
+#define CMDQ_WFE_WAIT_VALUE		0x1
+
+/*
+ * CMDQ_CODE_MASK:
+ *   set write mask
+ *   format: op mask
+ * CMDQ_CODE_WRITE:
+ *   write value into target register
+ *   format: op subsys address value
+ * CMDQ_CODE_JUMP:
+ *   jump by offset
+ *   format: op offset
+ * CMDQ_CODE_WFE:
+ *   wait for event and clear
+ *   it is just clear if no wait
+ *   format: [wait]  op event update:1 to_wait:1 wait:1
+ *           [clear] op event update:1 to_wait:0 wait:0
+ * CMDQ_CODE_EOC:
+ *   end of command
+ *   format: op irq_flag
+ */
+enum cmdq_code {
+	CMDQ_CODE_MASK = 0x02,
+	CMDQ_CODE_WRITE = 0x04,
+	CMDQ_CODE_JUMP = 0x10,
+	CMDQ_CODE_WFE = 0x20,
+	CMDQ_CODE_EOC = 0x40,
+};
+
+struct cmdq_cb_data {
+	bool	err;
+	void	*data;
+};
+
+typedef void (*cmdq_async_flush_cb)(struct cmdq_cb_data data);
+
+struct cmdq_task_cb {
+	cmdq_async_flush_cb	cb;
+	void			*data;
+};
+
+struct cmdq_pkt {
+	void			*va_base;
+	size_t			cmd_buf_size; /* command occupied size */
+	size_t			buf_size; /* real buffer size */
+	struct cmdq_task_cb	cb;
+};
+
+#endif /* __MTK_CMDQ_MAILBOX_H__ */