[PATCH v1, 1/1] mailbox: mtk-cmdq: fix gce timeout issue

CK Hu ck.hu at mediatek.com
Sun Aug 14 23:23:54 PDT 2022


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


> 
> > 
> > > 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-mediatek mailing list