[v7] Add perf support to the rockchip-dfi driver

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

Posted by Sascha Hauer 2 years, 7 months ago

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@huawei.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 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;
 };
-- 
2.39.2

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

Posted by Chanwoo Choi 2 years, 4 months ago

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@huawei.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  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. 
 
-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

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

Posted by Sascha Hauer 2 years, 3 months ago

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@huawei.com>
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  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 |

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

Posted by Chanwoo Choi 2 years, 3 months ago

On 23. 10. 16. 21:49, Sascha Hauer wrote:
> 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@huawei.com>
>>> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>> ---
>>>  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.
> 

Thanks.

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi