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

[Marek Vasut]

On 7/15/23 14:47, Mark Kettenis wrote:
>> 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.

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.