| Message ID | 20220815030740.28899-2-yongqiang.niu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix gce timeout issue | expand |
Hi, Yongqiang: On Mon, 2022-08-15 at 11:07 +0800, Yongqiang Niu wrote: > From: Yongqiang Niu <yongqiang.niu@mediatek.corp-partner.google.com> > > 1. enable gce ddr enable(gce reigster offset 0x48, bit 16 to 18) when > gce work, > and disable gce ddr enable when gce work job done Why need gce ddr enable? I think gce works fine without this. Describe more about this. Regards, CK > 2. split cmdq clk enable/disable api, and control gce ddr > enable/disable > in clk enable/disable function to make sure it could protect when > cmdq > is multiple used by display and mdp > > Signed-off-by: Yongqiang Niu < > yongqiang.niu@mediatek.corp-partner.google.com> > Signed-off-by: Allen-kh Cheng < > allen-kh.cheng@mediatek.corp-partner.google.com> > --- > drivers/mailbox/mtk-cmdq-mailbox.c | 57 ++++++++++++++++++++++++++ > ---- > 1 file changed, 51 insertions(+), 6 deletions(-) > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > b/drivers/mailbox/mtk-cmdq-mailbox.c > index 2578e5aaa935..64a47470f062 100644 > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > @@ -81,6 +81,8 @@ struct cmdq { > u8 shift_pa; > bool control_by_sw; > u32 gce_num; > + atomic_t usage; > + spinlock_t lock; > }; > > struct gce_plat { > @@ -90,6 +92,46 @@ struct gce_plat { > u32 gce_num; > }; > > +static s32 cmdq_clk_enable(struct cmdq *cmdq) > +{ > + s32 usage, ret; > + unsigned long flags; > + > + spin_lock_irqsave(&cmdq->lock, flags); > + > + usage = atomic_inc_return(&cmdq->usage); > + > + ret = clk_bulk_enable(cmdq->gce_num, cmdq->clocks); > + if (usage <=0 || ret < 0) { > + dev_err(cmdq->mbox.dev, "ref count %d ret %d suspend > %d\n", > + usage, ret, cmdq->suspended); > + } else if (usage == 1) { > + if (cmdq->control_by_sw) > + writel((0x7 << 16) + 0x7, cmdq->base + > GCE_GCTL_VALUE); > + } > + > + spin_unlock_irqrestore(&cmdq->lock, flags); > + > + return ret; > +} > + > +static void cmdq_clk_disable(struct cmdq *cmdq) > +{ > + s32 usage; > + > + usage = atomic_dec_return(&cmdq->usage); > + > + if (usage < 0) { > + dev_err(cmdq->mbox.dev, "ref count %d suspend %d\n", > + usage, cmdq->suspended); > + } else if (usage == 0) { > + if (cmdq->control_by_sw) > + writel(0x7, cmdq->base + GCE_GCTL_VALUE); > + } > + > + clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > +} > + > u8 cmdq_get_shift_pa(struct mbox_chan *chan) > { > struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, > mbox); > @@ -271,7 +313,8 @@ static void cmdq_thread_irq_handler(struct cmdq > *cmdq, > > if (list_empty(&thread->task_busy_list)) { > cmdq_thread_disable(cmdq, thread); > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > + > + cmdq_clk_disable(cmdq); > } > } > > @@ -360,8 +403,7 @@ static int cmdq_mbox_send_data(struct mbox_chan > *chan, void *data) > task->pkt = pkt; > > if (list_empty(&thread->task_busy_list)) { > - WARN_ON(clk_bulk_enable(cmdq->gce_num, cmdq->clocks)); > - > + WARN_ON(cmdq_clk_enable(cmdq) < 0); > /* > * The thread reset will clear thread related register > to 0, > * including pc, end, priority, irq, suspend and > enable. Thus > @@ -433,7 +475,7 @@ static void cmdq_mbox_shutdown(struct mbox_chan > *chan) > } > > cmdq_thread_disable(cmdq, thread); > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > + cmdq_clk_disable(cmdq); > > done: > /* > @@ -479,7 +521,8 @@ static int cmdq_mbox_flush(struct mbox_chan > *chan, unsigned long timeout) > > cmdq_thread_resume(thread); > cmdq_thread_disable(cmdq, thread); > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > + > + cmdq_clk_disable(cmdq); > > out: > spin_unlock_irqrestore(&thread->chan->lock, flags); > @@ -490,7 +533,8 @@ static int cmdq_mbox_flush(struct mbox_chan > *chan, unsigned long timeout) > spin_unlock_irqrestore(&thread->chan->lock, flags); > if (readl_poll_timeout_atomic(thread->base + > CMDQ_THR_ENABLE_TASK, > enable, enable == 0, 1, timeout)) > { > - dev_err(cmdq->mbox.dev, "Fail to wait GCE thread 0x%x > done\n", > + dev_err(cmdq->mbox.dev, > + "Fail to wait GCE thread 0x%x done\n", > (u32)(thread->base - cmdq->base)); > > return -EFAULT; > @@ -626,6 +670,7 @@ static int cmdq_probe(struct platform_device > *pdev) > > WARN_ON(clk_bulk_prepare(cmdq->gce_num, cmdq->clocks)); > > + spin_lock_init(&cmdq->lock); > cmdq_init(cmdq); > > return 0;
On Mon, 2022-08-15 at 13:24 +0800, CK Hu wrote: > Hi, Yongqiang: > > > On Mon, 2022-08-15 at 11:07 +0800, Yongqiang Niu wrote: > > From: Yongqiang Niu <yongqiang.niu@mediatek.corp-partner.google.com > > > > > > > 1. enable gce ddr enable(gce reigster offset 0x48, bit 16 to 18) > > when > > gce work, > > and disable gce ddr enable when gce work job done > > Why need gce ddr enable? I think gce works fine without this. > Describe > more about this. > > Regards, > CK this is only for some SOC which has flag "control_by_sw". for this kind of gce, there is a handshake flow between gce and ddr hardware, if not set ddr enable flag of gce, ddr will fall into idle mode, then gce instructions will not process done. we need set this flag of gce to tell ddr when gce is idle or busy controlled by software flow. > > > 2. split cmdq clk enable/disable api, and control gce ddr > > enable/disable > > in clk enable/disable function to make sure it could protect when > > cmdq > > is multiple used by display and mdp > > > > Signed-off-by: Yongqiang Niu < > > yongqiang.niu@mediatek.corp-partner.google.com> > > Signed-off-by: Allen-kh Cheng < > > allen-kh.cheng@mediatek.corp-partner.google.com> > > --- > > drivers/mailbox/mtk-cmdq-mailbox.c | 57 ++++++++++++++++++++++++++ > > ---- > > 1 file changed, 51 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > > b/drivers/mailbox/mtk-cmdq-mailbox.c > > index 2578e5aaa935..64a47470f062 100644 > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > @@ -81,6 +81,8 @@ struct cmdq { > > u8 shift_pa; > > bool control_by_sw; > > u32 gce_num; > > + atomic_t usage; > > + spinlock_t lock; > > }; > > > > struct gce_plat { > > @@ -90,6 +92,46 @@ struct gce_plat { > > u32 gce_num; > > }; > > > > +static s32 cmdq_clk_enable(struct cmdq *cmdq) > > +{ > > + s32 usage, ret; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&cmdq->lock, flags); > > + > > + usage = atomic_inc_return(&cmdq->usage); > > + > > + ret = clk_bulk_enable(cmdq->gce_num, cmdq->clocks); > > + if (usage <=0 || ret < 0) { > > + dev_err(cmdq->mbox.dev, "ref count %d ret %d suspend > > %d\n", > > + usage, ret, cmdq->suspended); > > + } else if (usage == 1) { > > + if (cmdq->control_by_sw) > > + writel((0x7 << 16) + 0x7, cmdq->base + > > GCE_GCTL_VALUE); > > + } > > + > > + spin_unlock_irqrestore(&cmdq->lock, flags); > > + > > + return ret; > > +} > > + > > +static void cmdq_clk_disable(struct cmdq *cmdq) > > +{ > > + s32 usage; > > + > > + usage = atomic_dec_return(&cmdq->usage); > > + > > + if (usage < 0) { > > + dev_err(cmdq->mbox.dev, "ref count %d suspend %d\n", > > + usage, cmdq->suspended); > > + } else if (usage == 0) { > > + if (cmdq->control_by_sw) > > + writel(0x7, cmdq->base + GCE_GCTL_VALUE); > > + } > > + > > + clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > +} > > + > > u8 cmdq_get_shift_pa(struct mbox_chan *chan) > > { > > struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, > > mbox); > > @@ -271,7 +313,8 @@ static void cmdq_thread_irq_handler(struct cmdq > > *cmdq, > > > > if (list_empty(&thread->task_busy_list)) { > > cmdq_thread_disable(cmdq, thread); > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > + > > + cmdq_clk_disable(cmdq); > > } > > } > > > > @@ -360,8 +403,7 @@ static int cmdq_mbox_send_data(struct mbox_chan > > *chan, void *data) > > task->pkt = pkt; > > > > if (list_empty(&thread->task_busy_list)) { > > - WARN_ON(clk_bulk_enable(cmdq->gce_num, cmdq->clocks)); > > - > > + WARN_ON(cmdq_clk_enable(cmdq) < 0); > > /* > > * The thread reset will clear thread related register > > to 0, > > * including pc, end, priority, irq, suspend and > > enable. Thus > > @@ -433,7 +475,7 @@ static void cmdq_mbox_shutdown(struct mbox_chan > > *chan) > > } > > > > cmdq_thread_disable(cmdq, thread); > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > + cmdq_clk_disable(cmdq); > > > > done: > > /* > > @@ -479,7 +521,8 @@ static int cmdq_mbox_flush(struct mbox_chan > > *chan, unsigned long timeout) > > > > cmdq_thread_resume(thread); > > cmdq_thread_disable(cmdq, thread); > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > + > > + cmdq_clk_disable(cmdq); > > > > out: > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > @@ -490,7 +533,8 @@ static int cmdq_mbox_flush(struct mbox_chan > > *chan, unsigned long timeout) > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > if (readl_poll_timeout_atomic(thread->base + > > CMDQ_THR_ENABLE_TASK, > > enable, enable == 0, 1, timeout)) > > { > > - dev_err(cmdq->mbox.dev, "Fail to wait GCE thread 0x%x > > done\n", > > + dev_err(cmdq->mbox.dev, > > + "Fail to wait GCE thread 0x%x done\n", > > (u32)(thread->base - cmdq->base)); > > > > return -EFAULT; > > @@ -626,6 +670,7 @@ static int cmdq_probe(struct platform_device > > *pdev) > > > > WARN_ON(clk_bulk_prepare(cmdq->gce_num, cmdq->clocks)); > > > > + spin_lock_init(&cmdq->lock); > > cmdq_init(cmdq); > > > > return 0; > >
Hi, Yongqiang: On Mon, 2022-08-15 at 13:51 +0800, yongqiang.niu wrote: > On Mon, 2022-08-15 at 13:24 +0800, CK Hu wrote: > > Hi, Yongqiang: > > > > > > On Mon, 2022-08-15 at 11:07 +0800, Yongqiang Niu wrote: > > > From: Yongqiang Niu < > > > yongqiang.niu@mediatek.corp-partner.google.com > > > > > > > > > > 1. enable gce ddr enable(gce reigster offset 0x48, bit 16 to 18) > > > when > > > gce work, > > > and disable gce ddr enable when gce work job done > > > > Why need gce ddr enable? I think gce works fine without this. > > Describe > > more about this. > > > > Regards, > > CK > > this is only for some SOC which has flag "control_by_sw". > for this kind of gce, there is a handshake flow between gce and ddr > hardware, > if not set ddr enable flag of gce, ddr will fall into idle mode, > then gce instructions will not process done. > we need set this flag of gce to tell ddr when gce is idle or busy > controlled by software flow. You [1] and Jason [2] has already enable control_by_sw and I believe you two have test your patch in general. Is the ddr problem a special case? How to operate to trigger this bug? Describe more in commit message to let us know why need this patch. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mailbox/mtk-cmdq-mailbox.c?h=v6.0-rc1&id=84fd4201b78b96f8d31f6a2624be27ad6306a9bc [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mailbox/mtk-cmdq-mailbox.c?h=v6.0-rc1&id=9388501fbb99a1b6a23f28634d125567a3b45a3d > > > > > > 2. split cmdq clk enable/disable api, and control gce ddr > > > enable/disable > > > in clk enable/disable function to make sure it could protect when > > > cmdq > > > is multiple used by display and mdp > > > > > > Signed-off-by: Yongqiang Niu < > > > yongqiang.niu@mediatek.corp-partner.google.com> > > > Signed-off-by: Allen-kh Cheng < > > > allen-kh.cheng@mediatek.corp-partner.google.com> > > > --- > > > drivers/mailbox/mtk-cmdq-mailbox.c | 57 > > > ++++++++++++++++++++++++++ > > > ---- > > > 1 file changed, 51 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > > > b/drivers/mailbox/mtk-cmdq-mailbox.c > > > index 2578e5aaa935..64a47470f062 100644 > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > > @@ -81,6 +81,8 @@ struct cmdq { > > > u8 shift_pa; > > > bool control_by_sw; > > > u32 gce_num; > > > + atomic_t usage; > > > + spinlock_t lock; > > > }; > > > > > > struct gce_plat { > > > @@ -90,6 +92,46 @@ struct gce_plat { > > > u32 gce_num; > > > }; > > > > > > +static s32 cmdq_clk_enable(struct cmdq *cmdq) > > > +{ > > > + s32 usage, ret; > > > + unsigned long flags; > > > + > > > + spin_lock_irqsave(&cmdq->lock, flags); > > > + > > > + usage = atomic_inc_return(&cmdq->usage); > > > + > > > + ret = clk_bulk_enable(cmdq->gce_num, cmdq->clocks); > > > + if (usage <=0 || ret < 0) { > > > + dev_err(cmdq->mbox.dev, "ref count %d ret %d suspend > > > %d\n", > > > + usage, ret, cmdq->suspended); > > > + } else if (usage == 1) { > > > + if (cmdq->control_by_sw) > > > + writel((0x7 << 16) + 0x7, cmdq->base + > > > GCE_GCTL_VALUE); > > > + } > > > + > > > + spin_unlock_irqrestore(&cmdq->lock, flags); > > > + > > > + return ret; > > > +} > > > + > > > +static void cmdq_clk_disable(struct cmdq *cmdq) > > > +{ > > > + s32 usage; > > > + > > > + usage = atomic_dec_return(&cmdq->usage); > > > + > > > + if (usage < 0) { > > > + dev_err(cmdq->mbox.dev, "ref count %d suspend %d\n", > > > + usage, cmdq->suspended); > > > + } else if (usage == 0) { > > > + if (cmdq->control_by_sw) > > > + writel(0x7, cmdq->base + GCE_GCTL_VALUE); > > > + } > > > + > > > + clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > +} > > > + > > > u8 cmdq_get_shift_pa(struct mbox_chan *chan) > > > { > > > struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, > > > mbox); > > > @@ -271,7 +313,8 @@ static void cmdq_thread_irq_handler(struct > > > cmdq > > > *cmdq, > > > > > > if (list_empty(&thread->task_busy_list)) { > > > cmdq_thread_disable(cmdq, thread); > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > + > > > + cmdq_clk_disable(cmdq); > > > } > > > } > > > > > > @@ -360,8 +403,7 @@ static int cmdq_mbox_send_data(struct > > > mbox_chan > > > *chan, void *data) > > > task->pkt = pkt; > > > > > > if (list_empty(&thread->task_busy_list)) { > > > - WARN_ON(clk_bulk_enable(cmdq->gce_num, cmdq->clocks)); > > > - > > > + WARN_ON(cmdq_clk_enable(cmdq) < 0); > > > /* > > > * The thread reset will clear thread related register > > > to 0, > > > * including pc, end, priority, irq, suspend and > > > enable. Thus > > > @@ -433,7 +475,7 @@ static void cmdq_mbox_shutdown(struct > > > mbox_chan > > > *chan) > > > } > > > > > > cmdq_thread_disable(cmdq, thread); > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > + cmdq_clk_disable(cmdq); > > > > > > done: > > > /* > > > @@ -479,7 +521,8 @@ static int cmdq_mbox_flush(struct mbox_chan > > > *chan, unsigned long timeout) > > > > > > cmdq_thread_resume(thread); > > > cmdq_thread_disable(cmdq, thread); > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > + > > > + cmdq_clk_disable(cmdq); > > > > > > out: > > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > > @@ -490,7 +533,8 @@ static int cmdq_mbox_flush(struct mbox_chan > > > *chan, unsigned long timeout) > > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > > if (readl_poll_timeout_atomic(thread->base + > > > CMDQ_THR_ENABLE_TASK, > > > enable, enable == 0, 1, timeout)) > > > { > > > - dev_err(cmdq->mbox.dev, "Fail to wait GCE thread 0x%x > > > done\n", > > > + dev_err(cmdq->mbox.dev, > > > + "Fail to wait GCE thread 0x%x done\n", > > > (u32)(thread->base - cmdq->base)); > > > > > > return -EFAULT; > > > @@ -626,6 +670,7 @@ static int cmdq_probe(struct platform_device > > > *pdev) > > > > > > WARN_ON(clk_bulk_prepare(cmdq->gce_num, cmdq->clocks)); > > > > > > + spin_lock_init(&cmdq->lock); > > > cmdq_init(cmdq); > > > > > > return 0; > > > > > >
On Mon, 2022-08-15 at 14:23 +0800, CK Hu wrote: > Hi, Yongqiang: > > On Mon, 2022-08-15 at 13:51 +0800, yongqiang.niu wrote: > > On Mon, 2022-08-15 at 13:24 +0800, CK Hu wrote: > > > Hi, Yongqiang: > > > > > > > > > On Mon, 2022-08-15 at 11:07 +0800, Yongqiang Niu wrote: > > > > From: Yongqiang Niu < > > > > yongqiang.niu@mediatek.corp-partner.google.com > > > > > > > > > > > > > 1. enable gce ddr enable(gce reigster offset 0x48, bit 16 to > > > > 18) > > > > when > > > > gce work, > > > > and disable gce ddr enable when gce work job done > > > > > > Why need gce ddr enable? I think gce works fine without this. > > > Describe > > > more about this. > > > > > > Regards, > > > CK > > > > this is only for some SOC which has flag "control_by_sw". > > for this kind of gce, there is a handshake flow between gce and ddr > > hardware, > > if not set ddr enable flag of gce, ddr will fall into idle mode, > > then gce instructions will not process done. > > we need set this flag of gce to tell ddr when gce is idle or busy > > controlled by software flow. > > You [1] and Jason [2] has already enable control_by_sw and I believe > you two have test your patch in general. Is the ddr problem a special > case? How to operate to trigger this bug? Describe more in commit > message to let us know why need this patch. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mailbox/mtk-cmdq-mailbox.c?h=v6.0-rc1&id=84fd4201b78b96f8d31f6a2624be27ad6306a9bc > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mailbox/mtk-cmdq-mailbox.c?h=v6.0-rc1&id=9388501fbb99a1b6a23f28634d125567a3b45a3d > ddr problem is a special case. when test suspend/resume case, gce sometimes will pull ddr, and ddr can not go to suspend. if we set gce register 0x48 to 0x7, will fix this gce pull ddr issue, as you have referred [1] and [2] (8192 and 8195) but for mt8186, the gce is more special, except setting of [1] and [2], we need add more setting set gce register 0x48 to (0x7 << 16 | 0x7) when gce working to make sure gce could process all instructions ok. this case just need normal bootup, if we not set this, display cmdq task will timeout, and chrome homescreen will always black screen. and for this patch, we have done these test on mt8186: 1.suspend/resume 2.boot up to home screen 3.playback video with youtube. mt8195 is under testing > > > > > > > > > > 2. split cmdq clk enable/disable api, and control gce ddr > > > > enable/disable > > > > in clk enable/disable function to make sure it could protect > > > > when > > > > cmdq > > > > is multiple used by display and mdp > > > > > > > > Signed-off-by: Yongqiang Niu < > > > > yongqiang.niu@mediatek.corp-partner.google.com> > > > > Signed-off-by: Allen-kh Cheng < > > > > allen-kh.cheng@mediatek.corp-partner.google.com> > > > > --- > > > > drivers/mailbox/mtk-cmdq-mailbox.c | 57 > > > > ++++++++++++++++++++++++++ > > > > ---- > > > > 1 file changed, 51 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > > > > b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > index 2578e5aaa935..64a47470f062 100644 > > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > @@ -81,6 +81,8 @@ struct cmdq { > > > > u8 shift_pa; > > > > bool control_by_sw; > > > > u32 gce_num; > > > > + atomic_t usage; > > > > + spinlock_t lock; > > > > }; > > > > > > > > struct gce_plat { > > > > @@ -90,6 +92,46 @@ struct gce_plat { > > > > u32 gce_num; > > > > }; > > > > > > > > +static s32 cmdq_clk_enable(struct cmdq *cmdq) > > > > +{ > > > > + s32 usage, ret; > > > > + unsigned long flags; > > > > + > > > > + spin_lock_irqsave(&cmdq->lock, flags); > > > > + > > > > + usage = atomic_inc_return(&cmdq->usage); > > > > + > > > > + ret = clk_bulk_enable(cmdq->gce_num, cmdq->clocks); > > > > + if (usage <=0 || ret < 0) { > > > > + dev_err(cmdq->mbox.dev, "ref count %d ret %d > > > > suspend > > > > %d\n", > > > > + usage, ret, cmdq->suspended); > > > > + } else if (usage == 1) { > > > > + if (cmdq->control_by_sw) > > > > + writel((0x7 << 16) + 0x7, cmdq->base + > > > > GCE_GCTL_VALUE); > > > > + } > > > > + > > > > + spin_unlock_irqrestore(&cmdq->lock, flags); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static void cmdq_clk_disable(struct cmdq *cmdq) > > > > +{ > > > > + s32 usage; > > > > + > > > > + usage = atomic_dec_return(&cmdq->usage); > > > > + > > > > + if (usage < 0) { > > > > + dev_err(cmdq->mbox.dev, "ref count %d suspend > > > > %d\n", > > > > + usage, cmdq->suspended); > > > > + } else if (usage == 0) { > > > > + if (cmdq->control_by_sw) > > > > + writel(0x7, cmdq->base + > > > > GCE_GCTL_VALUE); > > > > + } > > > > + > > > > + clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > +} > > > > + > > > > u8 cmdq_get_shift_pa(struct mbox_chan *chan) > > > > { > > > > struct cmdq *cmdq = container_of(chan->mbox, struct > > > > cmdq, > > > > mbox); > > > > @@ -271,7 +313,8 @@ static void cmdq_thread_irq_handler(struct > > > > cmdq > > > > *cmdq, > > > > > > > > if (list_empty(&thread->task_busy_list)) { > > > > cmdq_thread_disable(cmdq, thread); > > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > + > > > > + cmdq_clk_disable(cmdq); > > > > } > > > > } > > > > > > > > @@ -360,8 +403,7 @@ static int cmdq_mbox_send_data(struct > > > > mbox_chan > > > > *chan, void *data) > > > > task->pkt = pkt; > > > > > > > > if (list_empty(&thread->task_busy_list)) { > > > > - WARN_ON(clk_bulk_enable(cmdq->gce_num, cmdq- > > > > >clocks)); > > > > - > > > > + WARN_ON(cmdq_clk_enable(cmdq) < 0); > > > > /* > > > > * The thread reset will clear thread related > > > > register > > > > to 0, > > > > * including pc, end, priority, irq, suspend > > > > and > > > > enable. Thus > > > > @@ -433,7 +475,7 @@ static void cmdq_mbox_shutdown(struct > > > > mbox_chan > > > > *chan) > > > > } > > > > > > > > cmdq_thread_disable(cmdq, thread); > > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > + cmdq_clk_disable(cmdq); > > > > > > > > done: > > > > /* > > > > @@ -479,7 +521,8 @@ static int cmdq_mbox_flush(struct mbox_chan > > > > *chan, unsigned long timeout) > > > > > > > > cmdq_thread_resume(thread); > > > > cmdq_thread_disable(cmdq, thread); > > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > + > > > > + cmdq_clk_disable(cmdq); > > > > > > > > out: > > > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > > > @@ -490,7 +533,8 @@ static int cmdq_mbox_flush(struct mbox_chan > > > > *chan, unsigned long timeout) > > > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > > > if (readl_poll_timeout_atomic(thread->base + > > > > CMDQ_THR_ENABLE_TASK, > > > > enable, enable == 0, 1, > > > > timeout)) > > > > { > > > > - dev_err(cmdq->mbox.dev, "Fail to wait GCE > > > > thread 0x%x > > > > done\n", > > > > + dev_err(cmdq->mbox.dev, > > > > + "Fail to wait GCE thread 0x%x done\n", > > > > (u32)(thread->base - cmdq->base)); > > > > > > > > return -EFAULT; > > > > @@ -626,6 +670,7 @@ static int cmdq_probe(struct > > > > platform_device > > > > *pdev) > > > > > > > > WARN_ON(clk_bulk_prepare(cmdq->gce_num, cmdq->clocks)); > > > > > > > > + spin_lock_init(&cmdq->lock); > > > > cmdq_init(cmdq); > > > > > > > > return 0; > > > > > > > > > > > >
Hi, Yongqiang: On Mon, 2022-08-15 at 17:51 +0800, yongqiang.niu wrote: > On Mon, 2022-08-15 at 14:23 +0800, CK Hu wrote: > > Hi, Yongqiang: > > > > On Mon, 2022-08-15 at 13:51 +0800, yongqiang.niu wrote: > > > On Mon, 2022-08-15 at 13:24 +0800, CK Hu wrote: > > > > Hi, Yongqiang: > > > > > > > > > > > > On Mon, 2022-08-15 at 11:07 +0800, Yongqiang Niu wrote: > > > > > From: Yongqiang Niu < > > > > > yongqiang.niu@mediatek.corp-partner.google.com > > > > > > > > > > > > > > > > 1. enable gce ddr enable(gce reigster offset 0x48, bit 16 to > > > > > 18) > > > > > when > > > > > gce work, > > > > > and disable gce ddr enable when gce work job done > > > > > > > > Why need gce ddr enable? I think gce works fine without this. > > > > Describe > > > > more about this. > > > > > > > > Regards, > > > > CK > > > > > > this is only for some SOC which has flag "control_by_sw". > > > for this kind of gce, there is a handshake flow between gce and > > > ddr > > > hardware, > > > if not set ddr enable flag of gce, ddr will fall into idle mode, > > > then gce instructions will not process done. > > > we need set this flag of gce to tell ddr when gce is idle or busy > > > controlled by software flow. > > > > You [1] and Jason [2] has already enable control_by_sw and I > > believe > > you two have test your patch in general. Is the ddr problem a > > special > > case? How to operate to trigger this bug? Describe more in commit > > message to let us know why need this patch. > > > > [1] > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mailbox/mtk-cmdq-mailbox.c?h=v6.0-rc1&id=84fd4201b78b96f8d31f6a2624be27ad6306a9bc > > [2] > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mailbox/mtk-cmdq-mailbox.c?h=v6.0-rc1&id=9388501fbb99a1b6a23f28634d125567a3b45a3d > > > > ddr problem is a special case. > when test suspend/resume case, gce sometimes will pull ddr, and ddr > can > not go to suspend. > if we set gce register 0x48 to 0x7, will fix this gce pull ddr issue, > as you have referred [1] and [2] (8192 and 8195) > > but for mt8186, the gce is more special, except setting of [1] and > [2], > we need add more setting set gce register 0x48 to (0x7 << 16 | 0x7) > when gce working to make sure gce could process all instructions ok. > this case just need normal bootup, if we not set this, display cmdq > task will timeout, and chrome homescreen will always black screen. > > and for this patch, we have done these test on mt8186: > 1.suspend/resume > 2.boot up to home screen > 3.playback video with youtube. > > mt8195 is under testing I seems there are two problem. The first is suspend problem which is fixed by [1] and [2]. The second is boot problem which only happen in MT8186. So add a new driver private data for the boot problem. For the suspend problem, I think the correct suspend flow is: 1. Client driver suspend: flush all command in mailbox channel. 2. gce driver suspend: Do nothing to gce hardware because all gce thread is empty. 3. System suspend: suspend the ddr. If you find that gce pull the ddr when ddr suspend, that means client driver does not flush all command when suspend, so fix the client driver and gce would not pull ddr when ddr suspend. Regards, CK > > > > > > > > > > > > > > > 2. split cmdq clk enable/disable api, and control gce ddr > > > > > enable/disable > > > > > in clk enable/disable function to make sure it could protect > > > > > when > > > > > cmdq > > > > > is multiple used by display and mdp > > > > > > > > > > Signed-off-by: Yongqiang Niu < > > > > > yongqiang.niu@mediatek.corp-partner.google.com> > > > > > Signed-off-by: Allen-kh Cheng < > > > > > allen-kh.cheng@mediatek.corp-partner.google.com> > > > > > --- > > > > > drivers/mailbox/mtk-cmdq-mailbox.c | 57 > > > > > ++++++++++++++++++++++++++ > > > > > ---- > > > > > 1 file changed, 51 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > index 2578e5aaa935..64a47470f062 100644 > > > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > @@ -81,6 +81,8 @@ struct cmdq { > > > > > u8 shift_pa; > > > > > bool control_by_sw; > > > > > u32 gce_num; > > > > > + atomic_t usage; > > > > > + spinlock_t lock; > > > > > }; > > > > > > > > > > struct gce_plat { > > > > > @@ -90,6 +92,46 @@ struct gce_plat { > > > > > u32 gce_num; > > > > > }; > > > > > > > > > > +static s32 cmdq_clk_enable(struct cmdq *cmdq) > > > > > +{ > > > > > + s32 usage, ret; > > > > > + unsigned long flags; > > > > > + > > > > > + spin_lock_irqsave(&cmdq->lock, flags); > > > > > + > > > > > + usage = atomic_inc_return(&cmdq->usage); > > > > > + > > > > > + ret = clk_bulk_enable(cmdq->gce_num, cmdq->clocks); > > > > > + if (usage <=0 || ret < 0) { > > > > > + dev_err(cmdq->mbox.dev, "ref count %d ret %d > > > > > suspend > > > > > %d\n", > > > > > + usage, ret, cmdq->suspended); > > > > > + } else if (usage == 1) { > > > > > + if (cmdq->control_by_sw) > > > > > + writel((0x7 << 16) + 0x7, cmdq->base + > > > > > GCE_GCTL_VALUE); > > > > > + } > > > > > + > > > > > + spin_unlock_irqrestore(&cmdq->lock, flags); > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +static void cmdq_clk_disable(struct cmdq *cmdq) > > > > > +{ > > > > > + s32 usage; > > > > > + > > > > > + usage = atomic_dec_return(&cmdq->usage); > > > > > + > > > > > + if (usage < 0) { > > > > > + dev_err(cmdq->mbox.dev, "ref count %d suspend > > > > > %d\n", > > > > > + usage, cmdq->suspended); > > > > > + } else if (usage == 0) { > > > > > + if (cmdq->control_by_sw) > > > > > + writel(0x7, cmdq->base + > > > > > GCE_GCTL_VALUE); > > > > > + } > > > > > + > > > > > + clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > > +} > > > > > + > > > > > u8 cmdq_get_shift_pa(struct mbox_chan *chan) > > > > > { > > > > > struct cmdq *cmdq = container_of(chan->mbox, struct > > > > > cmdq, > > > > > mbox); > > > > > @@ -271,7 +313,8 @@ static void > > > > > cmdq_thread_irq_handler(struct > > > > > cmdq > > > > > *cmdq, > > > > > > > > > > if (list_empty(&thread->task_busy_list)) { > > > > > cmdq_thread_disable(cmdq, thread); > > > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > > + > > > > > + cmdq_clk_disable(cmdq); > > > > > } > > > > > } > > > > > > > > > > @@ -360,8 +403,7 @@ static int cmdq_mbox_send_data(struct > > > > > mbox_chan > > > > > *chan, void *data) > > > > > task->pkt = pkt; > > > > > > > > > > if (list_empty(&thread->task_busy_list)) { > > > > > - WARN_ON(clk_bulk_enable(cmdq->gce_num, cmdq- > > > > > > clocks)); > > > > > > > > > > - > > > > > + WARN_ON(cmdq_clk_enable(cmdq) < 0); > > > > > /* > > > > > * The thread reset will clear thread related > > > > > register > > > > > to 0, > > > > > * including pc, end, priority, irq, suspend > > > > > and > > > > > enable. Thus > > > > > @@ -433,7 +475,7 @@ static void cmdq_mbox_shutdown(struct > > > > > mbox_chan > > > > > *chan) > > > > > } > > > > > > > > > > cmdq_thread_disable(cmdq, thread); > > > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > > + cmdq_clk_disable(cmdq); > > > > > > > > > > done: > > > > > /* > > > > > @@ -479,7 +521,8 @@ static int cmdq_mbox_flush(struct > > > > > mbox_chan > > > > > *chan, unsigned long timeout) > > > > > > > > > > cmdq_thread_resume(thread); > > > > > cmdq_thread_disable(cmdq, thread); > > > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > > + > > > > > + cmdq_clk_disable(cmdq); > > > > > > > > > > out: > > > > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > > > > @@ -490,7 +533,8 @@ static int cmdq_mbox_flush(struct > > > > > mbox_chan > > > > > *chan, unsigned long timeout) > > > > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > > > > if (readl_poll_timeout_atomic(thread->base + > > > > > CMDQ_THR_ENABLE_TASK, > > > > > enable, enable == 0, 1, > > > > > timeout)) > > > > > { > > > > > - dev_err(cmdq->mbox.dev, "Fail to wait GCE > > > > > thread 0x%x > > > > > done\n", > > > > > + dev_err(cmdq->mbox.dev, > > > > > + "Fail to wait GCE thread 0x%x done\n", > > > > > (u32)(thread->base - cmdq->base)); > > > > > > > > > > return -EFAULT; > > > > > @@ -626,6 +670,7 @@ static int cmdq_probe(struct > > > > > platform_device > > > > > *pdev) > > > > > > > > > > WARN_ON(clk_bulk_prepare(cmdq->gce_num, cmdq->clocks)); > > > > > > > > > > + spin_lock_init(&cmdq->lock); > > > > > cmdq_init(cmdq); > > > > > > > > > > return 0; > > > > > > > > > > > > > > > > > > > >
On Tue, 2022-08-16 at 09:49 +0800, CK Hu wrote: > Hi, Yongqiang: > > On Mon, 2022-08-15 at 17:51 +0800, yongqiang.niu wrote: > > On Mon, 2022-08-15 at 14:23 +0800, CK Hu wrote: > > > Hi, Yongqiang: > > > > > > On Mon, 2022-08-15 at 13:51 +0800, yongqiang.niu wrote: > > > > On Mon, 2022-08-15 at 13:24 +0800, CK Hu wrote: > > > > > Hi, Yongqiang: > > > > > > > > > > > > > > > On Mon, 2022-08-15 at 11:07 +0800, Yongqiang Niu wrote: > > > > > > From: Yongqiang Niu < > > > > > > yongqiang.niu@mediatek.corp-partner.google.com > > > > > > > > > > > > > > > > > > > 1. enable gce ddr enable(gce reigster offset 0x48, bit 16 > > > > > > to > > > > > > 18) > > > > > > when > > > > > > gce work, > > > > > > and disable gce ddr enable when gce work job done > > > > > > > > > > Why need gce ddr enable? I think gce works fine without this. > > > > > Describe > > > > > more about this. > > > > > > > > > > Regards, > > > > > CK > > > > > > > > this is only for some SOC which has flag "control_by_sw". > > > > for this kind of gce, there is a handshake flow between gce and > > > > ddr > > > > hardware, > > > > if not set ddr enable flag of gce, ddr will fall into idle > > > > mode, > > > > then gce instructions will not process done. > > > > we need set this flag of gce to tell ddr when gce is idle or > > > > busy > > > > controlled by software flow. > > > > > > You [1] and Jason [2] has already enable control_by_sw and I > > > believe > > > you two have test your patch in general. Is the ddr problem a > > > special > > > case? How to operate to trigger this bug? Describe more in commit > > > message to let us know why need this patch. > > > > > > [1] > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mailbox/mtk-cmdq-mailbox.c?h=v6.0-rc1&id=84fd4201b78b96f8d31f6a2624be27ad6306a9bc > > > [2] > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mailbox/mtk-cmdq-mailbox.c?h=v6.0-rc1&id=9388501fbb99a1b6a23f28634d125567a3b45a3d > > > > > > > ddr problem is a special case. > > when test suspend/resume case, gce sometimes will pull ddr, and ddr > > can > > not go to suspend. > > if we set gce register 0x48 to 0x7, will fix this gce pull ddr > > issue, > > as you have referred [1] and [2] (8192 and 8195) > > > > but for mt8186, the gce is more special, except setting of [1] and > > [2], > > we need add more setting set gce register 0x48 to (0x7 << 16 | 0x7) > > when gce working to make sure gce could process all instructions > > ok. > > this case just need normal bootup, if we not set this, display cmdq > > task will timeout, and chrome homescreen will always black screen. > > > > and for this patch, we have done these test on mt8186: > > 1.suspend/resume > > 2.boot up to home screen > > 3.playback video with youtube. > > > > mt8195 is under testing > > I seems there are two problem. The first is suspend problem which is > fixed by [1] and [2]. The second is boot problem which only happen in > MT8186. So add a new driver private data for the boot problem. > > For the suspend problem, I think the correct suspend flow is: > > 1. Client driver suspend: flush all command in mailbox channel. > 2. gce driver suspend: Do nothing to gce hardware because all gce > thread is empty. > 3. System suspend: suspend the ddr. > > If you find that gce pull the ddr when ddr suspend, that means client > driver does not flush all command when suspend, so fix the client > driver and gce would not pull ddr when ddr suspend. > > Regards, > CK > 1.suspend issue is special gce hardware issue, gce client driver command already process done, but gce still pull ddr. 2. I will add new private data for MT8186 boot up issue in next version > > > > > > > > > > > > > > > > > > > > 2. split cmdq clk enable/disable api, and control gce ddr > > > > > > enable/disable > > > > > > in clk enable/disable function to make sure it could > > > > > > protect > > > > > > when > > > > > > cmdq > > > > > > is multiple used by display and mdp > > > > > > > > > > > > Signed-off-by: Yongqiang Niu < > > > > > > yongqiang.niu@mediatek.corp-partner.google.com> > > > > > > Signed-off-by: Allen-kh Cheng < > > > > > > allen-kh.cheng@mediatek.corp-partner.google.com> > > > > > > --- > > > > > > drivers/mailbox/mtk-cmdq-mailbox.c | 57 > > > > > > ++++++++++++++++++++++++++ > > > > > > ---- > > > > > > 1 file changed, 51 insertions(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > > b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > > index 2578e5aaa935..64a47470f062 100644 > > > > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > > > > > @@ -81,6 +81,8 @@ struct cmdq { > > > > > > u8 shift_pa; > > > > > > bool control_by_sw; > > > > > > u32 gce_num; > > > > > > + atomic_t usage; > > > > > > + spinlock_t lock; > > > > > > }; > > > > > > > > > > > > struct gce_plat { > > > > > > @@ -90,6 +92,46 @@ struct gce_plat { > > > > > > u32 gce_num; > > > > > > }; > > > > > > > > > > > > +static s32 cmdq_clk_enable(struct cmdq *cmdq) > > > > > > +{ > > > > > > + s32 usage, ret; > > > > > > + unsigned long flags; > > > > > > + > > > > > > + spin_lock_irqsave(&cmdq->lock, flags); > > > > > > + > > > > > > + usage = atomic_inc_return(&cmdq->usage); > > > > > > + > > > > > > + ret = clk_bulk_enable(cmdq->gce_num, cmdq->clocks); > > > > > > + if (usage <=0 || ret < 0) { > > > > > > + dev_err(cmdq->mbox.dev, "ref count %d ret %d > > > > > > suspend > > > > > > %d\n", > > > > > > + usage, ret, cmdq->suspended); > > > > > > + } else if (usage == 1) { > > > > > > + if (cmdq->control_by_sw) > > > > > > + writel((0x7 << 16) + 0x7, cmdq->base + > > > > > > GCE_GCTL_VALUE); > > > > > > + } > > > > > > + > > > > > > + spin_unlock_irqrestore(&cmdq->lock, flags); > > > > > > + > > > > > > + return ret; > > > > > > +} > > > > > > + > > > > > > +static void cmdq_clk_disable(struct cmdq *cmdq) > > > > > > +{ > > > > > > + s32 usage; > > > > > > + > > > > > > + usage = atomic_dec_return(&cmdq->usage); > > > > > > + > > > > > > + if (usage < 0) { > > > > > > + dev_err(cmdq->mbox.dev, "ref count %d suspend > > > > > > %d\n", > > > > > > + usage, cmdq->suspended); > > > > > > + } else if (usage == 0) { > > > > > > + if (cmdq->control_by_sw) > > > > > > + writel(0x7, cmdq->base + > > > > > > GCE_GCTL_VALUE); > > > > > > + } > > > > > > + > > > > > > + clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > > > +} > > > > > > + > > > > > > u8 cmdq_get_shift_pa(struct mbox_chan *chan) > > > > > > { > > > > > > struct cmdq *cmdq = container_of(chan->mbox, struct > > > > > > cmdq, > > > > > > mbox); > > > > > > @@ -271,7 +313,8 @@ static void > > > > > > cmdq_thread_irq_handler(struct > > > > > > cmdq > > > > > > *cmdq, > > > > > > > > > > > > if (list_empty(&thread->task_busy_list)) { > > > > > > cmdq_thread_disable(cmdq, thread); > > > > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > > > + > > > > > > + cmdq_clk_disable(cmdq); > > > > > > } > > > > > > } > > > > > > > > > > > > @@ -360,8 +403,7 @@ static int cmdq_mbox_send_data(struct > > > > > > mbox_chan > > > > > > *chan, void *data) > > > > > > task->pkt = pkt; > > > > > > > > > > > > if (list_empty(&thread->task_busy_list)) { > > > > > > - WARN_ON(clk_bulk_enable(cmdq->gce_num, cmdq- > > > > > > > clocks)); > > > > > > > > > > > > - > > > > > > + WARN_ON(cmdq_clk_enable(cmdq) < 0); > > > > > > /* > > > > > > * The thread reset will clear thread related > > > > > > register > > > > > > to 0, > > > > > > * including pc, end, priority, irq, suspend > > > > > > and > > > > > > enable. Thus > > > > > > @@ -433,7 +475,7 @@ static void cmdq_mbox_shutdown(struct > > > > > > mbox_chan > > > > > > *chan) > > > > > > } > > > > > > > > > > > > cmdq_thread_disable(cmdq, thread); > > > > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > > > + cmdq_clk_disable(cmdq); > > > > > > > > > > > > done: > > > > > > /* > > > > > > @@ -479,7 +521,8 @@ static int cmdq_mbox_flush(struct > > > > > > mbox_chan > > > > > > *chan, unsigned long timeout) > > > > > > > > > > > > cmdq_thread_resume(thread); > > > > > > cmdq_thread_disable(cmdq, thread); > > > > > > - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); > > > > > > + > > > > > > + cmdq_clk_disable(cmdq); > > > > > > > > > > > > out: > > > > > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > > > > > @@ -490,7 +533,8 @@ static int cmdq_mbox_flush(struct > > > > > > mbox_chan > > > > > > *chan, unsigned long timeout) > > > > > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > > > > > if (readl_poll_timeout_atomic(thread->base + > > > > > > CMDQ_THR_ENABLE_TASK, > > > > > > enable, enable == 0, 1, > > > > > > timeout)) > > > > > > { > > > > > > - dev_err(cmdq->mbox.dev, "Fail to wait GCE > > > > > > thread 0x%x > > > > > > done\n", > > > > > > + dev_err(cmdq->mbox.dev, > > > > > > + "Fail to wait GCE thread 0x%x done\n", > > > > > > (u32)(thread->base - cmdq->base)); > > > > > > > > > > > > return -EFAULT; > > > > > > @@ -626,6 +670,7 @@ static int cmdq_probe(struct > > > > > > platform_device > > > > > > *pdev) > > > > > > > > > > > > WARN_ON(clk_bulk_prepare(cmdq->gce_num, cmdq->clocks)); > > > > > > > > > > > > + spin_lock_init(&cmdq->lock); > > > > > > cmdq_init(cmdq); > > > > > > > > > > > > return 0; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c index 2578e5aaa935..64a47470f062 100644 --- a/drivers/mailbox/mtk-cmdq-mailbox.c +++ b/drivers/mailbox/mtk-cmdq-mailbox.c @@ -81,6 +81,8 @@ struct cmdq { u8 shift_pa; bool control_by_sw; u32 gce_num; + atomic_t usage; + spinlock_t lock; }; struct gce_plat { @@ -90,6 +92,46 @@ struct gce_plat { u32 gce_num; }; +static s32 cmdq_clk_enable(struct cmdq *cmdq) +{ + s32 usage, ret; + unsigned long flags; + + spin_lock_irqsave(&cmdq->lock, flags); + + usage = atomic_inc_return(&cmdq->usage); + + ret = clk_bulk_enable(cmdq->gce_num, cmdq->clocks); + if (usage <=0 || ret < 0) { + dev_err(cmdq->mbox.dev, "ref count %d ret %d suspend %d\n", + usage, ret, cmdq->suspended); + } else if (usage == 1) { + if (cmdq->control_by_sw) + writel((0x7 << 16) + 0x7, cmdq->base + GCE_GCTL_VALUE); + } + + spin_unlock_irqrestore(&cmdq->lock, flags); + + return ret; +} + +static void cmdq_clk_disable(struct cmdq *cmdq) +{ + s32 usage; + + usage = atomic_dec_return(&cmdq->usage); + + if (usage < 0) { + dev_err(cmdq->mbox.dev, "ref count %d suspend %d\n", + usage, cmdq->suspended); + } else if (usage == 0) { + if (cmdq->control_by_sw) + writel(0x7, cmdq->base + GCE_GCTL_VALUE); + } + + clk_bulk_disable(cmdq->gce_num, cmdq->clocks); +} + u8 cmdq_get_shift_pa(struct mbox_chan *chan) { struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, mbox); @@ -271,7 +313,8 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq, if (list_empty(&thread->task_busy_list)) { cmdq_thread_disable(cmdq, thread); - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); + + cmdq_clk_disable(cmdq); } } @@ -360,8 +403,7 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data) task->pkt = pkt; if (list_empty(&thread->task_busy_list)) { - WARN_ON(clk_bulk_enable(cmdq->gce_num, cmdq->clocks)); - + WARN_ON(cmdq_clk_enable(cmdq) < 0); /* * The thread reset will clear thread related register to 0, * including pc, end, priority, irq, suspend and enable. Thus @@ -433,7 +475,7 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan) } cmdq_thread_disable(cmdq, thread); - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); + cmdq_clk_disable(cmdq); done: /* @@ -479,7 +521,8 @@ static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long timeout) cmdq_thread_resume(thread); cmdq_thread_disable(cmdq, thread); - clk_bulk_disable(cmdq->gce_num, cmdq->clocks); + + cmdq_clk_disable(cmdq); out: spin_unlock_irqrestore(&thread->chan->lock, flags); @@ -490,7 +533,8 @@ static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long timeout) spin_unlock_irqrestore(&thread->chan->lock, flags); if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_ENABLE_TASK, enable, enable == 0, 1, timeout)) { - dev_err(cmdq->mbox.dev, "Fail to wait GCE thread 0x%x done\n", + dev_err(cmdq->mbox.dev, + "Fail to wait GCE thread 0x%x done\n", (u32)(thread->base - cmdq->base)); return -EFAULT; @@ -626,6 +670,7 @@ static int cmdq_probe(struct platform_device *pdev) WARN_ON(clk_bulk_prepare(cmdq->gce_num, cmdq->clocks)); + spin_lock_init(&cmdq->lock); cmdq_init(cmdq); return 0;