| Message ID | 1574819937-6246-6-git-send-email-dennis-yc.hsieh@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/14] dt-binding: gce: add gce header file for mt6779 | expand |
Hi, Dennis: On Wed, 2019-11-27 at 09:58 +0800, Dennis YC Hsieh wrote: > Do success callback in channel when shutdown. For those task not finish, > callback with error code thus client has chance to cleanup or reset. > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > --- > drivers/mailbox/mtk-cmdq-mailbox.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > index fd519b6f518b..c12a768d1175 100644 > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > @@ -450,6 +450,32 @@ static int cmdq_mbox_startup(struct mbox_chan *chan) > > static void cmdq_mbox_shutdown(struct mbox_chan *chan) > { > + struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv; > + struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev); > + struct cmdq_task *task, *tmp; > + unsigned long flags; > + > + spin_lock_irqsave(&thread->chan->lock, flags); > + if (list_empty(&thread->task_busy_list)) > + goto done; > + > + WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0); > + > + /* make sure executed tasks have success callback */ > + cmdq_thread_irq_handler(cmdq, thread); > + if (list_empty(&thread->task_busy_list)) > + goto done; > + > + list_for_each_entry_safe(task, tmp, &thread->task_busy_list, > + list_entry) { > + cmdq_task_exec_done(task, -ECONNABORTED); > + kfree(task); > + } > + > + cmdq_thread_disable(cmdq, thread); > + clk_disable(cmdq->clock); > +done: cmdq_thread_resume(thread); Regards, CK > + spin_unlock_irqrestore(&thread->chan->lock, flags); > } > > static const struct mbox_chan_ops cmdq_mbox_chan_ops = {
Hi CK, On Tue, 2019-12-10 at 10:49 +0800, CK Hu wrote: > Hi, Dennis: > > On Wed, 2019-11-27 at 09:58 +0800, Dennis YC Hsieh wrote: > > Do success callback in channel when shutdown. For those task not finish, > > callback with error code thus client has chance to cleanup or reset. > > > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > > --- > > drivers/mailbox/mtk-cmdq-mailbox.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > > index fd519b6f518b..c12a768d1175 100644 > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > @@ -450,6 +450,32 @@ static int cmdq_mbox_startup(struct mbox_chan *chan) > > > > static void cmdq_mbox_shutdown(struct mbox_chan *chan) > > { > > + struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv; > > + struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev); > > + struct cmdq_task *task, *tmp; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&thread->chan->lock, flags); > > + if (list_empty(&thread->task_busy_list)) > > + goto done; > > + > > + WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0); > > + > > + /* make sure executed tasks have success callback */ > > + cmdq_thread_irq_handler(cmdq, thread); > > + if (list_empty(&thread->task_busy_list)) > > + goto done; > > + > > + list_for_each_entry_safe(task, tmp, &thread->task_busy_list, > > + list_entry) { > > + cmdq_task_exec_done(task, -ECONNABORTED); > > + kfree(task); > > + } > > + > > + cmdq_thread_disable(cmdq, thread); > > + clk_disable(cmdq->clock); > > +done: > > cmdq_thread_resume(thread); > > Regards, > CK > Call resume here will cause violation. The thread->task_busy_list empty means no task work in gce and thread state should already disable without clock, which is what we want since client try to shut down this mbox channel. So I think we don't need resume here. Regards, Dennis > > + spin_unlock_irqrestore(&thread->chan->lock, flags); > > } > > > > static const struct mbox_chan_ops cmdq_mbox_chan_ops = { > >
Hi, Dennis: On Thu, 2019-12-12 at 09:13 +0800, Dennis-YC Hsieh wrote: > Hi CK, > > On Tue, 2019-12-10 at 10:49 +0800, CK Hu wrote: > > Hi, Dennis: > > > > On Wed, 2019-11-27 at 09:58 +0800, Dennis YC Hsieh wrote: > > > Do success callback in channel when shutdown. For those task not finish, > > > callback with error code thus client has chance to cleanup or reset. > > > > > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > > > --- > > > drivers/mailbox/mtk-cmdq-mailbox.c | 26 ++++++++++++++++++++++++++ > > > 1 file changed, 26 insertions(+) > > > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > > > index fd519b6f518b..c12a768d1175 100644 > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > > @@ -450,6 +450,32 @@ static int cmdq_mbox_startup(struct mbox_chan *chan) > > > > > > static void cmdq_mbox_shutdown(struct mbox_chan *chan) > > > { > > > + struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv; > > > + struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev); > > > + struct cmdq_task *task, *tmp; > > > + unsigned long flags; > > > + > > > + spin_lock_irqsave(&thread->chan->lock, flags); > > > + if (list_empty(&thread->task_busy_list)) > > > + goto done; > > > + > > > + WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0); > > > + > > > + /* make sure executed tasks have success callback */ > > > + cmdq_thread_irq_handler(cmdq, thread); > > > + if (list_empty(&thread->task_busy_list)) > > > + goto done; > > > + > > > + list_for_each_entry_safe(task, tmp, &thread->task_busy_list, > > > + list_entry) { > > > + cmdq_task_exec_done(task, -ECONNABORTED); > > > + kfree(task); > > > + } > > > + > > > + cmdq_thread_disable(cmdq, thread); > > > + clk_disable(cmdq->clock); > > > +done: > > > > cmdq_thread_resume(thread); > > > > Regards, > > CK > > > > Call resume here will cause violation. The thread->task_busy_list empty > means no task work in gce and thread state should already disable > without clock, which is what we want since client try to shut down this > mbox channel. So I think we don't need resume here. > OK. When client free channel, thread is suspended. Then client request channel, where do you resume thread? Regards, CK > > Regards, > Dennis > > > > + spin_unlock_irqrestore(&thread->chan->lock, flags); > > > } > > > > > > static const struct mbox_chan_ops cmdq_mbox_chan_ops = { > > > > > >
Hi CK, On Thu, 2019-12-12 at 09:31 +0800, CK Hu wrote: > Hi, Dennis: > > On Thu, 2019-12-12 at 09:13 +0800, Dennis-YC Hsieh wrote: > > Hi CK, > > > > On Tue, 2019-12-10 at 10:49 +0800, CK Hu wrote: > > > Hi, Dennis: > > > > > > On Wed, 2019-11-27 at 09:58 +0800, Dennis YC Hsieh wrote: > > > > Do success callback in channel when shutdown. For those task not finish, > > > > callback with error code thus client has chance to cleanup or reset. > > > > > > > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > > > > --- > > > > drivers/mailbox/mtk-cmdq-mailbox.c | 26 ++++++++++++++++++++++++++ > > > > 1 file changed, 26 insertions(+) > > > > > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > index fd519b6f518b..c12a768d1175 100644 > > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > @@ -450,6 +450,32 @@ static int cmdq_mbox_startup(struct mbox_chan *chan) > > > > > > > > static void cmdq_mbox_shutdown(struct mbox_chan *chan) > > > > { > > > > + struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv; > > > > + struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev); > > > > + struct cmdq_task *task, *tmp; > > > > + unsigned long flags; > > > > + > > > > + spin_lock_irqsave(&thread->chan->lock, flags); > > > > + if (list_empty(&thread->task_busy_list)) > > > > + goto done; > > > > + > > > > + WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0); > > > > + > > > > + /* make sure executed tasks have success callback */ > > > > + cmdq_thread_irq_handler(cmdq, thread); > > > > + if (list_empty(&thread->task_busy_list)) > > > > + goto done; > > > > + > > > > + list_for_each_entry_safe(task, tmp, &thread->task_busy_list, > > > > + list_entry) { > > > > + cmdq_task_exec_done(task, -ECONNABORTED); > > > > + kfree(task); > > > > + } > > > > + > > > > + cmdq_thread_disable(cmdq, thread); > > > > + clk_disable(cmdq->clock); > > > > +done: > > > > > > cmdq_thread_resume(thread); > > > > > > Regards, > > > CK > > > > > > > Call resume here will cause violation. The thread->task_busy_list empty > > means no task work in gce and thread state should already disable > > without clock, which is what we want since client try to shut down this > > mbox channel. So I think we don't need resume here. > > > > OK. When client free channel, thread is suspended. Then client request > channel, where do you resume thread? > when client send new pkt to new channel, cmdq_mbox_send_data() will enable thread. Regards, Dennis > Regards, > CK > > > > > Regards, > > Dennis > > > > > > + spin_unlock_irqrestore(&thread->chan->lock, flags); > > > > } > > > > > > > > static const struct mbox_chan_ops cmdq_mbox_chan_ops = { > > > > > > > > > > > >
Hi, Dennis: On Thu, 2019-12-12 at 09:51 +0800, Dennis-YC Hsieh wrote: > Hi CK, > > On Thu, 2019-12-12 at 09:31 +0800, CK Hu wrote: > > Hi, Dennis: > > > > On Thu, 2019-12-12 at 09:13 +0800, Dennis-YC Hsieh wrote: > > > Hi CK, > > > > > > On Tue, 2019-12-10 at 10:49 +0800, CK Hu wrote: > > > > Hi, Dennis: > > > > > > > > On Wed, 2019-11-27 at 09:58 +0800, Dennis YC Hsieh wrote: > > > > > Do success callback in channel when shutdown. For those task not finish, > > > > > callback with error code thus client has chance to cleanup or reset. > > > > > > > > > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > > > > > --- > > > > > drivers/mailbox/mtk-cmdq-mailbox.c | 26 ++++++++++++++++++++++++++ > > > > > 1 file changed, 26 insertions(+) > > > > > > > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > index fd519b6f518b..c12a768d1175 100644 > > > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > @@ -450,6 +450,32 @@ static int cmdq_mbox_startup(struct mbox_chan *chan) > > > > > > > > > > static void cmdq_mbox_shutdown(struct mbox_chan *chan) > > > > > { > > > > > + struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv; > > > > > + struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev); > > > > > + struct cmdq_task *task, *tmp; > > > > > + unsigned long flags; > > > > > + > > > > > + spin_lock_irqsave(&thread->chan->lock, flags); > > > > > + if (list_empty(&thread->task_busy_list)) > > > > > + goto done; > > > > > + > > > > > + WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0); > > > > > + > > > > > + /* make sure executed tasks have success callback */ > > > > > + cmdq_thread_irq_handler(cmdq, thread); > > > > > + if (list_empty(&thread->task_busy_list)) > > > > > + goto done; > > > > > + > > > > > + list_for_each_entry_safe(task, tmp, &thread->task_busy_list, > > > > > + list_entry) { > > > > > + cmdq_task_exec_done(task, -ECONNABORTED); > > > > > + kfree(task); > > > > > + } > > > > > + > > > > > + cmdq_thread_disable(cmdq, thread); > > > > > + clk_disable(cmdq->clock); > > > > > +done: > > > > > > > > cmdq_thread_resume(thread); > > > > > > > > Regards, > > > > CK > > > > > > > > > > Call resume here will cause violation. The thread->task_busy_list empty > > > means no task work in gce and thread state should already disable > > > without clock, which is what we want since client try to shut down this > > > mbox channel. So I think we don't need resume here. > > > > > > > OK. When client free channel, thread is suspended. Then client request > > channel, where do you resume thread? > > > > when client send new pkt to new channel, cmdq_mbox_send_data() will > enable thread. in cmdq_mbox_send_data(), it would run below command: WARN_ON(clk_enable(cmdq->clock) < 0); WARN_ON(cmdq_thread_reset(cmdq, thread) < 0); writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR); writel(task->pa_base + pkt->cmd_buf_size, thread->base + CMDQ_THR_END_ADDR); writel(thread->priority, thread->base + CMDQ_THR_PRIORITY); writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE); writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK); Do you mean CMDQ_THR_ENABLE_TASK is set to CMDQ_THR_ENABLED, then CMDQ_THR_SUSPEND_TASK would be automatically set to CMDQ_THR_RESUME? If this hardware work in so special behavior, please add comment for this. Regards, CK > > > Regards, > Dennis > > > Regards, > > CK > > > > > > > > Regards, > > > Dennis > > > > > > > > + spin_unlock_irqrestore(&thread->chan->lock, flags); > > > > > } > > > > > > > > > > static const struct mbox_chan_ops cmdq_mbox_chan_ops = { > > > > > > > > > > > > > > > > > > > >
Hi CK, On Thu, 2019-12-12 at 10:03 +0800, CK Hu wrote: > Hi, Dennis: > > On Thu, 2019-12-12 at 09:51 +0800, Dennis-YC Hsieh wrote: > > Hi CK, > > > > On Thu, 2019-12-12 at 09:31 +0800, CK Hu wrote: > > > Hi, Dennis: > > > > > > On Thu, 2019-12-12 at 09:13 +0800, Dennis-YC Hsieh wrote: > > > > Hi CK, > > > > > > > > On Tue, 2019-12-10 at 10:49 +0800, CK Hu wrote: > > > > > Hi, Dennis: > > > > > > > > > > On Wed, 2019-11-27 at 09:58 +0800, Dennis YC Hsieh wrote: > > > > > > Do success callback in channel when shutdown. For those task not finish, > > > > > > callback with error code thus client has chance to cleanup or reset. > > > > > > > > > > > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > > > > > > --- > > > > > > drivers/mailbox/mtk-cmdq-mailbox.c | 26 ++++++++++++++++++++++++++ > > > > > > 1 file changed, 26 insertions(+) > > > > > > > > > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > > index fd519b6f518b..c12a768d1175 100644 > > > > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > > @@ -450,6 +450,32 @@ static int cmdq_mbox_startup(struct mbox_chan *chan) > > > > > > > > > > > > static void cmdq_mbox_shutdown(struct mbox_chan *chan) > > > > > > { > > > > > > + struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv; > > > > > > + struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev); > > > > > > + struct cmdq_task *task, *tmp; > > > > > > + unsigned long flags; > > > > > > + > > > > > > + spin_lock_irqsave(&thread->chan->lock, flags); > > > > > > + if (list_empty(&thread->task_busy_list)) > > > > > > + goto done; > > > > > > + > > > > > > + WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0); > > > > > > + > > > > > > + /* make sure executed tasks have success callback */ > > > > > > + cmdq_thread_irq_handler(cmdq, thread); > > > > > > + if (list_empty(&thread->task_busy_list)) > > > > > > + goto done; > > > > > > + > > > > > > + list_for_each_entry_safe(task, tmp, &thread->task_busy_list, > > > > > > + list_entry) { > > > > > > + cmdq_task_exec_done(task, -ECONNABORTED); > > > > > > + kfree(task); > > > > > > + } > > > > > > + > > > > > > + cmdq_thread_disable(cmdq, thread); > > > > > > + clk_disable(cmdq->clock); > > > > > > +done: > > > > > > > > > > cmdq_thread_resume(thread); > > > > > > > > > > Regards, > > > > > CK > > > > > > > > > > > > > Call resume here will cause violation. The thread->task_busy_list empty > > > > means no task work in gce and thread state should already disable > > > > without clock, which is what we want since client try to shut down this > > > > mbox channel. So I think we don't need resume here. > > > > > > > > > > OK. When client free channel, thread is suspended. Then client request > > > channel, where do you resume thread? > > > > > > > when client send new pkt to new channel, cmdq_mbox_send_data() will > > enable thread. > > in cmdq_mbox_send_data(), it would run below command: > > WARN_ON(clk_enable(cmdq->clock) < 0); > WARN_ON(cmdq_thread_reset(cmdq, thread) < 0); > > writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR); > writel(task->pa_base + pkt->cmd_buf_size, > thread->base + CMDQ_THR_END_ADDR); > writel(thread->priority, thread->base + CMDQ_THR_PRIORITY); > writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE); > writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK); > > Do you mean CMDQ_THR_ENABLE_TASK is set to CMDQ_THR_ENABLED, then > CMDQ_THR_SUSPEND_TASK would be automatically set to CMDQ_THR_RESUME? If > this hardware work in so special behavior, please add comment for this. > > Regards, > CK > sorry for not clearly explain before call to cmdq_thread_reset() will reset thread status, which include suspend state. the CMDQ_THR_SUSPEND_TASK will be clear after reset, thus we can simply set CMDQ_THR_ENABLE_TASK to CMDQ_THR_ENABLED and then thread running again. I will add comment in both cmdq_mbox_send_data() and cmdq_mbox_shutdown() to clarify this hardware behavior. Regards, Dennis > > > > > > Regards, > > Dennis > > > > > Regards, > > > CK > > > > > > > > > > > Regards, > > > > Dennis > > > > > > > > > > + spin_unlock_irqrestore(&thread->chan->lock, flags); > > > > > > } > > > > > > > > > > > > static const struct mbox_chan_ops cmdq_mbox_chan_ops = { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c index fd519b6f518b..c12a768d1175 100644 --- a/drivers/mailbox/mtk-cmdq-mailbox.c +++ b/drivers/mailbox/mtk-cmdq-mailbox.c @@ -450,6 +450,32 @@ static int cmdq_mbox_startup(struct mbox_chan *chan) static void cmdq_mbox_shutdown(struct mbox_chan *chan) { + struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv; + struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev); + struct cmdq_task *task, *tmp; + unsigned long flags; + + spin_lock_irqsave(&thread->chan->lock, flags); + if (list_empty(&thread->task_busy_list)) + goto done; + + WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0); + + /* make sure executed tasks have success callback */ + cmdq_thread_irq_handler(cmdq, thread); + if (list_empty(&thread->task_busy_list)) + goto done; + + list_for_each_entry_safe(task, tmp, &thread->task_busy_list, + list_entry) { + cmdq_task_exec_done(task, -ECONNABORTED); + kfree(task); + } + + cmdq_thread_disable(cmdq, thread); + clk_disable(cmdq->clock); +done: + spin_unlock_irqrestore(&thread->chan->lock, flags); } static const struct mbox_chan_ops cmdq_mbox_chan_ops = {
Do success callback in channel when shutdown. For those task not finish, callback with error code thus client has chance to cleanup or reset. Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> --- drivers/mailbox/mtk-cmdq-mailbox.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)