Re: [PATCH v7 18/26] PM / devfreq: rockchip-dfi: account for multiple DDRMON_CTRL registers

[Sascha Hauer]

On Mon, Oct 09, 2023 at 07:19:04AM +0900, Chanwoo Choi wrote:
> On 23. 7. 4. 18:32, Sascha Hauer wrote:
> > The currently supported RK3399 has a set of registers per channel, but
> > it has only a single DDRMON_CTRL register. With upcoming RK3588 this
> > will be different, the RK3588 has a DDRMON_CTRL register per channel.
> > 
> > Instead of expecting a single DDRMON_CTRL register, loop over the
> > channels and write the channel specific DDRMON_CTRL register. Break
> > out early out of the loop when there is only a single DDRMON_CTRL
> > register like on the RK3399.
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 72 ++++++++++++++++++----------
> >  1 file changed, 48 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index 85ec93fd41858..2362d3953ba40 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -113,12 +113,13 @@ struct rockchip_dfi {
> >  	int burst_len;
> >  	int buswidth[DMC_MAX_CHANNELS];
> >  	int ddrmon_stride;
> > +	bool ddrmon_ctrl_single;
> >  };
> >  
> >  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> >  {
> >  	void __iomem *dfi_regs = dfi->regs;
> > -	int ret = 0;
> > +	int i, ret = 0;
> >  
> >  	mutex_lock(&dfi->mutex);
> >  
> > @@ -132,29 +133,41 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> >  		goto out;
> >  	}
> >  
> > -	/* clear DDRMON_CTRL setting */
> > -	writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN | DDRMON_CTRL_SOFTWARE_EN |
> > -		       DDRMON_CTRL_HARDWARE_EN), dfi_regs + DDRMON_CTRL);
> > +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> > +		u32 ctrl = 0;
> >  
> > -	/* set ddr type to dfi */
> > -	switch (dfi->ddr_type) {
> > -	case ROCKCHIP_DDRTYPE_LPDDR2:
> > -	case ROCKCHIP_DDRTYPE_LPDDR3:
> > -		writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23, DDRMON_CTRL_DDR_TYPE_MASK),
> > -			       dfi_regs + DDRMON_CTRL);
> > -		break;
> > -	case ROCKCHIP_DDRTYPE_LPDDR4:
> > -	case ROCKCHIP_DDRTYPE_LPDDR4X:
> > -		writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
> > -			       dfi_regs + DDRMON_CTRL);
> > -		break;
> > -	default:
> > -		break;
> > -	}
> > +		if (!(dfi->channel_mask & BIT(i)))
> > +			continue;
> >  
> > -	/* enable count, use software mode */
> > -	writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
> > -		       dfi_regs + DDRMON_CTRL);
> > +		/* clear DDRMON_CTRL setting */
> > +		writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN |
> > +			       DDRMON_CTRL_SOFTWARE_EN | DDRMON_CTRL_HARDWARE_EN),
> > +			       dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> > +
> > +		/* set ddr type to dfi */
> > +		switch (dfi->ddr_type) {
> > +		case ROCKCHIP_DDRTYPE_LPDDR2:
> > +		case ROCKCHIP_DDRTYPE_LPDDR3:
> > +			ctrl = DDRMON_CTRL_LPDDR23;
> > +			break;
> > +		case ROCKCHIP_DDRTYPE_LPDDR4:
> > +		case ROCKCHIP_DDRTYPE_LPDDR4X:
> > +			ctrl = DDRMON_CTRL_LPDDR4;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +
> > +		writel_relaxed(HIWORD_UPDATE(ctrl, DDRMON_CTRL_DDR_TYPE_MASK),
> > +			       dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> > +
> > +		/* enable count, use software mode */
> > +		writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
> > +			       dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> > +
> > +		if (dfi->ddrmon_ctrl_single)
> > +			break;
> > +	}
> >  out:
> >  	mutex_unlock(&dfi->mutex);
> >  
> > @@ -164,6 +177,7 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> >  static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
> >  {
> >  	void __iomem *dfi_regs = dfi->regs;
> > +	int i;
> >  
> >  	mutex_lock(&dfi->mutex);
> >  
> > @@ -174,8 +188,17 @@ static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
> >  	if (dfi->usecount > 0)
> >  		goto out;
> >  
> > -	writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
> > -		       dfi_regs + DDRMON_CTRL);
> > +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> > +		if (!(dfi->channel_mask & BIT(i)))
> > +			continue;
> > +
> > +		writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
> > +			      dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> > +
> > +		if (dfi->ddrmon_ctrl_single)
> > +			break;
> > +	}
> > +
> >  	clk_disable_unprepare(dfi->clk);
> >  out:
> >  	mutex_unlock(&dfi->mutex);
> > @@ -666,6 +689,7 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
> >  	dfi->buswidth[1] = FIELD_GET(RK3399_PMUGRF_OS_REG2_BW_CH1, val) == 0 ? 4 : 2;
> >  
> >  	dfi->ddrmon_stride = 0x14;
> > +	dfi->ddrmon_ctrl_single = true;
> >  
> >  	return 0;
> >  };
> 
> Even if rk3568 has the only one channle and don't need to check whether 'dfi->ddrmon_ctrl_single'
> is true or not because of 'if (!(dfi->channel_mask & BIT(i)))',
> I recommand the add 'dfi->ddrmon_ctrl_single = true;' for rk3568 in order to
> provide the number of DDRMON_CTRL reigster of rk3568.
> 
> If rk3568 doesn't have the 'ddrmon_ctrl_single', actually it is not easy
> to catch what why are there no initilization for rk3568.

Ok, will change.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |