On Wed, 2018-07-04 at 17:03 +0800, CK Hu wrote:
> Hi, Houlong:
>
> On Wed, 2018-07-04 at 08:10 +0800, houlong wei wrote:
> > On Fri, 2018-06-29 at 15:08 +0800, CK Hu wrote:
> > > Hi, Houlong:
> > >
> > > Some inline comment.
> > >
> > > On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > > > This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> > > > CMDQ is used to help write registers with critical time limitation,
> > > > such as updating display configuration during the vblank. It controls
> > > > Global Command Engine (GCE) hardware to achieve this requirement.
> > > > Currently, CMDQ only supports display related hardwares, but we expect
> > > > it can be extended to other hardwares for future requirements.
> > > >
> > > > Signed-off-by: Houlong Wei <houlong.wei@xxxxxxxxxxxx>
> > > > Signed-off-by: HS Liao <hs.liao@xxxxxxxxxxxx>
> > > > Signed-off-by: CK Hu <ck.hu@xxxxxxxxxxxx>
> > > > ---
> > > > drivers/mailbox/Kconfig | 10 +
> > > > drivers/mailbox/Makefile | 2 +
> > > > drivers/mailbox/mtk-cmdq-mailbox.c | 634 ++++++++++++++++++++++++++++++
> > > > include/linux/mailbox/mtk-cmdq-mailbox.h | 70 ++++
> > > > 4 files changed, 716 insertions(+)
> > > > create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
> > > > create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h
> > > >
> > >
> > > [...]
> > >
> > > > +
> > > > +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);
> > >
> > > The CMDQ_THR_SLOT_CYCLES looks like not relevant to reset. Maybe you
> > > just need to set this value when startup.
> >
> > Will move configuration of CMDQ_THR_SLOT_CYCLES to cmdq_xlate() where is
> > startup of a GCE thread.
> >
Since cmdq_xlate() is called when a client requests a channel, it may be
called more than once. Will move it to cmdq_probe().
> > >
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > >
> > > [...]
> > >
> > > > +
> > > > +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 = pkt->pa_base;
> > > > + 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(thread->priority, thread->base + CMDQ_THR_PRIORITY);
> > > > + writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
> > > > + writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
> > > > +
> > > > + if (thread->timeout_ms != CMDQ_NO_TIMEOUT)
> > > > + mod_timer(&thread->timeout,
> > > > + jiffies + msecs_to_jiffies(thread->timeout_ms));
> > >
> > > I think the timeout processing should be done by client driver. The
> > > total time to execute a command buffer does not depend on GCE HW speed
> > > because the WFE (wait for event) command would wait for client HW event,
> > > so the total time depend on how long a client HW send this event to GCE
> > > and the timeout processing should be client driver's job. Each client
> > > may have different timeout processing mechanism, for example, if display
> > > could dynamic change panel frame rate between 120Hz and 60Hz, and the
> > > timeout time is 2 frame, so it may dynamically change timeout time
> > > between 17ms and 33ms. Another reason is that display have interrupt
> > > every vblank, and it could check timeout in that interrupt, so the timer
> > > in cmdq driver looks redundant. Because each client would define its own
> > > timeout processing mechanism, so it's not wise to put timeout processing
> > > in cmdq driver.
> >
> > The client drivers' owners strongly hope to keep the current timeout
> > mechanism, the reasons are below.
> > 1. If remove, all clients should add timeout mechanism and the code will
> > be redundant.
> > 2. If timeout happens, only GCE driver can do reset and continue to
> > execute next packet.
>
> For the reason 2, GCE should not continue execute next packet because
> the packets may have dependency. So GCE driver could only drop all
> packet (this is what you do in cmdq_thread_handle_timeout()). For reason
> 1, you have a assumption that all client have the same request for
> timeout: constant timeout value or no timeout. If it's so, that's ok to
> put timeout mechanism in cmdq driver. But if one day, a new request for
> timeout, for example, dynamic timeout value, that means not all client
> driver implement timeout in the same way, putting timeout mechanism in
> cmdq driver does not reduce any thing but just move multiple client
> driver's code into cmdq driver. I would accept to put here only if all
> client driver use the same timeout mechanism.
The client drivers configure their timeout values via 'mboxes' properity
in device tree. So far, they handle timeout in same way.
>
> Regards,
> CK
>
> >
> > >
> > > > + } 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);
> > >
> > > Move this to client driver.
> >
> > map/unmap are common code for clients driver, could we move it to cmdq
> > helper?
> >
> > >
> > > > + 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 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,
> > >
> > > Because mbox->txdone_poll is false, so you need not to implement
> > > last_tx_done.
> > >
> > > Regards,
> > > CK
> >
> > Will remove 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->timeout_ms = sp->args[1];
> > > > + thread->priority = sp->args[2];
> > > > + thread->atomic_exec = (sp->args[3] != 0);
> > > > + thread->chan = &mbox->chans[ind];
> > > > +
> > > > + return &mbox->chans[ind];
> > > > +}
> > > > +
> > > [...]
> > >
> > >
> >
> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
