Hi Quentin, On 10/13/2026 1:04 PM, Quentin Schulz wrote: > 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.
Correct, and to be technical the register is also only used as a way to pass DRAM info from TPL (or vendor blob) to the next phases, for open DRAM init we could use something else, e.g. a bloblist or similar to pass this information along to SPL and/or U-Boot proper. However, for vendor blob we have the register and for newer blobs we also have some information passed as rk atags. > 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? For some parts it make sense, however not if we want to try and follow driver model or want to be able to one day support multiple SoCs using a single U-Boot proper build (using existing building blocks). We could even fully change how xPL pass DRAM info to next phase, however that would likely break mixing vendor DRAM init or SPL with mainline U-Boot proper, so may not be ideal if we want to keep backward compatibility with vendor blobs. One option could possible be to read register in SPL and pass information to U-Boot proper using a bloblist. For TPL only doing DRAM init I fully buy that we do not really need DM and it is enough to call a sdram_init(), similar to what we do on PX30. This is even the pattern I have done for bare minimal TPL testing on RK35xx SoCs in my work-in-progress rk3562 branch. However, there the sdram "driver" only export sdram_init() in TPL and works as the dummy DM driver for SPL/proper. For reading DRAM info in SPL, it could easily be a weak SoC-specific function. However, if we dream of a shared U-Boot proper binary for multiple SoCs, any use weak SoC-specific functions could likely make that harder. As long as we keep board or SoC specific functions out of U-Boot proper this is something that theoretically should be possible. If we need to do something board/SoC specific, we could possible use something that is matched on board/soc compatible. If we in future end up passing DRAM info to U-Boot proper using a SoC agnostic way, use of current read register RAM drivers would really be completely unnecessary. When it comes to the DMC node, we could possible instead bind the driver to root-node or something similar, however root-node is currently not being matched to any driver using compatible, so not sure how feasible that would be. Reducing complexity of the read register driver and keeping an awkward DMC node is at least something that has been proven to work ;-) > >>> >>> 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. I know OF_PLATDATA is scary and I would avoid it when possible. Mostly just want to make it possible for use in case the open TPL for RK3568 may need it, and also the driver name not matching device name and/or compatible is bugging me ;-) > >> 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. Spiting this into following seem reasonable to me: - ram: rockchip: <soc>: Simplify get_info() ops - ram: rockchip: <soc>: Use a constant for the PMUGRF base - ram: rockchip: <soc>: Rename driver to support xPL_OF_PLATDATA > >> 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. I fully agree, and this is one of the reasons I really do not like current DM lookup of a GRF/SYSCON udevice just to get the reg address to do a simple readl(). A lot of plumbing for a very simple and deterministic task. With this I at least think we remove some of that unnecessary plumbing, and we are just left with a bare minimum driver model wrapper that likely can be stripped away in future. > > 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). Great, I will send a v2. Regards, Jonas > > Cheers, > Quentin
