Hi Quentin, On 10/9/2026 12:29 AM, Quentin Schulz wrote: > Hi Jonas, > > On 8/4/26 12:57 PM, Jonas Karlman wrote: >> The PMUGRF base address is fixed and known at compile time. However, >> the base address is read from DT at runtime in this driver, delaying >> reading the two HW regs to determine RAM size by several ms. >> >> Change to use a constant for the PMUGRF base address to speed up reading >> RAM size and simplify the driver by removing use of probe and private >> data. The driver list entry name is also renamed to match the >> compatible, for possible use with OF_PLATDATA. >> > > Questions apply to both rk3568 and rk3588 patches. > > There's a lot to unpack here and it doesn't make things easy to review. > > 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 ;-) > > 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. Will split the rename to a separate commit for affected drivers in this series. The intention of this series is only to align RK35xx SoCs to be consistent. Picking up similar work for remaining SoCs from [1] is planned for a future series. [1] https://git.u-boot-project.org/u-boot/contributors/kwiboo/u-boot/-/commits/optimize > > 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] ;-). 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. [2] https://git.u-boot-project.org/u-boot/contributors/kwiboo/u-boot/-/tree/rk3562/drivers/ram/rockchip > > Overall, except issues of the rk3568 and rk3588 syscons still being used > unlike claimed (a bug) and some slight commit log rewording of the other > commit logs, this and the next patch's commit log is lacking information > that should be the answer(s) to 1 and 3 (and please split 2 into a > separate commit if you really want to). Will send a v2 refresh of this and other related/dependent series. Regards, Jonas > > Cheers, > Quentin
