Re: [PATCH v2 2/2] rockchip: rock5b-rk3588: Add support for ROCK 5B+

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Jonas,

On 8/11/25 11:17 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 8/11/2025 11:24 AM, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 8/1/25 7:09 PM, Jonas Karlman wrote:
[...]
>>> +#define PMU1GRF_BASE		0xfd58a000
>>> +#define OS_REG2_REG		0x208
>>> +
>>
>> I assume the register address and meaning is stable across all RK3588,
>> so maybe we should abstract that somewhere in the SoC file
>> (arch/arm/mach-rockchip/rk3588) so that the caller doesn't need to
>> specify them?
> 
> Correct, these are stable for the SoC and could be defined in soc
> related header and is something I have very slowly been working on in my
> optimize branch at [1] for a future series.
> 
> Right now we have some code using syscon_get_first_range() to get a base
> addr from DT, and newer code that instead use a define. See e.g.
> sdram_rk3588.c vs sdram_rk3576.c, getting the base from DT adds several
> ms delay and something that seem pointless when we know the stable addr
> at compile time.
> 

We could do something more akin to:

u8 _rockchip_sdram_type(phys_addr_t reg)
{
	u32 dram_type, version;
	u32 sys_reg2 = readl(reg);
	u32 sys_reg3 = readl(reg + 4);

	dram_type = (sys_reg2 >> SYS_REG_DDRTYPE_SHIFT) & SYS_REG_DDRTYPE_MASK;
	version = (sys_reg3 >> SYS_REG_VERSION_SHIFT) & SYS_REG_VERSION_MASK;
	if (version >= 3)
		dram_type |= ((sys_reg3 >> SYS_REG_EXTEND_DDRTYPE_SHIFT) &
			      SYS_REG_EXTEND_DDRTYPE_MASK) << 3;

	return dram_type;
}

u8 __weak rockchip_dram_type_reg(void)
{
     return 0;
}

u8 rockchip_sdram_type(void)
{
	phys_addr_t reg = rockchip_dram_type_reg();
	if (!reg)
		return UNUSED;

	return _rockchip_sdram_type(reg);
}

and define

u8 rockchip_dram_type_reg(void)
{
	return PMU1GRF_BASE + OS_REG2_REG;
}

in drivers/ram/rockchip/sdram_rk3588.c for example.

That's what I had in mind rather than going for a syscon.

> So for now I just kept this here with the expectation that this should
> be changed in a future series.
> 
> [1] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commit/3a11ab7fba8db133374e60edf8ab2b46195cf684
> 
>>
>>> +#define HW_ID_CHANNEL		5
>>> +
>>> +struct board_model {
>>> +	unsigned int dram;
>>> +	unsigned int low;
>>> +	unsigned int high;
>>> +	const char *fdtfile;
>>> +};
>>> +
>>> +static const struct board_model board_models[] = {
>>> +	{ LPDDR5, 4005, 4185, "rockchip/rk3588-rock-5b-plus.dtb" },
>>> +};
>>> +
>>> +static const struct board_model *get_board_model(void)
>>> +{
>>> +	unsigned int val, dram_type;
>>> +	int i, ret;
>>
>> i could be an unsigned number type as well. (and it should since we're
>> using it to access items in an array)
> 
> Sure, will change in v3.
> 
>>
>>> +
>>> +	dram_type = rockchip_sdram_type(PMU1GRF_BASE + OS_REG2_REG);
>>> +
>>> +	ret = adc_channel_single_shot("adc@fec10000", HW_ID_CHANNEL, &val);
>>> +	if (ret)
>>> +		return NULL;
>>> +
>>
>> While checking whether adc_channel_single_shot() is the right function
>> to call, I stumbled upon the call in arch/arm/mach-rockchip/boot_mode.c
>> which i believe wouldn't work for Rockchip SoCs whose DTSI uses adc@ as
>> node name instead of saradc@. That would be rk3328, rk3588, rk3576,
>> rk3528 and rv1126.
> 
> Correct, the saradc@ in boot_mode.c is typically used for "recovery" or
> "maskrom" detection. However, this uses a hardcoded saradc channel that
> may not necessarily be used for that purpose, especially on newer SoCs.
> 
> I suggest we do not try to change that to cover more SoCs without first
> adding a way to define what channel to use for the button detection,
> possible in a /config or a Kconfig option.
> 
>>
>> I guess there are different ways to fix this. We could have each SoC
>> define a constant string with the node name of the SARADC or have an
>> additional entry in /aliases for saradc in the DT? Maybe some other way
>> could be used as well, just throwing ideas :)
>>
>> Unrelated to this patch though, so feel free to ignore.
> 
> As noted above, I think it is something to fix/consider but it may have
> unintended consequences to blindly try and fix boot_mode.c for all RK
> SoCs without having some sort of board option to specify when and how
> the feature should be used.
> 

Agreed.

Cheers,
Quentin