Hi Quentin, Il giorno gio 8 ott 2026 alle ore 22:49 Quentin Schulz ha scritto: > > Hi Massimo, > > Thanks for the patches. Note that the timing is interesting as we were > considering having two variants of the same product, one with DDR3 and > one with DDR4 and we don't want to have with deal with multiple binaries > just for that.
Good to know! > > On 8/29/26 7:03 PM, Massimo Pegorer wrote: > > Not all the DDR type config options named in the Kconfig are supported > > by each one of the Rockchip SoCs. Moreover, the LPDDR4 option has a > > different meaning in the context of the RK3399 SoC than in the RV1126 > > one. Improve clarity splitting config options, using SoC specific help > > sections and adding depends on conditionals. > > Yeah, we shouldn't do that. > > > > > Involved Rockchip SoCs are: RK3399, PX30, RV1126. > > > > Signed-off-by: Massimo Pegorer <[email protected]> > > --- > > drivers/ram/rockchip/Kconfig | 29 ++++++++++++++++++++++++++++- > > 1 file changed, 28 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/ram/rockchip/Kconfig b/drivers/ram/rockchip/Kconfig > > index d707d09c1c8..61a3193b3f5 100644 > > --- a/drivers/ram/rockchip/Kconfig > > +++ b/drivers/ram/rockchip/Kconfig > > @@ -26,26 +26,53 @@ config RAM_ROCKCHIP_DEBUG > > > > config RAM_ROCKCHIP_DDR4 > > bool "DDR4 support for Rockchip SoCs" > > + depends on ROCKCHIP_PX30 || ROCKCHIP_RV1126 > > This kind of change is very welcome. > > > help > > This enables DDR4 sdram support instead of the default DDR3 support > > on Rockchip SoCs. > > > > We should get rid of the part that says "instead of the default DDR3 > support". We should have instead an option for DDR3 which is enabled by > default and then we don't have to care about that part. OK, I can add a new config option for DDR3. But in this case, we will also need to check at build time that at least one config option has been selected. > > > + 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. > > This one change though is less welcome. I don't want to have this in the > Kconfig help text. I suspect nobody will maintain this in the long term. I'm just describing the status quo. I agree there is a risk, but what do we prefer? The risk that nobody will maintain this in the long term, or the certainty the config options help is providing wrong information? A trivial Kconfig choice would solve the issue, of course, but as I would like to support multiple DDR configurations with the next patch, it is not the way. > > > + > > 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. > > > > + 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. > > + > > 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 > > 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. > > + > > config RAM_ROCKCHIP_LPDDR4 > > + depends on ROCKCHIP_RV1126 > > bool "LPDDR4 support for Rockchip SoCs" > > help > > - This enables LPDDR4 sdram code support for the platforms based > > + This enables LPDDR4 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. > > + > > +config RAM_ROCKCHIP_LPDDR4 > > + bool "LPDDR4 support for Rockchip SoCs" > > + depends on ROCKCHIP_RK3399 > > + help > > + This enables LPDDR4 sdram code support for the platforms based > > + on Rockchip RK3399 SoC. > > + > > Please do not duplicate the same symbol twice just to change its helptext. Ok, but please consider that RAM_ROCKCHIP_LPDDR4 has two different meanings, one in the context of RK339 and another in the context of RV1126. Within RV1126, it means including LPDDR4 support and not including neither the support for DDR4 nor the one for DDR3 (RV1126 can support: LPDDR4, DDR4, DDR3). This is like all other DDR config options that apply to PX30 and/or RV1126. Within RK3399, it means including LPDDR4 support and... ...not clear to me (indeed I left its help text unchanged): the RK3399 SDRAM driver seems able to support multiple DDR types by default (which ones I do not know, DDR3 and LPDDR3 at least), and LPDDR4 support seems to have been added on top. I would say that with this config option selected DDR types other than LPDDR4 will not work, but I am not sure. Therefore, would you prefer a single help text with two paragraphs, one describing the RV1126 scenario and another for the RK3399 one? But for the RK3399 default case, what config option should be used? The suggested new DDR3 one would not fit. For all these reasons, I would suggest having an RK3399 specific LPDDR4 config option, something like RAM_ROCKCHIP_LPDDR4_RK3399, because it is a really different scenario. > > For drivers that support multiple DRAM types but not at the same time, > the easiest answer would be to enforce that outside of Kconfig with > something like: > > #if defined(CONFIG_RAM_ROCKCHIP_LPDDR4) + > defined(CONFIG_RAM_ROCKCHIP_DDR4) + defined(CONFIG_RAM_ROCKCHIP_LPDDR2) > 1 > #error "This driver only supports one DRAM configuration at a time" > #endif > > in the driver. I don't like it too much as this should really be in the > Kconfig symbol but I couldn't think of a way to easily have only one > symbol among a list to be allowed to be selected (that's typically a > choice Kconfig) and also have the same symbols allowed to be selected as > many as possible, depending on the SoC. I think it's a good compromise > for now to fail the build by having this kind of check in the driver. I agree with you, both on what you do not like and on what should really be, too. I will follow the suggested approach in v2. > However, this won't play nice with COMPILE_TEST so not sure if it's > acceptable to Tom. I know nothing about COMPILE_TEST, but I see xPL builds depends on !COMPILE_TEST. And PX30 and RV1126 SDRAM support code is part of xPL, therefore could not be a problem, maybe. > > Ideally, we should also figure out which DRAM type(s) are supported by > DRAM drivers that do not make use of the Kconfig symbols. e.g. RK3066 > for example, and have them explicitly enabled in Kconfig for > completeness. That isn't required for this series. With which purpose? If there is nothing to select/deselect at configuration level, I would avoid this. Instead, I would add the information in the documentation. Thanks. Regards, Massimo > > Cheers, > Quentin
