Hi Jonas,
On 10/9/26 5:47 PM, Jonas Karlman wrote:
Hi Quentin,
On 10/9/2026 12:29 AM, Quentin Schulz wrote:
Hi Jonas,
On 8/4/26 12:57 PM, Jonas Karlman wrote:
[...]
1) I would really like to avoid hardcoding stuff. Did you try using
syscon_regmap_lookup_by_phandle(dev, "rockchip,grf"); after adding a
rockchip,grf = <&pmugrf>; property to the dmc node? (maybe it'd make
sense to name the property rockchip,pmugrf but that's a detail). I also
see benefit in being consistent across all drivers using the GRF when DM
is enabled. One issue I could foresee is that this typically will be
part of the TPL once we have an open DRAM init, and then we don't have
DM in TPL so we won't be able to read the device tree to get GRF via
that property. Is this your current worry?
I agree that we should avoid hardcoding stuff in general, with the CRU,
RAM and arch_cpu_init()+board_debug_uart_init() in xPL as exceptions.
Those are that core parts that typically may be needed in xPL, where
size and speed is important and/or DM or OF_PLATDATA may or may not be
involved.
Resolving values from DT has been proven to be very very slow in xPL
and pre-relocation phases, primarily due to slow API for navigating
nodes.
I also cannot understand why we should need to waste multiple CPU cycles,
energy and code size on resolving a fixed known constant value that
never is going to change. So no I have not nor do I plan to try and use
any DT lookup here ;-)
OK so I'm going to push this even further. Why do we even need a driver
for this?
I've been bothered for a long time by this DMC node not being described
in the bindings and nobody bothering to upstream it. Now that I've
looked a bit into it, I think except rk3288 and rk3399 (due to
Chromebooks), they all serve no real purpose once we are out of the xPL
stage that actually initializes the DRAM, they just report whatever size
the DRAM controller reports, by directly reading some register.
I would essentially remove all DMC nodes whose driver do not actually do
anything with the Device Tree, then remove the compatible from the
driver since we don't need it to probe the RAM driver. I hope the DRAM
driver still probes without a compatible (there for sure is some
mechanism as DM-less TPL can probe the DRAM driver just fine for the
SoCs for which we have an open DRAM init). Then it's clear there is no
mix between DT and non-DT logic.
But maybe we could push even more, and delete the part of the DMC driver
that does nothing but report the size and base of the DRAM. We can move
this code to arch/arm/mach-rockchip/ for example with a weak function
that we need to implement for each SoC. Then we would be left with DMC
drivers that actually do DRAM initialization (if there's an open DRAM
init!), and not something completely different when it's in a different
stage. We likely would need to know when we can then call
uclass_get_device(UCLASS_RAM,...) and expect it to work (I guess
CONFIG_IS_ENABLED(RAM)?), e.g. in arch/arm/mach-rockchip/tpl.c and
arch/arm/mach-rockchip/sdram.c. ram_get_info() can then be replaced with
this new weak SoC-specific function.
Does this make sense? What do you think?
2) why rename to match the compatible? What really matters is that they
are unique across the whole U-Boot code base. I don't see really the
benefit, but if it scratches an itch, I could see one commit that
renames all Rockchip DRAM drivers to be consistently named
rockchip_<soc>_dmc. Not a blocker.
The rename is "for possible use with OF_PLATDATA". With OF_PLATDATA the
driver is looked up using a DT compatible to driver name mapping.
I only see very old SoCs using that. SPL_OF_PLATDATA is only enabled for
the RK3288 Chromebooks, MK808 (rk3066) and Rock (RK3188).
TPL_OF_PLATDATA is enabled for PX5 (RK3368), RK3288, RK3328 and RV1126
(ok this one's not so old :) ). Do you have anything in mind related to
OF_PLATDATA? I just read doc/develop/driver-model/of-plat.rst and the
caveats section is scary.
Will split the rename to a separate commit for affected drivers in this
Thanks.
[...]
3) What is removing probe+priv_auto helping with? We now need to call
rockchip_sdram_size for each call to ram_get_info(), which we only have
one so it's not such a big deal but it's still a bit odd to me when the
DRAM isn't going to change between calls (so somewhat joining Simon on
the caching thing that's done here before your patch)
Simons concern where mostly related to caching the base address from DT,
not really related to caching the RAM size, if I understood correctly.
As mentioned in commit message, to simplify the driver. There is really
no need to do an extra malloc for priv data or pre-calculate DRAM size
at probe() time. Also to align with the RAM drivers that was added later
i.e. rk3576, rk3528 and rk3506. And that I have prepared for rk3562,
rv1126b, rv1106, rv1103b, rk3538 and rk3572 in [2] ;-).
Separate commit please, with its own justification, but fine with me on
the reasoning.
On a technical side; to simplify and prepare for possible future DRAM
init. We typically need two completely different parts from the RAM
driver depending on what phase is running.
During DRAM init (in TPL or possible SPL for older SoCs) we only need to
initialize DRAM and write encoded DRAM info to OS_REGx, before returning
to BROM.
During next xPL phase and in proper we typically only need to read and
decode the DRAM info from the same OS_REGx.
To keep the driver as simple as possible we only need to implement
following ops to help keep the complexity low:
.probe = init DRAM in TPL and do nothing in remaining phases
.get_info = return the decoded DRAM size
And for size constrained targets the entire driver could be replaced
with a size optimized sdram_init() function in TPL.
Circling back to what I suggested earlier in this mail, I'm not sure the
next xPL phase/proper part of the driver really makes sense to me. It's
like a massive overhead for what is essentially a call to one function
with one base register address.
OK for the hardcoding of the register base address, based on the idea we
could eventually move this out of the DMC driver (e.g. into SoC-specific
code in arch/arm/mach-rockchip).
Cheers,
Quentin
