[PATCH 1/3] apple: Set up file system firmware loader

[Mark Kettenis]

> Date: Fri, 14 Jul 2023 23:27:42 +0200
> From: Marek Vasut <marex at denx.de>
> 
> On 7/14/23 22:43, Mark Kettenis wrote:
> > Find the appropriate EFI system partition on the internal NVMe
> > storage and set the U-Boot environment variables such that
> > the file system firmware loader can load firmware from it.
> > 
> > Signed-off-by: Mark Kettenis <kettenis at openbsd.org>
> > ---
> >   arch/arm/mach-apple/board.c | 60 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 60 insertions(+)
> > 
> > diff --git a/arch/arm/mach-apple/board.c b/arch/arm/mach-apple/board.c
> > index d501948118..7799a0f916 100644
> > --- a/arch/arm/mach-apple/board.c
> > +++ b/arch/arm/mach-apple/board.c
> > @@ -8,6 +8,8 @@
> >   #include <dm/uclass-internal.h>
> >   #include <efi_loader.h>
> >   #include <lmb.h>
> > +#include <nvme.h>
> > +#include <part.h>
> >   
> >   #include <asm/armv8/mmu.h>
> >   #include <asm/global_data.h>
> > @@ -539,6 +541,60 @@ u64 get_page_table_size(void)
> >   	return SZ_256K;
> >   }
> >   
> > +static char *asahi_esp_devpart(void)
> > +{
> > +	struct disk_partition info;
> > +	struct blk_desc *nvme_blk;
> > +	const char *uuid = NULL;
> > +	int devnum, len, p, part, ret;
> > +	static char devpart[64];
> > +	struct udevice *dev;
> > +	ofnode node;
> > +
> > +	if (devpart[0])
> > +		return devpart;
> > +
> > +	node = ofnode_path("/chosen");
> > +	if (ofnode_valid(node)) {
> > +		uuid = ofnode_get_property(node, "asahi,efi-system-partition",
> > +					   &len);
> > +	}
> > +
> > +	nvme_scan_namespace();
> > +	for (devnum = 0, part = 0;; devnum++) {
> > +		nvme_blk = blk_get_devnum_by_uclass_id(UCLASS_NVME, devnum);
> > +		if (nvme_blk == NULL)
> > +			break;
> > +
> > +		dev = dev_get_parent(nvme_blk->bdev);
> > +		if (!device_is_compatible(dev, "apple,nvme-ans2"))
> 
> Can we somehow use ofnode_for_each_compatible_node() here ?
> That might simplify this code.

I don't really see how that would simplify things.  I'm iterating over
all NVMe devices here and then checking the compatible of the parent
to make sure I pick the on-board one.  I could do the inverse and
lookup the node first and then use that to find the NVMe block device,
but it will still involve a loop and several function calls.

> 
> > +			continue;
> > +
> > +		for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> > +			ret = part_get_info(nvme_blk, p, &info);
> > +			if (ret < 0)
> > +				break;
> > +
> > +			if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
> > +				if (uuid && strcasecmp(uuid, info.uuid) == 0) {
> > +					part = p;
> > +					break;
> > +				}
> > +				if (part == 0)
> > +					part = p;
> > +			}
> > +		}
> > +
> > +		if (part > 0)
> > +			break;
> > +	}
> > +
> > +	if (part > 0)
> > +		snprintf(devpart, sizeof(devpart), "%d:%d", devnum, part);
> > +
> > +	return devpart;
> > +}
> > +
> >   #define KERNEL_COMP_SIZE	SZ_128M
> >   
> >   int board_late_init(void)
> > @@ -546,6 +602,10 @@ int board_late_init(void)
> >   	struct lmb lmb;
> >   	u32 status = 0;
> >   
> > +	status |= env_set("storage_interface",
> > +			  blk_get_uclass_name(UCLASS_NVME));
> > +	status |= env_set("fw_dev_part", asahi_esp_devpart());
> 
> I think env_set() returns integer (and this could be negative too), so 
> you might want to check the return value instead of casting it to 
> unsigned integer.

I'm just using the existing idiom.  But maybe I should just check the
return value and throw a warning instead?  Not having the firmware
loader available isn't fatal.  It just means some of the USB ports
won't work.