Re: [PATCH v9, 2/4] mailbox: mtk-cmdq: add gce software ddr enable private data

[AngeloGioacchino Del Regno]

Il 07/10/22 03:51, yongqiang.niu ha scritto:
> On Thu, 2022-10-06 at 11:29 +0200, AngeloGioacchino Del Regno wrote:
> > Il 06/10/22 06:34, Yongqiang Niu ha scritto:
> > > if gce work control by software, we need set software enable
> > > for MT8186 Soc
> > >
> > > 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.
> > >
> > > 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
> > >
> > > Signed-off-by: Yongqiang Niu <yongqiang.niu@xxxxxxxxxxxx>
> > > ---
> > >    drivers/mailbox/mtk-cmdq-mailbox.c | 7 +++++++
> > >    1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > index c3cb24f51699..04eb44d89119 100644
> > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > @@ -39,6 +39,7 @@
> > >    
> > >    #define GCE_GCTL_VALUE			0x48
> > >    #define GCE_CTRL_BY_SW				GENMASK(2, 0)
> > > +#define GCE_DDR_EN				GENMASK(18, 16)
> > >    
> > >    #define CMDQ_THR_ACTIVE_SLOT_CYCLES	0x3200
> > >    #define CMDQ_THR_ENABLED		0x1
> > > @@ -81,6 +82,7 @@ struct cmdq {
> > >    	bool			suspended;
> > >    	u8			shift_pa;
> > >    	bool			control_by_sw;
> > > +	bool			sw_ddr_en;
> > >    	u32			gce_num;
> > >    };
> > >    
> > > @@ -88,6 +90,7 @@ struct gce_plat {
> > >    	u32 thread_nr;
> > >    	u8 shift;
> > >    	bool control_by_sw;
> > > +	bool sw_ddr_en;
> > >    	u32 gce_num;
> > >    };
> > >    
> > > @@ -132,6 +135,9 @@ static void cmdq_init(struct cmdq *cmdq)
> > >    	if (cmdq->control_by_sw)
> > >    		writel(GCE_CTRL_BY_SW, cmdq->base + GCE_GCTL_VALUE);
> > >    
> > > +	if (cmdq->sw_ddr_en)
> > > +		writel(GCE_DDR_EN | GCE_CTRL_BY_SW, cmdq->base +
> > > GCE_GCTL_VALUE);
> > > +
> >
> > No. That's redundant.
> > Here's a better way:
> >
> > 	u32 gctl_regval = 0;
> >
> > 	if (cmdq->control_by_sw)
> > 		gctl_regval = GCE_CTRL_BY_SW;
> > 	if (cmdq->sw_ddr_en)
> > 		gctl_regval |= GCE_DDR_EN;
> >
> > 	if (val)
> > 		writel(gctl_regval, cmdq->base + GCE_GCTL_VALUE);
> >
> > Regards,
> > Angelo
>
> thanks very much for your advise.
> shall i separate this into two patches?
> 1st one is
> 	u32 gctl_regval = 0;
> 	if (cmdq->control_by_sw)
> > 		gctl_regval = GCE_CTRL_BY_SW;
> > 	if (val)
> > 		writel(gctl_regval, cmdq->base + GCE_GCTL_VALUE);
>
>
>
> 2nd just add this
> 	if (cmdq->sw_ddr_en)
> > 		gctl_regval |= GCE_DDR_EN;
>
> or one patch is ok?

One patch is ok.

Thanks,
Angelo

> > >    	writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base +
> > > CMDQ_THR_SLOT_CYCLES);
> > >    	for (i = 0; i <= CMDQ_MAX_EVENT; i++)
> > >    		writel(i, cmdq->base + CMDQ_SYNC_TOKEN_UPDATE);
> > > @@ -545,6 +551,7 @@ static int cmdq_probe(struct platform_device
> > > *pdev)
> > >    	cmdq->thread_nr = plat_data->thread_nr;
> > >    	cmdq->shift_pa = plat_data->shift;
> > >    	cmdq->control_by_sw = plat_data->control_by_sw;
> > > +	cmdq->sw_ddr_en = plat_data->sw_ddr_en;
> > >    	cmdq->gce_num = plat_data->gce_num;
> > >    	cmdq->irq_mask = GENMASK(cmdq->thread_nr - 1, 0);
> > >    	err = devm_request_irq(dev, cmdq->irq, cmdq_irq_handler,
> > > IRQF_SHARED,