From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753920AbdBVDMv (ORCPT ); Tue, 21 Feb 2017 22:12:51 -0500 Received: from mailgw02.mediatek.com ([210.61.82.184]:24141 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751271AbdBVDMt (ORCPT ); Tue, 21 Feb 2017 22:12:49 -0500 Message-ID: <1487733150.15869.11.camel@mtksdaap41> Subject: Re: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver From: Horng-Shyang Liao To: Jassi Brar CC: Rob Herring , Matthias Brugger , Daniel Kurtz , "Sascha Hauer" , Devicetree List , Linux Kernel Mailing List , "linux-arm-kernel@lists.infradead.org" , , , Sascha Hauer , "Philipp Zabel" , Nicolas Boichat , "CK HU" , cawa cheng , Bibby Hsieh , YT Shen , Daoyuan Huang , Damon Chu , "Josh-YC Liu" , Glory Hung , Jiaguang Zhang , Dennis-YC Hsieh , Monica Wang , Houlong Wei , Date: Wed, 22 Feb 2017 11:12:30 +0800 In-Reply-To: References: <1483499169-16329-1-git-send-email-hs.liao@mediatek.com> <1483499169-16329-3-git-send-email-hs.liao@mediatek.com> <1485419833.990.1.camel@mtksdaap41> <1486359476.11424.33.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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-02-16 at 21:02 +0530, Jassi Brar wrote: > On Mon, Feb 6, 2017 at 11:07 AM, 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 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 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Horng-Shyang Liao Subject: Re: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver Date: Wed, 22 Feb 2017 11:12:30 +0800 Message-ID: <1487733150.15869.11.camel@mtksdaap41> References: <1483499169-16329-1-git-send-email-hs.liao@mediatek.com> <1483499169-16329-3-git-send-email-hs.liao@mediatek.com> <1485419833.990.1.camel@mtksdaap41> <1486359476.11424.33.camel@mtksdaap41> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jassi Brar Cc: Rob Herring , Matthias Brugger , Daniel Kurtz , Sascha Hauer , Devicetree List , Linux Kernel Mailing List , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, srv_heupstream-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, Sascha Hauer , Philipp Zabel , Nicolas Boichat , CK HU , cawa cheng , Bibby Hsieh , YT Shen , Daoyuan Huang , Damon Chu , Jo List-Id: devicetree@vger.kernel.org On Thu, 2017-02-16 at 21:02 +0530, Jassi Brar wrote: > On Mon, Feb 6, 2017 at 11:07 AM, 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 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 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: hs.liao@mediatek.com (Horng-Shyang Liao) Date: Wed, 22 Feb 2017 11:12:30 +0800 Subject: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver In-Reply-To: References: <1483499169-16329-1-git-send-email-hs.liao@mediatek.com> <1483499169-16329-3-git-send-email-hs.liao@mediatek.com> <1485419833.990.1.camel@mtksdaap41> <1486359476.11424.33.camel@mtksdaap41> Message-ID: <1487733150.15869.11.camel@mtksdaap41> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, 2017-02-16 at 21:02 +0530, Jassi Brar wrote: > On Mon, Feb 6, 2017 at 11:07 AM, 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 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 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