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

[Kever Yang]

On 2024/2/1 01:55, Quentin Schulz wrote:
> 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.

If SD card image get panic, we can easily remove and make it again.

I know why you do this now, because the removal of SD card is not so 
"easy" in your system, and you are not using SD card as external storage 
like tablet/SBCs.

>
> 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

It's OK to put it here.


Thanks,

- Kever

> 2) rk3588-jaguar-u-boot.dtsi
>
> ?
>
> Cheers,
> Quentin