From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,UNPARSEABLE_RELAY,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2C20AC6778C for ; Fri, 6 Jul 2018 02:04:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C398F240BC for ; Fri, 6 Jul 2018 02:04:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C398F240BC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=mediatek.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753787AbeGFCET (ORCPT ); Thu, 5 Jul 2018 22:04:19 -0400 Received: from Mailgw01.mediatek.com ([1.203.163.78]:14307 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1753560AbeGFCER (ORCPT ); Thu, 5 Jul 2018 22:04:17 -0400 X-UUID: d07bed83b0af43d9ab94e875eafa9b0b-20180706 Received: from mtkcas32.mediatek.inc [(172.27.4.250)] by mailgw01.mediatek.com (envelope-from ) (mailgw01.mediatek.com ESMTP with TLS) with ESMTP id 485153929; Fri, 06 Jul 2018 10:04:14 +0800 Received: from MTKCAS36.mediatek.inc (172.27.4.186) by MTKMBS31N1.mediatek.inc (172.27.4.69) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Fri, 6 Jul 2018 10:04:12 +0800 Received: from [10.17.3.153] (10.17.3.153) by MTKCAS36.mediatek.inc (172.27.4.170) with Microsoft SMTP Server id 15.0.1210.3 via Frontend Transport; Fri, 6 Jul 2018 10:04:11 +0800 Message-ID: <1530842651.28645.41.camel@mhfsdcap03> Subject: Re: [PATCH v22 2/4] mailbox: mediatek: Add Mediatek CMDQ driver From: houlong wei To: CK Hu CC: Jassi Brar , Matthias Brugger , Rob Herring , Daniel Kurtz , Sascha Hauer , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mediatek@lists.infradead.org" , srv_heupstream , Sascha Hauer , Philipp Zabel , "Nicolas Boichat" , Bibby Hsieh =?UTF-8?Q?=28=E8=AC=9D=E6=BF=9F=E9=81=A0=29?= , YT Shen =?UTF-8?Q?=28=E6=B2=88=E5=B2=B3=E9=9C=86=29?= , Daoyuan Huang =?UTF-8?Q?=28=E9=BB=83=E9=81=93=E5=8E=9F=29?= , Jiaguang Zhang =?UTF-8?Q?=28=E5=BC=A0=E5=8A=A0=E5=B9=BF=29?= , Dennis-YC Hsieh =?UTF-8?Q?=28=E8=AC=9D=E5=AE=87=E5=93=B2=29?= , Monica Wang =?UTF-8?Q?=28=E7=8E=8B=E5=AD=9F=E5=A9=B7=29?= , Hs Liao =?UTF-8?Q?=28=E5=BB=96=E5=AE=8F=E7=A5=A5=29?= , Ginny Chen =?UTF-8?Q?=28=E9=99=B3=E6=B2=BB=E5=82=91=29?= , Enzhu Wang =?UTF-8?Q?=28=E7=8E=8B=E6=81=A9=E6=9F=B1=29?= , Date: Fri, 6 Jul 2018 10:04:11 +0800 In-Reply-To: <1530694990.24031.15.camel@mtksdaap41> References: <1530098172-31385-1-git-send-email-houlong.wei@mediatek.com> <1530098172-31385-3-git-send-email-houlong.wei@mediatek.com> <1530256083.8518.36.camel@mtksdaap41> <1530663042.21991.64.camel@mhfsdcap03> <1530694990.24031.15.camel@mtksdaap41> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > > Signed-off-by: HS Liao > > > > Signed-off-by: CK Hu > > > > --- > > > > 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]; > > > > +} > > > > + > > > [...] > > > > > > > > > > > >