Re: FW: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

2018-01-18 Thread houlong wei 
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  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.

Re: FW: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

2018-01-18 Thread houlong wei 
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  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.

Re: FW: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

2018-01-18 Thread Jassi Brar 
On Mon, Jan 8, 2018 at 2:08 PM, 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?
>
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.

Re: FW: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

2018-01-18 Thread Jassi Brar 
On Mon, Jan 8, 2018 at 2:08 PM, 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?
>
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.

Re: FW: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

2018-01-17 Thread houlong wei 
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.l...@mediatek.com] 
> > Sent: Thursday, February 23, 2017 8:48 PM
> > To: Jassi Brar 
> > Cc: Rob Herring ; Matthias Brugger 
> > ; Daniel Kurtz ; Sascha Hauer 
> > ; Devicetree List ; 
> > Linux Kernel Mailing List ; 
> > linux-arm-ker...@lists.infradead.org; linux-media...@lists.infradead.org; 
> > srv_heupstream ; 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 (魏厚龙) ; 
> > Hs Liao (廖宏祥) 
> > 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  
> > > 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 
> > > >>  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 000..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(>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,

Re: FW: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

2018-01-17 Thread houlong wei 
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.l...@mediatek.com] 
> > Sent: Thursday, February 23, 2017 8:48 PM
> > To: Jassi Brar 
> > Cc: Rob Herring ; Matthias Brugger 
> > ; Daniel Kurtz ; Sascha Hauer 
> > ; Devicetree List ; 
> > Linux Kernel Mailing List ; 
> > linux-arm-ker...@lists.infradead.org; linux-media...@lists.infradead.org; 
> > srv_heupstream ; 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 (魏厚龙) ; 
> > Hs Liao (廖宏祥) 
> > 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  
> > > 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 
> > > >>  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 000..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(>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

Re: FW: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

2018-01-08 Thread houlong wei 
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.l...@mediatek.com] 
> Sent: Thursday, February 23, 2017 8:48 PM
> To: Jassi Brar 
> Cc: Rob Herring ; Matthias Brugger 
> ; Daniel Kurtz ; Sascha Hauer 
> ; Devicetree List ; Linux 
> Kernel Mailing List ; 
> linux-arm-ker...@lists.infradead.org; linux-media...@lists.infradead.org; 
> srv_heupstream ; 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 (魏厚龙) ; Hs Liao (廖宏祥) 
> 
> 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  
> > 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 
> > >>  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 000..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(>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

Re: FW: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

2018-01-08 Thread houlong wei 
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.l...@mediatek.com] 
> Sent: Thursday, February 23, 2017 8:48 PM
> To: Jassi Brar 
> Cc: Rob Herring ; Matthias Brugger 
> ; Daniel Kurtz ; Sascha Hauer 
> ; Devicetree List ; Linux 
> Kernel Mailing List ; 
> linux-arm-ker...@lists.infradead.org; linux-media...@lists.infradead.org; 
> srv_heupstream ; 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 (魏厚龙) ; Hs Liao (廖宏祥) 
> 
> 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  
> > 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 
> > >>  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 000..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(>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
> 
> 
>