Re: [PATCH v8, 3/4] mailbox: mtk-cmdq: add gce ddr enable support flow

[AngeloGioacchino Del Regno]

Il 04/10/22 11:39, yongqiang.niu ha scritto:
> On Mon, 2022-10-03 at 16:54 +0200, AngeloGioacchino Del Regno wrote:
> > Il 30/09/22 18:06, Yongqiang Niu ha scritto:
> > > add gce ddr enable control flow when gce suspend/resume
> > >
> > > Signed-off-by: Yongqiang Niu <yongqiang.niu@xxxxxxxxxxxx>
> > > ---
> > >    drivers/mailbox/mtk-cmdq-mailbox.c | 22 ++++++++++++++++++++++
> > >    1 file changed, 22 insertions(+)
> > >
> > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > index 04eb44d89119..2db82ff838ed 100644
> > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > @@ -94,6 +94,18 @@ struct gce_plat {
> > >    	u32 gce_num;
> > >    };
> > >    
> > > +static void cmdq_sw_ddr_enable(struct cmdq *cmdq, bool enable)
> > > +{
> > > +	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);
> >
> > My only concern here is about the previous value stored in the
> > GCE_GCTL_VALUE
> > register, as you're overwriting it in its entirety with
> > GCE_DDR_EN | GCE_CTRL_BY_SW.
> >
> > Can you guarantee that this register is not pre-initialized with some
> > value,
> > and that these are the only bits to be `1` in this register?
> >
> > Otherwise, you will have to readl and modify the bits instead... by
> > the way,
> > if this register doesn't get any changes during runtime, you may
> > cache it
> > at probe time to avoid reading it for every suspend/resume operation.
> >
> > Regards,
> > Angelo
>
> 0x48[2:0] means control by software
> 0x48[18:16] means ddr enable
> 0x48[2:0] is pre-condition of 0x48[18:16].
> if we want set 0x48[18:16] ddr enable, 0x48[2:0] must be set at same
> time.
> and only these bits is useful, other bits is useless bits
>
> we need set 0x48[18:16] to 0 disable gce access ddr when suspend.
> and  set 0x48[18:16] to 0x7 enable gce access ddr when resume, there
> will be cmdq client send task to process.
> this control flow should controlled in suspend/resume flow.

That's perfect. Thanks for the explanation.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>