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

Sascha Hauer
2025-06-11T07:33:01Z

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 |