Re: [PATCH v3 1/3] efi_loader: Add SPI I/O protocol support

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Paul, 

On Wed, Sep 21, 2022 at 05:06:26PM +0100, Paul Barker wrote:
> This addition allows UEFI applications running under u-boot to access
> peripherals on SPI busses. It is based on the UEFI Platform
> Initialization (PI) Specification, Version 1.7 Errata A (April 2020).
> Only the core functionality required to discover SPI peripherals and
> communicate with them is currently implemented. Other functionality such
> as the legacy SPI controller interface and the ability to update the SPI
> peripheral object associated with a particular SPI I/O protocol object
> is currently unimplemented.
> 
> The following protocols are defined:
> * EFI_SPI_CONFIGURATION_PROTOCOL
> * EFI_SPI_IO_PROTOCOL
> * EFI_LEGACY_SPI_CONTROLLER_PROTOCOL
> 
> Since there are no open source implementations of these protocols to use
> as an example, educated guesses/hacks have been made in cases where the
> UEFI PI specification is unclear and these are documented in comments.
> 
> This implementation has been tested on the SanCloud BBE Lite and allowed
> a UEFI test application to successfully communicate with a Micron
> Authenta flash device connected via the SPI bus. It has also been tested
> with the sandbox target using the included efi_selftest case.
> 
> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
> ---
>  MAINTAINERS                                  |   7 +
>  arch/sandbox/dts/test.dts                    |  13 +
>  include/efi_api.h                            |   4 +
>  include/efi_loader.h                         |   4 +
>  include/efi_spi_protocol.h                   | 166 +++++
>  lib/efi_loader/Kconfig                       |   8 +
>  lib/efi_loader/Makefile                      |   1 +
>  lib/efi_loader/efi_setup.c                   |   6 +
>  lib/efi_loader/efi_spi_protocol.c            | 614 +++++++++++++++++++
>  lib/efi_selftest/Makefile                    |   1 +
>  lib/efi_selftest/efi_selftest_spi_protocol.c | 284 +++++++++
>  lib/uuid.c                                   |   4 +
>  12 files changed, 1112 insertions(+)
>  create mode 100644 include/efi_spi_protocol.h
>  create mode 100644 lib/efi_loader/efi_spi_protocol.c
>  create mode 100644 lib/efi_selftest/efi_selftest_spi_protocol.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83346183ee4b..a58b2083a218 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -857,6 +857,13 @@ F:	tools/efivar.py
>  F:	tools/file2include.c
>  F:	tools/mkeficapsule.c
>  
> +EFI SPI SUPPORT
> +M:	Paul Barker <paul.barker@sancloud.com>
> +S:	Maintained
> +F:	include/efi_spi_protocol.h
> +F:	lib/efi_loader/efi_spi_protocol.c
> +F:	lib/efi_selftest/efi_selftest_spi_protocol.c
> +
>  EFI VARIABLES VIA OP-TEE
>  M:	Ilias Apalodimas <ilias.apalodimas@linaro.org>
>  S:	Maintained
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index 2761588f0dad..05c3e0377ac4 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -1185,6 +1185,13 @@
>  			compatible = "spansion,m25p16", "jedec,spi-nor";
>  			spi-max-frequency = <40000000>;
>  			sandbox,filename = "spi.bin";
> +
> +			uefi-spi-vendor = "spansion";
> +			uefi-spi-part-number = "mt25p16";
> +
> +			/* GUID in UEFI format: b881eb5d-ad92-4a48-8fdd-fa75a8e4c6b8 */
> +			uefi-spi-io-guid = [5d eb 81 b8 92 ad 48 4a
> +					    8f dd fa 75 a8 e4 c6 b8];
>  		};
>  		spi.bin@1 {
>  			reg = <1>;
> @@ -1193,6 +1200,12 @@
>  			sandbox,filename = "spi.bin";
>  			spi-cpol;
>  			spi-cpha;
> +
> +			uefi-spi-vendor = "spansion";
> +			uefi-spi-part-number = "mt25p16";

This is needed to identify the flash we want to access through the protocol
right? We keep dumping info on the DT that I am not sure it belongs there. 

> +			/* GUID in UEFI format: b6b39ecd-2b1f-a643-b8d7-3192d7cf7270 */
> +			uefi-spi-io-guid = [cd 9e b3 b6 1f 2b 43 a6
> +					    b8 d7 31 92 d7 cf 72 70];
>  		};
>  	};
>  

[...]

> + */
> +
> +#define LOG_CATEGORY LOGC_EFI
> +
> +#include <common.h>
> +#include <dm/device.h>
> +#include <dm/device-internal.h>
> +#include <dm/read.h>
> +#include <efi.h>
> +#include <efi_loader.h>
> +#include <efi_spi_protocol.h>
> +#include <malloc.h>
> +#include <spi.h>
> +
> +static efi_string_t convert_efi_string(const char *str)
> +{
> +	efi_string_t str_16, tmp;
> +	size_t sz_8, sz_16;
> +
> +	sz_8 = strlen(str);
> +	sz_16 = utf8_utf16_strlen(str);
> +	str_16 = calloc(sz_16 + 1, 2);
> +	if (!str_16)
> +		return NULL;
> +
> +	tmp = str_16;
> +	utf8_utf16_strcpy(&tmp, str);
> +
> +	return str_16;
> +}

This seems useful overall, mind moving it to lib/efi_loader/efi_string.c
and add sphinx style comments with the description?  And while at it replace
'2' with sizeof(u16)?

> +

[...]

> +static efi_status_t EFIAPI
> +efi_spi_io_transaction(const struct efi_spi_io_protocol *this,
> +		       enum efi_spi_transaction_type transaction_type,
> +		       bool debug_transaction,
> +		       u32 clock_hz,
> +		       u32 bus_width,
> +		       u32 frame_size,
> +		       u32 write_bytes,
> +		       u8 *write_buffer,
> +		       u32 read_bytes,
> +		       u8 *read_buffer)
> +{
> +	struct spi_slave *target;
> +	efi_status_t status = EFI_SUCCESS;
> +	int r;
> +
> +	/* We ignore the bus_width and frame_size arguments to this function as the
> +	 * appropriate bus configuration for the connected device will be performed
> +	 * during spi_claim_bus().
> +	 */
> +
> +	/* TODO: Print transaction details if debug_transaction is true. */
> +
> +	EFI_ENTRY("%p, %u, %u, %u, %u, %u, %u, %p, %u, %p",
> +		  this, transaction_type, debug_transaction,
> +		  clock_hz, bus_width, frame_size,
> +		  write_bytes, write_buffer, read_bytes, read_buffer);
> +
> +	if (!this)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	target = container_of(this, struct efi_spi_peripheral_priv, io_protocol)->target;
> +
> +	if (clock_hz > this->spi_peripheral->max_clock_hz)
> +		return EFI_EXIT(EFI_UNSUPPORTED);
> +
> +	r = spi_claim_bus(target);
> +	if (r != 0)
> +		return EFI_EXIT(EFI_DEVICE_ERROR);
> +	EFI_PRINT("SPI IO: Bus claimed\n");
> +
> +	if (clock_hz) {
> +		EFI_PRINT("SPI IO: Setting clock rate to %u Hz\n", clock_hz);
> +		spi_get_ops(target->dev->parent)->set_speed(target->dev->parent, clock_hz);

Is set_speed always implemented in the driver model or will we crash?
I think we should be using spi_set_speed_mode()?

> +	} else {
> +		EFI_PRINT("SPI IO: Using default clock rate\n");
> +	}
> +
> +	switch (transaction_type) {
> +	case SPI_TRANSACTION_FULL_DUPLEX:
> +		EFI_PRINT("SPI IO: Full-duplex\n");
> +		if (write_bytes != read_bytes || !write_bytes || !write_buffer || !read_buffer) {
> +			status = EFI_INVALID_PARAMETER;
> +			break;
> +		}
> +		if (debug_transaction)
> +			dump_buffer("SPI IO: write", write_bytes, write_buffer);
> +		r = spi_xfer(target, 8 * write_bytes,
> +			     write_buffer, read_buffer, SPI_XFER_ONCE);
> +		EFI_PRINT("SPI IO: xfer returned %d\n", r);
> +		if (debug_transaction)
> +			dump_buffer("SPI IO: read", read_bytes, read_buffer);
> +		status = (r == 0) ? EFI_SUCCESS : EFI_DEVICE_ERROR;
> +		break;
> +	case SPI_TRANSACTION_READ_ONLY:
> +		EFI_PRINT("SPI IO: Read-only\n");
> +		if (!read_bytes || !read_buffer) {
> +			status = EFI_INVALID_PARAMETER;
> +			break;
> +		}
> +		r = spi_xfer(target, 8 * read_bytes,
> +			     NULL, read_buffer, SPI_XFER_ONCE);
> +		EFI_PRINT("SPI IO: xfer returned %d\n", r);
> +		if (debug_transaction)
> +			dump_buffer("SPI IO: read", read_bytes, read_buffer);
> +		status = (r == 0) ? EFI_SUCCESS : EFI_DEVICE_ERROR;
> +		break;
> +	case SPI_TRANSACTION_WRITE_ONLY:
> +		EFI_PRINT("SPI IO: Write-only\n");
> +		if (!write_bytes || !write_buffer) {
> +			status = EFI_INVALID_PARAMETER;
> +			break;
> +		}
> +		if (debug_transaction)
> +			dump_buffer("SPI IO: write", write_bytes, write_buffer);
> +		r = spi_xfer(target, 8 * write_bytes,
> +			     write_buffer, NULL, SPI_XFER_ONCE);
> +		EFI_PRINT("SPI IO: xfer returned %d\n", r);
> +		status = (r == 0) ? EFI_SUCCESS : EFI_DEVICE_ERROR;
> +		break;
> +	case SPI_TRANSACTION_WRITE_THEN_READ:
> +		EFI_PRINT("SPI IO: Write-then-read\n");
> +		if (!write_bytes || !write_buffer || !read_bytes || !read_buffer) {
> +			status = EFI_INVALID_PARAMETER;
> +			break;
> +		}
> +		if (debug_transaction)
> +			dump_buffer("SPI IO: write", write_bytes, write_buffer);
> +		r = spi_xfer(target, 8 * write_bytes,
> +			     write_buffer, NULL, SPI_XFER_BEGIN);
> +		EFI_PRINT("SPI IO: xfer [1/2] returned %d\n", r);
> +		if (r != 0) {
> +			status = EFI_DEVICE_ERROR;
> +			break;
> +		}
> +		r = spi_xfer(target, 8 * read_bytes,
> +			     NULL, read_buffer, SPI_XFER_END);
> +		EFI_PRINT("SPI IO: xfer [2/2] returned %d\n", r);
> +		if (debug_transaction)
> +			dump_buffer("SPI IO: read", read_bytes, read_buffer);
> +		status = (r == 0) ? EFI_SUCCESS : EFI_DEVICE_ERROR;
> +		break;
> +	default:
> +		status = EFI_INVALID_PARAMETER;
> +		break;
> +	}
> +
> +	spi_release_bus(target);
> +	EFI_PRINT("SPI IO: Released bus\n");
> +	return EFI_EXIT(status);
> +}
> +
> +static struct efi_device_path null_device_path = {
> +	.type     = DEVICE_PATH_TYPE_END,
> +	.sub_type = DEVICE_PATH_SUB_TYPE_END,
> +	.length   = 4
> +};
> +
> +static struct efi_legacy_spi_controller_protocol
> +dummy_legacy_spi_controller_protocol = {
> +	.maximum_offset = 0,
> +	.maximum_range_bytes = 0,
> +	.range_register_count = 0,
> +	.erase_block_opcode = legacy_erase_block_opcode,
> +	.write_status_prefix = legacy_write_status_prefix,
> +	.bios_base_address = legacy_bios_base_address,
> +	.clear_spi_protect = legacy_clear_spi_protect,
> +	.is_range_protected = legacy_is_range_protected,
> +	.protect_next_range = legacy_protect_next_range,
> +	.lock_controller = legacy_lock_controller
> +};

Keeping in mind all these return EFI_UNSUPPORTED can we get rid of them and
set the legacy_spi_protocol to NULL?  Or defining them is mandatory from the PI spec?
Do you plan to implement it in the future?

> +
> +static efi_guid_t efi_spi_configuration_guid = EFI_SPI_CONFIGURATION_GUID;
> +
> +static void destroy_efi_spi_peripheral(struct efi_spi_peripheral *peripheral)
> +{
> +	struct efi_spi_peripheral_priv *priv =
> +		container_of(peripheral,
> +			     struct efi_spi_peripheral_priv,
> +			     peripheral);
> +	free(priv->peripheral.friendly_name);
> +	free(priv->part.vendor);
> +	free(priv->part.part_number);
> +	efi_delete_handle(priv->handle);

Does this handle has any more protocols? In theory we shouldn't be calling
this.  Uninstalling protocols from a handler should take care of this for
us.  IOW if the protocol you uninstalled was the last one on the handler,
we automatically delete it.

> +	free(priv);
> +}
> +
> +static void destroy_efi_spi_bus(struct efi_spi_bus *bus)
> +{
> +	struct efi_spi_peripheral *peripheral = bus->peripheral_list;
> +
> +	while (peripheral) {
> +		struct efi_spi_peripheral *next =
> +			peripheral->next_spi_peripheral;
> +		destroy_efi_spi_peripheral(peripheral);
> +		peripheral = next;
> +	}
> +	free(bus->friendly_name);
> +	free(bus);
> +}
> +
> +static efi_status_t efi_spi_new_handle(const efi_guid_t *guid, void *proto)
> +{
> +	efi_status_t status;
> +	efi_handle_t handle;
> +
> +	status = efi_create_handle(&handle);
> +	if (status != EFI_SUCCESS) {
> +		printf("Failed to create EFI handle\n");
> +		goto fail_1;
> +	}
> +
> +	status = efi_add_protocol(handle, guid, proto);
> +	if (status != EFI_SUCCESS) {
> +		printf("Failed to add protocol\n");
> +		goto fail_2;
> +	}
> +
> +	return EFI_SUCCESS;
> +
> +fail_2:
> +	efi_delete_handle(handle);
> +fail_1:
> +	return status;
> +}
> +
> +static void
> +efi_spi_init_part(struct efi_spi_part *part,
> +		  struct spi_slave *target,
> +		  efi_string_t vendor_utf16,
> +		  efi_string_t part_number_utf16
> +)
> +{
> +	part->vendor = vendor_utf16;
> +	part->part_number = part_number_utf16;
> +	part->min_clock_hz = 0;
> +	part->max_clock_hz = target->max_hz;
> +	part->chip_select_polarity =
> +		(target->mode & SPI_CS_HIGH) ? true : false;
> +}
> +
v> +static void
> +efi_spi_init_peripheral(struct efi_spi_peripheral *peripheral,
> +			struct efi_spi_part *part,
> +			struct efi_spi_bus *bus,
> +			struct spi_slave *target,
> +			efi_guid_t *guid,
> +			efi_string_t name_utf16
> +)
> +{
> +	peripheral->friendly_name = name_utf16;
> +	peripheral->spi_part = part;
> +	peripheral->next_spi_peripheral = NULL;
> +	peripheral->spi_peripheral_driver_guid = guid;
> +	peripheral->max_clock_hz = target->max_hz;
> +	peripheral->clock_polarity = (target->mode & SPI_CPOL) ? true : false;
> +	peripheral->clock_phase = (target->mode & SPI_CPHA) ? true : false;
> +	peripheral->attributes = 0;
> +	peripheral->configuration_data = NULL;
> +	peripheral->spi_bus = bus;
> +	peripheral->chip_select = efi_spi_peripheral_chip_select;
> +	peripheral->chip_select_parameter = NULL;
> +}
> +
> +static void
> +efi_spi_append_peripheral(struct efi_spi_peripheral *peripheral,
> +			  struct efi_spi_bus *bus
> +)
> +{
> +	if (bus->peripheral_list) {
> +		struct efi_spi_peripheral *tmp = bus->peripheral_list;
> +
> +		while (tmp->next_spi_peripheral)
> +			tmp = tmp->next_spi_peripheral;
> +
> +		tmp->next_spi_peripheral = peripheral;
> +	} else {
> +		bus->peripheral_list = peripheral;
> +	}
> +}
> +
> +static void
> +efi_spi_init_io_protocol(struct efi_spi_io_protocol *io_protocol,
> +			 struct efi_spi_peripheral *peripheral,
> +			 struct spi_slave *target
> +)
> +{
> +	u32 max_read, max_write;
> +
> +	io_protocol->spi_peripheral = peripheral;
> +	io_protocol->original_spi_peripheral = peripheral;
> +	io_protocol->legacy_spi_protocol = &dummy_legacy_spi_controller_protocol;
> +	io_protocol->transaction = efi_spi_io_transaction;
> +	io_protocol->update_spi_peripheral = efi_spi_io_update_spi_peripheral;
> +
> +	/* This is a bit of a hack. The EFI data structures do not allow us to
> +	 * represent a frame size greater than 32 bits.
> +	 */
> +	if (target->wordlen <= 32)
> +		io_protocol->frame_size_support_mask =
> +			1 << (target->wordlen - 1);
> +	else
> +		io_protocol->frame_size_support_mask = 0;
> +
> +	/* Again, this is a bit of a hack. The EFI data structures only allow
> +	 * for a single maximum transfer size whereas the u-boot spi_slave
> +	 * structure records maximum read transfer size and maximum write
> +	 * transfer size separately. So we need to take the minimum of these two
> +	 * values.
> +	 *
> +	 * In u-boot, a value of zero for these fields means there is no limit
> +	 * on the transfer size. However in the UEFI PI spec a value of zero is
> +	 * invalid so we'll use 0xFFFFFFFF as a substitute unlimited value.
> +	 */
> +	max_write = target->max_write_size ? target->max_write_size : 0xFFFFFFFF;
> +	max_read = target->max_read_size ? target->max_read_size : 0xFFFFFFFF;
> +	io_protocol->maximum_transfer_bytes = (max_read > max_write) ? max_write : max_read;
> +
> +	/* Hack++. Leave attributes set to zero since the flags listed in the
> +	 * UEFI PI spec have no defined numerical values and so cannot be used.
> +	 */
> +	io_protocol->attributes = 0;
> +}
> +
> +static efi_status_t
> +export_spi_peripheral(struct efi_spi_bus *bus, struct udevice *dev)
> +{
> +	efi_string_t name_utf16, vendor_utf16, part_number_utf16;
> +	struct efi_spi_peripheral_priv *priv;
> +	efi_status_t status;
> +	struct udevice *dev_bus = dev->parent;
> +	struct spi_slave *target;
> +	const char *name = dev_read_name(dev);
> +	const char *vendor = dev_read_string(dev, "uefi-spi-vendor");
> +	const char *part_number = dev_read_string(dev, "uefi-spi-part-number");
> +	efi_guid_t *guid =
> +		(efi_guid_t *)dev_read_u8_array_ptr(dev, "uefi-spi-io-guid", 16);
> +
> +	if (device_get_uclass_id(dev) == UCLASS_SPI_EMUL) {
> +		debug("Skipping emulated SPI peripheral %s\n", name);
> +		goto fail_1;
> +	}
> +
> +	if (!vendor || !part_number || !guid) {
> +		debug("Skipping SPI peripheral %s\n", name);
> +		status = EFI_UNSUPPORTED;
> +		goto fail_1;
> +	}
> +
> +	if (!device_active(dev)) {
> +		int ret = device_probe(dev);
> +		if (ret) {
> +			debug("Skipping SPI peripheral %s, probe failed\n", name);
> +			goto fail_1;
> +		}
> +	}
> +
> +	target = dev_get_parent_priv(dev);
> +	if (!target) {
> +		debug("Skipping uninitialized SPI peripheral %s\n", name);
> +		status = EFI_UNSUPPORTED;
> +		goto fail_1;
> +	}
> +
> +	debug("Registering SPI dev %d:%d, name %s\n",
> +	      dev_bus->seq_, spi_chip_select(dev), name);
> +
> +	priv = calloc(1, sizeof(*priv));
> +	if (!priv) {
> +		status = EFI_OUT_OF_RESOURCES;
> +		goto fail_1;
> +	}
> +
> +	vendor_utf16 = convert_efi_string(vendor);
> +	if (!vendor_utf16) {
> +		status = EFI_OUT_OF_RESOURCES;
> +		goto fail_2;
> +	}
> +
> +	part_number_utf16 = convert_efi_string(part_number);
> +	if (!part_number_utf16) {
> +		status = EFI_OUT_OF_RESOURCES;
> +		goto fail_3;
> +	}
> +
> +	name_utf16 = convert_efi_string(name);
> +	if (!name_utf16) {
> +		status = EFI_OUT_OF_RESOURCES;
> +		goto fail_4;
> +	}
> +
> +	priv->target = target;
> +
> +	efi_spi_init_part(&priv->part, target, vendor_utf16, part_number_utf16);
> +
> +	efi_spi_init_peripheral(&priv->peripheral, &priv->part,
> +				bus, target, guid, name_utf16);
> +
> +	efi_spi_append_peripheral(&priv->peripheral, bus);
> +
> +	efi_spi_init_io_protocol(&priv->io_protocol, &priv->peripheral, target);
> +
> +	status = efi_spi_new_handle(guid, &priv->io_protocol);
> +	if (status != EFI_SUCCESS)
> +		goto fail_5;
> +
> +	log_debug("Added EFI_SPI_IO_PROTOCOL for %s with guid "
> +		  "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n",
> +		  name,
> +		  guid->b[3], guid->b[2], guid->b[1], guid->b[0],
> +		  guid->b[5], guid->b[4], guid->b[7], guid->b[6],
> +		  guid->b[8], guid->b[9], guid->b[10], guid->b[11],
> +		  guid->b[12], guid->b[13], guid->b[14], guid->b[15]);
> +	return EFI_SUCCESS;
> +
> +fail_5:
> +	free(name_utf16);
> +fail_4:
> +	free(part_number_utf16);
> +fail_3:
> +	free(vendor_utf16);
> +fail_2:
> +	free(priv);
> +fail_1:
> +	return status;
> +}
> +
> +static struct efi_spi_bus *export_spi_bus(int i)
> +{
> +	struct efi_spi_bus *bus;
> +	struct udevice *dev, *child;
> +	const char *name;
> +	int r;
> +
> +	r = uclass_get_device(UCLASS_SPI, i, &dev);
> +	if (r < 0) {
> +		printf("Failed to get SPI bus %d\n", i);
> +		goto fail_1;
> +	}
> +
> +	name = dev_read_name(dev);
> +	debug("Registering SPI bus %d, name %s\n", i, name);
> +
> +	bus = calloc(1, sizeof(*bus));
> +	if (!bus)
> +		goto fail_1;
> +
> +	bus->friendly_name = convert_efi_string(name);
> +	if (!bus->friendly_name)
> +		goto fail_2;
> +
> +	bus->peripheral_list = NULL;
> +	bus->clock = efi_spi_bus_clock;
> +	bus->clock_parameter = NULL;
> +
> +	/* For the purposes of the current implementation, we do not need to expose
> +	 * the hardware device path to users of the SPI I/O protocol.
> +	 */
> +	bus->controller_path = &null_device_path;
> +
> +	device_foreach_child(child, dev) {
> +		efi_status_t status = export_spi_peripheral(bus, child);
> +
> +		if (status == EFI_OUT_OF_RESOURCES)
> +			goto fail_3;
> +	}
> +
> +	return bus;
> +
> +fail_3:
> +	destroy_efi_spi_bus(bus);
> +fail_2:
> +	free(bus);
> +fail_1:
> +	return NULL;
> +}
> +
> +efi_status_t efi_spi_protocol_register(void)
> +{
> +	efi_status_t status;
> +	struct efi_spi_configuration_protocol *proto;
> +	uint i;
> +
> +	printf("Registering EFI_SPI_CONFIGURATION_PROTOCOL\n");
> +
> +	proto = calloc(1, sizeof(*proto));
> +	if (!proto) {
> +		status = EFI_OUT_OF_RESOURCES;
> +		goto fail_1;
> +	}
> +
> +	proto->bus_count = uclass_id_count(UCLASS_SPI);
> +	proto->bus_list = calloc(proto->bus_count, sizeof(*proto->bus_list));
> +	if (!proto->bus_list) {
> +		status = EFI_OUT_OF_RESOURCES;
> +		goto fail_2;
> +	}
> +
> +	for (i = 0; i < proto->bus_count; i++) {
> +		proto->bus_list[i] = export_spi_bus(i);
> +		if (!proto->bus_list[i])
> +			goto fail_3;
> +	}
> +
> +	status = efi_spi_new_handle(&efi_spi_configuration_guid, proto);
> +	if (status != EFI_SUCCESS)
> +		goto fail_3;
> +
> +	return EFI_SUCCESS;
> +
> +fail_3:
> +	for (i = 0; i < proto->bus_count; i++) {
> +		if (proto->bus_list[i])
> +			destroy_efi_spi_bus(proto->bus_list[i]);
> +	}
> +	free(proto->bus_list);
> +fail_2:
> +	free(proto);
> +fail_1:
> +	return status;
> +}
> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile


Would you mind splitting of the selftest on a different  patchset?  

> index daac6c396820..2790fcd784e0 100644
> --- a/lib/efi_selftest/Makefile
> +++ b/lib/efi_selftest/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_EFI_LOADER_HII) += efi_selftest_hii.o
>  obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_selftest_rng.o
>  obj-$(CONFIG_EFI_GET_TIME) += efi_selftest_rtc.o
>  obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_selftest_tcg2.o
> +obj-$(CONFIG_EFI_SPI_PROTOCOL) += efi_selftest_spi_protocol.o
>  
>  ifeq ($(CONFIG_GENERATE_ACPI_TABLE),)
>  obj-y += efi_selftest_fdt.o
> diff --git a/lib/efi_selftest/efi_selftest_spi_protocol.c b/lib/efi_selftest/efi_selftest_spi_protocol.c
> new file mode 100644
> index 000000000000..946d04dbb557
> --- /dev/null
> +++ b/lib/efi_selftest/efi_selftest_spi_protocol.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2022 Micron Technology, Inc.
> + */
> +
> +#include <efi_selftest.h>
> +#include <efi_spi_protocol.h>
> +
> +static struct efi_boot_services *boottime;
> +static efi_guid_t efi_spi_configuration_guid = EFI_SPI_CONFIGURATION_GUID;
> +
> +static int setup(const efi_handle_t img_handle,
> +		 const struct efi_system_table *systable)
> +{
> +	boottime = systable->boottime;
> +	return EFI_ST_SUCCESS;
> +}
> +

[...]

Thanks!
/Ilias