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

[Jonas Karlman]

Hi Martin,

On 2023-11-07 12:43, Martin Roukala wrote:
> 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.

It is, and thanks for confirming that this issue is related to using
standard boot and the efi bootmeth/bootflow.

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

I fully agree that it should work, I just want to evaluate if we can solve
the root issue instead of adding a workaround for a single board.

This issue also affect other boards, such as NanoPi R5C that also only have
pcie network controllers, and is why I am trying to see if we can solve
this in a more generic way.

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

I do not disagree that this is fixing an issue, I am just trying to
evaluate if we can fix it at core instead of adding a workaround for a
single board.

The bootdev hunters for e.g. net and scsi both run pci_init() to ensure pci
has init and devices on pci bus can be found. Something similar could
possible be done for the efi bootmeth.

Adding following before the bootefi command is run should probably solve
this issue could there be a similar issue for pci storage devices unless
pci is init before efi app starts?


	if (IS_ENABLED(CONFIG_PCI)) {
		ret = pci_init();
		if (ret)
			log_warning("Failed to init PCI (%dE)\n", ret);
	}

	/*
	 * At some point we can add a real interface to bootefi so we can call
	 * this directly. For now, go through the CLI, like distro boot.
	 */
	snprintf(cmd, sizeof(cmd), "bootefi %lx %lx", kernel, fdt);
	if (run_command(cmd, 0))
		return log_msg_ret("run", -EINVAL);


https://source.denx.de/u-boot/u-boot/-/blob/master/boot/bootmeth_efi.c?ref_type=heads#L449
https://source.denx.de/u-boot/u-boot/-/blob/master/net/eth_bootdev.c?ref_type=heads#L75-80

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

We are still ~8 weeks from release, so hopefully we can find a generic
solution that covers more boards before then.

Regards,
Jonas

> 
> Cheers,
> Martin