Hi Massimo,
On 10/22/26 7:57 PM, Massimo Pegorer wrote:
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.
That could easily be done by having an invisible Kconfig symbol which is
selected by all of the DRAM types. Then we can have a
ifneq (CONFIG_ROCKCHIP_AT_LEAST_ONE_DRAM_TYPE,y)
$(error At least one DRAM type needs to be selected)
endif
in drivers/ram/rockchip/Makefile or something like that.
+ 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?
Fail the build if there's more than one selected in a driver that only
supports one at a time.
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.
Yes, that's not the right choice indeed.
+
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.
Just don't describe any of it outside of "DDR4 support". The symbol
should only be selectable if the SoC supports the DRAM type. For drivers
that support multiple types at once, let them be all selectable.
Otherwise, have the build fail if more than one are selected (e.g. from
within the driver directly). I would say most boards will only select
one anyway and you likely know which one you need for your board.
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.
CONFIG_RAM exists, so it's also available in proper, where COMPILE_TEST
could be selected?
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.
Consistency but also to prepare the driver for more types to be
supported in the future. But again, nothing required for this series.
Cheers,
Quentin
