Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582653 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver Jassi Brar <jassisinghbrar@gmail.com> - 2017-02-16 16:40 +0100
    Re: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2017-02-22 04:20 +0100
      Re: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver Jassi Brar <jassisinghbrar@gmail.com> - 2017-02-23 05:20 +0100
        Re: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2017-02-23 13:50 +0100

#1582653 — Re: [PATCH v20 2/4] mailbox: mediatek: Add Mediatek CMDQ driver

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() ?

[toc] | [next] | [standalone]

#1585903

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...."

May I use workqueue instead of tasklet?

Thanks,
HS

[toc] | [prev] | [next] | [standalone]

#1586638

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.

[toc] | [prev] | [next] | [standalone]

#1586865

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web