Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1516249820.13759.18.camel@mhfsdcap03>
Subject: Re: FW: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver
From:   houlong wei <houlong.wei@mediatek.com>
To:     Jassi Brar <jassisinghbrar@gmail.com>
CC:     Rob Herring <robh+dt@kernel.org>,
        "; Matthias Brugger" <matthias.bgg@gmail.com>,
        "; Daniel Kurtz" <djkurtz@chromium.org>,
        "; Sascha Hauer" <s.hauer@pengutronix.de>,
        "; Devicetree List" <devicetree@vger.kernel.org>,
        "; Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
        "; linux-arm-kernel@lists.infradead.org;
        linux-mediatek@lists.infradead.org; srv_heupstream" 
        <srv_heupstream@mediatek.com>,
        "; Sascha Hauer" <kernel@pengutronix.de>,
        "; Philipp Zabel" <p.zabel@pengutronix.de>,
        "; Nicolas Boichat" <drinkcat@chromium.org>,
        "; CK Hu =?UTF-8?Q?=28=E8=83=A1=E4=BF=8A=E5=85=89=29?=" 
        <ck.hu@mediatek.com>,
        "; Cawa Cheng =?UTF-8?Q?=28=E9=84=AD=E6=9B=84=E7=A6=A7=29?=" 
        <cawa.cheng@mediatek.com>,
        "; Bibby Hsieh =?UTF-8?Q?=28=E8=AC=9D=E6=BF=9F=E9=81=A0=29?=" 
        <Bibby.Hsieh@mediatek.com>,
        "; YT Shen =?UTF-8?Q?=28=E6=B2=88=E5=B2=B3=E9=9C=86=29?=" 
        <yt.shen@mediatek.com>,
        "; Daoyuan Huang =?UTF-8?Q?=28=E9=BB=83=E9=81=93=E5=8E=9F=29?=" 
        <Daoyuan.Huang@mediatek.com>,
        "; Damon Chu =?UTF-8?Q?=28=E6=9C=B1=E5=B3=BB=E8=B3=A2=29?=" 
        <damon.chu@mediatek.com>,
        "; Josh-YC Liu =?UTF-8?Q?=28=E5=8A=89=E8=82=B2=E8=AA=A0=29?=" 
        <Josh-YC.Liu@mediatek.com>,
        "; Glory Hung =?UTF-8?Q?=28=E6=B4=AA=E6=99=BA=E7=91=8B=29?=" 
        <glory.hung@mediatek.com>,
        "; Jiaguang Zhang =?UTF-8?Q?=28=E5=BC=A0=E5=8A=A0=E5=B9=BF=29?=" 
        <Jiaguang.Zhang@mediatek.com>,
        "; Dennis-YC Hsieh =?UTF-8?Q?=28=E8=AC=9D=E5=AE=87=E5=93=B2=29?=" 
        <Dennis-YC.Hsieh@mediatek.com>,
        "; Monica Wang =?UTF-8?Q?=28=E7=8E=8B=E5=AD=9F=E5=A9=B7=29?=" 
        <monica.wang@mediatek.com>,
        "; Houlong Wei =?UTF-8?Q?=28=E9=AD=8F=E5=8E=9A=E9=BE=99=29?=" 
        <houlong.wei@mediatek.com>,
        "; Hs Liao =?UTF-8?Q?=28=E5=BB=96=E5=AE=8F=E7=A5=A5=29?=" 
        <hs.liao@mediatek.com>
Date:   Thu, 18 Jan 2018 12:30:20 +0800
In-Reply-To: <1515400735.21044.35.camel@mhfsdcap03>
References: <1483499169-16329-1-git-send-email-hs.liao@mediatek.com>
         <1483499169-16329-3-git-send-email-hs.liao@mediatek.com>
         <CABb+yY23TUB8NpmpiO=cCmVFAVJMqKPfGgX14j+_Yj7tJvMOuQ@mail.gmail.com>
         <1485419833.990.1.camel@mtksdaap41>
         <CABb+yY13VaFpYz6EExvAXLpAqEWDJ4R50WG0DYbXO6iCESbA5A@mail.gmail.com>
         <1486359476.11424.33.camel@mtksdaap41>
         <CABb+yY0jCGqnrMLkkTNoEmLX4jDstWh7-UYhDyKjV-gFOnxpsg@mail.gmail.com>
         <1487733150.15869.11.camel@mtksdaap41>
         <CABb+yY2dzcR4fYvrSQK5jemwUSkYVJ8aWEmiE6wstw-Yyqy7bw@mail.gmail.com>
         <1487854104.17813.1.camel@mtksdaap41>
         <497f8e4ef7ae4c8a9b7b4ab259801306@mtkmbs01n1.mediatek.inc>
         <1515400735.21044.35.camel@mhfsdcap03>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Sorry to send the mail again because I missed some mail lists by
mistake.

Hi Jassi,

We prefer to use method (1) to move dma_map_single() outside of
spin_lock. Do you have any comment about this?

Thanks,
Houlong

On Mon, 2018-01-08 at 16:38 +0800, houlong wei 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?
> 
> Thanks
> Houlong
> 
> 
> > -----Original Message-----
> > From: Horng-Shyang Liao [mailto:hs.liao@mediatek.com] 
> > Sent: Thursday, February 23, 2017 8:48 PM
> > To: Jassi Brar <jassisinghbrar@gmail.com>
> > Cc: Rob Herring <robh+dt@kernel.org>; Matthias Brugger <matthias.bgg@gmail.com>; Daniel Kurtz <djkurtz@chromium.org>; Sascha Hauer <s.hauer@pengutronix.de>; Devicetree List <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; linux-arm-kernel@lists.infradead.org; linux-mediatek@lists.infradead.org; srv_heupstream <srv_heupstream@mediatek.com>; Sascha Hauer <kernel@pengutronix.de>; Philipp Zabel <p.zabel@pengutronix.de>; Nicolas Boichat <drinkcat@chromium.org>; CK Hu (胡俊光) <ck.hu@mediatek.com>; Cawa Cheng (鄭曄禧) <cawa.cheng@mediatek.com>; Bibby Hsieh (謝濟遠) <Bibby.Hsieh@mediatek.com>; YT Shen (沈岳霆) <yt.shen@mediatek.com>; Daoyuan Huang (黃道原) <Daoyuan.Huang@mediatek.com>; Damon Chu (朱峻賢) <damon.chu@mediatek.com>; Josh-YC Liu (劉育誠) <Josh-YC.Liu@mediatek.com>; Glory Hung (洪智瑋) <glory.hung@mediatek.com>; Jiaguang Zhang (张加广) <Jiaguang.Zhang@mediatek.com>; Dennis-YC Hsieh (謝宇哲) <Dennis-YC.Hsieh@mediatek.com>; Monica Wang (王孟婷) <monica.wang@mediatek.com>; Houlong Wei (魏厚龙) <houlong.wei@mediatek.com>; Hs Liao (廖宏祥) <hs.liao@mediatek.com>
> > Subject: Re: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver
> > 
> > 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
> > 
> > 
> > 
>