[PATCH 17/18] rockchip: rk3588: bind MMC controllers in U-Boot proper pre-reloc

[Kever Yang]

Hi Quentin,

On 2024/1/24 19:04, Quentin Schulz wrote:
> Hi Kever,
>
> On 1/24/24 11:35, Kever Yang wrote:
>> Hi Quentin,
>>
>> On 2024/1/23 22:49, Quentin Schulz wrote:
>>> From: Quentin Schulz <quentin.schulz at theobroma-systems.com>
>>>
>>> Since commit 9e644284ab81 ("dm: core: Report bootph-pre-ram/sram 
>>> node as
>>> pre-reloc after relocation"), bootph-pre-ram doesn't make U-Boot proper
>>> bind the device before relocation.
>>>
>>> While this is usually not much of an issue, it is when there's a lookup
>>> for devices by code running before the relocation. Such is the case of
>>> env_init() which calls env_driver_lookup() which calls
>>> env_get_location() which is a weak symbol and may call
>>> arch_env_get_location() also a weak symbol. Those are two functions 
>>> that
>>> may traverse UCLASS to find some devices (e.g.
>>> board/theobroma-systems/common/common.c:arch_env_get_location()).
>>
>> This sounds like we need to update arch_env_get_location() instead of 
>> enable mmc driver
>>
>> before relocate, because you we don't really need the mmc driver 
>> works here, there is no
>>
>> access requirement to mmc at this point, right?
>>
>
> All Rockchip SoCs except RK3588(S) and RK356x have it done this way, a 
> little bit of consistency wouldn't hurt :)

My point is not about you can not enabe the emmc before relocate, maybe 
I'm not clear enough for the reason.

All the driver bind/probed before the relocation will have to do the 
init sequence again later after relocation.

The emmc driver cost pretty much time at init, we should avoid to 
duplicate the init process if possible.

For this patch, you want to make it pre-relocate because you want to 
make sure the emmc is available for ENVL_MMC,

but there is no read or write requirement to the emmc at this point, 
which means we don't have to init the emmc at this point,

maybe we can check if the driver is enable if enough.

> I need to be able to find out if the device that was used to load 
> U-Boot proper is an MMC device so that I can tell 
> arch_env_get_location() to return ENVL_MMC; in that case.
>
> For that, I've used uclass_find_device_by_ofnode() which parses the 
> list of devices registered in the UCLASS_MMC (for that scenario). I 
> assume the only requirement is that the device needs to be bound, not 
> probed (haven't checked). If there's another way to do this 
> **properly**, I'm all ears. I would likely need to do the same for the 
> SPI controllers but since none of our RK3588-based products have 
> SPI-NOR, I don't need those (but it works on RK3399 just fine).
>
> I still think there's value in having consistency between all Rockchip 
> SoCs (and if applicable to other SoC vendors, then those as well). 


This is the point I do care, because I don't want the boot loader too 
heavy, especially the SPL and the U-Boot proper before relocate, 
although we can enable all the feature in it in technically.

Even for the rk3588 which is kind of powerful soc, still many project 
need it to boot fast, which require to remove all the redundant 
operation in the boot process.


For the feature record "spl-boot-device" in SPL and read in out in 
U-Boot proper,  and then Swap mmc0 and mmc1 in boot_targets if booted 
from SD-Card.

It's OK for Theobroma-Systems's board to enable it, but seems not also 
required by other boards.

Usually we consider the system in two stage: bootloader/BIOS 
stage(including all firmware before kernel) and OS stage(including 
kernel and Linux/Android OS),

and for those boards(eg. PC like) do have two different storage medium, 
they put bootloader in SPI flash and put OS firmware in other storage 
like emmc/SSD/SDcard.

In this case the U-Boot boot target does not need to know where it's from;

in another case which supports firmware update from SD card, the U-Boot 
boot target needs to set SDCard as highest priority, also no need to 
know where the U-Boot from.


Thanks,
- Kever
> We could still do both though, have this and then fix the need for 
> requiring sdmmc/sdhci devices to be bound/probed for env_init() 
> (arch_env_get_location()) to work.
>
> What is bothering you in enabling those drivers in U-Boot proper 
> before relocation?
>
> Cheers,
> Quentin