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

[Kever Yang]

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?

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.

>
> Boot from TPL+SPL from SD card, if U-Boot proper available on SD card, 
> load U-Boot proper from SD card, load environment from SD card (no 
> fallback for the environment), **preferably** (if the default 
> boot_target in default_environment is not different from the loaded 
> environment, i.e. the user has NOT already modified it) load from the 
> SD card the Image/dtb/fit and if not possible, then from eMMC, etc....
>
> Boot from TPL+SPL from eMMC, if U-Boot proper available on eMMC, load 
> U-Boot proper from eMMC, load environment from eMMC (no fallback for 
> the environment), **preferably** (if the default boot_target in 
> default_environment is not different from the loaded environment, i.e. 
> the user has NOT already modified it) load from the eMMC the 
> Image/dtb/fit and if not possible, then from SD card, etc....
>
> Boot from TPL+SPL from SPI flash, if U-Boot proper available on SPI 
> flash, load U-Boot proper from SPI flash, load environment from SPI 
> flash (no fallback for the environment), use boot_targets from the 
> environment (default or saved by user).
>
> Boot from TPL+SPL from SPI flash, if U-Boot proper NOT available on 
> SPI flash but eMMC, load U-Boot proper from eMMC, load environment 
> from eMMC (no fallback for the environment), **preferably** (if the 
> default boot_target in default_environment is not different from the 
> loaded environment, i.e. the user has NOT already modified it) load 
> from the eMMC the Image/dtb/fit and if not possible, then from SD 
> card, etc....
>
> Boot from TPL+SPL from SPI flash, if U-Boot proper NOT available on 
> SPI flash but SD card, load U-Boot proper from SD card, load 
> environment from SD card (no fallback for the environment), 
> **preferably** (if the default boot_target in default_environment is 
> not different from the loaded environment, i.e. the user has NOT 
> already modified it) load from the SD card the Image/dtb/fit and if 
> not possible, then from eMMC, etc....
>
> Boot from TPL+SPL from eMMC, if U-Boot proper NOT available on eMMC 
> but SPI flash, load U-Boot proper from SPI flash, load environment 
> from SPI flash (no fallback for the environment), use boot_targets 
> from the environment (default or saved by user).
>
> Boot from TPL+SPL from eMMC, if U-Boot proper NOT available on eMMC 
> but SD card, load U-Boot proper from SD card, load environment from SD 
> card (no fallback for the environment), **preferably** (if the default 
> boot_target in default_environment is not different from the loaded 
> environment, i.e. the user has NOT already modified it) load from the 
> SD card the Image/dtb/fit and if not possible, then from eMMC, etc....
>
> Boot from TPL+SPL from SD card, if U-Boot proper NOT available on SD 
> card but SPI flash, load U-Boot proper from SPI flash, load 
> environment from SPI flash (no fallback for the environment), use 
> boot_targets from the environment (default or saved by user).
>
> Boot from TPL+SPL from SD card, if U-Boot proper NOT available on SD 
> card but eMMC, load U-Boot proper from eMMC, load environment from 
> eMMC (no fallback for the environment), **preferably** (if the default 
> boot_target in default_environment is not different from the loaded 
> environment, i.e. the user has NOT already modified it) load from the 
> eMMC the Image/dtb/fit and if not possible, then from SD card, etc...
>
>
> > 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?

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


Thanks,

- Kever

> Why am I using U-Boot proper storage medium instead of the BootROM's? 
> In case the customer didn't design the carrierboard for our 
> System-on-Modules the same way we did for our devkit (Haikou) and 
> forgot to add a bypass for the eMMC, AND their eMMC has TPL+SPL 
> working but somehow got their U-Boot proper corrupted/not found. In 
> which case, we can insert an SD card and load U-Boot proper from it 
> for recovery. In which case, I want to boot stuff from the SD card and 
> not the eMMC. We could also imagine some kind of "built-in" recovery 
> mechanism for some admittedly very corner-case scenarios, where we 
> would have an SD card with everything needed to reflash the board 
> (e.g. swupdate/rauc/mender) that would only work if we cannot boot 
> TPL+SPL from eMMC or U-Boot proper from eMMC.
>
>>>
>>> It all boils down to sane defaults. If I understand correctly, you 
>>> want people to have systems which can boot really fast by default 
>>> but don't mind if people need to tinker to get things working 
>>> properly. I would prefer to have most things working by default and 
>>> let people tinker to make things faster.
>>
>> :(
>>
>> Then I would say you didin't understand correctly.
>>
>> I agree to let people tinker to make things faster and most things 
>> work by default, but don't make too many troubles for tinker if 
>> possible.
>>
>
> What is suggested here can be reverted with two lines in the 
> appropriate -u-boot.dtsi, I would find this acceptable for tinkerers 
> (but well... **I** would not need to do this with the current patch, 
> so maybe I'm biased :) ).
>
>>>
>>> There are a couple of drivers that are selected when using Rockchip 
>>> boards that shouldn't be necessary, e.g. ADC support, SPI flash 
>>> controller (to name the few that we have on RK3588 that we don't 
>>> need for Jaguar now), there are also DT nodes that are enabled by 
>>> default in rk3588(s)-u-boot.dtsi that aren't necessary for my use 
>>> case, this also increases the size of the DTB. I don't really get 
>>> where you want the line between convenience and speed be right now. 
>>> It'd be nice to have some kind of consensus/guideline if you already 
>>> have one in mind, at least I'd know where we want to go with this :)
>>
>> I never said we can't enable pre-reloc the emmc in rk3588 u-boot dt, 
>> that's not the key point.
>>
>> You add this patch to make arch_env_get_location() work, I suggest 
>> that if we don't really access to emmc,
>>
>> then update arch_env_get_location() might be better.
>>
>
> I cannot do what was suggested (see beginning of the mail) sadly.
>
> However, I started to look into having arch_env_get_location() behave 
> differently before and after relocation (since it's called multiple 
> times). I was able to have some proof-of-concept but I read more about 
> env_init() and.... it makes me very anxious to do it this way.
>
> 1) I would expect there's some kind of expectations that what 
> arch_env_get_location() doesn't change between calls with the same 
> prio, which I would be breaking. Even if there's maybe no such 
> expectation, this relies too much on the current state of env_init() 
> and later updates to that function may break stuff. Not great.
>
> 2) I do not understand what this env_init() is supposed to do. It 
> iterates over all possible env drivers and call their init callback, 
> which sets the gd->env_addr/gd->env_valid global variable structure... 
> but if the last env driver sets gd->env_addr to 0xbebe for example and 
> the env driver that the first (and ONLY!) env driver that successfully 
> can load (env_load()) should be using 0xcafe.... we may have some 
> issue there :) Though that point isn't really related to the 
> discussion we were having, rather what I found/didn't understand while 
> trying to implement your suggestion. I've started to discuss this with 
> Marek Vasut on IRC, maybe this will lead somewhere interesting :)
>
>> If it's not able to update arch_env_get_location because it doesn't 
>> work, then this patch is able to accept,
>>
>> and I would suggest arch_env_get_location() to leave in 
>> board/theobroma-systems/common/common.c.
>>
>
> Yes, I agree. I do not think it is a good idea to have 
> arch_env_get_location() currently in 
> board/theobroma-systems/common/common.c for all Rockchip boards. It's 
> just that I need this patch (or a version of it) so that 
> arch_env_get_location() in board/theobroma-systems/common/common.c works.
>
> Why I am advocating for having this in rk3588s-u-boot.dtsi is because:
> - I think that anything that is done with the environment before 
> relocation may need this? So if something calls e.g. env_load before 
> relocation, it would need the mmc controllers to be available before 
> relocation as well?
> - being consistent between all Rockchip SoCs,
>
> Lemme know if you have any question/remark.
>
> Cheers,
> Quentin