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?
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.
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)
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).
Cheers,
Quentin
