Hi Quentin, Il giorno gio 8 ott 2026 alle ore 22:49 Quentin Schulz ha scritto: > > Hi Massimo, > > On 8/29/26 7:03 PM, Massimo Pegorer wrote: > > Allow the inclusion of more than one DDR parameters set in the PX30 > > SDRAM driver. The driver applies included sets in a predefined order > > (DDR4, LPDDR2, LPDDR3, DDR3), stopping at the first one that makes > > What made you decide on this order?
I've just followed the original order the data header files are included and thus added to sdram_configs[] array. I have no hints to do any fine reasoning about this. Maybe Rockchip could help. > > > SDRAM initialization function to complete with success. > > > > The default fallback behaviour is preserved: if no DDR configuration > > option is selected, DDR3 parameters set is included. > > We shouldn't do that, we instead should have the DDR3 symbol default to > y for PX30 (and other SoCs if applicable), to preserve the current > state. I don't like hidden defaults like that because you may end up > with RAM_ROCKCHIP_DDR3 not selected and build a DRAM driver that only > supports DDR3, which is kinda odd to be honest. > > > > > Due to limited PX30 TPL maximum size, including support for more than > > one DDR type requires disabling some configuration options such as > > CONFIG_RAM_ROCKCHIP_DEBUG. > > > > Good to know, thanks. But i'm assuming this likely means we can only > have up to two DRAM configurations enabled at the same time? There are some optimizations that can be made to include all four DDR configurations. I will provide them as a further patch if these are merged. Moreover, with the work done by Jonas, we would have more space in PX30 TPL: I need to test these on top of his patches. > > I think the information printed by sdram_print_dram_type() is still > interesting. Maybe we should have some log level for the DRAM init so we > can select the verbosity of the debug output. In any case, just a > thought and not a blocker for this series at all. > > > Signed-off-by: Massimo Pegorer <[email protected]> > > --- > > drivers/ram/rockchip/Kconfig | 67 > > +++++++++++++++++++++++++++++++-------- > > No to those changes. Keep it simple please, no redefinition of the > symbol based on the SoC. For the RAM_ROCKCHIP_DEBUG needing to be > disabled, the information is quite nice for end users so i could see it > being added to all RAM_ROCKCHIP_ symbols as a hint (or have it somewhere > in doc/board/rockchip for example). OK, but I'm providing multi DDR support just for PX30, not for RV1126. Therefore, the logic behind DDR config options common to both of them (such as DDR4) will be different in the two scenarios. Would you prefer a single config option with two paragraphs in the help text, one describing the PX30 scenario and another for the RV1126 one? > > > drivers/ram/rockchip/sdram_px30.c | 35 ++++++++++++-------- > > 2 files changed, 74 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/ram/rockchip/Kconfig b/drivers/ram/rockchip/Kconfig > > index 61a3193b3f5..d74980402ad 100644 > > --- a/drivers/ram/rockchip/Kconfig > > +++ b/drivers/ram/rockchip/Kconfig > > @@ -24,38 +24,77 @@ config RAM_ROCKCHIP_DEBUG > > This is an option for developers to understand the ram drivers > > initialization, configurations and etc. > > > > +config RAM_ROCKCHIP_DDR3 > > + bool "DDR3 support for Rockchip SoCs" > > + depends on ROCKCHIP_PX30 > > + help > > + Enable DDR3 sdram support on Rockchip SoCs. > > + > > + Note that enabling support for multiple DDR/LPDDR types may cause > > TPL > > + to exceed its maximum size constraint (TPL_MAX_SIZE), depending on > > + other configuration options selection (such as RAM_ROCKCHIP_DEBUG). > > + > > + The driver applies selected sdram parameters sets in the following > > + order - DDR4 LPDDR2 LPDDR3 DDR3 - stopping at first set that makes > > + sdram_init_detect() to complete successfully. > > + > > + If no DDR/LPDDR option is selected, DDR3 sdram support is enabled by > > + default for backward compatibility with existing board > > configurations. > > + Therefore this option is only useful for multiple DDR/LPDDR types > > + selection. > > + > > config RAM_ROCKCHIP_DDR4 > > bool "DDR4 support for Rockchip SoCs" > > - depends on ROCKCHIP_PX30 || ROCKCHIP_RV1126 > > + depends on ROCKCHIP_PX30 > > help > > - This enables DDR4 sdram support instead of the default DDR3 support > > - on Rockchip SoCs. > > + Enable DDR4 sdram support on Rockchip SoCs. > > > > - Note that even if multiple DDR/LPDDR types are selected, support is > > - enabled only for the selected one that comes first in the following > > - list: LPDDR4, DDR4, LPDDR2, LPDDR3, DDR3. > > + Note that enabling support for multiple DDR/LPDDR types may cause > > TPL > > + to exceed its maximum size constraint (TPL_MAX_SIZE), depending on > > + other configuration options selection (such as RAM_ROCKCHIP_DEBUG). > > + > > + The driver applies selected sdram parameters sets in the following > > + order - DDR4 LPDDR2 LPDDR3 DDR3 - stopping at first set that makes > > + sdram_init_detect() to complete successfully. > > > > config RAM_ROCKCHIP_LPDDR2 > > bool "LPDDR2 support for Rockchip SoCs" > > depends on ROCKCHIP_PX30 > > help > > - This enables LPDDR2 sdram support instead of the default DDR3 > > support > > - on Rockchip SoCs. > > + Enable LPDDR2 sdram support on Rockchip SoCs. > > > > - Note that even if multiple DDR/LPDDR types are selected, support is > > - enabled only for the selected one that comes first in the following > > - list: LPDDR4, DDR4, LPDDR2, LPDDR3, DDR3. > > + Note that enabling support for multiple DDR/LPDDR types may cause > > TPL > > + to exceed its maximum size constraint (TPL_MAX_SIZE), depending on > > + other configuration options selection (such as RAM_ROCKCHIP_DEBUG). > > + > > + The driver applies selected sdram parameters sets in the following > > + order - DDR4 LPDDR2 LPDDR3 DDR3 - stopping at first set that makes > > + sdram_init_detect() to complete successfully. > > > > config RAM_ROCKCHIP_LPDDR3 > > bool "LPDDR3 support for Rockchip SoCs" > > depends on ROCKCHIP_PX30 > > help > > - This enables LPDDR3 sdram support instead of the default DDR3 > > support > > + Enable LPDDR3 sdram support on Rockchip SoCs. > > + > > + Note that enabling support for multiple DDR/LPDDR types may cause > > TPL > > + to exceed its maximum size constraint (TPL_MAX_SIZE), depending on > > + other configuration options selection (such as RAM_ROCKCHIP_DEBUG). > > + > > + The driver applies selected sdram parameters sets in the following > > + order - DDR4 LPDDR2 LPDDR3 DDR3 - stopping at first set that makes > > + sdram_init_detect() to complete successfully. > > + > > +config RAM_ROCKCHIP_DDR4 > > + bool "DDR4 support for Rockchip SoCs" > > + depends on ROCKCHIP_RV1126 > > + help > > + This enables DDR4 sdram support instead of the default DDR3 support > > on Rockchip SoCs. > > > > Note that even if multiple DDR/LPDDR types are selected, support is > > enabled only for the selected one that comes first in the following > > - list: LPDDR4, DDR4, LPDDR2, LPDDR3, DDR3. > > + list: LPDDR4, DDR4, DDR3. > > > > config RAM_ROCKCHIP_LPDDR4 > > depends on ROCKCHIP_RV1126 > > @@ -66,7 +105,7 @@ config RAM_ROCKCHIP_LPDDR4 > > > > Note that even if multiple DDR/LPDDR types are selected, support is > > enabled only for the selected one that comes first in the following > > - list: LPDDR4, DDR4, LPDDR2, LPDDR3, DDR3. > > + list: LPDDR4, DDR4, DDR3. > > > > config RAM_ROCKCHIP_LPDDR4 > > bool "LPDDR4 support for Rockchip SoCs" > > diff --git a/drivers/ram/rockchip/sdram_px30.c > > b/drivers/ram/rockchip/sdram_px30.c > > index 37e62120504..ac9f5827d1d 100644 > > --- a/drivers/ram/rockchip/sdram_px30.c > > +++ b/drivers/ram/rockchip/sdram_px30.c > > @@ -126,11 +126,17 @@ struct dram_info dram_info; > > struct px30_sdram_params sdram_configs[] = { > > #if defined(CONFIG_RAM_ROCKCHIP_DDR4) > > #include "sdram-px30-ddr4-detect-333.inc" > > -#elif defined(CONFIG_RAM_ROCKCHIP_LPDDR2) > > +#endif > > +#if defined(CONFIG_RAM_ROCKCHIP_LPDDR2) > > #include "sdram-px30-lpddr2-detect-333.inc" > > -#elif defined(CONFIG_RAM_ROCKCHIP_LPDDR3) > > +#endif > > +#if defined(CONFIG_RAM_ROCKCHIP_LPDDR3) > > #include "sdram-px30-lpddr3-detect-333.inc" > > -#else > > +#endif > > +#if defined(CONFIG_RAM_ROCKCHIP_DDR3) || \ > > + (!defined(CONFIG_RAM_ROCKCHIP_DDR4) && \ > > + !defined(CONFIG_RAM_ROCKCHIP_LPDDR2) && \ > > + !defined(CONFIG_RAM_ROCKCHIP_LPDDR3)) > > #include "sdram-px30-ddr3-detect-333.inc" > > #endif > > }; > > @@ -682,19 +688,11 @@ out: > > return ret; > > } > > > > -struct px30_sdram_params > > - *get_default_sdram_config(void) > > -{ > > - sdram_configs[0].skew = &skew; > > - > > - return &sdram_configs[0]; > > -} > > - > > /* return: 0 = success, other = fail */ > > int sdram_init(void) > > { > > struct px30_sdram_params *sdram_params; > > - int ret = 0; > > + int ret, i = 0; > > > > dram_info.phy = (void *)DDR_PHY_BASE_ADDR; > > dram_info.pctl = (void *)DDRC_BASE_ADDR; > > @@ -704,12 +702,21 @@ int sdram_init(void) > > dram_info.ddr_grf = (void *)DDR_GRF_BASE_ADDR; > > dram_info.pmugrf = (void *)PMUGRF_BASE_ADDR; > > > > - sdram_params = get_default_sdram_config(); > > - ret = sdram_init_detect(&dram_info, sdram_params); > > + do { > > + sdram_configs[i].skew = &skew; > > + sdram_params = &sdram_configs[i]; > > + ret = sdram_init_detect(&dram_info, sdram_params); > > + } while (ret && ++i < ARRAY_SIZE(sdram_configs)); > > A simple for-loop on the ARRAY_SIZE() would be easier to read. Once you > have a match (ret = 0), break out of the loop and check ret before > continuing in the code. OK. > > > > > if (ret) > > goto error; > > > > + if (ARRAY_SIZE(sdram_configs) > 1) { > > + printascii("Applied sdram config: "); > > + printdec(i); > > + printascii("\n"); > > This is not useful to the user as it's reflecting the index in an array > that is not stable by design. We should instead report the DDR type (if > we really want to). You can do this by simply calling > sdram_print_dram_type(sdram_params->base.dramtype); Of course, this > won't do anything right now since we need to disable RAM_ROCKCHIP_DEBUG > to be able to compile support for more than one DRAM type. I will drop these lines. > > I will only be able to test your series in November once I'm back in > office, but I for sure will. In the meantime, you can always send a v2 > and/or discuss the feedback :) Thanks, it would be really useful to have further test results as feedback. In the meantime, a will send a v2. Regards, Massimo > > Cheers, > Quentin
