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
