On Thu, 2017-02-16 at 21:02 +0530, Jassi Brar wrote:

On Mon, Feb 6, 2017 at 11:07 AM, Horng-Shyang Liao [off-list ref] wrote:

quoted

Hi Jassi,

On Wed, 2017-02-01 at 10:52 +0530, Jassi Brar wrote:

quoted

On Thu, Jan 26, 2017 at 2:07 PM, Horng-Shyang Liao [off-list ref] wrote:

quoted

Hi Jassi,

On Thu, 2017-01-26 at 10:08 +0530, Jassi Brar wrote:

quoted

On Wed, Jan 4, 2017 at 8:36 AM, HS Liao [off-list ref] wrote:

quoted

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

...

quoted

+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