| Message ID | 20230714204323.27220-2-kettenis@openbsd.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Load ASMedia XHCI controller firmware | expand |
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@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. > + 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.
> Date: Fri, 14 Jul 2023 23:27:42 +0200 > From: Marek Vasut <marex@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@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.
On 7/15/23 14:47, Mark Kettenis wrote: >> Date: Fri, 14 Jul 2023 23:27:42 +0200 >> From: Marek Vasut <marex@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@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. What about: " struct blk_desc *desc; ofnode_for_each_compatible_node(node, "apple,nvme-ans2") { uclass_get_device_by_ofnode(UCLASS_NVME, node, &blk_dev); desc = dev_get_uclass_plat(blk_dev); for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) { part_get_info(desc, part, &info); ... } } " I'm _not_ sure anymore whether this is actually easier though. What do you think ? >>> + 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. That's better, yeah. I don't think you can safely do bitwise operations on signed types.
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")) + 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()); + lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob); /* somewhat based on the Linux Kernel boot requirements:
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@openbsd.org> --- arch/arm/mach-apple/board.c | 60 +++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)