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@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxx>

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 |