[PATCH v1, 1/1] mailbox: mtk-cmdq: fix gce timeout issue
yongqiang.niu
yongqiang.niu at mediatek.com
Mon Aug 15 19:14:00 PDT 2022
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 at 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 at mediatek.corp-partner.google.com>
> > > > > > Signed-off-by: Allen-kh Cheng <
> > > > > > allen-kh.cheng at 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;
> > > > >
> > > > >
> > > >
> > > >
> > >
> > >
> >
> >
>
>
More information about the linux-arm-kernel
mailing list