[PATCH 0/2] RK3588 rockchip-dfi enhancements

[PATCH 0/2] RK3588 rockchip-dfi enhancements

[Nicolas Frattaroli]

This series consists of two related patches. The first fixes the memory
cycle counter on RK3588, which read half of what it should've been
reading. You can easily verify this with

  perf stat -a -e rockchip_ddr/cycles/ sleep 1

and then dividing the result by the number of Hertz the ddr init settles
on, which for LPDDR4X on RK3588 appears to be 2112MHz.

The second adds support for measuring memory bandwidth with LPDDR5
memory. Results have been validated by comparing its reported bandwidth
with that reported by stress-ng --stream 8 --timeout 15, which line up
almost perfectly.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Nicolas Frattaroli (2):
      PM / devfreq: rockchip-dfi: double count on RK3588
      PM / devfreq: rockchip-dfi: add support for LPDDR5

 drivers/devfreq/event/rockchip-dfi.c | 91 ++++++++++++++++++++++++++++--------
 include/soc/rockchip/rk3588_grf.h    |  8 +++-
 include/soc/rockchip/rockchip_grf.h  |  1 +
 3 files changed, 79 insertions(+), 21 deletions(-)
---
base-commit: ba2b2250bbaf005016ba85e345add6e19116a1ea
change-id: 20250530-rk3588-dfi-improvements-f646424715d2

Best regards,
-- 
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH 1/2] PM / devfreq: rockchip-dfi: double count on RK3588

[Nicolas Frattaroli]

On RK3588 with LPDDR4X memory, the cycle count as returned by

  perf stat -a -e rockchip_ddr/cycles/ sleep 1

consistently reads half as much as what the actual DDR frequency is at.
For a LPDDR4X module running at 2112MHz, I get more like 1056059916
cycles per second, which is almost bang-on half what it should be. No,
I'm not mixing up megatransfers and megahertz.

Consulting the downstream driver, this appears to be because the RK3588
hardware specifically (and RK3528 as well, for future reference) needs a
multiplier of 2 to get to the correct frequency with everything but
LPDDR5.

The RK3588's actual memory bandwidth measurements in MB/s are correct
however, as confirmed with stress-ng --stream. This makes me think the
access counters are not affected in the same way. This tracks with the
vendor kernel not multiplying the access counts either.

Solve this by adding a new member to the dfi struct, which each SoC can
set to whatever they want, but defaults to 1 if left unset by the SoC
init functions. The event_get_count op can then use this multiplier if
the cycle count is requested.

The cycle multiplier is not used in rockchip_dfi_get_event because the
vendor driver doesn't use it there either, and we don't do other actual
bandwidth unit conversion stuff in there anyway.

Fixes: 481d97ba61e1 ("PM / devfreq: rockchip-dfi: add support for RK3588")
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/devfreq/event/rockchip-dfi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 0470d7c175f4f6bb3955e36c713f4c55538d1a87..54effb63519653d20b40eed88681330983399a77 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -116,6 +116,7 @@ struct rockchip_dfi {
 	int buswidth[DMC_MAX_CHANNELS];
 	int ddrmon_stride;
 	bool ddrmon_ctrl_single;
+	unsigned int count_multiplier;	/* number of data clocks per count */
 };
 
 static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
@@ -435,7 +436,7 @@ static u64 rockchip_ddr_perf_event_get_count(struct perf_event *event)
 
 	switch (event->attr.config) {
 	case PERF_EVENT_CYCLES:
-		count = total.c[0].clock_cycles;
+		count = total.c[0].clock_cycles * dfi->count_multiplier;
 		break;
 	case PERF_EVENT_READ_BYTES:
 		for (i = 0; i < dfi->max_channels; i++)
@@ -655,6 +656,9 @@ static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
 		break;
 	}
 
+	if (!dfi->count_multiplier)
+		dfi->count_multiplier = 1;
+
 	ret = perf_pmu_register(pmu, "rockchip_ddr", -1);
 	if (ret)
 		return ret;
@@ -751,6 +755,7 @@ static int rk3588_dfi_init(struct rockchip_dfi *dfi)
 	dfi->max_channels = 4;
 
 	dfi->ddrmon_stride = 0x4000;
+	dfi->count_multiplier = 2;
 
 	return 0;
 };

-- 
2.49.0

[PATCH 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

[Nicolas Frattaroli]

The Rockchip RK3588 SoC can also support LPDDR5 memory. This type of
memory needs some special case handling in the rockchip-dfi driver.

Add support for it in rockchip-dfi, as well as the needed GRF register
definitions.

This has been tested as returning both the right cycle count and
bandwidth on a LPDDR5 board where the CKR bit is 1. I couldn't test
whether the values are correct on a system where CKR is 0, as I'm not
savvy enough with the Rockchip tooling to know whether this can be set
in the DDR init blob.

Downstream has some special case handling for a hardware version where
not just the control bits differ, but also the register. Since I don't
know whether that hardware version is in any production silicon, it's
left unimplemented for now, with an error message urging users to report
if they have such a system.

There is a slight change of behaviour for non-LPDDR5 systems: instead of
writing 0 as the control flags to the control register and pretending
everything is alright if the memory type is unknown, we now explicitly
return an error.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/devfreq/event/rockchip-dfi.c | 84 ++++++++++++++++++++++++++++--------
 include/soc/rockchip/rk3588_grf.h    |  8 +++-
 include/soc/rockchip/rockchip_grf.h  |  1 +
 3 files changed, 73 insertions(+), 20 deletions(-)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 54effb63519653d20b40eed88681330983399a77..5a2c9badcc64c552303c2f55c52e5420dec5ffc1 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -34,15 +34,18 @@
 
 /* DDRMON_CTRL */
 #define DDRMON_CTRL	0x04
+#define DDRMON_CTRL_LPDDR5		BIT(6)
 #define DDRMON_CTRL_DDR4		BIT(5)
 #define DDRMON_CTRL_LPDDR4		BIT(4)
 #define DDRMON_CTRL_HARDWARE_EN		BIT(3)
 #define DDRMON_CTRL_LPDDR23		BIT(2)
 #define DDRMON_CTRL_SOFTWARE_EN		BIT(1)
 #define DDRMON_CTRL_TIMER_CNT_EN	BIT(0)
-#define DDRMON_CTRL_DDR_TYPE_MASK	(DDRMON_CTRL_DDR4 | \
+#define DDRMON_CTRL_DDR_TYPE_MASK	(DDRMON_CTRL_LPDDR5 | \
+					 DDRMON_CTRL_DDR4 | \
 					 DDRMON_CTRL_LPDDR4 | \
 					 DDRMON_CTRL_LPDDR23)
+#define DDRMON_CTRL_LP5_BANK_MODE_MASK	GENMASK(8, 7)
 
 #define DDRMON_CH0_WR_NUM		0x20
 #define DDRMON_CH0_RD_NUM		0x24
@@ -116,13 +119,60 @@ struct rockchip_dfi {
 	int buswidth[DMC_MAX_CHANNELS];
 	int ddrmon_stride;
 	bool ddrmon_ctrl_single;
+	u32 lp5_bank_mode;
+	bool lp5_ckr;	/* true if in 4:1 command-to-data clock ratio mode */
 	unsigned int count_multiplier;	/* number of data clocks per count */
 };
 
+static int rockchip_dfi_ddrtype_to_ctrl(struct rockchip_dfi *dfi, u32 *ctrl,
+					u32 *mask)
+{
+	u32 ddrmon_ver;
+
+	*mask = DDRMON_CTRL_DDR_TYPE_MASK;
+
+	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;
+	case ROCKCHIP_DDRTYPE_LPDDR5:
+		ddrmon_ver = readl_relaxed(dfi->regs);
+		if (ddrmon_ver < 0x40) {
+			*ctrl = DDRMON_CTRL_LPDDR5 | dfi->lp5_bank_mode;
+			*mask |= DDRMON_CTRL_LP5_BANK_MODE_MASK;
+			break;
+		}
+
+		/*
+		 * As it is unknown whether the unpleasant special case
+		 * behaviour used by the vendor kernel is needed for any
+		 * shipping hardware, ask users to report if they have
+		 * some of that hardware.
+		 */
+		dev_err(&dfi->edev->dev,
+			"unsupported DDRMON version 0x%04X, please let linux-rockchip know!\n",
+			ddrmon_ver);
+		return -EOPNOTSUPP;
+	default:
+		dev_err(&dfi->edev->dev, "unsupported memory type 0x%X\n",
+			dfi->ddr_type);
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
 {
 	void __iomem *dfi_regs = dfi->regs;
 	int i, ret = 0;
+	u32 ctrl;
+	u32 ctrl_mask;
 
 	mutex_lock(&dfi->mutex);
 
@@ -136,8 +186,11 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
 		goto out;
 	}
 
+	ret = rockchip_dfi_ddrtype_to_ctrl(dfi, &ctrl, &ctrl_mask);
+	if (ret)
+		goto out;
+
 	for (i = 0; i < dfi->max_channels; i++) {
-		u32 ctrl = 0;
 
 		if (!(dfi->channel_mask & BIT(i)))
 			continue;
@@ -147,21 +200,7 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
 			       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),
+		writel_relaxed(HIWORD_UPDATE(ctrl, ctrl_mask),
 			       dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
 
 		/* enable count, use software mode */
@@ -652,6 +691,7 @@ static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
 		break;
 	case ROCKCHIP_DDRTYPE_LPDDR4:
 	case ROCKCHIP_DDRTYPE_LPDDR4X:
+	case ROCKCHIP_DDRTYPE_LPDDR5:
 		dfi->burst_len = 16;
 		break;
 	}
@@ -730,7 +770,7 @@ static int rk3568_dfi_init(struct rockchip_dfi *dfi)
 static int rk3588_dfi_init(struct rockchip_dfi *dfi)
 {
 	struct regmap *regmap_pmu = dfi->regmap_pmu;
-	u32 reg2, reg3, reg4;
+	u32 reg2, reg3, reg4, reg6;
 
 	regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG2, &reg2);
 	regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG3, &reg3);
@@ -757,6 +797,14 @@ static int rk3588_dfi_init(struct rockchip_dfi *dfi)
 	dfi->ddrmon_stride = 0x4000;
 	dfi->count_multiplier = 2;
 
+	if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR5) {
+		regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG6, &reg6);
+		dfi->lp5_bank_mode = FIELD_GET(RK3588_PMUGRF_OS_REG6_LP5_BANK_MODE, reg6) << 7;
+		dfi->lp5_ckr = FIELD_GET(RK3588_PMUGRF_OS_REG6_LP5_CKR, reg6);
+		if (dfi->lp5_ckr)
+			dfi->count_multiplier *= 2;
+	}
+
 	return 0;
 };
 
diff --git a/include/soc/rockchip/rk3588_grf.h b/include/soc/rockchip/rk3588_grf.h
index 630b35a550640e57f1b5a50dfbe362653a7cbcc1..02a7b2432d9942e15a77424c44fefec189faaa33 100644
--- a/include/soc/rockchip/rk3588_grf.h
+++ b/include/soc/rockchip/rk3588_grf.h
@@ -12,7 +12,11 @@
 #define RK3588_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3		GENMASK(13, 12)
 #define RK3588_PMUGRF_OS_REG3_SYSREG_VERSION		GENMASK(31, 28)
 
-#define RK3588_PMUGRF_OS_REG4           0x210
-#define RK3588_PMUGRF_OS_REG5           0x214
+#define RK3588_PMUGRF_OS_REG4				0x210
+#define RK3588_PMUGRF_OS_REG5				0x214
+#define RK3588_PMUGRF_OS_REG6				0x218
+#define RK3588_PMUGRF_OS_REG6_LP5_BANK_MODE		GENMASK(2, 1)
+/* Whether the LPDDR5 is in 2:1 (= 0) or 4:1 (= 1) CKR a.k.a. DQS mode */
+#define RK3588_PMUGRF_OS_REG6_LP5_CKR			BIT(0)
 
 #endif /* __SOC_RK3588_GRF_H */
diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
index e46fd72aea8d1f649768a3269b85176dacceef0e..41c7bb26fd5387df85e5b58186b67bf74706f360 100644
--- a/include/soc/rockchip/rockchip_grf.h
+++ b/include/soc/rockchip/rockchip_grf.h
@@ -13,6 +13,7 @@ enum {
 	ROCKCHIP_DDRTYPE_LPDDR3	= 6,
 	ROCKCHIP_DDRTYPE_LPDDR4	= 7,
 	ROCKCHIP_DDRTYPE_LPDDR4X = 8,
+	ROCKCHIP_DDRTYPE_LPDDR5	= 9,
 };
 
 #endif /* __SOC_ROCKCHIP_GRF_H */

-- 
2.49.0

Re: [PATCH 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 3825 bytes --]

Hi Nicolas,

On Fri May 30, 2025 at 3:38 PM CEST, Nicolas Frattaroli wrote:
> <snip>
> Downstream has some special case handling for a hardware version where
> not just the control bits differ, but also the register. Since I don't
> know whether that hardware version is in any production silicon, it's
> left unimplemented for now, with an error message urging users to report
> if they have such a system.
> <snip>
> ---
>  drivers/devfreq/event/rockchip-dfi.c | 84 ++++++++++++++++++++++++++++--------
>  include/soc/rockchip/rk3588_grf.h    |  8 +++-
>  include/soc/rockchip/rockchip_grf.h  |  1 +
>  3 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 54effb63519653d20b40eed88681330983399a77..5a2c9badcc64c552303c2f55c52e5420dec5ffc1 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -34,15 +34,18 @@
>  
>  /* DDRMON_CTRL */
>  #define DDRMON_CTRL	0x04
> +#define DDRMON_CTRL_LPDDR5		BIT(6)
>  #define DDRMON_CTRL_DDR4		BIT(5)
>  #define DDRMON_CTRL_LPDDR4		BIT(4)
>  #define DDRMON_CTRL_HARDWARE_EN		BIT(3)
>  #define DDRMON_CTRL_LPDDR23		BIT(2)
>  #define DDRMON_CTRL_SOFTWARE_EN		BIT(1)
>  #define DDRMON_CTRL_TIMER_CNT_EN	BIT(0)
> -#define DDRMON_CTRL_DDR_TYPE_MASK	(DDRMON_CTRL_DDR4 | \
> +#define DDRMON_CTRL_DDR_TYPE_MASK	(DDRMON_CTRL_LPDDR5 | \
> +					 DDRMON_CTRL_DDR4 | \
>  					 DDRMON_CTRL_LPDDR4 | \
>  					 DDRMON_CTRL_LPDDR23)
> +#define DDRMON_CTRL_LP5_BANK_MODE_MASK	GENMASK(8, 7)
>  
>  #define DDRMON_CH0_WR_NUM		0x20
>  #define DDRMON_CH0_RD_NUM		0x24
> @@ -116,13 +119,60 @@ struct rockchip_dfi {
>  	int buswidth[DMC_MAX_CHANNELS];
>  	int ddrmon_stride;
>  	bool ddrmon_ctrl_single;
> +	u32 lp5_bank_mode;
> +	bool lp5_ckr;	/* true if in 4:1 command-to-data clock ratio mode */
>  	unsigned int count_multiplier;	/* number of data clocks per count */
>  };
>  
> +static int rockchip_dfi_ddrtype_to_ctrl(struct rockchip_dfi *dfi, u32 *ctrl,
> +					u32 *mask)
> +{
> +	u32 ddrmon_ver;
> +
> +	*mask = DDRMON_CTRL_DDR_TYPE_MASK;
> +
> +	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;
> +	case ROCKCHIP_DDRTYPE_LPDDR5:
> +		ddrmon_ver = readl_relaxed(dfi->regs);
> +		if (ddrmon_ver < 0x40) {
> +			*ctrl = DDRMON_CTRL_LPDDR5 | dfi->lp5_bank_mode;
> +			*mask |= DDRMON_CTRL_LP5_BANK_MODE_MASK;
> +			break;
> +		}
> +
> +		/*
> +		 * As it is unknown whether the unpleasant special case
> +		 * behaviour used by the vendor kernel is needed for any
> +		 * shipping hardware, ask users to report if they have
> +		 * some of that hardware.
> +		 */
> +		dev_err(&dfi->edev->dev,
> +			"unsupported DDRMON version 0x%04X, please let linux-rockchip know!\n",
> +			ddrmon_ver);
> +		return -EOPNOTSUPP;

I'm guessing you mean the linux-rockchip mailing list here? If so, I
think it's better to make that explicit as 'Joe User' who may run into
this issue may not be aware of that mailing list. The 'linux' and
'rockchip' name combo is used in quite a few places.

Cheers,
  Diederik

> +	default:
> +		dev_err(&dfi->edev->dev, "unsupported memory type 0x%X\n",
> +			dfi->ddr_type);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
>  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>  {
>  	void __iomem *dfi_regs = dfi->regs;
>  	int i, ret = 0;
> +	u32 ctrl;
> +	u32 ctrl_mask;
>  
>  	mutex_lock(&dfi->mutex);
>  
> @@ -136,8 +186,11 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> <snip>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

[Nicolas Frattaroli]

Hello Diederik,

On Wednesday, 4 June 2025 10:24:33 Central European Summer Time Diederik de Haas wrote:
> Hi Nicolas,
> 
> On Fri May 30, 2025 at 3:38 PM CEST, Nicolas Frattaroli wrote:
> > [...]
> > +	case ROCKCHIP_DDRTYPE_LPDDR5:
> > +		ddrmon_ver = readl_relaxed(dfi->regs);
> > +		if (ddrmon_ver < 0x40) {
> > +			*ctrl = DDRMON_CTRL_LPDDR5 | dfi->lp5_bank_mode;
> > +			*mask |= DDRMON_CTRL_LP5_BANK_MODE_MASK;
> > +			break;
> > +		}
> > +
> > +		/*
> > +		 * As it is unknown whether the unpleasant special case
> > +		 * behaviour used by the vendor kernel is needed for any
> > +		 * shipping hardware, ask users to report if they have
> > +		 * some of that hardware.
> > +		 */
> > +		dev_err(&dfi->edev->dev,
> > +			"unsupported DDRMON version 0x%04X, please let linux-rockchip know!\n",
> > +			ddrmon_ver);
> > +		return -EOPNOTSUPP;
> 
> I'm guessing you mean the linux-rockchip mailing list here? If so, I
> think it's better to make that explicit as 'Joe User' who may run into
> this issue may not be aware of that mailing list. The 'linux' and
> 'rockchip' name combo is used in quite a few places.

I agree it's ambiguous, the message as it is right now is the way it is
because we're not allowed to linebreak user-facing messages for grep-
ability and I also don't want to exceed 100 lines of width (though this
is the one case where we're allowed to).

I suppose I should just replace it with the e-mail address of the list.
That should be clear enough and this error message also won't end up in
random boot logs strewn across the internet if it really is just some
engineering sample hardware or similar that's affected.

Kind regards,
Nicolas Frattaroli

> 
> Cheers,
>   Diederik
> 
> > +	default:
> > +		dev_err(&dfi->edev->dev, "unsupported memory type 0x%X\n",
> > +			dfi->ddr_type);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> >  {
> >  	void __iomem *dfi_regs = dfi->regs;
> >  	int i, ret = 0;
> > +	u32 ctrl;
> > +	u32 ctrl_mask;
> >  
> >  	mutex_lock(&dfi->mutex);
> >  
> > @@ -136,8 +186,11 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> > <snip>
> 

Re: [PATCH 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 2522 bytes --]

Hi,

On Thu Jun 5, 2025 at 5:14 PM CEST, Nicolas Frattaroli wrote:
> On Wednesday, 4 June 2025 10:24:33 Central European Summer Time Diederik de Haas wrote:
>> On Fri May 30, 2025 at 3:38 PM CEST, Nicolas Frattaroli wrote:
>> > [...]
>> > +	case ROCKCHIP_DDRTYPE_LPDDR5:
>> > +		ddrmon_ver = readl_relaxed(dfi->regs);
>> > +		if (ddrmon_ver < 0x40) {
>> > +			*ctrl = DDRMON_CTRL_LPDDR5 | dfi->lp5_bank_mode;
>> > +			*mask |= DDRMON_CTRL_LP5_BANK_MODE_MASK;
>> > +			break;
>> > +		}
>> > +
>> > +		/*
>> > +		 * As it is unknown whether the unpleasant special case
>> > +		 * behaviour used by the vendor kernel is needed for any
>> > +		 * shipping hardware, ask users to report if they have
>> > +		 * some of that hardware.
>> > +		 */
>> > +		dev_err(&dfi->edev->dev,
>> > +			"unsupported DDRMON version 0x%04X, please let linux-rockchip know!\n",
>> > +			ddrmon_ver);
>> > +		return -EOPNOTSUPP;
>> 
>> I'm guessing you mean the linux-rockchip mailing list here? If so, I
>> think it's better to make that explicit as 'Joe User' who may run into
>> this issue may not be aware of that mailing list. The 'linux' and
>> 'rockchip' name combo is used in quite a few places.
>
> I agree it's ambiguous, the message as it is right now is the way it is
> because we're not allowed to linebreak user-facing messages for grep-
> ability and I also don't want to exceed 100 lines of width (though this
> is the one case where we're allowed to).
>
> I suppose I should just replace it with the e-mail address of the list.
> That should be clear enough and this error message also won't end up in
> random boot logs strewn across the internet if it really is just some
> engineering sample hardware or similar that's affected.

I understand the issue. Maybe this discussion itself will help make it
appear high enough in the search results to fix the ambiguity ;-)
I'll leave it up to you if or how to change anything (or not).

Cheers,
  Diederik

>> > +	default:
>> > +		dev_err(&dfi->edev->dev, "unsupported memory type 0x%X\n",
>> > +			dfi->ddr_type);
>> > +		return -EOPNOTSUPP;
>> > +	}
>> > +
>> > +	return 0;
>> > +}
>> > +
>> >  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>> >  {
>> >  	void __iomem *dfi_regs = dfi->regs;
>> >  	int i, ret = 0;
>> > +	u32 ctrl;
>> > +	u32 ctrl_mask;
>> >  
>> >  	mutex_lock(&dfi->mutex);
>> >  
>> > @@ -136,8 +186,11 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>> > <snip>
>> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

[Sascha Hauer]

On Fri, May 30, 2025 at 03:38:09PM +0200, Nicolas Frattaroli wrote:
> The Rockchip RK3588 SoC can also support LPDDR5 memory. This type of
> memory needs some special case handling in the rockchip-dfi driver.
> 
> Add support for it in rockchip-dfi, as well as the needed GRF register
> definitions.
> 
> This has been tested as returning both the right cycle count and
> bandwidth on a LPDDR5 board where the CKR bit is 1. I couldn't test
> whether the values are correct on a system where CKR is 0, as I'm not
> savvy enough with the Rockchip tooling to know whether this can be set
> in the DDR init blob.
> 
> Downstream has some special case handling for a hardware version where
> not just the control bits differ, but also the register. Since I don't
> know whether that hardware version is in any production silicon, it's
> left unimplemented for now, with an error message urging users to report
> if they have such a system.
> 
> There is a slight change of behaviour for non-LPDDR5 systems: instead of
> writing 0 as the control flags to the control register and pretending
> everything is alright if the memory type is unknown, we now explicitly
> return an error.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/devfreq/event/rockchip-dfi.c | 84 ++++++++++++++++++++++++++++--------
>  include/soc/rockchip/rk3588_grf.h    |  8 +++-
>  include/soc/rockchip/rockchip_grf.h  |  1 +
>  3 files changed, 73 insertions(+), 20 deletions(-)
> 
> @@ -147,21 +200,7 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>  			       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),
> +		writel_relaxed(HIWORD_UPDATE(ctrl, ctrl_mask),
>  			       dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);

You could move the HIWORD_UPDATE(ctrl, ctrl_mask) to
rockchip_dfi_ddrtype_to_ctrl() and by that you only have to pass one
u32* to that function.

That's just nitpicking though, so for the series:

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>

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 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

[Chanwoo Choi]

Hi,

I'm sorry for late reply.

Looks good to me. But, this patch contains the change of following header.
If there are ACK about change in include/soc/rockchip, I'll merge this series.

include/soc/rockchip/rk3588_grf.h    |  8 +++-
include/soc/rockchip/rockchip_grf.h  |  1 +

On Fri, May 30, 2025 at 10:39 PM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> The Rockchip RK3588 SoC can also support LPDDR5 memory. This type of
> memory needs some special case handling in the rockchip-dfi driver.
>
> Add support for it in rockchip-dfi, as well as the needed GRF register
> definitions.
>
> This has been tested as returning both the right cycle count and
> bandwidth on a LPDDR5 board where the CKR bit is 1. I couldn't test
> whether the values are correct on a system where CKR is 0, as I'm not
> savvy enough with the Rockchip tooling to know whether this can be set
> in the DDR init blob.
>
> Downstream has some special case handling for a hardware version where
> not just the control bits differ, but also the register. Since I don't
> know whether that hardware version is in any production silicon, it's
> left unimplemented for now, with an error message urging users to report
> if they have such a system.
>
> There is a slight change of behaviour for non-LPDDR5 systems: instead of
> writing 0 as the control flags to the control register and pretending
> everything is alright if the memory type is unknown, we now explicitly
> return an error.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/devfreq/event/rockchip-dfi.c | 84 ++++++++++++++++++++++++++++--------
>  include/soc/rockchip/rk3588_grf.h    |  8 +++-
>  include/soc/rockchip/rockchip_grf.h  |  1 +
>  3 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 54effb63519653d20b40eed88681330983399a77..5a2c9badcc64c552303c2f55c52e5420dec5ffc1 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -34,15 +34,18 @@
>
>  /* DDRMON_CTRL */
>  #define DDRMON_CTRL    0x04
> +#define DDRMON_CTRL_LPDDR5             BIT(6)
>  #define DDRMON_CTRL_DDR4               BIT(5)
>  #define DDRMON_CTRL_LPDDR4             BIT(4)
>  #define DDRMON_CTRL_HARDWARE_EN                BIT(3)
>  #define DDRMON_CTRL_LPDDR23            BIT(2)
>  #define DDRMON_CTRL_SOFTWARE_EN                BIT(1)
>  #define DDRMON_CTRL_TIMER_CNT_EN       BIT(0)
> -#define DDRMON_CTRL_DDR_TYPE_MASK      (DDRMON_CTRL_DDR4 | \
> +#define DDRMON_CTRL_DDR_TYPE_MASK      (DDRMON_CTRL_LPDDR5 | \
> +                                        DDRMON_CTRL_DDR4 | \
>                                          DDRMON_CTRL_LPDDR4 | \
>                                          DDRMON_CTRL_LPDDR23)
> +#define DDRMON_CTRL_LP5_BANK_MODE_MASK GENMASK(8, 7)
>
>  #define DDRMON_CH0_WR_NUM              0x20
>  #define DDRMON_CH0_RD_NUM              0x24
> @@ -116,13 +119,60 @@ struct rockchip_dfi {
>         int buswidth[DMC_MAX_CHANNELS];
>         int ddrmon_stride;
>         bool ddrmon_ctrl_single;
> +       u32 lp5_bank_mode;
> +       bool lp5_ckr;   /* true if in 4:1 command-to-data clock ratio mode */
>         unsigned int count_multiplier;  /* number of data clocks per count */
>  };
>
> +static int rockchip_dfi_ddrtype_to_ctrl(struct rockchip_dfi *dfi, u32 *ctrl,
> +                                       u32 *mask)
> +{
> +       u32 ddrmon_ver;
> +
> +       *mask = DDRMON_CTRL_DDR_TYPE_MASK;
> +
> +       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;
> +       case ROCKCHIP_DDRTYPE_LPDDR5:
> +               ddrmon_ver = readl_relaxed(dfi->regs);
> +               if (ddrmon_ver < 0x40) {
> +                       *ctrl = DDRMON_CTRL_LPDDR5 | dfi->lp5_bank_mode;
> +                       *mask |= DDRMON_CTRL_LP5_BANK_MODE_MASK;
> +                       break;
> +               }
> +
> +               /*
> +                * As it is unknown whether the unpleasant special case
> +                * behaviour used by the vendor kernel is needed for any
> +                * shipping hardware, ask users to report if they have
> +                * some of that hardware.
> +                */
> +               dev_err(&dfi->edev->dev,
> +                       "unsupported DDRMON version 0x%04X, please let linux-rockchip know!\n",
> +                       ddrmon_ver);
> +               return -EOPNOTSUPP;
> +       default:
> +               dev_err(&dfi->edev->dev, "unsupported memory type 0x%X\n",
> +                       dfi->ddr_type);
> +               return -EOPNOTSUPP;
> +       }
> +
> +       return 0;
> +}
> +
>  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>  {
>         void __iomem *dfi_regs = dfi->regs;
>         int i, ret = 0;
> +       u32 ctrl;
> +       u32 ctrl_mask;
>
>         mutex_lock(&dfi->mutex);
>
> @@ -136,8 +186,11 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>                 goto out;
>         }
>
> +       ret = rockchip_dfi_ddrtype_to_ctrl(dfi, &ctrl, &ctrl_mask);
> +       if (ret)
> +               goto out;
> +
>         for (i = 0; i < dfi->max_channels; i++) {
> -               u32 ctrl = 0;
>
>                 if (!(dfi->channel_mask & BIT(i)))
>                         continue;
> @@ -147,21 +200,7 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>                                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),
> +               writel_relaxed(HIWORD_UPDATE(ctrl, ctrl_mask),
>                                dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
>
>                 /* enable count, use software mode */
> @@ -652,6 +691,7 @@ static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
>                 break;
>         case ROCKCHIP_DDRTYPE_LPDDR4:
>         case ROCKCHIP_DDRTYPE_LPDDR4X:
> +       case ROCKCHIP_DDRTYPE_LPDDR5:
>                 dfi->burst_len = 16;
>                 break;
>         }
> @@ -730,7 +770,7 @@ static int rk3568_dfi_init(struct rockchip_dfi *dfi)
>  static int rk3588_dfi_init(struct rockchip_dfi *dfi)
>  {
>         struct regmap *regmap_pmu = dfi->regmap_pmu;
> -       u32 reg2, reg3, reg4;
> +       u32 reg2, reg3, reg4, reg6;
>
>         regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG2, &reg2);
>         regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG3, &reg3);
> @@ -757,6 +797,14 @@ static int rk3588_dfi_init(struct rockchip_dfi *dfi)
>         dfi->ddrmon_stride = 0x4000;
>         dfi->count_multiplier = 2;
>
> +       if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR5) {
> +               regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG6, &reg6);
> +               dfi->lp5_bank_mode = FIELD_GET(RK3588_PMUGRF_OS_REG6_LP5_BANK_MODE, reg6) << 7;
> +               dfi->lp5_ckr = FIELD_GET(RK3588_PMUGRF_OS_REG6_LP5_CKR, reg6);
> +               if (dfi->lp5_ckr)
> +                       dfi->count_multiplier *= 2;
> +       }
> +
>         return 0;
>  };
>
> diff --git a/include/soc/rockchip/rk3588_grf.h b/include/soc/rockchip/rk3588_grf.h
> index 630b35a550640e57f1b5a50dfbe362653a7cbcc1..02a7b2432d9942e15a77424c44fefec189faaa33 100644
> --- a/include/soc/rockchip/rk3588_grf.h
> +++ b/include/soc/rockchip/rk3588_grf.h
> @@ -12,7 +12,11 @@
>  #define RK3588_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3         GENMASK(13, 12)
>  #define RK3588_PMUGRF_OS_REG3_SYSREG_VERSION           GENMASK(31, 28)
>
> -#define RK3588_PMUGRF_OS_REG4           0x210
> -#define RK3588_PMUGRF_OS_REG5           0x214
> +#define RK3588_PMUGRF_OS_REG4                          0x210
> +#define RK3588_PMUGRF_OS_REG5                          0x214
> +#define RK3588_PMUGRF_OS_REG6                          0x218
> +#define RK3588_PMUGRF_OS_REG6_LP5_BANK_MODE            GENMASK(2, 1)
> +/* Whether the LPDDR5 is in 2:1 (= 0) or 4:1 (= 1) CKR a.k.a. DQS mode */
> +#define RK3588_PMUGRF_OS_REG6_LP5_CKR                  BIT(0)
>
>  #endif /* __SOC_RK3588_GRF_H */
> diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
> index e46fd72aea8d1f649768a3269b85176dacceef0e..41c7bb26fd5387df85e5b58186b67bf74706f360 100644
> --- a/include/soc/rockchip/rockchip_grf.h
> +++ b/include/soc/rockchip/rockchip_grf.h
> @@ -13,6 +13,7 @@ enum {
>         ROCKCHIP_DDRTYPE_LPDDR3 = 6,
>         ROCKCHIP_DDRTYPE_LPDDR4 = 7,
>         ROCKCHIP_DDRTYPE_LPDDR4X = 8,
> +       ROCKCHIP_DDRTYPE_LPDDR5 = 9,
>  };
>
>  #endif /* __SOC_ROCKCHIP_GRF_H */
>
> --
> 2.49.0
>
>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

[Heiko Stübner]

Hi,

Am Samstag, 6. September 2025, 18:09:53 Mitteleuropäische Sommerzeit schrieb Chanwoo Choi:
> I'm sorry for late reply.
> 
> Looks good to me. But, this patch contains the change of following header.
> If there are ACK about change in include/soc/rockchip, I'll merge this series.
> 
> include/soc/rockchip/rk3588_grf.h    |  8 +++-
> include/soc/rockchip/rockchip_grf.h  |  1 +

done :-)

Thanks
Heiko


> On Fri, May 30, 2025 at 10:39 PM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> >
> > The Rockchip RK3588 SoC can also support LPDDR5 memory. This type of
> > memory needs some special case handling in the rockchip-dfi driver.
> >
> > Add support for it in rockchip-dfi, as well as the needed GRF register
> > definitions.
> >
> > This has been tested as returning both the right cycle count and
> > bandwidth on a LPDDR5 board where the CKR bit is 1. I couldn't test
> > whether the values are correct on a system where CKR is 0, as I'm not
> > savvy enough with the Rockchip tooling to know whether this can be set
> > in the DDR init blob.
> >
> > Downstream has some special case handling for a hardware version where
> > not just the control bits differ, but also the register. Since I don't
> > know whether that hardware version is in any production silicon, it's
> > left unimplemented for now, with an error message urging users to report
> > if they have such a system.
> >
> > There is a slight change of behaviour for non-LPDDR5 systems: instead of
> > writing 0 as the control flags to the control register and pretending
> > everything is alright if the memory type is unknown, we now explicitly
> > return an error.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 84 ++++++++++++++++++++++++++++--------
> >  include/soc/rockchip/rk3588_grf.h    |  8 +++-
> >  include/soc/rockchip/rockchip_grf.h  |  1 +
> >  3 files changed, 73 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index 54effb63519653d20b40eed88681330983399a77..5a2c9badcc64c552303c2f55c52e5420dec5ffc1 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -34,15 +34,18 @@
> >
> >  /* DDRMON_CTRL */
> >  #define DDRMON_CTRL    0x04
> > +#define DDRMON_CTRL_LPDDR5             BIT(6)
> >  #define DDRMON_CTRL_DDR4               BIT(5)
> >  #define DDRMON_CTRL_LPDDR4             BIT(4)
> >  #define DDRMON_CTRL_HARDWARE_EN                BIT(3)
> >  #define DDRMON_CTRL_LPDDR23            BIT(2)
> >  #define DDRMON_CTRL_SOFTWARE_EN                BIT(1)
> >  #define DDRMON_CTRL_TIMER_CNT_EN       BIT(0)
> > -#define DDRMON_CTRL_DDR_TYPE_MASK      (DDRMON_CTRL_DDR4 | \
> > +#define DDRMON_CTRL_DDR_TYPE_MASK      (DDRMON_CTRL_LPDDR5 | \
> > +                                        DDRMON_CTRL_DDR4 | \
> >                                          DDRMON_CTRL_LPDDR4 | \
> >                                          DDRMON_CTRL_LPDDR23)
> > +#define DDRMON_CTRL_LP5_BANK_MODE_MASK GENMASK(8, 7)
> >
> >  #define DDRMON_CH0_WR_NUM              0x20
> >  #define DDRMON_CH0_RD_NUM              0x24
> > @@ -116,13 +119,60 @@ struct rockchip_dfi {
> >         int buswidth[DMC_MAX_CHANNELS];
> >         int ddrmon_stride;
> >         bool ddrmon_ctrl_single;
> > +       u32 lp5_bank_mode;
> > +       bool lp5_ckr;   /* true if in 4:1 command-to-data clock ratio mode */
> >         unsigned int count_multiplier;  /* number of data clocks per count */
> >  };
> >
> > +static int rockchip_dfi_ddrtype_to_ctrl(struct rockchip_dfi *dfi, u32 *ctrl,
> > +                                       u32 *mask)
> > +{
> > +       u32 ddrmon_ver;
> > +
> > +       *mask = DDRMON_CTRL_DDR_TYPE_MASK;
> > +
> > +       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;
> > +       case ROCKCHIP_DDRTYPE_LPDDR5:
> > +               ddrmon_ver = readl_relaxed(dfi->regs);
> > +               if (ddrmon_ver < 0x40) {
> > +                       *ctrl = DDRMON_CTRL_LPDDR5 | dfi->lp5_bank_mode;
> > +                       *mask |= DDRMON_CTRL_LP5_BANK_MODE_MASK;
> > +                       break;
> > +               }
> > +
> > +               /*
> > +                * As it is unknown whether the unpleasant special case
> > +                * behaviour used by the vendor kernel is needed for any
> > +                * shipping hardware, ask users to report if they have
> > +                * some of that hardware.
> > +                */
> > +               dev_err(&dfi->edev->dev,
> > +                       "unsupported DDRMON version 0x%04X, please let linux-rockchip know!\n",
> > +                       ddrmon_ver);
> > +               return -EOPNOTSUPP;
> > +       default:
> > +               dev_err(&dfi->edev->dev, "unsupported memory type 0x%X\n",
> > +                       dfi->ddr_type);
> > +               return -EOPNOTSUPP;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> >  {
> >         void __iomem *dfi_regs = dfi->regs;
> >         int i, ret = 0;
> > +       u32 ctrl;
> > +       u32 ctrl_mask;
> >
> >         mutex_lock(&dfi->mutex);
> >
> > @@ -136,8 +186,11 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> >                 goto out;
> >         }
> >
> > +       ret = rockchip_dfi_ddrtype_to_ctrl(dfi, &ctrl, &ctrl_mask);
> > +       if (ret)
> > +               goto out;
> > +
> >         for (i = 0; i < dfi->max_channels; i++) {
> > -               u32 ctrl = 0;
> >
> >                 if (!(dfi->channel_mask & BIT(i)))
> >                         continue;
> > @@ -147,21 +200,7 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> >                                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),
> > +               writel_relaxed(HIWORD_UPDATE(ctrl, ctrl_mask),
> >                                dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> >
> >                 /* enable count, use software mode */
> > @@ -652,6 +691,7 @@ static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
> >                 break;
> >         case ROCKCHIP_DDRTYPE_LPDDR4:
> >         case ROCKCHIP_DDRTYPE_LPDDR4X:
> > +       case ROCKCHIP_DDRTYPE_LPDDR5:
> >                 dfi->burst_len = 16;
> >                 break;
> >         }
> > @@ -730,7 +770,7 @@ static int rk3568_dfi_init(struct rockchip_dfi *dfi)
> >  static int rk3588_dfi_init(struct rockchip_dfi *dfi)
> >  {
> >         struct regmap *regmap_pmu = dfi->regmap_pmu;
> > -       u32 reg2, reg3, reg4;
> > +       u32 reg2, reg3, reg4, reg6;
> >
> >         regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG2, &reg2);
> >         regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG3, &reg3);
> > @@ -757,6 +797,14 @@ static int rk3588_dfi_init(struct rockchip_dfi *dfi)
> >         dfi->ddrmon_stride = 0x4000;
> >         dfi->count_multiplier = 2;
> >
> > +       if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR5) {
> > +               regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG6, &reg6);
> > +               dfi->lp5_bank_mode = FIELD_GET(RK3588_PMUGRF_OS_REG6_LP5_BANK_MODE, reg6) << 7;
> > +               dfi->lp5_ckr = FIELD_GET(RK3588_PMUGRF_OS_REG6_LP5_CKR, reg6);
> > +               if (dfi->lp5_ckr)
> > +                       dfi->count_multiplier *= 2;
> > +       }
> > +
> >         return 0;
> >  };
> >
> > diff --git a/include/soc/rockchip/rk3588_grf.h b/include/soc/rockchip/rk3588_grf.h
> > index 630b35a550640e57f1b5a50dfbe362653a7cbcc1..02a7b2432d9942e15a77424c44fefec189faaa33 100644
> > --- a/include/soc/rockchip/rk3588_grf.h
> > +++ b/include/soc/rockchip/rk3588_grf.h
> > @@ -12,7 +12,11 @@
> >  #define RK3588_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3         GENMASK(13, 12)
> >  #define RK3588_PMUGRF_OS_REG3_SYSREG_VERSION           GENMASK(31, 28)
> >
> > -#define RK3588_PMUGRF_OS_REG4           0x210
> > -#define RK3588_PMUGRF_OS_REG5           0x214
> > +#define RK3588_PMUGRF_OS_REG4                          0x210
> > +#define RK3588_PMUGRF_OS_REG5                          0x214
> > +#define RK3588_PMUGRF_OS_REG6                          0x218
> > +#define RK3588_PMUGRF_OS_REG6_LP5_BANK_MODE            GENMASK(2, 1)
> > +/* Whether the LPDDR5 is in 2:1 (= 0) or 4:1 (= 1) CKR a.k.a. DQS mode */
> > +#define RK3588_PMUGRF_OS_REG6_LP5_CKR                  BIT(0)
> >
> >  #endif /* __SOC_RK3588_GRF_H */
> > diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
> > index e46fd72aea8d1f649768a3269b85176dacceef0e..41c7bb26fd5387df85e5b58186b67bf74706f360 100644
> > --- a/include/soc/rockchip/rockchip_grf.h
> > +++ b/include/soc/rockchip/rockchip_grf.h
> > @@ -13,6 +13,7 @@ enum {
> >         ROCKCHIP_DDRTYPE_LPDDR3 = 6,
> >         ROCKCHIP_DDRTYPE_LPDDR4 = 7,
> >         ROCKCHIP_DDRTYPE_LPDDR4X = 8,
> > +       ROCKCHIP_DDRTYPE_LPDDR5 = 9,
> >  };
> >
> >  #endif /* __SOC_ROCKCHIP_GRF_H */
> >
> > --
> > 2.49.0
> >
> >
> 
> 
> 

Re: [PATCH 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

[Chanwoo Choi]

Hi,

On Sun, Sep 7, 2025 at 3:38 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi,
>
> Am Samstag, 6. September 2025, 18:09:53 Mitteleuropäische Sommerzeit schrieb Chanwoo Choi:
> > I'm sorry for late reply.
> >
> > Looks good to me. But, this patch contains the change of following header.
> > If there are ACK about change in include/soc/rockchip, I'll merge this series.
> >
> > include/soc/rockchip/rk3588_grf.h    |  8 +++-
> > include/soc/rockchip/rockchip_grf.h  |  1 +
>
> done :-)

Thanks.

Applied it.

(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] PM / devfreq: rockchip-dfi: add support for LPDDR5

[Heiko Stübner]

Am Freitag, 30. Mai 2025, 15:38:09 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> The Rockchip RK3588 SoC can also support LPDDR5 memory. This type of
> memory needs some special case handling in the rockchip-dfi driver.
> 
> Add support for it in rockchip-dfi, as well as the needed GRF register
> definitions.
> 
> This has been tested as returning both the right cycle count and
> bandwidth on a LPDDR5 board where the CKR bit is 1. I couldn't test
> whether the values are correct on a system where CKR is 0, as I'm not
> savvy enough with the Rockchip tooling to know whether this can be set
> in the DDR init blob.
> 
> Downstream has some special case handling for a hardware version where
> not just the control bits differ, but also the register. Since I don't
> know whether that hardware version is in any production silicon, it's
> left unimplemented for now, with an error message urging users to report
> if they have such a system.
> 
> There is a slight change of behaviour for non-LPDDR5 systems: instead of
> writing 0 as the control flags to the control register and pretending
> everything is alright if the memory type is unknown, we now explicitly
> return an error.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

header additions
Acked-by: Heiko Stuebner <heiko@sntech.de>

> diff --git a/include/soc/rockchip/rk3588_grf.h b/include/soc/rockchip/rk3588_grf.h
> index 630b35a550640e57f1b5a50dfbe362653a7cbcc1..02a7b2432d9942e15a77424c44fefec189faaa33 100644
> --- a/include/soc/rockchip/rk3588_grf.h
> +++ b/include/soc/rockchip/rk3588_grf.h
> @@ -12,7 +12,11 @@
>  #define RK3588_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3		GENMASK(13, 12)
>  #define RK3588_PMUGRF_OS_REG3_SYSREG_VERSION		GENMASK(31, 28)
>  
> -#define RK3588_PMUGRF_OS_REG4           0x210
> -#define RK3588_PMUGRF_OS_REG5           0x214
> +#define RK3588_PMUGRF_OS_REG4				0x210
> +#define RK3588_PMUGRF_OS_REG5				0x214
> +#define RK3588_PMUGRF_OS_REG6				0x218
> +#define RK3588_PMUGRF_OS_REG6_LP5_BANK_MODE		GENMASK(2, 1)
> +/* Whether the LPDDR5 is in 2:1 (= 0) or 4:1 (= 1) CKR a.k.a. DQS mode */
> +#define RK3588_PMUGRF_OS_REG6_LP5_CKR			BIT(0)
>  
>  #endif /* __SOC_RK3588_GRF_H */
> diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
> index e46fd72aea8d1f649768a3269b85176dacceef0e..41c7bb26fd5387df85e5b58186b67bf74706f360 100644
> --- a/include/soc/rockchip/rockchip_grf.h
> +++ b/include/soc/rockchip/rockchip_grf.h
> @@ -13,6 +13,7 @@ enum {
>  	ROCKCHIP_DDRTYPE_LPDDR3	= 6,
>  	ROCKCHIP_DDRTYPE_LPDDR4	= 7,
>  	ROCKCHIP_DDRTYPE_LPDDR4X = 8,
> +	ROCKCHIP_DDRTYPE_LPDDR5	= 9,
>  };
>  
>  #endif /* __SOC_ROCKCHIP_GRF_H */
> 
> 

Re: [PATCH 0/2] RK3588 rockchip-dfi enhancements

[Nicolas Frattaroli]

On Friday, 30 May 2025 15:38:07 Central European Summer Time Nicolas Frattaroli wrote:
> This series consists of two related patches. The first fixes the memory
> cycle counter on RK3588, which read half of what it should've been
> reading. You can easily verify this with
> 
>   perf stat -a -e rockchip_ddr/cycles/ sleep 1
> 
> and then dividing the result by the number of Hertz the ddr init settles
> on, which for LPDDR4X on RK3588 appears to be 2112MHz.
> 
> The second adds support for measuring memory bandwidth with LPDDR5
> memory. Results have been validated by comparing its reported bandwidth
> with that reported by stress-ng --stream 8 --timeout 15, which line up
> almost perfectly.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> Nicolas Frattaroli (2):
>       PM / devfreq: rockchip-dfi: double count on RK3588
>       PM / devfreq: rockchip-dfi: add support for LPDDR5
> 
>  drivers/devfreq/event/rockchip-dfi.c | 91 ++++++++++++++++++++++++++++--------
>  include/soc/rockchip/rk3588_grf.h    |  8 +++-
>  include/soc/rockchip/rockchip_grf.h  |  1 +
>  3 files changed, 79 insertions(+), 21 deletions(-)
> ---
> base-commit: ba2b2250bbaf005016ba85e345add6e19116a1ea
> change-id: 20250530-rk3588-dfi-improvements-f646424715d2
> 
> Best regards,
> 

I see someone has merged patch 1 into -next without leaving a message,
but patch 2 was not picked.

Could this mysterious patron saint of PM / devfreq make themselves
heard and let me know whether they would like to see a second revision
of patch 2, and if so, what said revision should address?

I am asking specifically as I would like to continue working on the
driver in a follow-up series, and BYEWORD_UPDATE already complicates
the situation by being another in-flight patch.

Kind regards,
Nicolas Frattaroli