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

[Quentin Schulz]

Hi Kever,

On 1/29/24 11:35, Kever Yang wrote:
> Hi Quentin,
> 
> On 2024/1/27 00:18, Quentin Schulz wrote:
>> Hi Kever,
>>
>> On 1/26/24 11:56, Kever Yang wrote:
>>> Hi Quentin,
>>>
>>> On 2024/1/26 17:32, Quentin Schulz wrote:
>>>> Hi Kever,
>>>>
>>>> On 1/26/24 09:58, Kever Yang wrote:
>>>>> 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.
>>>>>
>>>>
>>>> Now I need to know which SoC we are booting at build time so I can 
>>>> check which drivers are supposed to be built, check those symbols 
>>>> are enabled, then traverse the Device Tree with hardcoded DT node to 
>>>> locations of MMC, SPI flash controllers, check if those are enabled 
>>>> and finger-cross that those drivers will actually bind/probe 
>>>> properly later on. That's A LOT of checks to be made.
>>>
>>> This is not what I want to say.
>>>
>>> What I mean is something  like this for arch_env_get_location() is 
>>> enough, you don't have to bind/probe the emmc for env.
>>>
>>> 1623                 if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC) && 
>>> IS_ENABLED(CONFIG_MMC))
>>> 1624                         return ENVL_MMC;
>>>
>>
>> I do need to know if the device we loaded U-Boot proper from is an MMC 
>> device or a SPI flash, so this is not enough for arch_env_get_location().
>>
>> I could do it in a hack-ish way though and check if the U-Boot proper 
>> load medium DT node name starts with /mmc, or /spi and if neither, 
>> then return NOWHERE, but then we lose the ability of returning NOWHERE 
>> if the driver wasn't compiled in or the device didn't bind in 
>> pre-reloc, we're missing some failsafe mechanism I could have with 
>> this patch :)
>>
>> [...]
>>
>>>>> 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.
>>>>>
>>>>
>>>> So... this means we need a different U-Boot if we're booting from SD 
>>>> card so it can know which boot target to use by default? Or a 
>>>> different environment for SD card? or requiring the user to stop the 
>>>> boot process and manually change the priority? Or what are you 
>>>> suggesting?
>>>
>>> No, we don't need a different U-Boot, we can always set SD card boot 
>>> first in boot target in all case, if SD card don't have an available 
>>> firmware it will fall back to use eMMC, this works for the board I have.
>>>
>>> Maybe I didn't understand correctly why you have to do "Swap mmc0 and 
>>> mmc1 in boot_targets if booted from SD-Card"?
>>>
>>
>> I think I got things mixed up and this was not a necessary discussion. 
>> But since we're here, I will try to explain what we want to do with 
>> the whole process on Theobroma's boards.
>>
>> arch_env_get_location() is the only one impacted by this patch (as far 
>> as I could tell). setup_boottargets() from 
>> board/theobroma-systems/common/common.c is a vendor-specific 
>> implementation I do not see worthy of being merged to a code section 
>> used by all Rockchip boards, it's basically just a design/policy 
>> choice we made, but there's nothing interesting to make it the default 
>> (though it should be generic enough that one could simply call those 
>> functions from their board file and it should just work (tm)).
>>
>> So the whole thing is basically about wanting a fallback mechanism.
> 
> I can understand this purpose, but it seems you make it a little bit 
> complicate, does your customers understand this? Or maybe you don't need 
> customer to understand all these rules?
> 

I haven't received any complaint so far.

If our customers want to do something differently, they can always 
change the code so that it works for them the way they want it to work, 
or even simply just change boot_targets in their environment if only the 
kernel+dtb image storage medium is what they care about. For our three 
SoMs, I see U-Boot as supported for our SoMs with our devkit, with other 
carrierboards a different U-Boot is likely required anyway.

> This rule is simple for rk3588 vendor SDK:
> 
> Bootrom default order is: SPI -> eMMC-> SDcard ->USB(can chose only one 
> of it now for rk3588)
> 
> SPL default order is: SDcard ->SPI-> eMMC (SDcard is set to higher 
> priority, only if there is a uboot partition and available firmware)
> 
> U-Boot default order is: SDcard -> eMMC (usually not using SPI for system)
> 
> If SDcard has no firmware, then it don't affect the boot flow; if the 
> SDcard has available firmware, it means a firmware update need for 
> eMMC/SPI. Since the SDcard is removal, we can make and replace it again 
> which always fallback available if SPI/eMMC has something wring in 
> U-Boot/System.
> 

I know, and I've received multiple complaints from colleagues about this 
already. There's a LOT of benefits from knowing that in the scenario in 
which everything is properly flashed, everything is loaded from the same 
medium. I myself have been surprised a few times as to why when I 
flashed the eMMC and have an old SD card connected it boots the U-Boot 
proper from the SD card and not from the eMMC.

I appreciate what we're doing is not everyone's cup of tea, but it works 
for us for now, so I don't see a need to revisit this logic.

[...]

>> > No, we don't need a different U-Boot, we can always set SD card boot
>> > first in boot target in all case, if SD card don't have an available
>> > firmware it will fall back to use eMMC, this works for the board I 
>> have.
>>
>> If we do this, simply having an SD card with a partition with an 
>> Image/dtb/fit/extlinux.conf will be taken by default instead of the 
>> ones on the eMMC, even if we booted from the eMMC. I want to make sure 
>> that if I'm booting from the eMMC, I'm booting everything from the 
>> eMMC and not "if you have an SD card, maybe from the SD card". 
> 
> In this case if U-Boot/kernel get panic in eMMC image, the system not 
> able to recovery?
> 

What happens if the SD card image is the one that panics? You also won't 
be able to recover... except if you remove the SD card entirely.

For us, if the eMMC U-Boot proper/kernel image is found but panics, you 
insert an SD card and you hold the BIOS button when booting, then 
everything will be booted from the SD card, otherwise it's booting from 
the eMMC.

In both cases a human is required to intervene, either to insert and 
remove the SD card to start a recovery, or to press the BIOS button. For 
Jaguar specifically, inserting and removing the SD card is not fun (it 
may also be hidden below an adapter (commonly called "hat" I think?) 
making its access even more difficult).

And again, this is the **default**, if a user changes it in the 
environment (to e.g. match the Rockchip SDK behavior), we will not 
change it automatically.

> Anyway, you have your choice, since all these are stay in your board and 
> not affect other boards, it's OK to be accept(the little addition 
> -u-boot-dtsi  is fine).
> 

I am a bit confused.

Where do you want the sdmmc/sdhci controllers bootph-all property be put:
1) rk3588s-u-boot.dtsi
2) rk3588-jaguar-u-boot.dtsi

?

Cheers,
Quentin