[PATCH] rockchip: rk3588-rock-5b: Enable CONFIG_PCI_INIT_R to support EFI networking

[Martin Roukala]

Hi Jonas,

On 11/7/23 12:41, Jonas Karlman wrote:
> Hi Martin,
> 
> On 2023-11-07 10:45, Martin Roukala wrote:
>> Hi Jonas,
>>
>> On 06/11/2023 20:15, Jonas Karlman wrote:
>>> Hi Martin,
>>>
>>> On 2023-11-04 14:04, Martin Roukala (né Peres) wrote:
>>>> When u-boot chainloads an EFI bootloader such as iPXE, we want to have
>>>> already initialized the PCI subsystem so that network driver is loaded
>>>> and ready to use by the bootloader.
>>>
>>> This change slows down boot from emmc/sd-card where scanning for pci
>>> devices typically is unnecessary.
>>
>> True.
>>
>>>
>>> If pci must be initialized and scanned before an EFI app is started to
>>> find pci network controllers, then pci should probably be initialized by
>>> the efi bootmeth, not by enabling CONFIG_PCI_INIT_R for a single board.
>>
>> Unfortunately, it isn't that simple. I am chainloading iPXE immediately,
>> so we never end up calling bootmeth... and so iPXE fails to find any NIC
>> because u-boot did not initialize the PCI subsystem.
> 
> How are you chainloading iPXE?, if it is not started using standard boot
> procedure (the efi bootmeth). If you are running custom script to load
> iPXE you can probably run "pci enum;net list;" before loading iPXE to
> init pci network.

I am loading iPXE using the bootflow way... AKA, no script at all, just 
have the ipxe EFI binary in /EFI/BOOT/BOOTAA64.EFI in a GPT partition.

I thought this was the preferred way of doing things: moving away from 
these hacky and purpose-built scripts in favor of more generic boot 
flows that don't require recompiling u-boot or a script when wanting to 
change the boot target.

In other words, making u-boot having a sane user-facing interface, and 
thus being a good target to be flashed in an SPI memory on the device 
and thus allowing distros to ship generic EFI images for more than one 
board.

> 
>>
>> IMO, the defconfig should work in all situations. If someone wants to
>> speed up their use case, they are free to disable the features they
>> don't need.
> 
> IMO, the defconfig should work for generic use case with standard boot,
> and with the lazy loading principle of not probing devices/drivers until
> they are needed/used.

Indeed, you are right that it shouldn't work in **ALL** situations. 
However, I believe chainloading is still a pretty standard boot 
situation, don't you think? If you don't think so, I hope I can make my 
case for it... or just look at the boot flow of your development machine ;)

> 
> PCI_INIT_R will force probing of pci devices that may not be needed and
> add a ~1 second timeout delay for each unoccupied M.2 slot on this board.

That's indeed annoying, but still less annoying than not seeing any NIC 
detected and having to wonder why they don't show up (missing driver? 
missing DT entries? Missing bus? ...).

In my view, sub-optimal is less important than less compatible, 
especially in the EFI/bootflow world where u-boot would not have to be 
recompiled all the time. Up to 2s boot time increase is a minor 
annoyance in CI, but I don't believe it has any impact on actual users.

I know I am new to the wild world of arm64 bootloaders, but in the x86 
world the default boot configuration is not "Fast boot". And even there, 
you can specify to which degree you want to limit device enumeration... 
AKA, the *_INIT_R config options :)

I'm all for speed, but let's keep defaults reasonable ;)

>> A generic solution may be for u-boot to start probing the PCI bus when
>> asked to enumerate the NICs through SNP by the EFI binary... but until
>> this is done, the defaults should just work.
> 
> Agree, this should be solved using a generic solution. Do not agree on
> changing defaults for a specific use-case that affect other use-cases
> and where other workarounds may be applied.

Agreed, but I would still want this patch to be merged before moving 
forward as I am sure I won't be the only one who has to bisect this 
regression. Speaking of, isn't there a non-regression rule in u-boot?

Cheers,
Martin