> Date: Fri, 14 Jul 2023 23:27:42 +0200
> From: Marek Vasut <ma...@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 <kette...@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.
