[PATCH v5] mailbox: mtk-cmdq: fix gce timeout issue

[yongqiang.niu]

On Tue, 2022-09-27 at 13:14 +0200, AngeloGioacchino Del Regno wrote:
> Il 27/09/22 10:39, Yongqiang Niu ha scritto:
> > 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
> > 2. add cmdq ddr enable/disable api, and control gce ddr
> > enable/disable
> > to make sure it could protect when cmdq is multiple used by display
> > and mdp
> > 
> > this is only for some SOC which has flag "gce_ddr_en".
> > 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.
> > 
> > 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 with this patch, we have done these test on mt8186:
> > 1.suspend/resume
> > 2.boot up to home screen
> > 3.playback video with youtube.
> > 
> > suspend issue is special gce hardware issue, gce client  driver
> > command already process done, but gce still pull ddr.
> > 
> > Signed-off-by: Yongqiang Niu <yongqiang.niu at mediatek.com>
> > ---
> > change since v4:
> > 1. remove spin lock control flow
> > 2. move ddr control into suspend/resume/remove function
> > ---
> > 
> > ---
> >   drivers/mailbox/mtk-cmdq-mailbox.c | 44
> > +++++++++++++++++++++++++++++-
> >   1 file changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 9465f9081515..e9ead72b9bd3 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -38,6 +38,8 @@
> >   #define CMDQ_THR_PRIORITY		0x40
> >   
> >   #define GCE_GCTL_VALUE			0x48
> > +#define GCE_CTRL_BY_SW				GENMASK(18, 16)
> > +#define GCE_DDR_EN				GENMASK(2, 0)
> >   
> >   #define CMDQ_THR_ACTIVE_SLOT_CYCLES	0x3200
> >   #define CMDQ_THR_ENABLED		0x1
> > @@ -80,6 +82,7 @@ struct cmdq {
> >   	bool			suspended;
> >   	u8			shift_pa;
> >   	bool			control_by_sw;
> > +	bool			sw_ddr_en;
> >   	u32			gce_num;
> >   };
> >   
> > @@ -87,9 +90,25 @@ struct gce_plat {
> >   	u32 thread_nr;
> >   	u8 shift;
> >   	bool control_by_sw;
> > +	bool sw_ddr_en;
> >   	u32 gce_num;
> >   };
> >   
> > +static void cmdq_sw_ddr_enable(struct cmdq *cmdq, bool enable)
> > +{
> > +	if (!cmdq->sw_ddr_en)
> > +		return;
> > +
> > +	WARN_ON(clk_bulk_enable(cmdq->gce_num, cmdq->clocks));
> > +
> > +	if (enable)
> > +		writel(GCE_DDR_EN + GCE_CTRL_BY_SW, cmdq->base +
> > GCE_GCTL_VALUE);
> 
> Adding up values to activate bits in this case works, but that's
> wrong.
> You want GCE_DDR_EN | GCE_CTRL_BY_SW.
> 
> Regards,
> Angelo

this will be fixed in next version.