[PATCH v2 0/3] Support UEFI SPI I/O protocol

[PATCH v2 0/3] Support UEFI SPI I/O protocol

[Paul Barker]

These patches add support for the UEFI SPI I/O protocol defined in the
UEFI Platform Initialization (PI) Specification, Version 1.7 Errata A
(April 2020). This allows a UEFI application to interact with devices
on the SPI bus.

The code here is self-contained and easy to enable/disable at compile
time. Compilation testing with am335x_evm_defconfig shows that the size
of u-boot.img increases by less than 2kB when CONFIG_EFI_SPI_PROTOCOL
is enabled.

Changes since v1:

* Do not enable CONFIG_EFI_SPI_PROTOCOL by default.

* Add efi_seltest unit test.

* Mark functions with EFIAPI where necessary.

* Handle debug_transaction argument to efi_spi_io_transaction().

* Handle a value of zero for target->max_read_size &
  target->max_write_size.

* Probe inactive SPI devices when initializing the EFI SPI protocol to
  ensure that dev_get_parent_priv() returns valid data and the exported
  devices are ready to use.

* Skip emulated SPI peripherals. These appear as duplicates within the
  list of devices on the bus when using the sandbox SPI drivers.

* Make most printf statements debug only.

Paul Barker (3):
  efi_loader: Add SPI I/O protocol support
  arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export
  am335x_evm_defconfig: Enable Micron SPI flash support

 MAINTAINERS                                  |   7 +
 arch/arm/dts/am335x-sancloud-bbe-lite.dts    |   8 +-
 arch/sandbox/dts/test.dts                    |   8 +
 configs/am335x_evm_defconfig                 |   1 +
 include/efi_loader.h                         |   4 +
 include/efi_spi_protocol.h                   | 158 +++++
 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 | 237 +++++++
 12 files changed, 1051 insertions(+), 2 deletions(-)
 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


base-commit: 707b17f64e71fad1615ea25082f2e928f712e366
-- 
2.25.1

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

[Paul Barker]

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.

Since there are no open source implementations of this protocol 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                    |   8 +
 include/efi_loader.h                         |   4 +
 include/efi_spi_protocol.h                   | 158 +++++
 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 | 237 +++++++
 10 files changed, 1044 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 4d1930f76e44..1b5d9c37576b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -856,6 +856,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 d1a8cc7bfb7e..5ac59140b748 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -1185,6 +1185,10 @@
 			compatible = "spansion,m25p16", "jedec,spi-nor";
 			spi-max-frequency = <40000000>;
 			sandbox,filename = "spi.bin";
+
+			uefi-spi-vendor = "spansion";
+			uefi-spi-part-number = "mt25p16";
+			uefi-spi-io-guid = [5deb81b8 92ad 484a 8fdd fa75a8e4c6b8];
 		};
 		spi.bin@1 {
 			reg = <1>;
@@ -1193,6 +1197,10 @@
 			sandbox,filename = "spi.bin";
 			spi-cpol;
 			spi-cpha;
+
+			uefi-spi-vendor = "spansion";
+			uefi-spi-part-number = "mt25p16";
+			uefi-spi-io-guid = [cd9eb3b6 1f2b 43a6 b8d7 3192d7cf7270];
 		};
 	};
 
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 3a63a1f75fdf..2eb96b08205b 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -537,6 +537,10 @@ efi_status_t efi_rng_register(void);
 efi_status_t efi_tcg2_register(void);
 /* Called by efi_init_obj_list() to install RISCV_EFI_BOOT_PROTOCOL */
 efi_status_t efi_riscv_register(void);
+/* Called by efi_init_obj_list() to install EFI_SPI_CONFIGURATION_PROTOCOL &
+ * EFI_SPI_IO_PROTOCOL
+ */
+efi_status_t efi_spi_protocol_register(void);
 /* Called by efi_init_obj_list() to do initial measurement */
 efi_status_t efi_tcg2_do_initial_measurement(void);
 /* measure the pe-coff image, extend PCR and add Event Log */
diff --git a/include/efi_spi_protocol.h b/include/efi_spi_protocol.h
new file mode 100644
index 000000000000..1a4456bd9349
--- /dev/null
+++ b/include/efi_spi_protocol.h
@@ -0,0 +1,158 @@
+/* SPDX-License-Identifier: BSD-2-Clause-Patent */
+/*
+ * Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ */
+
+#ifndef _EFI_SPI_PROTOCOL_H
+#define _EFI_SPI_PROTOCOL_H
+
+#include <efi.h>
+#include <efi_api.h>
+
+#define EFI_SPI_CONFIGURATION_GUID  \
+	EFI_GUID(0x85a6d3e6, 0xb65b, 0x4afc,     \
+		 0xb3, 0x8f, 0xc6, 0xd5, 0x4a, 0xf6, 0xdd, 0xc8)
+
+struct efi_spi_peripheral;
+
+struct efi_spi_part {
+	efi_string_t vendor;
+	efi_string_t part_number;
+	u32 min_clock_hz;
+	u32 max_clock_hz;
+	bool chip_select_polarity;
+};
+
+struct efi_spi_bus {
+	efi_string_t friendly_name;
+	struct efi_spi_peripheral *peripheral_list;
+	struct efi_device_path *controller_path;
+
+	efi_status_t
+	(EFIAPI * clock)(const struct efi_spi_peripheral *spi_peripheral,
+		u32 *clock_hz);
+
+	void *clock_parameter;
+};
+
+struct efi_spi_peripheral {
+	struct efi_spi_peripheral *next_spi_peripheral;
+	efi_string_t friendly_name;
+	efi_guid_t *spi_peripheral_driver_guid;
+	struct efi_spi_part *spi_part;
+	u32 max_clock_hz;
+	bool clock_polarity;
+	bool clock_phase;
+	u32 attributes;
+	void *configuration_data;
+	struct efi_spi_bus *spi_bus;
+
+	efi_status_t
+	(EFIAPI * chip_select)(const struct efi_spi_peripheral *spi_peripheral,
+		bool pin_value);
+
+	void *chip_select_parameter;
+};
+
+struct efi_spi_configuration_protocol {
+	u32 bus_count;
+	struct efi_spi_bus **bus_list;
+};
+
+#define EFI_LEGACY_SPI_CONTROLLER_GUID  \
+	EFI_GUID(0x39136fc7, 0x1a11, 0x49de,         \
+		 0xbf, 0x35, 0x0e, 0x78, 0xdd, 0xb5, 0x24, 0xfc)
+
+struct efi_legacy_spi_controller_protocol;
+
+struct efi_legacy_spi_controller_protocol {
+	u32 maximum_offset;
+	u32 maximum_range_bytes;
+	u32 range_register_count;
+
+	efi_status_t
+	(EFIAPI * erase_block_opcode)(const struct efi_legacy_spi_controller_protocol *this,
+		u8 erase_block_opcode);
+
+	efi_status_t
+	(EFIAPI * write_status_prefix)(const struct efi_legacy_spi_controller_protocol *this,
+		u8 write_status_prefix);
+
+	efi_status_t
+	(EFIAPI * bios_base_address)(const struct efi_legacy_spi_controller_protocol *this,
+		u32 bios_base_address);
+
+	efi_status_t
+	(EFIAPI * clear_spi_protect)(const struct efi_legacy_spi_controller_protocol *this);
+
+	bool
+	(EFIAPI * is_range_protected)(const struct efi_legacy_spi_controller_protocol *this,
+		u32 bios_address,
+		u32 blocks_to_protect);
+
+	efi_status_t
+	(EFIAPI * protect_next_range)(const struct efi_legacy_spi_controller_protocol *this,
+		u32 bios_address,
+		u32 blocks_to_protect);
+
+	efi_status_t
+	(EFIAPI * lock_controller)(const struct efi_legacy_spi_controller_protocol *this);
+};
+
+struct efi_spi_io_protocol;
+
+enum efi_spi_transaction_type {
+	SPI_TRANSACTION_FULL_DUPLEX,
+	SPI_TRANSACTION_WRITE_ONLY,
+	SPI_TRANSACTION_READ_ONLY,
+	SPI_TRANSACTION_WRITE_THEN_READ
+};
+
+struct efi_spi_bus_transaction {
+	struct efi_spi_peripheral *spi_peripheral;
+	enum efi_spi_transaction_type transaction_type;
+	bool debug_transaction;
+	u32 bus_width;
+	u32 frame_size;
+	u32 write_bytes;
+	u8 *write_buffer;
+	u32 read_bytes;
+	u8 *read_buffer;
+};
+
+struct efi_spi_io_protocol {
+	struct efi_spi_peripheral *spi_peripheral;
+	struct efi_spi_peripheral *original_spi_peripheral;
+	u32 frame_size_support_mask;
+	u32 maximum_transfer_bytes;
+	u32 attributes;
+	struct efi_legacy_spi_controller_protocol *legacy_spi_protocol;
+
+	efi_status_t
+	(EFIAPI * 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);
+
+	efi_status_t
+	(EFIAPI * update_spi_peripheral)(struct efi_spi_io_protocol *this,
+		struct efi_spi_peripheral *spi_peripheral);
+};
+
+struct efi_spi_peripheral_priv {
+	struct efi_spi_peripheral peripheral;
+	struct efi_spi_part part;
+	struct efi_spi_io_protocol io_protocol;
+	efi_handle_t handle;
+	struct spi_slave *target;
+};
+
+efi_status_t efi_spi_protocol_register(void);
+
+#endif
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index e3f2402d0e8e..c5899a6f6e52 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -394,4 +394,12 @@ config EFI_RISCV_BOOT_PROTOCOL
 	  replace the transfer via the device-tree. The latter is not
 	  possible on systems using ACPI.
 
+config EFI_SPI_PROTOCOL
+	bool "EFI SPI protocol support"
+	depends on DM_SPI
+	help
+	  Provide implementations of EFI_SPI_CONFIGURATION_PROTOCOL and
+	  EFI_SPI_IO_PROTOCOL to allow UEFI applications to access devices
+	  connected via SPI bus.
+
 endif
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index f54c244c3268..9fa0d27927b6 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -74,6 +74,7 @@ obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
 obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
 obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
 obj-$(CONFIG_EFI_RISCV_BOOT_PROTOCOL) += efi_riscv.o
+obj-$(CONFIG_EFI_SPI_PROTOCOL) += efi_spi_protocol.o
 obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
 obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o
 
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 492ecf4cb15c..ef1ee9862b72 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -295,6 +295,12 @@ efi_status_t efi_init_obj_list(void)
 			goto out;
 	}
 
+	if (IS_ENABLED(CONFIG_EFI_SPI_PROTOCOL)) {
+		ret = efi_spi_protocol_register();
+		if (ret != EFI_SUCCESS)
+			goto out;
+	}
+
 	/* Secure boot */
 	ret = efi_init_secure_boot();
 	if (ret != EFI_SUCCESS)
diff --git a/lib/efi_loader/efi_spi_protocol.c b/lib/efi_loader/efi_spi_protocol.c
new file mode 100644
index 000000000000..c491741cbbf1
--- /dev/null
+++ b/lib/efi_loader/efi_spi_protocol.c
@@ -0,0 +1,614 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2022 Micron Technology, Inc.
+ */
+
+#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;
+}
+
+static void dump_buffer(const char *msg, u32 length, u8 *buffer)
+{
+	u32 i;
+	EFI_PRINT("%s %d bytes:", msg, length);
+	for (i = 0; i < length; i++)
+		EFI_PRINT(" %02x", buffer[i]);
+	EFI_PRINT("\n");
+}
+
+static efi_status_t EFIAPI
+efi_spi_bus_clock(const struct efi_spi_peripheral *spi_peripheral,
+		  u32 *clock_hz)
+{
+	EFI_ENTRY("%p, %p", spi_peripheral, clock_hz);
+	return EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+static efi_status_t EFIAPI
+efi_spi_peripheral_chip_select(const struct efi_spi_peripheral *spi_peripheral,
+			       bool pin_value)
+{
+	EFI_ENTRY("%p, %d", spi_peripheral, pin_value);
+	return EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+static efi_status_t EFIAPI
+legacy_erase_block_opcode(const struct efi_legacy_spi_controller_protocol *this,
+			  u8 erase_block_opcode)
+{
+	EFI_ENTRY("%p, %u", this, erase_block_opcode);
+	return EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+static efi_status_t EFIAPI
+legacy_write_status_prefix(const struct efi_legacy_spi_controller_protocol *this,
+			   u8 write_status_prefix)
+{
+	EFI_ENTRY("%p, %u", this, write_status_prefix);
+	return EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+static efi_status_t EFIAPI
+legacy_bios_base_address(const struct efi_legacy_spi_controller_protocol *this,
+			 u32 bios_base_address)
+{
+	EFI_ENTRY("%p, %u", this, bios_base_address);
+	return EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+static efi_status_t EFIAPI
+legacy_clear_spi_protect(const struct efi_legacy_spi_controller_protocol *this)
+{
+	EFI_ENTRY("%p", this);
+	return EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+static bool EFIAPI
+legacy_is_range_protected(const struct efi_legacy_spi_controller_protocol *this,
+			  u32 bios_address,
+			  u32 blocks_to_protect)
+{
+	EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
+	return EFI_EXIT(false);
+}
+
+static efi_status_t EFIAPI
+legacy_protect_next_range(const struct efi_legacy_spi_controller_protocol *this,
+			  u32 bios_address,
+			  u32 blocks_to_protect)
+{
+	EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
+	return EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+static efi_status_t EFIAPI
+legacy_lock_controller(const struct efi_legacy_spi_controller_protocol *this)
+{
+	EFI_ENTRY("%p", this);
+	return EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+static efi_status_t EFIAPI
+efi_spi_io_update_spi_peripheral(struct efi_spi_io_protocol *this,
+				 struct efi_spi_peripheral *spi_peripheral)
+{
+	EFI_ENTRY("%p, %p", this, spi_peripheral);
+	return EFI_EXIT(EFI_UNSUPPORTED);
+}
+
+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);
+	} 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
+};
+
+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);
+	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;
+}
+
+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;
+
+	printf("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[0], guid->b[1], guid->b[2], guid->b[3],
+	       guid->b[4], guid->b[5], guid->b[6], guid->b[7],
+	       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
index 33536c9ec021..c9b6e4fbb452 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -62,6 +62,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..2a3723d3931f
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_spi_protocol.c
@@ -0,0 +1,237 @@
+// 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;
+}
+
+static int test_peripheral(struct efi_spi_peripheral *p, struct efi_spi_bus *bus)
+{
+	struct efi_spi_io_protocol *io_protocol;
+	u8 req[5], resp[5];
+	efi_status_t ret;
+
+	if (!p->friendly_name) {
+		efi_st_error("SPI peripheral lacks a friendly name\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!p->spi_peripheral_driver_guid) {
+		efi_st_error("SPI peripheral lacks driver GUID\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!p->spi_part) {
+		efi_st_error("SPI peripheral lacks SPI part definition\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!p->max_clock_hz) {
+		efi_st_error("SPI peripheral has a max clock rate of zero\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!p->spi_bus) {
+		efi_st_error("SPI peripheral lack pointer to SPI bus\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (p->spi_bus != bus) {
+		efi_st_error("SPI peripheral spi_bus pointer points to the wrong bus\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!p->chip_select) {
+		efi_st_error("SPI peripheral lacks chip_select function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!p->spi_part->vendor) {
+		efi_st_error("SPI part lacks vendor string\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!p->spi_part->part_number) {
+		efi_st_error("SPI part lacks part number string\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (p->spi_part->min_clock_hz > p->spi_part->max_clock_hz) {
+		efi_st_error("SPI part min clock rate is greater than max clock rate\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (p->spi_part->max_clock_hz != p->max_clock_hz) {
+		efi_st_error("SPI part max clock rate does not match peripheral max clock rate\n");
+		return EFI_ST_FAILURE;
+	}
+
+	ret = boottime->locate_protocol(p->spi_peripheral_driver_guid,
+					NULL, (void **)&io_protocol);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("SPI IO protocol not available\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (io_protocol->spi_peripheral != p) {
+		efi_st_error("SPI IO protocol spi_peripheral pointer points to the wrong peripheral\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (io_protocol->original_spi_peripheral != p) {
+		efi_st_error("SPI IO protocol original_spi_peripheral pointer points to the wrong peripheral\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->maximum_transfer_bytes) {
+		efi_st_error("SPI IO protocol has zero maximum transfer size\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->legacy_spi_protocol) {
+		efi_st_error("SPI IO protocol lacks legacy SPI protocol\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->transaction) {
+		efi_st_error("SPI IO protocol lacks transaction function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->update_spi_peripheral) {
+		efi_st_error("SPI IO protocol lacks update_spi_peripheral function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->legacy_spi_protocol->erase_block_opcode) {
+		efi_st_error("SPI legacy controller protocol lacks erase_block_opcode function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->legacy_spi_protocol->write_status_prefix) {
+		efi_st_error("SPI legacy controller protocol lacks write_status_prefix function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->legacy_spi_protocol->bios_base_address) {
+		efi_st_error("SPI legacy controller protocol lacks bios_base_address function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->legacy_spi_protocol->clear_spi_protect) {
+		efi_st_error("SPI legacy controller protocol lacks clear_spi_protect function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->legacy_spi_protocol->is_range_protected) {
+		efi_st_error("SPI legacy controller protocol lacks is_range_protected function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->legacy_spi_protocol->protect_next_range) {
+		efi_st_error("SPI legacy controller protocol lacks protect_next_range function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!io_protocol->legacy_spi_protocol->lock_controller) {
+		efi_st_error("SPI legacy controller protocol lacks lock_controller function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	req[0] = 0x9f;
+	ret = io_protocol->transaction(io_protocol,
+				       SPI_TRANSACTION_FULL_DUPLEX,
+				       false, 0, 1, 8,
+				       sizeof(req), req,
+				       sizeof(resp), resp);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("SPI transaction failed\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if ((resp[0] != 0xff) || (resp[1] != 0x20) || (resp[2] != 0x20) || (resp[3] != 0x15)) {
+		efi_st_error("Incorrect response from sandbox SPI flash emulator\n");
+		return EFI_ST_FAILURE;
+	}
+
+	return EFI_ST_SUCCESS;
+}
+
+static int test_bus(struct efi_spi_bus *bus)
+{
+	struct efi_spi_peripheral *p;
+	int status;
+
+	if (!bus->friendly_name) {
+		efi_st_error("SPI bus lacks a friendly name\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!bus->peripheral_list) {
+		efi_st_error("SPI bus has zero peripherals\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!bus->clock) {
+		efi_st_error("SPI bus lacks clock function\n");
+		return EFI_ST_FAILURE;
+	}
+
+	for (p = bus->peripheral_list; p; p = p->next_spi_peripheral) {
+		status = test_peripheral(p, bus);
+		if (status) {
+			efi_st_error("Failed testing SPI peripheral\n");
+			return status;
+		}
+	}
+
+	return EFI_ST_SUCCESS;
+}
+
+static int execute(void)
+{
+	struct efi_spi_configuration_protocol *spi;
+	efi_status_t ret;
+	int status;
+	u32 i;
+
+	ret = boottime->locate_protocol(&efi_spi_configuration_guid,
+					NULL, (void **)&spi);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("SPI configuration protocol not available\n");
+		return EFI_ST_FAILURE;
+	}
+
+	if (!spi->bus_count) {
+		efi_st_error("SPI configuration protocol has zero busses\n");
+		return EFI_ST_FAILURE;
+	}
+
+	for (i = 0; i < spi->bus_count; i++) {
+		status = test_bus(spi->bus_list[i]);
+		if (status) {
+			efi_st_error("Failed testing SPI bus %d\n", i);
+			return status;
+		}
+	}
+
+	return EFI_ST_SUCCESS;
+}
+
+EFI_UNIT_TEST(spi_protocol) = {
+    .name = "SPI protocol",
+    .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
+    .setup = setup,
+    .execute = execute,
+};
-- 
2.25.1

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

[Heinrich Schuchardt]

On 8/4/22 12:34, 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.
>
> Since there are no open source implementations of this protocol 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.

The commit message should describe which protocols are actually implemented.

I still have no clue why any PI protocol should be implemented U-Boot.
You neither descibe it here nor in the cover-letter.

I would expect capsule updates to be used to update the SPI flash.

>
> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
> ---
>   MAINTAINERS                                  |   7 +
>   arch/sandbox/dts/test.dts                    |   8 +
>   include/efi_loader.h                         |   4 +
>   include/efi_spi_protocol.h                   | 158 +++++
>   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 | 237 +++++++
>   10 files changed, 1044 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 4d1930f76e44..1b5d9c37576b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -856,6 +856,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 d1a8cc7bfb7e..5ac59140b748 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -1185,6 +1185,10 @@
>   			compatible = "spansion,m25p16", "jedec,spi-nor";
>   			spi-max-frequency = <40000000>;
>   			sandbox,filename = "spi.bin";
> +
> +			uefi-spi-vendor = "spansion";
> +			uefi-spi-part-number = "mt25p16";
> +			uefi-spi-io-guid = [5deb81b8 92ad 484a 8fdd fa75a8e4c6b8];
>   		};
>   		spi.bin@1 {
>   			reg = <1>;
> @@ -1193,6 +1197,10 @@
>   			sandbox,filename = "spi.bin";
>   			spi-cpol;
>   			spi-cpha;
> +
> +			uefi-spi-vendor = "spansion";

Does the sandbox provide anything that is related to vendor spansion?
Or could the vendor be "sandbox"?

> +			uefi-spi-part-number = "mt25p16";

Does the sandbox really emulate a
"Micron M25P16 Serial Flash Embedded Memory"?

> +			uefi-spi-io-guid = [cd9eb3b6 1f2b 43a6 b8d7 3192d7cf7270];

This is a byte-string. The byte order in the array differs from the low
endian string representation used in the UEFI specification. To avoid
confusion, please, add a space after each byte and add a comment with
the string representation.

>   		};
>   	};
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 3a63a1f75fdf..2eb96b08205b 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -537,6 +537,10 @@ efi_status_t efi_rng_register(void);
>   efi_status_t efi_tcg2_register(void);
>   /* Called by efi_init_obj_list() to install RISCV_EFI_BOOT_PROTOCOL */
>   efi_status_t efi_riscv_register(void);
> +/* Called by efi_init_obj_list() to install EFI_SPI_CONFIGURATION_PROTOCOL &
> + * EFI_SPI_IO_PROTOCOL
> + */
> +efi_status_t efi_spi_protocol_register(void);
>   /* Called by efi_init_obj_list() to do initial measurement */
>   efi_status_t efi_tcg2_do_initial_measurement(void);
>   /* measure the pe-coff image, extend PCR and add Event Log */
> diff --git a/include/efi_spi_protocol.h b/include/efi_spi_protocol.h
> new file mode 100644
> index 000000000000..1a4456bd9349
> --- /dev/null
> +++ b/include/efi_spi_protocol.h
> @@ -0,0 +1,158 @@
> +/* SPDX-License-Identifier: BSD-2-Clause-Patent */
> +/*
> + * Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>

Is this really a copy from EDK II and not based on the specification?

Please, add a reference to the
"UEFI Platform Initialization (PI) Specification".
and a description what this include is about.


> + */
> +
> +#ifndef _EFI_SPI_PROTOCOL_H
> +#define _EFI_SPI_PROTOCOL_H
> +
> +#include <efi.h>
> +#include <efi_api.h>
> +
> +#define EFI_SPI_CONFIGURATION_GUID  \
> +	EFI_GUID(0x85a6d3e6, 0xb65b, 0x4afc,     \
> +		 0xb3, 0x8f, 0xc6, 0xd5, 0x4a, 0xf6, 0xdd, 0xc8)

Please, add a comment that this GUID is defined in the "UEFI Platform
Initialization (PI) Specification".

Please, add the GUID to list_guid[]in lib/uuid.c .

> +
> +struct efi_spi_peripheral;
> +
> +struct efi_spi_part {
> +	efi_string_t vendor;
> +	efi_string_t part_number;

Why did you remove "const" used in the specifications?

> +	u32 min_clock_hz;
> +	u32 max_clock_hz;
> +	bool chip_select_polarity;
> +};
> +
> +struct efi_spi_bus {
> +	efi_string_t friendly_name;
> +	struct efi_spi_peripheral *peripheral_list;
> +	struct efi_device_path *controller_path;
> +
> +	efi_status_t
> +	(EFIAPI * clock)(const struct efi_spi_peripheral *spi_peripheral,
> +		u32 *clock_hz);
> +
> +	void *clock_parameter;
> +};
> +
> +struct efi_spi_peripheral {
> +	struct efi_spi_peripheral *next_spi_peripheral;
> +	efi_string_t friendly_name;
> +	efi_guid_t *spi_peripheral_driver_guid;
> +	struct efi_spi_part *spi_part;
> +	u32 max_clock_hz;
> +	bool clock_polarity;
> +	bool clock_phase;
> +	u32 attributes;
> +	void *configuration_data;
> +	struct efi_spi_bus *spi_bus;
> +
> +	efi_status_t
> +	(EFIAPI * chip_select)(const struct efi_spi_peripheral *spi_peripheral,
> +		bool pin_value);
> +
> +	void *chip_select_parameter;
> +};
> +
> +struct efi_spi_configuration_protocol {
> +	u32 bus_count;
> +	struct efi_spi_bus **bus_list;
> +};
> +
> +#define EFI_LEGACY_SPI_CONTROLLER_GUID  \
> +	EFI_GUID(0x39136fc7, 0x1a11, 0x49de,         \
> +		 0xbf, 0x35, 0x0e, 0x78, 0xdd, 0xb5, 0x24, 0xfc)

The commit message clear states that the legacy protocol will not be
implemented. Why should we implement any *legacy*?

> +
> +struct efi_legacy_spi_controller_protocol;
> +
> +struct efi_legacy_spi_controller_protocol {
> +	u32 maximum_offset;
> +	u32 maximum_range_bytes;
> +	u32 range_register_count;
> +
> +	efi_status_t
> +	(EFIAPI * erase_block_opcode)(const struct efi_legacy_spi_controller_protocol *this,
> +		u8 erase_block_opcode);
> +
> +	efi_status_t
> +	(EFIAPI * write_status_prefix)(const struct efi_legacy_spi_controller_protocol *this,
> +		u8 write_status_prefix);
> +
> +	efi_status_t
> +	(EFIAPI * bios_base_address)(const struct efi_legacy_spi_controller_protocol *this,
> +		u32 bios_base_address);
> +
> +	efi_status_t
> +	(EFIAPI * clear_spi_protect)(const struct efi_legacy_spi_controller_protocol *this);
> +
> +	bool
> +	(EFIAPI * is_range_protected)(const struct efi_legacy_spi_controller_protocol *this,
> +		u32 bios_address,
> +		u32 blocks_to_protect);
> +
> +	efi_status_t
> +	(EFIAPI * protect_next_range)(const struct efi_legacy_spi_controller_protocol *this,
> +		u32 bios_address,
> +		u32 blocks_to_protect);
> +
> +	efi_status_t
> +	(EFIAPI * lock_controller)(const struct efi_legacy_spi_controller_protocol *this);
> +};
> +
> +struct efi_spi_io_protocol;
> +
> +enum efi_spi_transaction_type {
> +	SPI_TRANSACTION_FULL_DUPLEX,
> +	SPI_TRANSACTION_WRITE_ONLY,
> +	SPI_TRANSACTION_READ_ONLY,
> +	SPI_TRANSACTION_WRITE_THEN_READ
> +};
> +
> +struct efi_spi_bus_transaction {
> +	struct efi_spi_peripheral *spi_peripheral;
> +	enum efi_spi_transaction_type transaction_type;
> +	bool debug_transaction;
> +	u32 bus_width;
> +	u32 frame_size;
> +	u32 write_bytes;
> +	u8 *write_buffer;
> +	u32 read_bytes;
> +	u8 *read_buffer;
> +};
> +
> +struct efi_spi_io_protocol {
> +	struct efi_spi_peripheral *spi_peripheral;
> +	struct efi_spi_peripheral *original_spi_peripheral;
> +	u32 frame_size_support_mask;
> +	u32 maximum_transfer_bytes;
> +	u32 attributes;
> +	struct efi_legacy_spi_controller_protocol *legacy_spi_protocol;
> +
> +	efi_status_t
> +	(EFIAPI * 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);
> +
> +	efi_status_t
> +	(EFIAPI * update_spi_peripheral)(struct efi_spi_io_protocol *this,
> +		struct efi_spi_peripheral *spi_peripheral);
> +};
> +
> +struct efi_spi_peripheral_priv {
> +	struct efi_spi_peripheral peripheral;
> +	struct efi_spi_part part;
> +	struct efi_spi_io_protocol io_protocol;
> +	efi_handle_t handle;
> +	struct spi_slave *target;
> +};
> +
> +efi_status_t efi_spi_protocol_register(void);
> +
> +#endif
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index e3f2402d0e8e..c5899a6f6e52 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -394,4 +394,12 @@ config EFI_RISCV_BOOT_PROTOCOL
>   	  replace the transfer via the device-tree. The latter is not
>   	  possible on systems using ACPI.
>
> +config EFI_SPI_PROTOCOL
> +	bool "EFI SPI protocol support"
> +	depends on DM_SPI
> +	help
> +	  Provide implementations of EFI_SPI_CONFIGURATION_PROTOCOL and
> +	  EFI_SPI_IO_PROTOCOL to allow UEFI applications to access devices
> +	  connected via SPI bus.
> +
>   endif
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index f54c244c3268..9fa0d27927b6 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -74,6 +74,7 @@ obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
>   obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
>   obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
>   obj-$(CONFIG_EFI_RISCV_BOOT_PROTOCOL) += efi_riscv.o
> +obj-$(CONFIG_EFI_SPI_PROTOCOL) += efi_spi_protocol.o
>   obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
>   obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o
>
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 492ecf4cb15c..ef1ee9862b72 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -295,6 +295,12 @@ efi_status_t efi_init_obj_list(void)
>   			goto out;
>   	}
>
> +	if (IS_ENABLED(CONFIG_EFI_SPI_PROTOCOL)) {
> +		ret = efi_spi_protocol_register();
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +	}
> +
>   	/* Secure boot */
>   	ret = efi_init_secure_boot();
>   	if (ret != EFI_SUCCESS)
> diff --git a/lib/efi_loader/efi_spi_protocol.c b/lib/efi_loader/efi_spi_protocol.c
> new file mode 100644
> index 000000000000..c491741cbbf1
> --- /dev/null
> +++ b/lib/efi_loader/efi_spi_protocol.c
> @@ -0,0 +1,614 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2022 Micron Technology, Inc.
> + */
> +
> +#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;
> +}
> +
> +static void dump_buffer(const char *msg, u32 length, u8 *buffer)
> +{
> +	u32 i;
> +	EFI_PRINT("%s %d bytes:", msg, length);
> +	for (i = 0; i < length; i++)
> +		EFI_PRINT(" %02x", buffer[i]);
> +	EFI_PRINT("\n");
> +}
> +
> +static efi_status_t EFIAPI
> +efi_spi_bus_clock(const struct efi_spi_peripheral *spi_peripheral,
> +		  u32 *clock_hz)
> +{
> +	EFI_ENTRY("%p, %p", spi_peripheral, clock_hz);
> +	return EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +static efi_status_t EFIAPI
> +efi_spi_peripheral_chip_select(const struct efi_spi_peripheral *spi_peripheral,
> +			       bool pin_value)
> +{
> +	EFI_ENTRY("%p, %d", spi_peripheral, pin_value);
> +	return EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +static efi_status_t EFIAPI
> +legacy_erase_block_opcode(const struct efi_legacy_spi_controller_protocol *this,
> +			  u8 erase_block_opcode)
> +{
> +	EFI_ENTRY("%p, %u", this, erase_block_opcode);
> +	return EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +static efi_status_t EFIAPI
> +legacy_write_status_prefix(const struct efi_legacy_spi_controller_protocol *this,
> +			   u8 write_status_prefix)
> +{
> +	EFI_ENTRY("%p, %u", this, write_status_prefix);
> +	return EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +static efi_status_t EFIAPI
> +legacy_bios_base_address(const struct efi_legacy_spi_controller_protocol *this,
> +			 u32 bios_base_address)
> +{
> +	EFI_ENTRY("%p, %u", this, bios_base_address);
> +	return EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +static efi_status_t EFIAPI
> +legacy_clear_spi_protect(const struct efi_legacy_spi_controller_protocol *this)
> +{
> +	EFI_ENTRY("%p", this);
> +	return EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +static bool EFIAPI
> +legacy_is_range_protected(const struct efi_legacy_spi_controller_protocol *this,
> +			  u32 bios_address,
> +			  u32 blocks_to_protect)
> +{
> +	EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
> +	return EFI_EXIT(false);
> +}
> +
> +static efi_status_t EFIAPI
> +legacy_protect_next_range(const struct efi_legacy_spi_controller_protocol *this,
> +			  u32 bios_address,
> +			  u32 blocks_to_protect)
> +{
> +	EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
> +	return EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +static efi_status_t EFIAPI
> +legacy_lock_controller(const struct efi_legacy_spi_controller_protocol *this)
> +{
> +	EFI_ENTRY("%p", this);
> +	return EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +static efi_status_t EFIAPI
> +efi_spi_io_update_spi_peripheral(struct efi_spi_io_protocol *this,
> +				 struct efi_spi_peripheral *spi_peripheral)
> +{
> +	EFI_ENTRY("%p, %p", this, spi_peripheral);
> +	return EFI_EXIT(EFI_UNSUPPORTED);
> +}
> +
> +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);
> +	} 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
> +};

In the commit message you said that the legacy protocol will not be
implemented.

Why would a *legacy* be implemented?

> +
> +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);
> +	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;
> +}
> +
> +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;
> +
> +	printf("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",

This information does not interest any user. Please, use log_debug().

> +	       name,
> +	       guid->b[0], guid->b[1], guid->b[2], guid->b[3],
> +	       guid->b[4], guid->b[5], guid->b[6], guid->b[7],
> +	       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
> index 33536c9ec021..c9b6e4fbb452 100644
> --- a/lib/efi_selftest/Makefile
> +++ b/lib/efi_selftest/Makefile
> @@ -62,6 +62,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..2a3723d3931f
> --- /dev/null
> +++ b/lib/efi_selftest/efi_selftest_spi_protocol.c
> @@ -0,0 +1,237 @@
> +// 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;
> +}
> +
> +static int test_peripheral(struct efi_spi_peripheral *p, struct efi_spi_bus *bus)
> +{
> +	struct efi_spi_io_protocol *io_protocol;
> +	u8 req[5], resp[5];
> +	efi_status_t ret;
> +
> +	if (!p->friendly_name) {
> +		efi_st_error("SPI peripheral lacks a friendly name\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!p->spi_peripheral_driver_guid) {
> +		efi_st_error("SPI peripheral lacks driver GUID\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!p->spi_part) {
> +		efi_st_error("SPI peripheral lacks SPI part definition\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!p->max_clock_hz) {
> +		efi_st_error("SPI peripheral has a max clock rate of zero\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!p->spi_bus) {
> +		efi_st_error("SPI peripheral lack pointer to SPI bus\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (p->spi_bus != bus) {
> +		efi_st_error("SPI peripheral spi_bus pointer points to the wrong bus\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!p->chip_select) {
> +		efi_st_error("SPI peripheral lacks chip_select function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!p->spi_part->vendor) {
> +		efi_st_error("SPI part lacks vendor string\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!p->spi_part->part_number) {
> +		efi_st_error("SPI part lacks part number string\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (p->spi_part->min_clock_hz > p->spi_part->max_clock_hz) {
> +		efi_st_error("SPI part min clock rate is greater than max clock rate\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (p->spi_part->max_clock_hz != p->max_clock_hz) {
> +		efi_st_error("SPI part max clock rate does not match peripheral max clock rate\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	ret = boottime->locate_protocol(p->spi_peripheral_driver_guid,
> +					NULL, (void **)&io_protocol);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("SPI IO protocol not available\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (io_protocol->spi_peripheral != p) {
> +		efi_st_error("SPI IO protocol spi_peripheral pointer points to the wrong peripheral\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (io_protocol->original_spi_peripheral != p) {
> +		efi_st_error("SPI IO protocol original_spi_peripheral pointer points to the wrong peripheral\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->maximum_transfer_bytes) {
> +		efi_st_error("SPI IO protocol has zero maximum transfer size\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->legacy_spi_protocol) {
> +		efi_st_error("SPI IO protocol lacks legacy SPI protocol\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->transaction) {
> +		efi_st_error("SPI IO protocol lacks transaction function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->update_spi_peripheral) {
> +		efi_st_error("SPI IO protocol lacks update_spi_peripheral function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->legacy_spi_protocol->erase_block_opcode) {
> +		efi_st_error("SPI legacy controller protocol lacks erase_block_opcode function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->legacy_spi_protocol->write_status_prefix) {
> +		efi_st_error("SPI legacy controller protocol lacks write_status_prefix function\n");
> +		return EFI_ST_FAILURE;
> +	}

I would have expected that you actually call each of the functions.

> +
> +	if (!io_protocol->legacy_spi_protocol->bios_base_address) {
> +		efi_st_error("SPI legacy controller protocol lacks bios_base_address function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->legacy_spi_protocol->clear_spi_protect) {
> +		efi_st_error("SPI legacy controller protocol lacks clear_spi_protect function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->legacy_spi_protocol->is_range_protected) {
> +		efi_st_error("SPI legacy controller protocol lacks is_range_protected function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->legacy_spi_protocol->protect_next_range) {
> +		efi_st_error("SPI legacy controller protocol lacks protect_next_range function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!io_protocol->legacy_spi_protocol->lock_controller) {
> +		efi_st_error("SPI legacy controller protocol lacks lock_controller function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	req[0] = 0x9f;
> +	ret = io_protocol->transaction(io_protocol,
> +				       SPI_TRANSACTION_FULL_DUPLEX,
> +				       false, 0, 1, 8,
> +				       sizeof(req), req,
> +				       sizeof(resp), resp);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("SPI transaction failed\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if ((resp[0] != 0xff) || (resp[1] != 0x20) || (resp[2] != 0x20) || (resp[3] != 0x15)) {
> +		efi_st_error("Incorrect response from sandbox SPI flash emulator\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	return EFI_ST_SUCCESS;
> +}
> +
> +static int test_bus(struct efi_spi_bus *bus)
> +{
> +	struct efi_spi_peripheral *p;
> +	int status;
> +
> +	if (!bus->friendly_name) {
> +		efi_st_error("SPI bus lacks a friendly name\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!bus->peripheral_list) {
> +		efi_st_error("SPI bus has zero peripherals\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!bus->clock) {
> +		efi_st_error("SPI bus lacks clock function\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	for (p = bus->peripheral_list; p; p = p->next_spi_peripheral) {
> +		status = test_peripheral(p, bus);
> +		if (status) {
> +			efi_st_error("Failed testing SPI peripheral\n");
> +			return status;
> +		}
> +	}
> +
> +	return EFI_ST_SUCCESS;
> +}
> +
> +static int execute(void)
> +{
> +	struct efi_spi_configuration_protocol *spi;
> +	efi_status_t ret;
> +	int status;
> +	u32 i;
> +
> +	ret = boottime->locate_protocol(&efi_spi_configuration_guid,
> +					NULL, (void **)&spi);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("SPI configuration protocol not available\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (!spi->bus_count) {
> +		efi_st_error("SPI configuration protocol has zero busses\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	for (i = 0; i < spi->bus_count; i++) {
> +		status = test_bus(spi->bus_list[i]);
> +		if (status) {
> +			efi_st_error("Failed testing SPI bus %d\n", i);
> +			return status;
> +		}
> +	}

This test does not test anything changing any state of SPI flash. Why is
this patch needed if that is all that is usable?

Best regards

Heinrich

> +
> +	return EFI_ST_SUCCESS;
> +}
> +
> +EFI_UNIT_TEST(spi_protocol) = {
> +    .name = "SPI protocol",
> +    .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
> +    .setup = setup,
> +    .execute = execute,
> +};

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

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 52206 bytes --]

Hi Henrich,

Many thanks for your review on this patch. I've responded to things 
inline below. I'm also available on IRC (paulbarker) if you want to 
discuss further.

On 13/08/2022 20:20, Heinrich Schuchardt wrote:
> On 8/4/22 12:34, 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.
>>
>> Since there are no open source implementations of this protocol 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.
> 
> The commit message should describe which protocols are actually 
> implemented.

I'll add this to the commit message in v3.

> 
> I still have no clue why any PI protocol should be implemented U-Boot.
> You neither descibe it here nor in the cover-letter.

Implementing the SPI I/O protocol allows a UEFI application running 
under u-boot to enumerate and communicate with devices on the SPI bus. 
This protocol fulfils the same purpose as the "SD MMC Pass Thru 
Protocol" and "NVM Express Pass Through Protocol" described in the main 
UEFI spec, I'm unsure why SPI bus access was deferred to the PI spec. If 
these patches are accepted we plan to also implement the SD/MMC and NVMe 
I/O protocols.

The broader goal here is to support better encapsulation of "pre-boot" 
application code which needs to communicate with a SPI, MMC or NVMe 
devices before Linux is booted. This pre-boot application can then run 
on both embedded devices running u-boot and PC-like devices running a 
UEFI BIOS. In our case the application will interact with the Authenta 
flash security features to verify integrity and to generate ephemeral 
keys based on the flash contents. Our application links against mbedtls 
and so it would be non-trivial (in terms of both implementation and 
licensing) to integrate this directly into u-boot.

We have previously explored using the "standalone" application support 
(e.g. in examples/standalone in the u-boot source) but as that is not 
well maintained we have moved to a UEFI implementation.

Let me know if you need any more info than that. Once you're happy with 
the explanation I'll add it to the cover letter in v3.

> 
> I would expect capsule updates to be used to update the SPI flash.

Our application is not intended to perform firmware or capsule updates 
or to make any writes to the flash.

> 
>>
>> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
>> ---
>>   MAINTAINERS                                  |   7 +
>>   arch/sandbox/dts/test.dts                    |   8 +
>>   include/efi_loader.h                         |   4 +
>>   include/efi_spi_protocol.h                   | 158 +++++
>>   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 | 237 +++++++
>>   10 files changed, 1044 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 4d1930f76e44..1b5d9c37576b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -856,6 +856,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 d1a8cc7bfb7e..5ac59140b748 100644
>> --- a/arch/sandbox/dts/test.dts
>> +++ b/arch/sandbox/dts/test.dts
>> @@ -1185,6 +1185,10 @@
>>               compatible = "spansion,m25p16", "jedec,spi-nor";
>>               spi-max-frequency = <40000000>;
>>               sandbox,filename = "spi.bin";
>> +
>> +            uefi-spi-vendor = "spansion";
>> +            uefi-spi-part-number = "mt25p16";
>> +            uefi-spi-io-guid = [5deb81b8 92ad 484a 8fdd fa75a8e4c6b8];
>>           };
>>           spi.bin@1 {
>>               reg = <1>;
>> @@ -1193,6 +1197,10 @@
>>               sandbox,filename = "spi.bin";
>>               spi-cpol;
>>               spi-cpha;
>> +
>> +            uefi-spi-vendor = "spansion";
> 
> Does the sandbox provide anything that is related to vendor spansion?
> Or could the vendor be "sandbox"?

This is based on the "spansion,m25p16" compatible string used above. 
I've used the same vendor and part number for consistency.

> 
>> +            uefi-spi-part-number = "mt25p16";
> 
> Does the sandbox really emulate a
> "Micron M25P16 Serial Flash Embedded Memory"?

As above.

> 
>> +            uefi-spi-io-guid = [cd9eb3b6 1f2b 43a6 b8d7 3192d7cf7270];
> 
> This is a byte-string. The byte order in the array differs from the low
> endian string representation used in the UEFI specification. To avoid
> confusion, please, add a space after each byte and add a comment with
> the string representation.

I'll make this change for v3.

> 
>>           };
>>       };
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 3a63a1f75fdf..2eb96b08205b 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -537,6 +537,10 @@ efi_status_t efi_rng_register(void);
>>   efi_status_t efi_tcg2_register(void);
>>   /* Called by efi_init_obj_list() to install RISCV_EFI_BOOT_PROTOCOL */
>>   efi_status_t efi_riscv_register(void);
>> +/* Called by efi_init_obj_list() to install 
>> EFI_SPI_CONFIGURATION_PROTOCOL &
>> + * EFI_SPI_IO_PROTOCOL
>> + */
>> +efi_status_t efi_spi_protocol_register(void);
>>   /* Called by efi_init_obj_list() to do initial measurement */
>>   efi_status_t efi_tcg2_do_initial_measurement(void);
>>   /* measure the pe-coff image, extend PCR and add Event Log */
>> diff --git a/include/efi_spi_protocol.h b/include/efi_spi_protocol.h
>> new file mode 100644
>> index 000000000000..1a4456bd9349
>> --- /dev/null
>> +++ b/include/efi_spi_protocol.h
>> @@ -0,0 +1,158 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause-Patent */
>> +/*
>> + * Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> 
> Is this really a copy from EDK II and not based on the specification?

This is copied from EDK II and modified to fit the u-boot coding style.

> 
> Please, add a reference to the
> "UEFI Platform Initialization (PI) Specification".
> and a description what this include is about.

I'll add this in v3.

> 
> 
>> + */
>> +
>> +#ifndef _EFI_SPI_PROTOCOL_H
>> +#define _EFI_SPI_PROTOCOL_H
>> +
>> +#include <efi.h>
>> +#include <efi_api.h>
>> +
>> +#define EFI_SPI_CONFIGURATION_GUID  \
>> +    EFI_GUID(0x85a6d3e6, 0xb65b, 0x4afc,     \
>> +         0xb3, 0x8f, 0xc6, 0xd5, 0x4a, 0xf6, 0xdd, 0xc8)
> 
> Please, add a comment that this GUID is defined in the "UEFI Platform
> Initialization (PI) Specification".
> 
> Please, add the GUID to list_guid[]in lib/uuid.c .

Will do in v3.

> 
>> +
>> +struct efi_spi_peripheral;
>> +
>> +struct efi_spi_part {
>> +    efi_string_t vendor;
>> +    efi_string_t part_number;
> 
> Why did you remove "const" used in the specifications?

These structures are const in the spec as the UEFI application code is 
not expected to modify them. However, we need to be able to modify these 
fields in u-boot before starting a UEFI application in order to populate 
them with the correct data.

> 
>> +    u32 min_clock_hz;
>> +    u32 max_clock_hz;
>> +    bool chip_select_polarity;
>> +};
>> +
>> +struct efi_spi_bus {
>> +    efi_string_t friendly_name;
>> +    struct efi_spi_peripheral *peripheral_list;
>> +    struct efi_device_path *controller_path;
>> +
>> +    efi_status_t
>> +    (EFIAPI * clock)(const struct efi_spi_peripheral *spi_peripheral,
>> +        u32 *clock_hz);
>> +
>> +    void *clock_parameter;
>> +};
>> +
>> +struct efi_spi_peripheral {
>> +    struct efi_spi_peripheral *next_spi_peripheral;
>> +    efi_string_t friendly_name;
>> +    efi_guid_t *spi_peripheral_driver_guid;
>> +    struct efi_spi_part *spi_part;
>> +    u32 max_clock_hz;
>> +    bool clock_polarity;
>> +    bool clock_phase;
>> +    u32 attributes;
>> +    void *configuration_data;
>> +    struct efi_spi_bus *spi_bus;
>> +
>> +    efi_status_t
>> +    (EFIAPI * chip_select)(const struct efi_spi_peripheral 
>> *spi_peripheral,
>> +        bool pin_value);
>> +
>> +    void *chip_select_parameter;
>> +};
>> +
>> +struct efi_spi_configuration_protocol {
>> +    u32 bus_count;
>> +    struct efi_spi_bus **bus_list;
>> +};
>> +
>> +#define EFI_LEGACY_SPI_CONTROLLER_GUID  \
>> +    EFI_GUID(0x39136fc7, 0x1a11, 0x49de,         \
>> +         0xbf, 0x35, 0x0e, 0x78, 0xdd, 0xb5, 0x24, 0xfc)
> 
> The commit message clear states that the legacy protocol will not be
> implemented. Why should we implement any *legacy*?
I'd rather ignore the whole thing and use a NULL pointer but it doesn't 
look like that's an option.

`struct efi_spi_io_protocol` contains a pointer to a `struct 
efi_legacy_spi_controller_protocol` and the spec does not seem to allow 
this or the function pointers in the legacy protocol object to be NULL. 
I don't think we can assume that a UEFI application will check for NULL 
pointers before de-referencing given the wording in the spec, so we 
instead need to provide some valid functions which at least return an 
EFI_UNSUPPORTED error condition.

> 
>> +
>> +struct efi_legacy_spi_controller_protocol;
>> +
>> +struct efi_legacy_spi_controller_protocol {
>> +    u32 maximum_offset;
>> +    u32 maximum_range_bytes;
>> +    u32 range_register_count;
>> +
>> +    efi_status_t
>> +    (EFIAPI * erase_block_opcode)(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +        u8 erase_block_opcode);
>> +
>> +    efi_status_t
>> +    (EFIAPI * write_status_prefix)(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +        u8 write_status_prefix);
>> +
>> +    efi_status_t
>> +    (EFIAPI * bios_base_address)(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +        u32 bios_base_address);
>> +
>> +    efi_status_t
>> +    (EFIAPI * clear_spi_protect)(const struct 
>> efi_legacy_spi_controller_protocol *this);
>> +
>> +    bool
>> +    (EFIAPI * is_range_protected)(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +        u32 bios_address,
>> +        u32 blocks_to_protect);
>> +
>> +    efi_status_t
>> +    (EFIAPI * protect_next_range)(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +        u32 bios_address,
>> +        u32 blocks_to_protect);
>> +
>> +    efi_status_t
>> +    (EFIAPI * lock_controller)(const struct 
>> efi_legacy_spi_controller_protocol *this);
>> +};
>> +
>> +struct efi_spi_io_protocol;
>> +
>> +enum efi_spi_transaction_type {
>> +    SPI_TRANSACTION_FULL_DUPLEX,
>> +    SPI_TRANSACTION_WRITE_ONLY,
>> +    SPI_TRANSACTION_READ_ONLY,
>> +    SPI_TRANSACTION_WRITE_THEN_READ
>> +};
>> +
>> +struct efi_spi_bus_transaction {
>> +    struct efi_spi_peripheral *spi_peripheral;
>> +    enum efi_spi_transaction_type transaction_type;
>> +    bool debug_transaction;
>> +    u32 bus_width;
>> +    u32 frame_size;
>> +    u32 write_bytes;
>> +    u8 *write_buffer;
>> +    u32 read_bytes;
>> +    u8 *read_buffer;
>> +};
>> +
>> +struct efi_spi_io_protocol {
>> +    struct efi_spi_peripheral *spi_peripheral;
>> +    struct efi_spi_peripheral *original_spi_peripheral;
>> +    u32 frame_size_support_mask;
>> +    u32 maximum_transfer_bytes;
>> +    u32 attributes;
>> +    struct efi_legacy_spi_controller_protocol *legacy_spi_protocol;
>> +
>> +    efi_status_t
>> +    (EFIAPI * 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);
>> +
>> +    efi_status_t
>> +    (EFIAPI * update_spi_peripheral)(struct efi_spi_io_protocol *this,
>> +        struct efi_spi_peripheral *spi_peripheral);
>> +};
>> +
>> +struct efi_spi_peripheral_priv {
>> +    struct efi_spi_peripheral peripheral;
>> +    struct efi_spi_part part;
>> +    struct efi_spi_io_protocol io_protocol;
>> +    efi_handle_t handle;
>> +    struct spi_slave *target;
>> +};
>> +
>> +efi_status_t efi_spi_protocol_register(void);
>> +
>> +#endif
>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>> index e3f2402d0e8e..c5899a6f6e52 100644
>> --- a/lib/efi_loader/Kconfig
>> +++ b/lib/efi_loader/Kconfig
>> @@ -394,4 +394,12 @@ config EFI_RISCV_BOOT_PROTOCOL
>>         replace the transfer via the device-tree. The latter is not
>>         possible on systems using ACPI.
>>
>> +config EFI_SPI_PROTOCOL
>> +    bool "EFI SPI protocol support"
>> +    depends on DM_SPI
>> +    help
>> +      Provide implementations of EFI_SPI_CONFIGURATION_PROTOCOL and
>> +      EFI_SPI_IO_PROTOCOL to allow UEFI applications to access devices
>> +      connected via SPI bus.
>> +
>>   endif
>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>> index f54c244c3268..9fa0d27927b6 100644
>> --- a/lib/efi_loader/Makefile
>> +++ b/lib/efi_loader/Makefile
>> @@ -74,6 +74,7 @@ obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
>>   obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
>>   obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
>>   obj-$(CONFIG_EFI_RISCV_BOOT_PROTOCOL) += efi_riscv.o
>> +obj-$(CONFIG_EFI_SPI_PROTOCOL) += efi_spi_protocol.o
>>   obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
>>   obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o
>>
>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>> index 492ecf4cb15c..ef1ee9862b72 100644
>> --- a/lib/efi_loader/efi_setup.c
>> +++ b/lib/efi_loader/efi_setup.c
>> @@ -295,6 +295,12 @@ efi_status_t efi_init_obj_list(void)
>>               goto out;
>>       }
>>
>> +    if (IS_ENABLED(CONFIG_EFI_SPI_PROTOCOL)) {
>> +        ret = efi_spi_protocol_register();
>> +        if (ret != EFI_SUCCESS)
>> +            goto out;
>> +    }
>> +
>>       /* Secure boot */
>>       ret = efi_init_secure_boot();
>>       if (ret != EFI_SUCCESS)
>> diff --git a/lib/efi_loader/efi_spi_protocol.c 
>> b/lib/efi_loader/efi_spi_protocol.c
>> new file mode 100644
>> index 000000000000..c491741cbbf1
>> --- /dev/null
>> +++ b/lib/efi_loader/efi_spi_protocol.c
>> @@ -0,0 +1,614 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2022 Micron Technology, Inc.
>> + */
>> +
>> +#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;
>> +}
>> +
>> +static void dump_buffer(const char *msg, u32 length, u8 *buffer)
>> +{
>> +    u32 i;
>> +    EFI_PRINT("%s %d bytes:", msg, length);
>> +    for (i = 0; i < length; i++)
>> +        EFI_PRINT(" %02x", buffer[i]);
>> +    EFI_PRINT("\n");
>> +}
>> +
>> +static efi_status_t EFIAPI
>> +efi_spi_bus_clock(const struct efi_spi_peripheral *spi_peripheral,
>> +          u32 *clock_hz)
>> +{
>> +    EFI_ENTRY("%p, %p", spi_peripheral, clock_hz);
>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>> +}
>> +
>> +static efi_status_t EFIAPI
>> +efi_spi_peripheral_chip_select(const struct efi_spi_peripheral 
>> *spi_peripheral,
>> +                   bool pin_value)
>> +{
>> +    EFI_ENTRY("%p, %d", spi_peripheral, pin_value);
>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>> +}
>> +
>> +static efi_status_t EFIAPI
>> +legacy_erase_block_opcode(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +              u8 erase_block_opcode)
>> +{
>> +    EFI_ENTRY("%p, %u", this, erase_block_opcode);
>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>> +}
>> +
>> +static efi_status_t EFIAPI
>> +legacy_write_status_prefix(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +               u8 write_status_prefix)
>> +{
>> +    EFI_ENTRY("%p, %u", this, write_status_prefix);
>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>> +}
>> +
>> +static efi_status_t EFIAPI
>> +legacy_bios_base_address(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +             u32 bios_base_address)
>> +{
>> +    EFI_ENTRY("%p, %u", this, bios_base_address);
>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>> +}
>> +
>> +static efi_status_t EFIAPI
>> +legacy_clear_spi_protect(const struct 
>> efi_legacy_spi_controller_protocol *this)
>> +{
>> +    EFI_ENTRY("%p", this);
>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>> +}
>> +
>> +static bool EFIAPI
>> +legacy_is_range_protected(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +              u32 bios_address,
>> +              u32 blocks_to_protect)
>> +{
>> +    EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
>> +    return EFI_EXIT(false);
>> +}
>> +
>> +static efi_status_t EFIAPI
>> +legacy_protect_next_range(const struct 
>> efi_legacy_spi_controller_protocol *this,
>> +              u32 bios_address,
>> +              u32 blocks_to_protect)
>> +{
>> +    EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>> +}
>> +
>> +static efi_status_t EFIAPI
>> +legacy_lock_controller(const struct 
>> efi_legacy_spi_controller_protocol *this)
>> +{
>> +    EFI_ENTRY("%p", this);
>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>> +}
>> +
>> +static efi_status_t EFIAPI
>> +efi_spi_io_update_spi_peripheral(struct efi_spi_io_protocol *this,
>> +                 struct efi_spi_peripheral *spi_peripheral)
>> +{
>> +    EFI_ENTRY("%p, %p", this, spi_peripheral);
>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>> +}
>> +
>> +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);
>> +    } 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
>> +};
> 
> In the commit message you said that the legacy protocol will not be
> implemented.
> 
> Why would a *legacy* be implemented?

As explained above.

> 
>> +
>> +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);
>> +    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;
>> +}
>> +
>> +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;
>> +
>> +    printf("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",
> 
> This information does not interest any user. Please, use log_debug().

I'll change this in v3.

> 
>> +           name,
>> +           guid->b[0], guid->b[1], guid->b[2], guid->b[3],
>> +           guid->b[4], guid->b[5], guid->b[6], guid->b[7],
>> +           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
>> index 33536c9ec021..c9b6e4fbb452 100644
>> --- a/lib/efi_selftest/Makefile
>> +++ b/lib/efi_selftest/Makefile
>> @@ -62,6 +62,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..2a3723d3931f
>> --- /dev/null
>> +++ b/lib/efi_selftest/efi_selftest_spi_protocol.c
>> @@ -0,0 +1,237 @@
>> +// 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;
>> +}
>> +
>> +static int test_peripheral(struct efi_spi_peripheral *p, struct 
>> efi_spi_bus *bus)
>> +{
>> +    struct efi_spi_io_protocol *io_protocol;
>> +    u8 req[5], resp[5];
>> +    efi_status_t ret;
>> +
>> +    if (!p->friendly_name) {
>> +        efi_st_error("SPI peripheral lacks a friendly name\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!p->spi_peripheral_driver_guid) {
>> +        efi_st_error("SPI peripheral lacks driver GUID\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!p->spi_part) {
>> +        efi_st_error("SPI peripheral lacks SPI part definition\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!p->max_clock_hz) {
>> +        efi_st_error("SPI peripheral has a max clock rate of zero\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!p->spi_bus) {
>> +        efi_st_error("SPI peripheral lack pointer to SPI bus\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (p->spi_bus != bus) {
>> +        efi_st_error("SPI peripheral spi_bus pointer points to the 
>> wrong bus\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!p->chip_select) {
>> +        efi_st_error("SPI peripheral lacks chip_select function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!p->spi_part->vendor) {
>> +        efi_st_error("SPI part lacks vendor string\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!p->spi_part->part_number) {
>> +        efi_st_error("SPI part lacks part number string\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (p->spi_part->min_clock_hz > p->spi_part->max_clock_hz) {
>> +        efi_st_error("SPI part min clock rate is greater than max 
>> clock rate\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (p->spi_part->max_clock_hz != p->max_clock_hz) {
>> +        efi_st_error("SPI part max clock rate does not match 
>> peripheral max clock rate\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    ret = boottime->locate_protocol(p->spi_peripheral_driver_guid,
>> +                    NULL, (void **)&io_protocol);
>> +    if (ret != EFI_SUCCESS) {
>> +        efi_st_error("SPI IO protocol not available\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (io_protocol->spi_peripheral != p) {
>> +        efi_st_error("SPI IO protocol spi_peripheral pointer points 
>> to the wrong peripheral\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (io_protocol->original_spi_peripheral != p) {
>> +        efi_st_error("SPI IO protocol original_spi_peripheral pointer 
>> points to the wrong peripheral\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->maximum_transfer_bytes) {
>> +        efi_st_error("SPI IO protocol has zero maximum transfer 
>> size\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->legacy_spi_protocol) {
>> +        efi_st_error("SPI IO protocol lacks legacy SPI protocol\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->transaction) {
>> +        efi_st_error("SPI IO protocol lacks transaction function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->update_spi_peripheral) {
>> +        efi_st_error("SPI IO protocol lacks update_spi_peripheral 
>> function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->legacy_spi_protocol->erase_block_opcode) {
>> +        efi_st_error("SPI legacy controller protocol lacks 
>> erase_block_opcode function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->legacy_spi_protocol->write_status_prefix) {
>> +        efi_st_error("SPI legacy controller protocol lacks 
>> write_status_prefix function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
> 
> I would have expected that you actually call each of the functions.

I can extend this in v3 to call each function and confirm they return 
EFI_UNSUPPORTED.

> 
>> +
>> +    if (!io_protocol->legacy_spi_protocol->bios_base_address) {
>> +        efi_st_error("SPI legacy controller protocol lacks 
>> bios_base_address function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->legacy_spi_protocol->clear_spi_protect) {
>> +        efi_st_error("SPI legacy controller protocol lacks 
>> clear_spi_protect function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->legacy_spi_protocol->is_range_protected) {
>> +        efi_st_error("SPI legacy controller protocol lacks 
>> is_range_protected function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->legacy_spi_protocol->protect_next_range) {
>> +        efi_st_error("SPI legacy controller protocol lacks 
>> protect_next_range function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!io_protocol->legacy_spi_protocol->lock_controller) {
>> +        efi_st_error("SPI legacy controller protocol lacks 
>> lock_controller function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    req[0] = 0x9f;
>> +    ret = io_protocol->transaction(io_protocol,
>> +                       SPI_TRANSACTION_FULL_DUPLEX,
>> +                       false, 0, 1, 8,
>> +                       sizeof(req), req,
>> +                       sizeof(resp), resp);
>> +    if (ret != EFI_SUCCESS) {
>> +        efi_st_error("SPI transaction failed\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if ((resp[0] != 0xff) || (resp[1] != 0x20) || (resp[2] != 0x20) 
>> || (resp[3] != 0x15)) {
>> +        efi_st_error("Incorrect response from sandbox SPI flash 
>> emulator\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    return EFI_ST_SUCCESS;
>> +}
>> +
>> +static int test_bus(struct efi_spi_bus *bus)
>> +{
>> +    struct efi_spi_peripheral *p;
>> +    int status;
>> +
>> +    if (!bus->friendly_name) {
>> +        efi_st_error("SPI bus lacks a friendly name\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!bus->peripheral_list) {
>> +        efi_st_error("SPI bus has zero peripherals\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!bus->clock) {
>> +        efi_st_error("SPI bus lacks clock function\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    for (p = bus->peripheral_list; p; p = p->next_spi_peripheral) {
>> +        status = test_peripheral(p, bus);
>> +        if (status) {
>> +            efi_st_error("Failed testing SPI peripheral\n");
>> +            return status;
>> +        }
>> +    }
>> +
>> +    return EFI_ST_SUCCESS;
>> +}
>> +
>> +static int execute(void)
>> +{
>> +    struct efi_spi_configuration_protocol *spi;
>> +    efi_status_t ret;
>> +    int status;
>> +    u32 i;
>> +
>> +    ret = boottime->locate_protocol(&efi_spi_configuration_guid,
>> +                    NULL, (void **)&spi);
>> +    if (ret != EFI_SUCCESS) {
>> +        efi_st_error("SPI configuration protocol not available\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    if (!spi->bus_count) {
>> +        efi_st_error("SPI configuration protocol has zero busses\n");
>> +        return EFI_ST_FAILURE;
>> +    }
>> +
>> +    for (i = 0; i < spi->bus_count; i++) {
>> +        status = test_bus(spi->bus_list[i]);
>> +        if (status) {
>> +            efi_st_error("Failed testing SPI bus %d\n", i);
>> +            return status;
>> +        }
>> +    }
> 
> This test does not test anything changing any state of SPI flash. Why is
> this patch needed if that is all that is usable?

The test here is confirming that io_protocol->transaction() works as 
expected, without re-testing the u-boot spi driver model or sandbox spi 
drivers as they should be already tested elsewhere.

I could extend this to also test the SPI_TRANSACTION_WRITE_THEN_READ 
transaction type. Testing the other transaction types 
(SPI_TRANSACTION_WRITE_ONLY & SPI_TRANSACTION_READ_ONLY) would require 
extending the sandbox spi drivers since there's not really a valid way 
to use these with an emulated jedec flash device.

> 
> Best regards
> 
> Heinrich
> 
>> +
>> +    return EFI_ST_SUCCESS;
>> +}
>> +
>> +EFI_UNIT_TEST(spi_protocol) = {
>> +    .name = "SPI protocol",
>> +    .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
>> +    .setup = setup,
>> +    .execute = execute,
>> +};
> 

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

e: paul.barker@sancloud.com
w: https://sancloud.com/

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7645 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 54131 bytes --]

Hi Heinrich,

I'm sending a ping on this as there were a few questions in my response 
to your feedback and I'd like to move this forward. I've highlighted the 
key items with further comments below.

On 25/08/2022 11:58, Paul Barker wrote:
> Hi Henrich,
> 
> Many thanks for your review on this patch. I've responded to things 
> inline below. I'm also available on IRC (paulbarker) if you want to 
> discuss further.
> 
> On 13/08/2022 20:20, Heinrich Schuchardt wrote:
>> On 8/4/22 12:34, 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.
>>>
>>> Since there are no open source implementations of this protocol 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.
>>
>> The commit message should describe which protocols are actually 
>> implemented.
> 
> I'll add this to the commit message in v3.
> 
>>
>> I still have no clue why any PI protocol should be implemented U-Boot.
>> You neither descibe it here nor in the cover-letter.
> 
> Implementing the SPI I/O protocol allows a UEFI application running 
> under u-boot to enumerate and communicate with devices on the SPI bus. 
> This protocol fulfils the same purpose as the "SD MMC Pass Thru 
> Protocol" and "NVM Express Pass Through Protocol" described in the main 
> UEFI spec, I'm unsure why SPI bus access was deferred to the PI spec. If 
> these patches are accepted we plan to also implement the SD/MMC and NVMe 
> I/O protocols.
> 
> The broader goal here is to support better encapsulation of "pre-boot" 
> application code which needs to communicate with a SPI, MMC or NVMe 
> devices before Linux is booted. This pre-boot application can then run 
> on both embedded devices running u-boot and PC-like devices running a 
> UEFI BIOS. In our case the application will interact with the Authenta 
> flash security features to verify integrity and to generate ephemeral 
> keys based on the flash contents. Our application links against mbedtls 
> and so it would be non-trivial (in terms of both implementation and 
> licensing) to integrate this directly into u-boot.
> 
> We have previously explored using the "standalone" application support 
> (e.g. in examples/standalone in the u-boot source) but as that is not 
> well maintained we have moved to a UEFI implementation.
> 
> Let me know if you need any more info than that. Once you're happy with 
> the explanation I'll add it to the cover letter in v3.

Could you let me know if this is sufficient?

> 
>>
>> I would expect capsule updates to be used to update the SPI flash.
> 
> Our application is not intended to perform firmware or capsule updates 
> or to make any writes to the flash.
> 
>>
>>>
>>> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
>>> ---
>>>   MAINTAINERS                                  |   7 +
>>>   arch/sandbox/dts/test.dts                    |   8 +
>>>   include/efi_loader.h                         |   4 +
>>>   include/efi_spi_protocol.h                   | 158 +++++
>>>   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 | 237 +++++++
>>>   10 files changed, 1044 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 4d1930f76e44..1b5d9c37576b 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -856,6 +856,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 d1a8cc7bfb7e..5ac59140b748 100644
>>> --- a/arch/sandbox/dts/test.dts
>>> +++ b/arch/sandbox/dts/test.dts
>>> @@ -1185,6 +1185,10 @@
>>>               compatible = "spansion,m25p16", "jedec,spi-nor";
>>>               spi-max-frequency = <40000000>;
>>>               sandbox,filename = "spi.bin";
>>> +
>>> +            uefi-spi-vendor = "spansion";
>>> +            uefi-spi-part-number = "mt25p16";
>>> +            uefi-spi-io-guid = [5deb81b8 92ad 484a 8fdd fa75a8e4c6b8];
>>>           };
>>>           spi.bin@1 {
>>>               reg = <1>;
>>> @@ -1193,6 +1197,10 @@
>>>               sandbox,filename = "spi.bin";
>>>               spi-cpol;
>>>               spi-cpha;
>>> +
>>> +            uefi-spi-vendor = "spansion";
>>
>> Does the sandbox provide anything that is related to vendor spansion?
>> Or could the vendor be "sandbox"?
> 
> This is based on the "spansion,m25p16" compatible string used above. 
> I've used the same vendor and part number for consistency.

Are you happy with this?

> 
>>
>>> +            uefi-spi-part-number = "mt25p16";
>>
>> Does the sandbox really emulate a
>> "Micron M25P16 Serial Flash Embedded Memory"?
> 
> As above.
> 
>>
>>> +            uefi-spi-io-guid = [cd9eb3b6 1f2b 43a6 b8d7 3192d7cf7270];
>>
>> This is a byte-string. The byte order in the array differs from the low
>> endian string representation used in the UEFI specification. To avoid
>> confusion, please, add a space after each byte and add a comment with
>> the string representation.
> 
> I'll make this change for v3.
> 
>>
>>>           };
>>>       };
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 3a63a1f75fdf..2eb96b08205b 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -537,6 +537,10 @@ efi_status_t efi_rng_register(void);
>>>   efi_status_t efi_tcg2_register(void);
>>>   /* Called by efi_init_obj_list() to install RISCV_EFI_BOOT_PROTOCOL */
>>>   efi_status_t efi_riscv_register(void);
>>> +/* Called by efi_init_obj_list() to install 
>>> EFI_SPI_CONFIGURATION_PROTOCOL &
>>> + * EFI_SPI_IO_PROTOCOL
>>> + */
>>> +efi_status_t efi_spi_protocol_register(void);
>>>   /* Called by efi_init_obj_list() to do initial measurement */
>>>   efi_status_t efi_tcg2_do_initial_measurement(void);
>>>   /* measure the pe-coff image, extend PCR and add Event Log */
>>> diff --git a/include/efi_spi_protocol.h b/include/efi_spi_protocol.h
>>> new file mode 100644
>>> index 000000000000..1a4456bd9349
>>> --- /dev/null
>>> +++ b/include/efi_spi_protocol.h
>>> @@ -0,0 +1,158 @@
>>> +/* SPDX-License-Identifier: BSD-2-Clause-Patent */
>>> +/*
>>> + * Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>
>> Is this really a copy from EDK II and not based on the specification?
> 
> This is copied from EDK II and modified to fit the u-boot coding style.
> 
>>
>> Please, add a reference to the
>> "UEFI Platform Initialization (PI) Specification".
>> and a description what this include is about.
> 
> I'll add this in v3.
> 
>>
>>
>>> + */
>>> +
>>> +#ifndef _EFI_SPI_PROTOCOL_H
>>> +#define _EFI_SPI_PROTOCOL_H
>>> +
>>> +#include <efi.h>
>>> +#include <efi_api.h>
>>> +
>>> +#define EFI_SPI_CONFIGURATION_GUID  \
>>> +    EFI_GUID(0x85a6d3e6, 0xb65b, 0x4afc,     \
>>> +         0xb3, 0x8f, 0xc6, 0xd5, 0x4a, 0xf6, 0xdd, 0xc8)
>>
>> Please, add a comment that this GUID is defined in the "UEFI Platform
>> Initialization (PI) Specification".
>>
>> Please, add the GUID to list_guid[]in lib/uuid.c .
> 
> Will do in v3.
> 
>>
>>> +
>>> +struct efi_spi_peripheral;
>>> +
>>> +struct efi_spi_part {
>>> +    efi_string_t vendor;
>>> +    efi_string_t part_number;
>>
>> Why did you remove "const" used in the specifications?
> 
> These structures are const in the spec as the UEFI application code is 
> not expected to modify them. However, we need to be able to modify these 
> fields in u-boot before starting a UEFI application in order to populate 
> them with the correct data.

Are you happy with this explanation?

> 
>>
>>> +    u32 min_clock_hz;
>>> +    u32 max_clock_hz;
>>> +    bool chip_select_polarity;
>>> +};
>>> +
>>> +struct efi_spi_bus {
>>> +    efi_string_t friendly_name;
>>> +    struct efi_spi_peripheral *peripheral_list;
>>> +    struct efi_device_path *controller_path;
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * clock)(const struct efi_spi_peripheral *spi_peripheral,
>>> +        u32 *clock_hz);
>>> +
>>> +    void *clock_parameter;
>>> +};
>>> +
>>> +struct efi_spi_peripheral {
>>> +    struct efi_spi_peripheral *next_spi_peripheral;
>>> +    efi_string_t friendly_name;
>>> +    efi_guid_t *spi_peripheral_driver_guid;
>>> +    struct efi_spi_part *spi_part;
>>> +    u32 max_clock_hz;
>>> +    bool clock_polarity;
>>> +    bool clock_phase;
>>> +    u32 attributes;
>>> +    void *configuration_data;
>>> +    struct efi_spi_bus *spi_bus;
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * chip_select)(const struct efi_spi_peripheral 
>>> *spi_peripheral,
>>> +        bool pin_value);
>>> +
>>> +    void *chip_select_parameter;
>>> +};
>>> +
>>> +struct efi_spi_configuration_protocol {
>>> +    u32 bus_count;
>>> +    struct efi_spi_bus **bus_list;
>>> +};
>>> +
>>> +#define EFI_LEGACY_SPI_CONTROLLER_GUID  \
>>> +    EFI_GUID(0x39136fc7, 0x1a11, 0x49de,         \
>>> +         0xbf, 0x35, 0x0e, 0x78, 0xdd, 0xb5, 0x24, 0xfc)
>>
>> The commit message clear states that the legacy protocol will not be
>> implemented. Why should we implement any *legacy*?
> I'd rather ignore the whole thing and use a NULL pointer but it doesn't 
> look like that's an option.
> 
> `struct efi_spi_io_protocol` contains a pointer to a `struct 
> efi_legacy_spi_controller_protocol` and the spec does not seem to allow 
> this or the function pointers in the legacy protocol object to be NULL. 
> I don't think we can assume that a UEFI application will check for NULL 
> pointers before de-referencing given the wording in the spec, so we 
> instead need to provide some valid functions which at least return an 
> EFI_UNSUPPORTED error condition.

Are you happy with this explanation?

> 
>>
>>> +
>>> +struct efi_legacy_spi_controller_protocol;
>>> +
>>> +struct efi_legacy_spi_controller_protocol {
>>> +    u32 maximum_offset;
>>> +    u32 maximum_range_bytes;
>>> +    u32 range_register_count;
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * erase_block_opcode)(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +        u8 erase_block_opcode);
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * write_status_prefix)(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +        u8 write_status_prefix);
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * bios_base_address)(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +        u32 bios_base_address);
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * clear_spi_protect)(const struct 
>>> efi_legacy_spi_controller_protocol *this);
>>> +
>>> +    bool
>>> +    (EFIAPI * is_range_protected)(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +        u32 bios_address,
>>> +        u32 blocks_to_protect);
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * protect_next_range)(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +        u32 bios_address,
>>> +        u32 blocks_to_protect);
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * lock_controller)(const struct 
>>> efi_legacy_spi_controller_protocol *this);
>>> +};
>>> +
>>> +struct efi_spi_io_protocol;
>>> +
>>> +enum efi_spi_transaction_type {
>>> +    SPI_TRANSACTION_FULL_DUPLEX,
>>> +    SPI_TRANSACTION_WRITE_ONLY,
>>> +    SPI_TRANSACTION_READ_ONLY,
>>> +    SPI_TRANSACTION_WRITE_THEN_READ
>>> +};
>>> +
>>> +struct efi_spi_bus_transaction {
>>> +    struct efi_spi_peripheral *spi_peripheral;
>>> +    enum efi_spi_transaction_type transaction_type;
>>> +    bool debug_transaction;
>>> +    u32 bus_width;
>>> +    u32 frame_size;
>>> +    u32 write_bytes;
>>> +    u8 *write_buffer;
>>> +    u32 read_bytes;
>>> +    u8 *read_buffer;
>>> +};
>>> +
>>> +struct efi_spi_io_protocol {
>>> +    struct efi_spi_peripheral *spi_peripheral;
>>> +    struct efi_spi_peripheral *original_spi_peripheral;
>>> +    u32 frame_size_support_mask;
>>> +    u32 maximum_transfer_bytes;
>>> +    u32 attributes;
>>> +    struct efi_legacy_spi_controller_protocol *legacy_spi_protocol;
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * 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);
>>> +
>>> +    efi_status_t
>>> +    (EFIAPI * update_spi_peripheral)(struct efi_spi_io_protocol *this,
>>> +        struct efi_spi_peripheral *spi_peripheral);
>>> +};
>>> +
>>> +struct efi_spi_peripheral_priv {
>>> +    struct efi_spi_peripheral peripheral;
>>> +    struct efi_spi_part part;
>>> +    struct efi_spi_io_protocol io_protocol;
>>> +    efi_handle_t handle;
>>> +    struct spi_slave *target;
>>> +};
>>> +
>>> +efi_status_t efi_spi_protocol_register(void);
>>> +
>>> +#endif
>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>> index e3f2402d0e8e..c5899a6f6e52 100644
>>> --- a/lib/efi_loader/Kconfig
>>> +++ b/lib/efi_loader/Kconfig
>>> @@ -394,4 +394,12 @@ config EFI_RISCV_BOOT_PROTOCOL
>>>         replace the transfer via the device-tree. The latter is not
>>>         possible on systems using ACPI.
>>>
>>> +config EFI_SPI_PROTOCOL
>>> +    bool "EFI SPI protocol support"
>>> +    depends on DM_SPI
>>> +    help
>>> +      Provide implementations of EFI_SPI_CONFIGURATION_PROTOCOL and
>>> +      EFI_SPI_IO_PROTOCOL to allow UEFI applications to access devices
>>> +      connected via SPI bus.
>>> +
>>>   endif
>>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>>> index f54c244c3268..9fa0d27927b6 100644
>>> --- a/lib/efi_loader/Makefile
>>> +++ b/lib/efi_loader/Makefile
>>> @@ -74,6 +74,7 @@ obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
>>>   obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
>>>   obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
>>>   obj-$(CONFIG_EFI_RISCV_BOOT_PROTOCOL) += efi_riscv.o
>>> +obj-$(CONFIG_EFI_SPI_PROTOCOL) += efi_spi_protocol.o
>>>   obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
>>>   obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o
>>>
>>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>>> index 492ecf4cb15c..ef1ee9862b72 100644
>>> --- a/lib/efi_loader/efi_setup.c
>>> +++ b/lib/efi_loader/efi_setup.c
>>> @@ -295,6 +295,12 @@ efi_status_t efi_init_obj_list(void)
>>>               goto out;
>>>       }
>>>
>>> +    if (IS_ENABLED(CONFIG_EFI_SPI_PROTOCOL)) {
>>> +        ret = efi_spi_protocol_register();
>>> +        if (ret != EFI_SUCCESS)
>>> +            goto out;
>>> +    }
>>> +
>>>       /* Secure boot */
>>>       ret = efi_init_secure_boot();
>>>       if (ret != EFI_SUCCESS)
>>> diff --git a/lib/efi_loader/efi_spi_protocol.c 
>>> b/lib/efi_loader/efi_spi_protocol.c
>>> new file mode 100644
>>> index 000000000000..c491741cbbf1
>>> --- /dev/null
>>> +++ b/lib/efi_loader/efi_spi_protocol.c
>>> @@ -0,0 +1,614 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (c) 2022 Micron Technology, Inc.
>>> + */
>>> +
>>> +#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;
>>> +}
>>> +
>>> +static void dump_buffer(const char *msg, u32 length, u8 *buffer)
>>> +{
>>> +    u32 i;
>>> +    EFI_PRINT("%s %d bytes:", msg, length);
>>> +    for (i = 0; i < length; i++)
>>> +        EFI_PRINT(" %02x", buffer[i]);
>>> +    EFI_PRINT("\n");
>>> +}
>>> +
>>> +static efi_status_t EFIAPI
>>> +efi_spi_bus_clock(const struct efi_spi_peripheral *spi_peripheral,
>>> +          u32 *clock_hz)
>>> +{
>>> +    EFI_ENTRY("%p, %p", spi_peripheral, clock_hz);
>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI
>>> +efi_spi_peripheral_chip_select(const struct efi_spi_peripheral 
>>> *spi_peripheral,
>>> +                   bool pin_value)
>>> +{
>>> +    EFI_ENTRY("%p, %d", spi_peripheral, pin_value);
>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI
>>> +legacy_erase_block_opcode(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +              u8 erase_block_opcode)
>>> +{
>>> +    EFI_ENTRY("%p, %u", this, erase_block_opcode);
>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI
>>> +legacy_write_status_prefix(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +               u8 write_status_prefix)
>>> +{
>>> +    EFI_ENTRY("%p, %u", this, write_status_prefix);
>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI
>>> +legacy_bios_base_address(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +             u32 bios_base_address)
>>> +{
>>> +    EFI_ENTRY("%p, %u", this, bios_base_address);
>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI
>>> +legacy_clear_spi_protect(const struct 
>>> efi_legacy_spi_controller_protocol *this)
>>> +{
>>> +    EFI_ENTRY("%p", this);
>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>> +}
>>> +
>>> +static bool EFIAPI
>>> +legacy_is_range_protected(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +              u32 bios_address,
>>> +              u32 blocks_to_protect)
>>> +{
>>> +    EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
>>> +    return EFI_EXIT(false);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI
>>> +legacy_protect_next_range(const struct 
>>> efi_legacy_spi_controller_protocol *this,
>>> +              u32 bios_address,
>>> +              u32 blocks_to_protect)
>>> +{
>>> +    EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI
>>> +legacy_lock_controller(const struct 
>>> efi_legacy_spi_controller_protocol *this)
>>> +{
>>> +    EFI_ENTRY("%p", this);
>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI
>>> +efi_spi_io_update_spi_peripheral(struct efi_spi_io_protocol *this,
>>> +                 struct efi_spi_peripheral *spi_peripheral)
>>> +{
>>> +    EFI_ENTRY("%p, %p", this, spi_peripheral);
>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>> +}
>>> +
>>> +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);
>>> +    } 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
>>> +};
>>
>> In the commit message you said that the legacy protocol will not be
>> implemented.
>>
>> Why would a *legacy* be implemented?
> 
> As explained above.
> 
>>
>>> +
>>> +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);
>>> +    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;
>>> +}
>>> +
>>> +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;
>>> +
>>> +    printf("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", 
>>>
>>
>> This information does not interest any user. Please, use log_debug().
> 
> I'll change this in v3.
> 
>>
>>> +           name,
>>> +           guid->b[0], guid->b[1], guid->b[2], guid->b[3],
>>> +           guid->b[4], guid->b[5], guid->b[6], guid->b[7],
>>> +           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
>>> index 33536c9ec021..c9b6e4fbb452 100644
>>> --- a/lib/efi_selftest/Makefile
>>> +++ b/lib/efi_selftest/Makefile
>>> @@ -62,6 +62,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..2a3723d3931f
>>> --- /dev/null
>>> +++ b/lib/efi_selftest/efi_selftest_spi_protocol.c
>>> @@ -0,0 +1,237 @@
>>> +// 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;
>>> +}
>>> +
>>> +static int test_peripheral(struct efi_spi_peripheral *p, struct 
>>> efi_spi_bus *bus)
>>> +{
>>> +    struct efi_spi_io_protocol *io_protocol;
>>> +    u8 req[5], resp[5];
>>> +    efi_status_t ret;
>>> +
>>> +    if (!p->friendly_name) {
>>> +        efi_st_error("SPI peripheral lacks a friendly name\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!p->spi_peripheral_driver_guid) {
>>> +        efi_st_error("SPI peripheral lacks driver GUID\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!p->spi_part) {
>>> +        efi_st_error("SPI peripheral lacks SPI part definition\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!p->max_clock_hz) {
>>> +        efi_st_error("SPI peripheral has a max clock rate of zero\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!p->spi_bus) {
>>> +        efi_st_error("SPI peripheral lack pointer to SPI bus\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (p->spi_bus != bus) {
>>> +        efi_st_error("SPI peripheral spi_bus pointer points to the 
>>> wrong bus\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!p->chip_select) {
>>> +        efi_st_error("SPI peripheral lacks chip_select function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!p->spi_part->vendor) {
>>> +        efi_st_error("SPI part lacks vendor string\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!p->spi_part->part_number) {
>>> +        efi_st_error("SPI part lacks part number string\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (p->spi_part->min_clock_hz > p->spi_part->max_clock_hz) {
>>> +        efi_st_error("SPI part min clock rate is greater than max 
>>> clock rate\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (p->spi_part->max_clock_hz != p->max_clock_hz) {
>>> +        efi_st_error("SPI part max clock rate does not match 
>>> peripheral max clock rate\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    ret = boottime->locate_protocol(p->spi_peripheral_driver_guid,
>>> +                    NULL, (void **)&io_protocol);
>>> +    if (ret != EFI_SUCCESS) {
>>> +        efi_st_error("SPI IO protocol not available\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (io_protocol->spi_peripheral != p) {
>>> +        efi_st_error("SPI IO protocol spi_peripheral pointer points 
>>> to the wrong peripheral\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (io_protocol->original_spi_peripheral != p) {
>>> +        efi_st_error("SPI IO protocol original_spi_peripheral 
>>> pointer points to the wrong peripheral\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->maximum_transfer_bytes) {
>>> +        efi_st_error("SPI IO protocol has zero maximum transfer 
>>> size\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->legacy_spi_protocol) {
>>> +        efi_st_error("SPI IO protocol lacks legacy SPI protocol\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->transaction) {
>>> +        efi_st_error("SPI IO protocol lacks transaction function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->update_spi_peripheral) {
>>> +        efi_st_error("SPI IO protocol lacks update_spi_peripheral 
>>> function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->legacy_spi_protocol->erase_block_opcode) {
>>> +        efi_st_error("SPI legacy controller protocol lacks 
>>> erase_block_opcode function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->legacy_spi_protocol->write_status_prefix) {
>>> +        efi_st_error("SPI legacy controller protocol lacks 
>>> write_status_prefix function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>
>> I would have expected that you actually call each of the functions.
> 
> I can extend this in v3 to call each function and confirm they return 
> EFI_UNSUPPORTED.
> 
>>
>>> +
>>> +    if (!io_protocol->legacy_spi_protocol->bios_base_address) {
>>> +        efi_st_error("SPI legacy controller protocol lacks 
>>> bios_base_address function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->legacy_spi_protocol->clear_spi_protect) {
>>> +        efi_st_error("SPI legacy controller protocol lacks 
>>> clear_spi_protect function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->legacy_spi_protocol->is_range_protected) {
>>> +        efi_st_error("SPI legacy controller protocol lacks 
>>> is_range_protected function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->legacy_spi_protocol->protect_next_range) {
>>> +        efi_st_error("SPI legacy controller protocol lacks 
>>> protect_next_range function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!io_protocol->legacy_spi_protocol->lock_controller) {
>>> +        efi_st_error("SPI legacy controller protocol lacks 
>>> lock_controller function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    req[0] = 0x9f;
>>> +    ret = io_protocol->transaction(io_protocol,
>>> +                       SPI_TRANSACTION_FULL_DUPLEX,
>>> +                       false, 0, 1, 8,
>>> +                       sizeof(req), req,
>>> +                       sizeof(resp), resp);
>>> +    if (ret != EFI_SUCCESS) {
>>> +        efi_st_error("SPI transaction failed\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if ((resp[0] != 0xff) || (resp[1] != 0x20) || (resp[2] != 0x20) 
>>> || (resp[3] != 0x15)) {
>>> +        efi_st_error("Incorrect response from sandbox SPI flash 
>>> emulator\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    return EFI_ST_SUCCESS;
>>> +}
>>> +
>>> +static int test_bus(struct efi_spi_bus *bus)
>>> +{
>>> +    struct efi_spi_peripheral *p;
>>> +    int status;
>>> +
>>> +    if (!bus->friendly_name) {
>>> +        efi_st_error("SPI bus lacks a friendly name\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!bus->peripheral_list) {
>>> +        efi_st_error("SPI bus has zero peripherals\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!bus->clock) {
>>> +        efi_st_error("SPI bus lacks clock function\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    for (p = bus->peripheral_list; p; p = p->next_spi_peripheral) {
>>> +        status = test_peripheral(p, bus);
>>> +        if (status) {
>>> +            efi_st_error("Failed testing SPI peripheral\n");
>>> +            return status;
>>> +        }
>>> +    }
>>> +
>>> +    return EFI_ST_SUCCESS;
>>> +}
>>> +
>>> +static int execute(void)
>>> +{
>>> +    struct efi_spi_configuration_protocol *spi;
>>> +    efi_status_t ret;
>>> +    int status;
>>> +    u32 i;
>>> +
>>> +    ret = boottime->locate_protocol(&efi_spi_configuration_guid,
>>> +                    NULL, (void **)&spi);
>>> +    if (ret != EFI_SUCCESS) {
>>> +        efi_st_error("SPI configuration protocol not available\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    if (!spi->bus_count) {
>>> +        efi_st_error("SPI configuration protocol has zero busses\n");
>>> +        return EFI_ST_FAILURE;
>>> +    }
>>> +
>>> +    for (i = 0; i < spi->bus_count; i++) {
>>> +        status = test_bus(spi->bus_list[i]);
>>> +        if (status) {
>>> +            efi_st_error("Failed testing SPI bus %d\n", i);
>>> +            return status;
>>> +        }
>>> +    }
>>
>> This test does not test anything changing any state of SPI flash. Why is
>> this patch needed if that is all that is usable?
> 
> The test here is confirming that io_protocol->transaction() works as 
> expected, without re-testing the u-boot spi driver model or sandbox spi 
> drivers as they should be already tested elsewhere.
> 
> I could extend this to also test the SPI_TRANSACTION_WRITE_THEN_READ 
> transaction type. Testing the other transaction types 
> (SPI_TRANSACTION_WRITE_ONLY & SPI_TRANSACTION_READ_ONLY) would require 
> extending the sandbox spi drivers since there's not really a valid way 
> to use these with an emulated jedec flash device.

Are you happy with this explanation?

> 
>>
>> Best regards
>>
>> Heinrich
>>
>>> +
>>> +    return EFI_ST_SUCCESS;
>>> +}
>>> +
>>> +EFI_UNIT_TEST(spi_protocol) = {
>>> +    .name = "SPI protocol",
>>> +    .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
>>> +    .setup = setup,
>>> +    .execute = execute,
>>> +};
>>
> 

Thanks,

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

e: paul.barker@sancloud.com
w: https://sancloud.com/

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7645 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Heinrich Schuchardt]

On 9/12/22 18:14, Paul Barker wrote:
> Hi Heinrich,
>
> I'm sending a ping on this as there were a few questions in my response
> to your feedback and I'd like to move this forward. I've highlighted the
> key items with further comments below.
>
> On 25/08/2022 11:58, Paul Barker wrote:
>> Hi Henrich,
>>
>> Many thanks for your review on this patch. I've responded to things
>> inline below. I'm also available on IRC (paulbarker) if you want to
>> discuss further.
>>
>> On 13/08/2022 20:20, Heinrich Schuchardt wrote:
>>> On 8/4/22 12:34, 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.
>>>>
>>>> Since there are no open source implementations of this protocol 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.
>>>
>>> The commit message should describe which protocols are actually
>>> implemented.
>>
>> I'll add this to the commit message in v3.
>>
>>>
>>> I still have no clue why any PI protocol should be implemented U-Boot.
>>> You neither descibe it here nor in the cover-letter.
>>
>> Implementing the SPI I/O protocol allows a UEFI application running
>> under u-boot to enumerate and communicate with devices on the SPI bus.
>> This protocol fulfils the same purpose as the "SD MMC Pass Thru
>> Protocol" and "NVM Express Pass Through Protocol" described in the
>> main UEFI spec, I'm unsure why SPI bus access was deferred to the PI
>> spec. If these patches are accepted we plan to also implement the
>> SD/MMC and NVMe I/O protocols.
>>
>> The broader goal here is to support better encapsulation of "pre-boot"
>> application code which needs to communicate with a SPI, MMC or NVMe
>> devices before Linux is booted. This pre-boot application can then run
>> on both embedded devices running u-boot and PC-like devices running a
>> UEFI BIOS. In our case the application will interact with the Authenta
>> flash security features to verify integrity and to generate ephemeral
>> keys based on the flash contents. Our application links against
>> mbedtls and so it would be non-trivial (in terms of both
>> implementation and licensing) to integrate this directly into u-boot.
>>
>> We have previously explored using the "standalone" application support
>> (e.g. in examples/standalone in the u-boot source) but as that is not
>> well maintained we have moved to a UEFI implementation.
>>
>> Let me know if you need any more info than that. Once you're happy
>> with the explanation I'll add it to the cover letter in v3.
>
> Could you let me know if this is sufficient?

Thanks to linking this to Authenta flash, cf.

https://www.micron.com/-/media/client/global/documents/products/product-flyer/authenta_technology_solutions_brief.pdf

The other answers where also helpfule to better undestand this patch.
Please, send v3 and the we can add this functionality.

Best regards

Heinrich

>
>>
>>>
>>> I would expect capsule updates to be used to update the SPI flash.
>>
>> Our application is not intended to perform firmware or capsule updates
>> or to make any writes to the flash.
>>
>>>
>>>>
>>>> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
>>>> ---
>>>>   MAINTAINERS                                  |   7 +
>>>>   arch/sandbox/dts/test.dts                    |   8 +
>>>>   include/efi_loader.h                         |   4 +
>>>>   include/efi_spi_protocol.h                   | 158 +++++
>>>>   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 | 237 +++++++
>>>>   10 files changed, 1044 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 4d1930f76e44..1b5d9c37576b 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -856,6 +856,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 d1a8cc7bfb7e..5ac59140b748 100644
>>>> --- a/arch/sandbox/dts/test.dts
>>>> +++ b/arch/sandbox/dts/test.dts
>>>> @@ -1185,6 +1185,10 @@
>>>>               compatible = "spansion,m25p16", "jedec,spi-nor";
>>>>               spi-max-frequency = <40000000>;
>>>>               sandbox,filename = "spi.bin";
>>>> +
>>>> +            uefi-spi-vendor = "spansion";
>>>> +            uefi-spi-part-number = "mt25p16";
>>>> +            uefi-spi-io-guid = [5deb81b8 92ad 484a 8fdd fa75a8e4c6b8];
>>>>           };
>>>>           spi.bin@1 {
>>>>               reg = <1>;
>>>> @@ -1193,6 +1197,10 @@
>>>>               sandbox,filename = "spi.bin";
>>>>               spi-cpol;
>>>>               spi-cpha;
>>>> +
>>>> +            uefi-spi-vendor = "spansion";
>>>
>>> Does the sandbox provide anything that is related to vendor spansion?
>>> Or could the vendor be "sandbox"?
>>
>> This is based on the "spansion,m25p16" compatible string used above.
>> I've used the same vendor and part number for consistency.
>
> Are you happy with this?
>
>>
>>>
>>>> +            uefi-spi-part-number = "mt25p16";
>>>
>>> Does the sandbox really emulate a
>>> "Micron M25P16 Serial Flash Embedded Memory"?
>>
>> As above.
>>
>>>
>>>> +            uefi-spi-io-guid = [cd9eb3b6 1f2b 43a6 b8d7 3192d7cf7270];
>>>
>>> This is a byte-string. The byte order in the array differs from the low
>>> endian string representation used in the UEFI specification. To avoid
>>> confusion, please, add a space after each byte and add a comment with
>>> the string representation.
>>
>> I'll make this change for v3.
>>
>>>
>>>>           };
>>>>       };
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 3a63a1f75fdf..2eb96b08205b 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -537,6 +537,10 @@ efi_status_t efi_rng_register(void);
>>>>   efi_status_t efi_tcg2_register(void);
>>>>   /* Called by efi_init_obj_list() to install
>>>> RISCV_EFI_BOOT_PROTOCOL */
>>>>   efi_status_t efi_riscv_register(void);
>>>> +/* Called by efi_init_obj_list() to install
>>>> EFI_SPI_CONFIGURATION_PROTOCOL &
>>>> + * EFI_SPI_IO_PROTOCOL
>>>> + */
>>>> +efi_status_t efi_spi_protocol_register(void);
>>>>   /* Called by efi_init_obj_list() to do initial measurement */
>>>>   efi_status_t efi_tcg2_do_initial_measurement(void);
>>>>   /* measure the pe-coff image, extend PCR and add Event Log */
>>>> diff --git a/include/efi_spi_protocol.h b/include/efi_spi_protocol.h
>>>> new file mode 100644
>>>> index 000000000000..1a4456bd9349
>>>> --- /dev/null
>>>> +++ b/include/efi_spi_protocol.h
>>>> @@ -0,0 +1,158 @@
>>>> +/* SPDX-License-Identifier: BSD-2-Clause-Patent */
>>>> +/*
>>>> + * Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>>
>>> Is this really a copy from EDK II and not based on the specification?
>>
>> This is copied from EDK II and modified to fit the u-boot coding style.
>>
>>>
>>> Please, add a reference to the
>>> "UEFI Platform Initialization (PI) Specification".
>>> and a description what this include is about.
>>
>> I'll add this in v3.
>>
>>>
>>>
>>>> + */
>>>> +
>>>> +#ifndef _EFI_SPI_PROTOCOL_H
>>>> +#define _EFI_SPI_PROTOCOL_H
>>>> +
>>>> +#include <efi.h>
>>>> +#include <efi_api.h>
>>>> +
>>>> +#define EFI_SPI_CONFIGURATION_GUID  \
>>>> +    EFI_GUID(0x85a6d3e6, 0xb65b, 0x4afc,     \
>>>> +         0xb3, 0x8f, 0xc6, 0xd5, 0x4a, 0xf6, 0xdd, 0xc8)
>>>
>>> Please, add a comment that this GUID is defined in the "UEFI Platform
>>> Initialization (PI) Specification".
>>>
>>> Please, add the GUID to list_guid[]in lib/uuid.c .
>>
>> Will do in v3.
>>
>>>
>>>> +
>>>> +struct efi_spi_peripheral;
>>>> +
>>>> +struct efi_spi_part {
>>>> +    efi_string_t vendor;
>>>> +    efi_string_t part_number;
>>>
>>> Why did you remove "const" used in the specifications?
>>
>> These structures are const in the spec as the UEFI application code is
>> not expected to modify them. However, we need to be able to modify
>> these fields in u-boot before starting a UEFI application in order to
>> populate them with the correct data.
>
> Are you happy with this explanation?
>
>>
>>>
>>>> +    u32 min_clock_hz;
>>>> +    u32 max_clock_hz;
>>>> +    bool chip_select_polarity;
>>>> +};
>>>> +
>>>> +struct efi_spi_bus {
>>>> +    efi_string_t friendly_name;
>>>> +    struct efi_spi_peripheral *peripheral_list;
>>>> +    struct efi_device_path *controller_path;
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * clock)(const struct efi_spi_peripheral *spi_peripheral,
>>>> +        u32 *clock_hz);
>>>> +
>>>> +    void *clock_parameter;
>>>> +};
>>>> +
>>>> +struct efi_spi_peripheral {
>>>> +    struct efi_spi_peripheral *next_spi_peripheral;
>>>> +    efi_string_t friendly_name;
>>>> +    efi_guid_t *spi_peripheral_driver_guid;
>>>> +    struct efi_spi_part *spi_part;
>>>> +    u32 max_clock_hz;
>>>> +    bool clock_polarity;
>>>> +    bool clock_phase;
>>>> +    u32 attributes;
>>>> +    void *configuration_data;
>>>> +    struct efi_spi_bus *spi_bus;
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * chip_select)(const struct efi_spi_peripheral
>>>> *spi_peripheral,
>>>> +        bool pin_value);
>>>> +
>>>> +    void *chip_select_parameter;
>>>> +};
>>>> +
>>>> +struct efi_spi_configuration_protocol {
>>>> +    u32 bus_count;
>>>> +    struct efi_spi_bus **bus_list;
>>>> +};
>>>> +
>>>> +#define EFI_LEGACY_SPI_CONTROLLER_GUID  \
>>>> +    EFI_GUID(0x39136fc7, 0x1a11, 0x49de,         \
>>>> +         0xbf, 0x35, 0x0e, 0x78, 0xdd, 0xb5, 0x24, 0xfc)
>>>
>>> The commit message clear states that the legacy protocol will not be
>>> implemented. Why should we implement any *legacy*?
>> I'd rather ignore the whole thing and use a NULL pointer but it
>> doesn't look like that's an option.
>>
>> `struct efi_spi_io_protocol` contains a pointer to a `struct
>> efi_legacy_spi_controller_protocol` and the spec does not seem to
>> allow this or the function pointers in the legacy protocol object to
>> be NULL. I don't think we can assume that a UEFI application will
>> check for NULL pointers before de-referencing given the wording in the
>> spec, so we instead need to provide some valid functions which at
>> least return an EFI_UNSUPPORTED error condition.
>
> Are you happy with this explanation?
>
>>
>>>
>>>> +
>>>> +struct efi_legacy_spi_controller_protocol;
>>>> +
>>>> +struct efi_legacy_spi_controller_protocol {
>>>> +    u32 maximum_offset;
>>>> +    u32 maximum_range_bytes;
>>>> +    u32 range_register_count;
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * erase_block_opcode)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +        u8 erase_block_opcode);
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * write_status_prefix)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +        u8 write_status_prefix);
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * bios_base_address)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +        u32 bios_base_address);
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * clear_spi_protect)(const struct
>>>> efi_legacy_spi_controller_protocol *this);
>>>> +
>>>> +    bool
>>>> +    (EFIAPI * is_range_protected)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +        u32 bios_address,
>>>> +        u32 blocks_to_protect);
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * protect_next_range)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +        u32 bios_address,
>>>> +        u32 blocks_to_protect);
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * lock_controller)(const struct
>>>> efi_legacy_spi_controller_protocol *this);
>>>> +};
>>>> +
>>>> +struct efi_spi_io_protocol;
>>>> +
>>>> +enum efi_spi_transaction_type {
>>>> +    SPI_TRANSACTION_FULL_DUPLEX,
>>>> +    SPI_TRANSACTION_WRITE_ONLY,
>>>> +    SPI_TRANSACTION_READ_ONLY,
>>>> +    SPI_TRANSACTION_WRITE_THEN_READ
>>>> +};
>>>> +
>>>> +struct efi_spi_bus_transaction {
>>>> +    struct efi_spi_peripheral *spi_peripheral;
>>>> +    enum efi_spi_transaction_type transaction_type;
>>>> +    bool debug_transaction;
>>>> +    u32 bus_width;
>>>> +    u32 frame_size;
>>>> +    u32 write_bytes;
>>>> +    u8 *write_buffer;
>>>> +    u32 read_bytes;
>>>> +    u8 *read_buffer;
>>>> +};
>>>> +
>>>> +struct efi_spi_io_protocol {
>>>> +    struct efi_spi_peripheral *spi_peripheral;
>>>> +    struct efi_spi_peripheral *original_spi_peripheral;
>>>> +    u32 frame_size_support_mask;
>>>> +    u32 maximum_transfer_bytes;
>>>> +    u32 attributes;
>>>> +    struct efi_legacy_spi_controller_protocol *legacy_spi_protocol;
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * 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);
>>>> +
>>>> +    efi_status_t
>>>> +    (EFIAPI * update_spi_peripheral)(struct efi_spi_io_protocol *this,
>>>> +        struct efi_spi_peripheral *spi_peripheral);
>>>> +};
>>>> +
>>>> +struct efi_spi_peripheral_priv {
>>>> +    struct efi_spi_peripheral peripheral;
>>>> +    struct efi_spi_part part;
>>>> +    struct efi_spi_io_protocol io_protocol;
>>>> +    efi_handle_t handle;
>>>> +    struct spi_slave *target;
>>>> +};
>>>> +
>>>> +efi_status_t efi_spi_protocol_register(void);
>>>> +
>>>> +#endif
>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>>> index e3f2402d0e8e..c5899a6f6e52 100644
>>>> --- a/lib/efi_loader/Kconfig
>>>> +++ b/lib/efi_loader/Kconfig
>>>> @@ -394,4 +394,12 @@ config EFI_RISCV_BOOT_PROTOCOL
>>>>         replace the transfer via the device-tree. The latter is not
>>>>         possible on systems using ACPI.
>>>>
>>>> +config EFI_SPI_PROTOCOL
>>>> +    bool "EFI SPI protocol support"
>>>> +    depends on DM_SPI
>>>> +    help
>>>> +      Provide implementations of EFI_SPI_CONFIGURATION_PROTOCOL and
>>>> +      EFI_SPI_IO_PROTOCOL to allow UEFI applications to access devices
>>>> +      connected via SPI bus.
>>>> +
>>>>   endif
>>>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>>>> index f54c244c3268..9fa0d27927b6 100644
>>>> --- a/lib/efi_loader/Makefile
>>>> +++ b/lib/efi_loader/Makefile
>>>> @@ -74,6 +74,7 @@ obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
>>>>   obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
>>>>   obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
>>>>   obj-$(CONFIG_EFI_RISCV_BOOT_PROTOCOL) += efi_riscv.o
>>>> +obj-$(CONFIG_EFI_SPI_PROTOCOL) += efi_spi_protocol.o
>>>>   obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
>>>>   obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o
>>>>
>>>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>>>> index 492ecf4cb15c..ef1ee9862b72 100644
>>>> --- a/lib/efi_loader/efi_setup.c
>>>> +++ b/lib/efi_loader/efi_setup.c
>>>> @@ -295,6 +295,12 @@ efi_status_t efi_init_obj_list(void)
>>>>               goto out;
>>>>       }
>>>>
>>>> +    if (IS_ENABLED(CONFIG_EFI_SPI_PROTOCOL)) {
>>>> +        ret = efi_spi_protocol_register();
>>>> +        if (ret != EFI_SUCCESS)
>>>> +            goto out;
>>>> +    }
>>>> +
>>>>       /* Secure boot */
>>>>       ret = efi_init_secure_boot();
>>>>       if (ret != EFI_SUCCESS)
>>>> diff --git a/lib/efi_loader/efi_spi_protocol.c
>>>> b/lib/efi_loader/efi_spi_protocol.c
>>>> new file mode 100644
>>>> index 000000000000..c491741cbbf1
>>>> --- /dev/null
>>>> +++ b/lib/efi_loader/efi_spi_protocol.c
>>>> @@ -0,0 +1,614 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Copyright (c) 2022 Micron Technology, Inc.
>>>> + */
>>>> +
>>>> +#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;
>>>> +}
>>>> +
>>>> +static void dump_buffer(const char *msg, u32 length, u8 *buffer)
>>>> +{
>>>> +    u32 i;
>>>> +    EFI_PRINT("%s %d bytes:", msg, length);
>>>> +    for (i = 0; i < length; i++)
>>>> +        EFI_PRINT(" %02x", buffer[i]);
>>>> +    EFI_PRINT("\n");
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +efi_spi_bus_clock(const struct efi_spi_peripheral *spi_peripheral,
>>>> +          u32 *clock_hz)
>>>> +{
>>>> +    EFI_ENTRY("%p, %p", spi_peripheral, clock_hz);
>>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +efi_spi_peripheral_chip_select(const struct efi_spi_peripheral
>>>> *spi_peripheral,
>>>> +                   bool pin_value)
>>>> +{
>>>> +    EFI_ENTRY("%p, %d", spi_peripheral, pin_value);
>>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_erase_block_opcode(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +              u8 erase_block_opcode)
>>>> +{
>>>> +    EFI_ENTRY("%p, %u", this, erase_block_opcode);
>>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_write_status_prefix(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +               u8 write_status_prefix)
>>>> +{
>>>> +    EFI_ENTRY("%p, %u", this, write_status_prefix);
>>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_bios_base_address(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +             u32 bios_base_address)
>>>> +{
>>>> +    EFI_ENTRY("%p, %u", this, bios_base_address);
>>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_clear_spi_protect(const struct
>>>> efi_legacy_spi_controller_protocol *this)
>>>> +{
>>>> +    EFI_ENTRY("%p", this);
>>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static bool EFIAPI
>>>> +legacy_is_range_protected(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +              u32 bios_address,
>>>> +              u32 blocks_to_protect)
>>>> +{
>>>> +    EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
>>>> +    return EFI_EXIT(false);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_protect_next_range(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> +              u32 bios_address,
>>>> +              u32 blocks_to_protect)
>>>> +{
>>>> +    EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
>>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_lock_controller(const struct
>>>> efi_legacy_spi_controller_protocol *this)
>>>> +{
>>>> +    EFI_ENTRY("%p", this);
>>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +efi_spi_io_update_spi_peripheral(struct efi_spi_io_protocol *this,
>>>> +                 struct efi_spi_peripheral *spi_peripheral)
>>>> +{
>>>> +    EFI_ENTRY("%p, %p", this, spi_peripheral);
>>>> +    return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +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);
>>>> +    } 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
>>>> +};
>>>
>>> In the commit message you said that the legacy protocol will not be
>>> implemented.
>>>
>>> Why would a *legacy* be implemented?
>>
>> As explained above.
>>
>>>
>>>> +
>>>> +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);
>>>> +    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;
>>>> +}
>>>> +
>>>> +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;
>>>> +
>>>> +    printf("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",
>>>
>>> This information does not interest any user. Please, use log_debug().
>>
>> I'll change this in v3.
>>
>>>
>>>> +           name,
>>>> +           guid->b[0], guid->b[1], guid->b[2], guid->b[3],
>>>> +           guid->b[4], guid->b[5], guid->b[6], guid->b[7],
>>>> +           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
>>>> index 33536c9ec021..c9b6e4fbb452 100644
>>>> --- a/lib/efi_selftest/Makefile
>>>> +++ b/lib/efi_selftest/Makefile
>>>> @@ -62,6 +62,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..2a3723d3931f
>>>> --- /dev/null
>>>> +++ b/lib/efi_selftest/efi_selftest_spi_protocol.c
>>>> @@ -0,0 +1,237 @@
>>>> +// 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;
>>>> +}
>>>> +
>>>> +static int test_peripheral(struct efi_spi_peripheral *p, struct
>>>> efi_spi_bus *bus)
>>>> +{
>>>> +    struct efi_spi_io_protocol *io_protocol;
>>>> +    u8 req[5], resp[5];
>>>> +    efi_status_t ret;
>>>> +
>>>> +    if (!p->friendly_name) {
>>>> +        efi_st_error("SPI peripheral lacks a friendly name\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!p->spi_peripheral_driver_guid) {
>>>> +        efi_st_error("SPI peripheral lacks driver GUID\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!p->spi_part) {
>>>> +        efi_st_error("SPI peripheral lacks SPI part definition\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!p->max_clock_hz) {
>>>> +        efi_st_error("SPI peripheral has a max clock rate of zero\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!p->spi_bus) {
>>>> +        efi_st_error("SPI peripheral lack pointer to SPI bus\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (p->spi_bus != bus) {
>>>> +        efi_st_error("SPI peripheral spi_bus pointer points to the
>>>> wrong bus\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!p->chip_select) {
>>>> +        efi_st_error("SPI peripheral lacks chip_select function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!p->spi_part->vendor) {
>>>> +        efi_st_error("SPI part lacks vendor string\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!p->spi_part->part_number) {
>>>> +        efi_st_error("SPI part lacks part number string\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (p->spi_part->min_clock_hz > p->spi_part->max_clock_hz) {
>>>> +        efi_st_error("SPI part min clock rate is greater than max
>>>> clock rate\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (p->spi_part->max_clock_hz != p->max_clock_hz) {
>>>> +        efi_st_error("SPI part max clock rate does not match
>>>> peripheral max clock rate\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    ret = boottime->locate_protocol(p->spi_peripheral_driver_guid,
>>>> +                    NULL, (void **)&io_protocol);
>>>> +    if (ret != EFI_SUCCESS) {
>>>> +        efi_st_error("SPI IO protocol not available\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (io_protocol->spi_peripheral != p) {
>>>> +        efi_st_error("SPI IO protocol spi_peripheral pointer points
>>>> to the wrong peripheral\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (io_protocol->original_spi_peripheral != p) {
>>>> +        efi_st_error("SPI IO protocol original_spi_peripheral
>>>> pointer points to the wrong peripheral\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->maximum_transfer_bytes) {
>>>> +        efi_st_error("SPI IO protocol has zero maximum transfer
>>>> size\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->legacy_spi_protocol) {
>>>> +        efi_st_error("SPI IO protocol lacks legacy SPI protocol\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->transaction) {
>>>> +        efi_st_error("SPI IO protocol lacks transaction function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->update_spi_peripheral) {
>>>> +        efi_st_error("SPI IO protocol lacks update_spi_peripheral
>>>> function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->legacy_spi_protocol->erase_block_opcode) {
>>>> +        efi_st_error("SPI legacy controller protocol lacks
>>>> erase_block_opcode function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->legacy_spi_protocol->write_status_prefix) {
>>>> +        efi_st_error("SPI legacy controller protocol lacks
>>>> write_status_prefix function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>
>>> I would have expected that you actually call each of the functions.
>>
>> I can extend this in v3 to call each function and confirm they return
>> EFI_UNSUPPORTED.
>>
>>>
>>>> +
>>>> +    if (!io_protocol->legacy_spi_protocol->bios_base_address) {
>>>> +        efi_st_error("SPI legacy controller protocol lacks
>>>> bios_base_address function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->legacy_spi_protocol->clear_spi_protect) {
>>>> +        efi_st_error("SPI legacy controller protocol lacks
>>>> clear_spi_protect function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->legacy_spi_protocol->is_range_protected) {
>>>> +        efi_st_error("SPI legacy controller protocol lacks
>>>> is_range_protected function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->legacy_spi_protocol->protect_next_range) {
>>>> +        efi_st_error("SPI legacy controller protocol lacks
>>>> protect_next_range function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!io_protocol->legacy_spi_protocol->lock_controller) {
>>>> +        efi_st_error("SPI legacy controller protocol lacks
>>>> lock_controller function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    req[0] = 0x9f;
>>>> +    ret = io_protocol->transaction(io_protocol,
>>>> +                       SPI_TRANSACTION_FULL_DUPLEX,
>>>> +                       false, 0, 1, 8,
>>>> +                       sizeof(req), req,
>>>> +                       sizeof(resp), resp);
>>>> +    if (ret != EFI_SUCCESS) {
>>>> +        efi_st_error("SPI transaction failed\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if ((resp[0] != 0xff) || (resp[1] != 0x20) || (resp[2] != 0x20)
>>>> || (resp[3] != 0x15)) {
>>>> +        efi_st_error("Incorrect response from sandbox SPI flash
>>>> emulator\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    return EFI_ST_SUCCESS;
>>>> +}
>>>> +
>>>> +static int test_bus(struct efi_spi_bus *bus)
>>>> +{
>>>> +    struct efi_spi_peripheral *p;
>>>> +    int status;
>>>> +
>>>> +    if (!bus->friendly_name) {
>>>> +        efi_st_error("SPI bus lacks a friendly name\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!bus->peripheral_list) {
>>>> +        efi_st_error("SPI bus has zero peripherals\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!bus->clock) {
>>>> +        efi_st_error("SPI bus lacks clock function\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    for (p = bus->peripheral_list; p; p = p->next_spi_peripheral) {
>>>> +        status = test_peripheral(p, bus);
>>>> +        if (status) {
>>>> +            efi_st_error("Failed testing SPI peripheral\n");
>>>> +            return status;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return EFI_ST_SUCCESS;
>>>> +}
>>>> +
>>>> +static int execute(void)
>>>> +{
>>>> +    struct efi_spi_configuration_protocol *spi;
>>>> +    efi_status_t ret;
>>>> +    int status;
>>>> +    u32 i;
>>>> +
>>>> +    ret = boottime->locate_protocol(&efi_spi_configuration_guid,
>>>> +                    NULL, (void **)&spi);
>>>> +    if (ret != EFI_SUCCESS) {
>>>> +        efi_st_error("SPI configuration protocol not available\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (!spi->bus_count) {
>>>> +        efi_st_error("SPI configuration protocol has zero busses\n");
>>>> +        return EFI_ST_FAILURE;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < spi->bus_count; i++) {
>>>> +        status = test_bus(spi->bus_list[i]);
>>>> +        if (status) {
>>>> +            efi_st_error("Failed testing SPI bus %d\n", i);
>>>> +            return status;
>>>> +        }
>>>> +    }
>>>
>>> This test does not test anything changing any state of SPI flash. Why is
>>> this patch needed if that is all that is usable?
>>
>> The test here is confirming that io_protocol->transaction() works as
>> expected, without re-testing the u-boot spi driver model or sandbox
>> spi drivers as they should be already tested elsewhere.
>>
>> I could extend this to also test the SPI_TRANSACTION_WRITE_THEN_READ
>> transaction type. Testing the other transaction types
>> (SPI_TRANSACTION_WRITE_ONLY & SPI_TRANSACTION_READ_ONLY) would require
>> extending the sandbox spi drivers since there's not really a valid way
>> to use these with an emulated jedec flash device.
>
> Are you happy with this explanation?
>
>>
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>> +
>>>> +    return EFI_ST_SUCCESS;
>>>> +}
>>>> +
>>>> +EFI_UNIT_TEST(spi_protocol) = {
>>>> +    .name = "SPI protocol",
>>>> +    .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
>>>> +    .setup = setup,
>>>> +    .execute = execute,
>>>> +};
>>>
>>
>
> Thanks,
>

[PATCH v2 2/3] arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export

[Paul Barker]

Add properties to the Authenta SPI flash device node to enable access by
a UEFI application using a fixed GUID. Also specify that this device is
JEDEC compatible so that it is correctly initialized when running
`sf probe`.

Signed-off-by: Paul Barker <paul.barker@sancloud.com>
---
 arch/arm/dts/am335x-sancloud-bbe-lite.dts | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/dts/am335x-sancloud-bbe-lite.dts b/arch/arm/dts/am335x-sancloud-bbe-lite.dts
index d6ef19311a91..01866f3c3ff7 100644
--- a/arch/arm/dts/am335x-sancloud-bbe-lite.dts
+++ b/arch/arm/dts/am335x-sancloud-bbe-lite.dts
@@ -37,14 +37,18 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&bb_spi0_pins>;
 
-	channel@0 {
+	authenta-flash@0 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		compatible = "micron,spi-authenta";
+		compatible = "micron,spi-authenta", "jedec,spi-nor";
 
 		reg = <0>;
 		spi-max-frequency = <16000000>;
 		spi-cpha;
+
+		uefi-spi-vendor = "micron";
+		uefi-spi-part-number = "mt25ql128abb";
+		uefi-spi-io-guid = [30671277 caa4 8643 b341 881fe18e7f7d];
 	};
 };
-- 
2.25.1

[PATCH v2 3/3] am335x_evm_defconfig: Enable Micron SPI flash support

[Paul Barker]

Signed-off-by: Paul Barker <paul.barker@sancloud.com>
---
 configs/am335x_evm_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/am335x_evm_defconfig b/configs/am335x_evm_defconfig
index b500ed0fdd8d..4984d66dc1ce 100644
--- a/configs/am335x_evm_defconfig
+++ b/configs/am335x_evm_defconfig
@@ -92,6 +92,7 @@ CONFIG_SYS_NAND_U_BOOT_LOCATIONS=y
 CONFIG_SYS_NAND_U_BOOT_OFFS=0xc0000
 CONFIG_DM_SPI_FLASH=y
 CONFIG_SF_DEFAULT_SPEED=24000000
+CONFIG_SPI_FLASH_STMICRO=y
 CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_PHY_ATHEROS=y
 CONFIG_PHY_SMSC=y
-- 
2.25.1