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

[PATCH v5 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.

This support is initially intended to be used to communicate with SPI
devices from "pre-boot" UEFI applications which perform setup before
the main OS is loaded. Other use cases may also be supported, however
this is not intended to be a replacement for UEFI capsule updates.

The "pre-boot" UEFI application which we are currently developing will
interact with a Micron Authenta[1] flash device, sending vendor-specific
commands over the SPI bus to make use of Authenta security features, to
verify flash integrity and to generate ephemeral keys based on the flash
contents.

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.

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

Changes since v4:

* Dropped patches which have already been merged.

* Re-based on top of my other patch series [2] to avoid conflicts in
  am335x-sancloud-bbe-lite-u-boot.dtsi.

* Dropped `am335x_evm_defconfig: Enable Micron SPI flash support` as
  this was moved to my other patch series [3].

* Reformat code to fit in 80 chars per line where possible.

* Use efi_install_multiple_protocol_interfaces() instead of
  efi_create_handle() & efi_add_protocol().

* Rename dtb properties to start with 'u-boot,'.

* Move SanCloud BBE Lite dtb changes to
  am335x-sancloud-bbe-lite-u-boot.dtsi.

* Convert remaining printf() calls to debug() calls.

[2]: https://lore.kernel.org/u-boot/20221114124243.3719037-1-paul.barker@sancloud.com/
[3]: https://lore.kernel.org/u-boot/20221114124243.3719037-7-paul.barker@sancloud.com/

Changes since v3:

* Add implementation of spi_set_speed() so we can use this in
  efi_spi_io_transaction().

* Rename convert_efi_string() to efi_convert_string() and move to
  efi_string.c as a common helper function.

* Call spi_set_speed() instead of manually de-referencing the function
  pointer in struct dm_spi_ops.

* Don't call efi_delete_handle() in destroy_efi_spi_peripheral().

* Use '%pUl' to print guid string.

* Split efi_selftest changes into a separate patch.

Changes since v2:

* Added description to efi_spi_protocol.h.

* Moved definition of EFI_SPI_CONFIGURATION_GUID to efi_api.h and
  added it to the list in lib/uuid.c.

* Fixed UEFI GUID byte order in test.dts and in debug output.

* Use log_debug() instead of printf() for debug output.

* Add test cases to confirm that efi_legacy_spi_controller_protocol
  functions return EFI_UNSUPPORTED (as they are not currently
  implemented)

Changes since v1:

* 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.

* Add efi_seltest unit test.

* Do not enable config EFI_SPI_PROTOCOL by default.

* 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.

Paul Barker (3):
  efi_loader: Add SPI I/O protocol support
  efi_selftest: Add tests for SPI protocol support
  arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export

 MAINTAINERS                                   |   7 +
 .../dts/am335x-sancloud-bbe-lite-u-boot.dtsi  |  13 +-
 arch/arm/dts/am335x-sancloud-bbe-lite.dts     |   2 +-
 arch/sandbox/dts/test.dts                     |  13 +
 configs/am335x_evm_defconfig                  |   1 +
 include/efi_api.h                             |   4 +
 include/efi_loader.h                          |   4 +
 include/efi_spi_protocol.h                    | 166 +++++
 lib/efi_loader/Kconfig                        |   8 +
 lib/efi_loader/Makefile                       |   1 +
 lib/efi_loader/efi_setup.c                    |   6 +
 lib/efi_loader/efi_spi_protocol.c             | 576 ++++++++++++++++++
 lib/efi_selftest/Makefile                     |   1 +
 lib/efi_selftest/efi_selftest_spi_protocol.c  | 284 +++++++++
 lib/uuid.c                                    |   4 +
 15 files changed, 1086 insertions(+), 4 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

-- 
2.25.1

[PATCH v5 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.

The following protocols are defined:
* EFI_SPI_CONFIGURATION_PROTOCOL
* EFI_SPI_IO_PROTOCOL
* EFI_LEGACY_SPI_CONTROLLER_PROTOCOL

Since there are no open source implementations of these protocols to use
as an example, educated guesses/hacks have been made in cases where the
UEFI PI specification is unclear and these are documented in comments.

This implementation has been tested on the SanCloud BBE Lite and allowed
a UEFI test application to successfully communicate with a Micron
Authenta flash device connected via the SPI bus.

Signed-off-by: Paul Barker <paul.barker@sancloud.com>
---
 MAINTAINERS                       |   6 +
 configs/am335x_evm_defconfig      |   1 +
 include/efi_api.h                 |   4 +
 include/efi_loader.h              |   4 +
 include/efi_spi_protocol.h        | 166 +++++++++
 lib/efi_loader/Kconfig            |   8 +
 lib/efi_loader/Makefile           |   1 +
 lib/efi_loader/efi_setup.c        |   6 +
 lib/efi_loader/efi_spi_protocol.c | 576 ++++++++++++++++++++++++++++++
 lib/uuid.c                        |   4 +
 10 files changed, 776 insertions(+)
 create mode 100644 include/efi_spi_protocol.h
 create mode 100644 lib/efi_loader/efi_spi_protocol.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 09edf8883d3a..6a82cdbbb474 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -926,6 +926,12 @@ 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
+
 ENVIRONMENT
 M:	Joe Hershberger <joe.hershberger@ni.com>
 R:	Wolfgang Denk <wd@denx.de>
diff --git a/configs/am335x_evm_defconfig b/configs/am335x_evm_defconfig
index f73123e0b71d..ca8f4e62246a 100644
--- a/configs/am335x_evm_defconfig
+++ b/configs/am335x_evm_defconfig
@@ -123,3 +123,4 @@ CONFIG_WDT=y
 CONFIG_DYNAMIC_CRC_TABLE=y
 CONFIG_RSA=y
 CONFIG_LZO=y
+CONFIG_EFI_SPI_PROTOCOL=y
diff --git a/include/efi_api.h b/include/efi_api.h
index 9bb0d44ac8d5..eebefbe1585a 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -454,6 +454,10 @@ struct efi_runtime_services {
 	EFI_GUID(0x607f766c, 0x7455, 0x42be, 0x93, \
 		 0x0b, 0xe4, 0xd7, 0x6d, 0xb2, 0x72, 0x0f)
 
+#define EFI_SPI_CONFIGURATION_GUID  \
+	EFI_GUID(0x85a6d3e6, 0xb65b, 0x4afc,     \
+		 0xb3, 0x8f, 0xc6, 0xd5, 0x4a, 0xf6, 0xdd, 0xc8)
+
 #define RISCV_EFI_BOOT_PROTOCOL_GUID \
 	EFI_GUID(0xccd15fec, 0x6f73, 0x4eec, 0x83, \
 		 0x95, 0x3e, 0x69, 0xe4, 0xb9, 0x40, 0xbf)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0c6c95ba4641..da5edc4a843b 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -553,6 +553,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..1a247a6a6f85
--- /dev/null
+++ b/include/efi_spi_protocol.h
@@ -0,0 +1,166 @@
+/* SPDX-License-Identifier: BSD-2-Clause-Patent */
+/*
+ * Copyright (c) 2017, Intel Corporation. All rights reserved.
+ * Copyright (c) 2022 Micron Technology, Inc.
+ *
+ * This header is based on code from EDK II, with modifications to match the
+ * u-boot coding style. It defines data structures for the following protocols
+ * found in the UEFI Platform Initialization (PI) Specification, version 1.7
+ * errata A:
+ *
+ *    - EFI_SPI_CONFIGURATION_PROTOCOL
+ *    - EFI_SPI_IO_PROTOCOL
+ *    - EFI_LEGACY_SPI_CONTROLLER_PROTOCOL (required by the I/O protocol as the
+ *      spec does not appear to allow the legacy_spi_protocol pointer to be NULL
+ *      in struct efi_spi_io_protocol)
+ */
+
+#ifndef _EFI_SPI_PROTOCOL_H
+#define _EFI_SPI_PROTOCOL_H
+
+#include <efi.h>
+#include <efi_api.h>
+
+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 e2b643871bf8..2e4d92ed01a3 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -405,4 +405,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 8738757dd2c1..e0a793103ba5 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
 obj-$(CONFIG_EFI_ECPT) += efi_conformance.o
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 543764113530..2a76e5731d2c 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -289,6 +289,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..50610b887cf2
--- /dev/null
+++ b/lib/efi_loader/efi_spi_protocol.c
@@ -0,0 +1,576 @@
+// 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 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_set_speed(target, 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);
+	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 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;
+	efi_handle_t handle = NULL;
+	struct udevice *dev_bus = dev->parent;
+	struct spi_slave *target;
+	const char *name = dev_read_name(dev);
+	const char *vendor = dev_read_string(dev, "u-boot,uefi-spi-vendor");
+	const char *part_number = dev_read_string(dev,
+			"u-boot,uefi-spi-part-number");
+	efi_guid_t *guid = (efi_guid_t *)dev_read_u8_array_ptr(dev,
+			"u-boot,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 = efi_convert_string(vendor);
+	if (!vendor_utf16) {
+		status = EFI_OUT_OF_RESOURCES;
+		goto fail_2;
+	}
+
+	part_number_utf16 = efi_convert_string(part_number);
+	if (!part_number_utf16) {
+		status = EFI_OUT_OF_RESOURCES;
+		goto fail_3;
+	}
+
+	name_utf16 = efi_convert_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_install_multiple_protocol_interfaces(&handle, guid,
+							  &priv->io_protocol,
+							  NULL);
+	if (status != EFI_SUCCESS)
+		goto fail_5;
+
+	debug("Added EFI_SPI_IO_PROTOCOL for %s with guid %pUl\n", name, guid);
+	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) {
+		debug("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 = efi_convert_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;
+	efi_handle_t handle = NULL;
+	struct efi_spi_configuration_protocol *proto;
+	uint i;
+
+	debug("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_install_multiple_protocol_interfaces(&handle,
+							  &efi_spi_configuration_guid,
+							  proto, NULL);
+	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/uuid.c b/lib/uuid.c
index 465e1ac38f57..3f723b732588 100644
--- a/lib/uuid.c
+++ b/lib/uuid.c
@@ -187,6 +187,10 @@ static const struct {
 		"TCG2",
 		EFI_TCG2_PROTOCOL_GUID,
 		},
+	{
+		"SPI Protocol Stack",
+		EFI_SPI_CONFIGURATION_GUID
+	},
 	{
 		"System Partition",
 		PARTITION_SYSTEM_GUID
-- 
2.25.1

[PATCH v5 2/3] efi_selftest: Add tests for SPI protocol support

[Paul Barker]

These tests ensure that the new UEFI SPI I/O protocol support works as
expected. They intended to execute on the sandbox target.

Signed-off-by: Paul Barker <paul.barker@sancloud.com>
---
 MAINTAINERS                                  |   1 +
 arch/sandbox/dts/test.dts                    |  13 +
 lib/efi_selftest/Makefile                    |   1 +
 lib/efi_selftest/efi_selftest_spi_protocol.c | 284 +++++++++++++++++++
 4 files changed, 299 insertions(+)
 create mode 100644 lib/efi_selftest/efi_selftest_spi_protocol.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 6a82cdbbb474..54742e759e4c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -931,6 +931,7 @@ 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
 
 ENVIRONMENT
 M:	Joe Hershberger <joe.hershberger@ni.com>
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index dffe10adbf47..003af61113a8 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -1253,6 +1253,13 @@
 			compatible = "spansion,m25p16", "jedec,spi-nor";
 			spi-max-frequency = <40000000>;
 			sandbox,filename = "spi.bin";
+
+			u-boot,uefi-spi-vendor = "spansion";
+			u-boot,uefi-spi-part-number = "mt25p16";
+
+			/* GUID in UEFI format: b881eb5d-ad92-4a48-8fdd-fa75a8e4c6b8 */
+			u-boot,uefi-spi-io-guid = [5d eb 81 b8 92 ad 48 4a
+						   8f dd fa 75 a8 e4 c6 b8];
 		};
 		spi.bin@1 {
 			reg = <1>;
@@ -1261,6 +1268,12 @@
 			sandbox,filename = "spi.bin";
 			spi-cpol;
 			spi-cpha;
+
+			u-boot,uefi-spi-vendor = "spansion";
+			u-boot,uefi-spi-part-number = "mt25p16";
+			/* GUID in UEFI format: b6b39ecd-2b1f-a643-b8d7-3192d7cf7270 */
+			u-boot,uefi-spi-io-guid = [cd 9e b3 b6 1f 2b 43 a6
+						   b8 d7 31 92 d7 cf 72 70];
 		};
 	};
 
diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index e4d75420bff6..e2c4fcd40795 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_EFI_LOADER_HII) += efi_selftest_hii.o
 obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_selftest_rng.o
 obj-$(CONFIG_EFI_GET_TIME) += efi_selftest_rtc.o
 obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_selftest_tcg2.o
+obj-$(CONFIG_EFI_SPI_PROTOCOL) += efi_selftest_spi_protocol.o
 
 ifeq ($(CONFIG_GENERATE_ACPI_TABLE),)
 obj-y += efi_selftest_fdt.o
diff --git a/lib/efi_selftest/efi_selftest_spi_protocol.c b/lib/efi_selftest/efi_selftest_spi_protocol.c
new file mode 100644
index 000000000000..946d04dbb557
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_spi_protocol.c
@@ -0,0 +1,284 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2022 Micron Technology, Inc.
+ */
+
+#include <efi_selftest.h>
+#include <efi_spi_protocol.h>
+
+static struct efi_boot_services *boottime;
+static efi_guid_t efi_spi_configuration_guid = EFI_SPI_CONFIGURATION_GUID;
+
+static int setup(const efi_handle_t img_handle,
+		 const struct efi_system_table *systable)
+{
+	boottime = systable->boottime;
+	return EFI_ST_SUCCESS;
+}
+
+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->erase_block_opcode(
+			io_protocol->legacy_spi_protocol,
+			0) != EFI_UNSUPPORTED) {
+		efi_st_error("Incorrect return value from SPI legacy controller protocol 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->write_status_prefix(
+			io_protocol->legacy_spi_protocol,
+			0) != EFI_UNSUPPORTED) {
+		efi_st_error("Incorrect return value from SPI legacy controller protocol 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->bios_base_address(
+			io_protocol->legacy_spi_protocol,
+			0) != EFI_UNSUPPORTED) {
+		efi_st_error("Incorrect return value from SPI legacy controller protocol 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->clear_spi_protect(
+			io_protocol->legacy_spi_protocol) != EFI_UNSUPPORTED) {
+		efi_st_error("Incorrect return value from SPI legacy controller protocol 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->is_range_protected(
+			io_protocol->legacy_spi_protocol,
+			0, 0)) {
+		efi_st_error("Incorrect return value from SPI legacy controller protocol 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->protect_next_range(
+			io_protocol->legacy_spi_protocol,
+			0, 0) != EFI_UNSUPPORTED) {
+		efi_st_error("Incorrect return value from SPI legacy controller protocol 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;
+	}
+
+	if (io_protocol->legacy_spi_protocol->lock_controller(
+			io_protocol->legacy_spi_protocol) != EFI_UNSUPPORTED) {
+		efi_st_error("Incorrect return value from SPI legacy controller protocol 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

[PATCH v5 3/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.

Signed-off-by: Paul Barker <paul.barker@sancloud.com>
---
 arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi | 13 ++++++++++---
 arch/arm/dts/am335x-sancloud-bbe-lite.dts         |  2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
index 01c105ebb383..6c4ff67f9a4b 100644
--- a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
+++ b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
@@ -38,7 +38,14 @@
 
 &spi0 {
 	u-boot,dm-pre-reloc;
-	channel@0 {
-		u-boot,dm-pre-reloc;
-	};
+};
+
+&authenta_flash {
+	u-boot,dm-pre-reloc;
+
+	u-boot,uefi-spi-vendor = "micron";
+	u-boot,uefi-spi-part-number = "mt25ql128abb";
+	/* GUID in UEFI format: 77126730-a4ca-4386-b341-881fe18e7f7d */
+	u-boot,uefi-spi-io-guid = [30 67 12 77 ca a4 86 43
+				   b3 41 88 1f e1 8e 7f 7d];
 };
diff --git a/arch/arm/dts/am335x-sancloud-bbe-lite.dts b/arch/arm/dts/am335x-sancloud-bbe-lite.dts
index 8ffbc72dc57e..3909092a140f 100644
--- a/arch/arm/dts/am335x-sancloud-bbe-lite.dts
+++ b/arch/arm/dts/am335x-sancloud-bbe-lite.dts
@@ -37,7 +37,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&bb_spi0_pins>;
 
-	channel@0 {
+	authenta_flash: channel@0 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-- 
2.25.1

Re: [PATCH v5 0/3] Support UEFI SPI I/O protocol

[Paul Barker]

On 23/11/2022 17:50, Paul Barker wrote:
> 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.
> 
> This support is initially intended to be used to communicate with SPI
> devices from "pre-boot" UEFI applications which perform setup before
> the main OS is loaded. Other use cases may also be supported, however
> this is not intended to be a replacement for UEFI capsule updates.
> 
> The "pre-boot" UEFI application which we are currently developing will
> interact with a Micron Authenta[1] flash device, sending vendor-specific
> commands over the SPI bus to make use of Authenta security features, to
> verify flash integrity and to generate ephemeral keys based on the flash
> contents.
> 
> 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.
> 
> [1]: https://www.micron.com/-/media/client/global/documents/products/product-flyer/authenta_technology_solutions_brief.pdf
> 
> Changes since v4:
> 
> * Dropped patches which have already been merged.
> 
> * Re-based on top of my other patch series [2] to avoid conflicts in
>   am335x-sancloud-bbe-lite-u-boot.dtsi.
> 
> * Dropped `am335x_evm_defconfig: Enable Micron SPI flash support` as
>   this was moved to my other patch series [3].
> 
> * Reformat code to fit in 80 chars per line where possible.
> 
> * Use efi_install_multiple_protocol_interfaces() instead of
>   efi_create_handle() & efi_add_protocol().
> 
> * Rename dtb properties to start with 'u-boot,'.
> 
> * Move SanCloud BBE Lite dtb changes to
>   am335x-sancloud-bbe-lite-u-boot.dtsi.
> 
> * Convert remaining printf() calls to debug() calls.
> 
> [2]: https://lore.kernel.org/u-boot/20221114124243.3719037-1-paul.barker@sancloud.com/
> [3]: https://lore.kernel.org/u-boot/20221114124243.3719037-7-paul.barker@sancloud.com/
> 
> Changes since v3:
> 
> * Add implementation of spi_set_speed() so we can use this in
>   efi_spi_io_transaction().
> 
> * Rename convert_efi_string() to efi_convert_string() and move to
>   efi_string.c as a common helper function.
> 
> * Call spi_set_speed() instead of manually de-referencing the function
>   pointer in struct dm_spi_ops.
> 
> * Don't call efi_delete_handle() in destroy_efi_spi_peripheral().
> 
> * Use '%pUl' to print guid string.
> 
> * Split efi_selftest changes into a separate patch.
> 
> Changes since v2:
> 
> * Added description to efi_spi_protocol.h.
> 
> * Moved definition of EFI_SPI_CONFIGURATION_GUID to efi_api.h and
>   added it to the list in lib/uuid.c.
> 
> * Fixed UEFI GUID byte order in test.dts and in debug output.
> 
> * Use log_debug() instead of printf() for debug output.
> 
> * Add test cases to confirm that efi_legacy_spi_controller_protocol
>   functions return EFI_UNSUPPORTED (as they are not currently
>   implemented)
> 
> Changes since v1:
> 
> * 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.
> 
> * Add efi_seltest unit test.
> 
> * Do not enable config EFI_SPI_PROTOCOL by default.
> 
> * 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.
> 
> Paul Barker (3):
>   efi_loader: Add SPI I/O protocol support
>   efi_selftest: Add tests for SPI protocol support
>   arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export
> 
>  MAINTAINERS                                   |   7 +
>  .../dts/am335x-sancloud-bbe-lite-u-boot.dtsi  |  13 +-
>  arch/arm/dts/am335x-sancloud-bbe-lite.dts     |   2 +-
>  arch/sandbox/dts/test.dts                     |  13 +
>  configs/am335x_evm_defconfig                  |   1 +
>  include/efi_api.h                             |   4 +
>  include/efi_loader.h                          |   4 +
>  include/efi_spi_protocol.h                    | 166 +++++
>  lib/efi_loader/Kconfig                        |   8 +
>  lib/efi_loader/Makefile                       |   1 +
>  lib/efi_loader/efi_setup.c                    |   6 +
>  lib/efi_loader/efi_spi_protocol.c             | 576 ++++++++++++++++++
>  lib/efi_selftest/Makefile                     |   1 +
>  lib/efi_selftest/efi_selftest_spi_protocol.c  | 284 +++++++++
>  lib/uuid.c                                    |   4 +
>  15 files changed, 1086 insertions(+), 4 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
> 

Sending a gentle ping on this series... is there any further feedback?
Are there any outstanding issues to resolve?

Thanks,

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

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

Re: [PATCH v5 0/3] Support UEFI SPI I/O protocol

[Ilias Apalodimas]

Hi Paul

[...]

> >
>
> Sending a gentle ping on this series... is there any further feedback?
> Are there any outstanding issues to resolve?
>

Thanks for the patience and apologies for the slow review.  I didn't
have any serious objections apart from the DT stuff on the previous
version.  I'll go through this during the week and let you know

Thanks
/Ilias
> Thanks,
>
> --
> Paul Barker
> Principal Software Engineer
> SanCloud Ltd
>
> e: paul.barker@sancloud.com
> w: https://sancloud.com/
>

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

[Ilias Apalodimas]

Hi Paul,

Apologies for the delayed reply.

[...]

> +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;
> +	efi_handle_t handle = NULL;
> +	struct udevice *dev_bus = dev->parent;
> +	struct spi_slave *target;
> +	const char *name = dev_read_name(dev);
> +	const char *vendor = dev_read_string(dev, "u-boot,uefi-spi-vendor");
> +	const char *part_number = dev_read_string(dev,
> +			"u-boot,uefi-spi-part-number");
> +	efi_guid_t *guid = (efi_guid_t *)dev_read_u8_array_ptr(dev,
> +			"u-boot,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 = efi_convert_string(vendor);
> +	if (!vendor_utf16) {
> +		status = EFI_OUT_OF_RESOURCES;
> +		goto fail_2;
> +	}
> +
> +	part_number_utf16 = efi_convert_string(part_number);
> +	if (!part_number_utf16) {
> +		status = EFI_OUT_OF_RESOURCES;
> +		goto fail_3;
> +	}
> +
> +	name_utf16 = efi_convert_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_install_multiple_protocol_interfaces(&handle, guid,
> +							  &priv->io_protocol,
> +							  NULL);

There's a protocols installed here as well as in
efi_spi_protocol_register().  But I don't see those being uninstalled
somewhere.  Shouldn't destroy_efi_spi_bus() call
efi_uninstall_multiple_protocol_interfaces() as well ?


> +	if (status != EFI_SUCCESS)
> +		goto fail_5;
> +
> +	debug("Added EFI_SPI_IO_PROTOCOL for %s with guid %pUl\n", name, guid);
> +	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) {
> +		debug("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 = efi_convert_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;
> +	efi_handle_t handle = NULL;
> +	struct efi_spi_configuration_protocol *proto;
> +	uint i;
> +
> +	debug("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_install_multiple_protocol_interfaces(&handle,
> +							  &efi_spi_configuration_guid,
> +							  proto, NULL);
> +	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/uuid.c b/lib/uuid.c
> index 465e1ac38f57..3f723b732588 100644
> --- a/lib/uuid.c
> +++ b/lib/uuid.c
> @@ -187,6 +187,10 @@ static const struct {
>  		"TCG2",
>  		EFI_TCG2_PROTOCOL_GUID,
>  		},
> +	{
> +		"SPI Protocol Stack",
> +		EFI_SPI_CONFIGURATION_GUID
> +	},
>  	{
>  		"System Partition",
>  		PARTITION_SYSTEM_GUID
> --
> 2.25.1
>


Regards
/Ilias

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

[Simon Glass]

Hi Paul,

On Wed, 23 Nov 2022 at 10:50, Paul Barker <paul.barker@sancloud.com> wrote:
>
> This addition allows UEFI applications running under u-boot to access
> peripherals on SPI busses. It is based on the UEFI Platform
> Initialization (PI) Specification, Version 1.7 Errata A (April 2020).
> Only the core functionality required to discover SPI peripherals and
> communicate with them is currently implemented. Other functionality such
> as the legacy SPI controller interface and the ability to update the SPI
> peripheral object associated with a particular SPI I/O protocol object
> is currently unimplemented.
>
> The following protocols are defined:
> * EFI_SPI_CONFIGURATION_PROTOCOL
> * EFI_SPI_IO_PROTOCOL
> * EFI_LEGACY_SPI_CONTROLLER_PROTOCOL
>
> Since there are no open source implementations of these protocols to use
> as an example, educated guesses/hacks have been made in cases where the
> UEFI PI specification is unclear and these are documented in comments.
>
> This implementation has been tested on the SanCloud BBE Lite and allowed
> a UEFI test application to successfully communicate with a Micron
> Authenta flash device connected via the SPI bus.
>
> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
> ---
>  MAINTAINERS                       |   6 +
>  configs/am335x_evm_defconfig      |   1 +
>  include/efi_api.h                 |   4 +
>  include/efi_loader.h              |   4 +
>  include/efi_spi_protocol.h        | 166 +++++++++
>  lib/efi_loader/Kconfig            |   8 +
>  lib/efi_loader/Makefile           |   1 +
>  lib/efi_loader/efi_setup.c        |   6 +
>  lib/efi_loader/efi_spi_protocol.c | 576 ++++++++++++++++++++++++++++++
>  lib/uuid.c                        |   4 +
>  10 files changed, 776 insertions(+)
>  create mode 100644 include/efi_spi_protocol.h
>  create mode 100644 lib/efi_loader/efi_spi_protocol.c

This should have a sandbox test as well. See test/dm/spi.c or sf.c for examples.

Regards,
Simon

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

[Paul Barker]

On 14/12/2022 04:39, Simon Glass wrote:
> Hi Paul,
> 
> On Wed, 23 Nov 2022 at 10:50, Paul Barker <paul.barker@sancloud.com> wrote:
>>
>> This addition allows UEFI applications running under u-boot to access
>> peripherals on SPI busses. It is based on the UEFI Platform
>> Initialization (PI) Specification, Version 1.7 Errata A (April 2020).
>> Only the core functionality required to discover SPI peripherals and
>> communicate with them is currently implemented. Other functionality such
>> as the legacy SPI controller interface and the ability to update the SPI
>> peripheral object associated with a particular SPI I/O protocol object
>> is currently unimplemented.
>>
>> The following protocols are defined:
>> * EFI_SPI_CONFIGURATION_PROTOCOL
>> * EFI_SPI_IO_PROTOCOL
>> * EFI_LEGACY_SPI_CONTROLLER_PROTOCOL
>>
>> Since there are no open source implementations of these protocols to use
>> as an example, educated guesses/hacks have been made in cases where the
>> UEFI PI specification is unclear and these are documented in comments.
>>
>> This implementation has been tested on the SanCloud BBE Lite and allowed
>> a UEFI test application to successfully communicate with a Micron
>> Authenta flash device connected via the SPI bus.
>>
>> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
>> ---
>>  MAINTAINERS                       |   6 +
>>  configs/am335x_evm_defconfig      |   1 +
>>  include/efi_api.h                 |   4 +
>>  include/efi_loader.h              |   4 +
>>  include/efi_spi_protocol.h        | 166 +++++++++
>>  lib/efi_loader/Kconfig            |   8 +
>>  lib/efi_loader/Makefile           |   1 +
>>  lib/efi_loader/efi_setup.c        |   6 +
>>  lib/efi_loader/efi_spi_protocol.c | 576 ++++++++++++++++++++++++++++++
>>  lib/uuid.c                        |   4 +
>>  10 files changed, 776 insertions(+)
>>  create mode 100644 include/efi_spi_protocol.h
>>  create mode 100644 lib/efi_loader/efi_spi_protocol.c
> 
> This should have a sandbox test as well. See test/dm/spi.c or sf.c for examples.

Hi Simon,

There is a test case in patch 2 of this series, is that what you're
looking for? It was split out as requested in the review of v3 of this
series.

Thanks,

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

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

Re: [PATCH v5 2/3] efi_selftest: Add tests for SPI protocol support

[Simon Glass]

On Wed, 23 Nov 2022 at 09:50, Paul Barker <paul.barker@sancloud.com> wrote:
>
> These tests ensure that the new UEFI SPI I/O protocol support works as
> expected. They intended to execute on the sandbox target.
>
> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
> ---
>  MAINTAINERS                                  |   1 +
>  arch/sandbox/dts/test.dts                    |  13 +
>  lib/efi_selftest/Makefile                    |   1 +
>  lib/efi_selftest/efi_selftest_spi_protocol.c | 284 +++++++++++++++++++
>  4 files changed, 299 insertions(+)
>  create mode 100644 lib/efi_selftest/efi_selftest_spi_protocol.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

Yes, that's it. Thanks.


- Simon

On Wed, 14 Dec 2022 at 01:57, Paul Barker <paul.barker@sancloud.com> wrote:
>
> On 14/12/2022 04:39, Simon Glass wrote:
> > Hi Paul,
> >
> > On Wed, 23 Nov 2022 at 10:50, Paul Barker <paul.barker@sancloud.com> wrote:
> >>
> >> This addition allows UEFI applications running under u-boot to access
> >> peripherals on SPI busses. It is based on the UEFI Platform
> >> Initialization (PI) Specification, Version 1.7 Errata A (April 2020).
> >> Only the core functionality required to discover SPI peripherals and
> >> communicate with them is currently implemented. Other functionality such
> >> as the legacy SPI controller interface and the ability to update the SPI
> >> peripheral object associated with a particular SPI I/O protocol object
> >> is currently unimplemented.
> >>
> >> The following protocols are defined:
> >> * EFI_SPI_CONFIGURATION_PROTOCOL
> >> * EFI_SPI_IO_PROTOCOL
> >> * EFI_LEGACY_SPI_CONTROLLER_PROTOCOL
> >>
> >> Since there are no open source implementations of these protocols to use
> >> as an example, educated guesses/hacks have been made in cases where the
> >> UEFI PI specification is unclear and these are documented in comments.
> >>
> >> This implementation has been tested on the SanCloud BBE Lite and allowed
> >> a UEFI test application to successfully communicate with a Micron
> >> Authenta flash device connected via the SPI bus.
> >>
> >> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
> >> ---
> >>  MAINTAINERS                       |   6 +
> >>  configs/am335x_evm_defconfig      |   1 +
> >>  include/efi_api.h                 |   4 +
> >>  include/efi_loader.h              |   4 +
> >>  include/efi_spi_protocol.h        | 166 +++++++++
> >>  lib/efi_loader/Kconfig            |   8 +
> >>  lib/efi_loader/Makefile           |   1 +
> >>  lib/efi_loader/efi_setup.c        |   6 +
> >>  lib/efi_loader/efi_spi_protocol.c | 576 ++++++++++++++++++++++++++++++
> >>  lib/uuid.c                        |   4 +
> >>  10 files changed, 776 insertions(+)
> >>  create mode 100644 include/efi_spi_protocol.h
> >>  create mode 100644 lib/efi_loader/efi_spi_protocol.c
> >
> > This should have a sandbox test as well. See test/dm/spi.c or sf.c for examples.
>
> Hi Simon,
>
> There is a test case in patch 2 of this series, is that what you're
> looking for? It was split out as requested in the review of v3 of this
> series.
>
> Thanks,
>
> --
> Paul Barker
> Principal Software Engineer
> SanCloud Ltd
>
> e: paul.barker@sancloud.com
> w: https://sancloud.com/
>

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

[Rob Herring]

On Wed, Nov 23, 2022 at 05:50:06PM +0000, Paul Barker wrote:
> Add properties to the Authenta SPI flash device node to enable access by
> a UEFI application using a fixed GUID.
> 
> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
> ---
>  arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi | 13 ++++++++++---
>  arch/arm/dts/am335x-sancloud-bbe-lite.dts         |  2 +-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
> index 01c105ebb383..6c4ff67f9a4b 100644
> --- a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
> +++ b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
> @@ -38,7 +38,14 @@
>  
>  &spi0 {
>  	u-boot,dm-pre-reloc;
> -	channel@0 {
> -		u-boot,dm-pre-reloc;
> -	};
> +};
> +
> +&authenta_flash {
> +	u-boot,dm-pre-reloc;
> +
> +	u-boot,uefi-spi-vendor = "micron";
> +	u-boot,uefi-spi-part-number = "mt25ql128abb";

Looks like a compatible string. Yet, the flash node compatible string, 
micron,spi-authenta, is not documented (though in use for spidev). So 
use a compatible string for the flash that is specific to the flash 
model. I assume there is some reason the specific model is needed?

> +	/* GUID in UEFI format: 77126730-a4ca-4386-b341-881fe18e7f7d */
> +	u-boot,uefi-spi-io-guid = [30 67 12 77 ca a4 86 43
> +				   b3 41 88 1f e1 8e 7f 7d];

We need to define first in the DT spec the format for GUIDs. I don't 
think there are any existing cases though some have been proposed. IMO, 
I would make this a string instead. The byte array is not that 
readable with its little endian order. A GUID as a string is readily 
identifiable as a GUID.

Why is this u-boot specific? Another UEFI implementation doesn't need 
the GUID?

Rob

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

[Paul Barker]

On 20/12/2022 15:55, Rob Herring wrote:
> On Wed, Nov 23, 2022 at 05:50:06PM +0000, Paul Barker wrote:
>> Add properties to the Authenta SPI flash device node to enable access by
>> a UEFI application using a fixed GUID.
>>
>> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
>> ---
>>  arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi | 13 ++++++++++---
>>  arch/arm/dts/am335x-sancloud-bbe-lite.dts         |  2 +-
>>  2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
>> index 01c105ebb383..6c4ff67f9a4b 100644
>> --- a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
>> +++ b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
>> @@ -38,7 +38,14 @@
>>  
>>  &spi0 {
>>  	u-boot,dm-pre-reloc;
>> -	channel@0 {
>> -		u-boot,dm-pre-reloc;
>> -	};
>> +};
>> +
>> +&authenta_flash {
>> +	u-boot,dm-pre-reloc;
>> +
>> +	u-boot,uefi-spi-vendor = "micron";
>> +	u-boot,uefi-spi-part-number = "mt25ql128abb";
> 
> Looks like a compatible string. Yet, the flash node compatible string, 
> micron,spi-authenta, is not documented (though in use for spidev). So 
> use a compatible string for the flash that is specific to the flash 
> model. I assume there is some reason the specific model is needed?

For context, the UEFI Platform Initialization (PI) spec defines
EFI_SPI_PART, EFI_SPI_PERIPHERAL and EFI_SPI_IO_PROTOCOL structures.
I'm referencing v1.7 Errata A. See https://uefi.org/specifications for
downloads.

The EFI_SPI_PART structure has "Vendor" and "PartNumber" fields. We need
something to put in those fields and the device tree is the best place
to store the data. These properties are in the `-u-boot.dtsi` file so
they won't be submitted to the Linux kernel.

The `micron,spi-authenta` compatible string is unfortunate in my
opinion. I plan to replace this with two entries, `micron,mt25ql128abb`
and `jedec,spi-nor`, once we've got MTD support for the Authenta parts
merged into the mainline kernel. That work is currently on hold, I'll
be working with Micron in the new year to unblock it. We could submit
a change to the compatible property in the meantime if you think it's
worth getting a temporary improvement in before the MTD changes are
done.

>> +	/* GUID in UEFI format: 77126730-a4ca-4386-b341-881fe18e7f7d */
>> +	u-boot,uefi-spi-io-guid = [30 67 12 77 ca a4 86 43
>> +				   b3 41 88 1f e1 8e 7f 7d];
> 
> We need to define first in the DT spec the format for GUIDs. I don't 
> think there are any existing cases though some have been proposed. IMO, 
> I would make this a string instead. The byte array is not that 
> readable with its little endian order. A GUID as a string is readily 
> identifiable as a GUID.

I see there is a `uuid_str_to_bin()` function in u-boot so I could make
this a string property and convert it to a binary GUID at runtime. This
would make the dts more readable so I'll make the change for v6 of this
series.

> Why is this u-boot specific? Another UEFI implementation doesn't need 
> the GUID?

Each EFI_SPI_IO_PROTOCOL instance needs its own GUID so that it can
be located at runtime. These GUIDs are not pre-defined by the spec,
instead an arbitrary per-device GUID is used and is stored in the
SpiPeripheralDriverGuid field of the relevant EFI_SPI_PERIPHERAL
instance. We could randomly generate these GUIDs at runtime since a UEFI
application can walk the tree of SPI busses and peripherals, looking
for a match in the Vendor and PartNumber fields defined above, to get
the appropriate GUID. However, storing a known value in the device tree
allows a UEFI application to just lookup the GUID without needing to
walk the SPI tree.

I'm happy to consider alternatives to the current implementation,
however we need to work within the limits of the UEFI PI spec.

Thanks,

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

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

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

[Paul Barker]

On 13/12/2022 07:15, Ilias Apalodimas wrote:
> Hi Paul,
> 
> Apologies for the delayed reply.
> 
> [...]
> 
>> +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;
>> +	efi_handle_t handle = NULL;
>> +	struct udevice *dev_bus = dev->parent;
>> +	struct spi_slave *target;
>> +	const char *name = dev_read_name(dev);
>> +	const char *vendor = dev_read_string(dev, "u-boot,uefi-spi-vendor");
>> +	const char *part_number = dev_read_string(dev,
>> +			"u-boot,uefi-spi-part-number");
>> +	efi_guid_t *guid = (efi_guid_t *)dev_read_u8_array_ptr(dev,
>> +			"u-boot,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 = efi_convert_string(vendor);
>> +	if (!vendor_utf16) {
>> +		status = EFI_OUT_OF_RESOURCES;
>> +		goto fail_2;
>> +	}
>> +
>> +	part_number_utf16 = efi_convert_string(part_number);
>> +	if (!part_number_utf16) {
>> +		status = EFI_OUT_OF_RESOURCES;
>> +		goto fail_3;
>> +	}
>> +
>> +	name_utf16 = efi_convert_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_install_multiple_protocol_interfaces(&handle, guid,
>> +							  &priv->io_protocol,
>> +							  NULL);
> 
> There's a protocols installed here as well as in
> efi_spi_protocol_register().  But I don't see those being uninstalled
> somewhere.  Shouldn't destroy_efi_spi_bus() call
> efi_uninstall_multiple_protocol_interfaces() as well ?

Yes, `destroy_efi_spi_bus()` and `destroy_efi_spi_peripheral()`
should cleanup everything created by `export_spi_bus()` and
`export_spi_peripheral()` respectively.

I think we can just call `efi_delete_handle()` on the relevant handle in
`destroy_efi_spi_peripheral()` as that will remove all protocols anyway
and the call is simpler. I can make that change in v6 of the series.

Thanks,

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

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

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

[Heinrich Schuchardt]

On 12/24/22 13:25, Paul Barker wrote:
> On 13/12/2022 07:15, Ilias Apalodimas wrote:
>> Hi Paul,
>>
>> Apologies for the delayed reply.
>>
>> [...]
>>
>>> +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;
>>> +	efi_handle_t handle = NULL;
>>> +	struct udevice *dev_bus = dev->parent;
>>> +	struct spi_slave *target;
>>> +	const char *name = dev_read_name(dev);
>>> +	const char *vendor = dev_read_string(dev, "u-boot,uefi-spi-vendor");
>>> +	const char *part_number = dev_read_string(dev,
>>> +			"u-boot,uefi-spi-part-number");
>>> +	efi_guid_t *guid = (efi_guid_t *)dev_read_u8_array_ptr(dev,
>>> +			"u-boot,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 = efi_convert_string(vendor);
>>> +	if (!vendor_utf16) {
>>> +		status = EFI_OUT_OF_RESOURCES;
>>> +		goto fail_2;
>>> +	}
>>> +
>>> +	part_number_utf16 = efi_convert_string(part_number);
>>> +	if (!part_number_utf16) {
>>> +		status = EFI_OUT_OF_RESOURCES;
>>> +		goto fail_3;
>>> +	}
>>> +
>>> +	name_utf16 = efi_convert_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_install_multiple_protocol_interfaces(&handle, guid,
>>> +							  &priv->io_protocol,
>>> +							  NULL);
>>
>> There's a protocols installed here as well as in
>> efi_spi_protocol_register().  But I don't see those being uninstalled
>> somewhere.  Shouldn't destroy_efi_spi_bus() call
>> efi_uninstall_multiple_protocol_interfaces() as well ?
>
> Yes, `destroy_efi_spi_bus()` and `destroy_efi_spi_peripheral()`
> should cleanup everything created by `export_spi_bus()` and
> `export_spi_peripheral()` respectively.
>
> I think we can just call `efi_delete_handle()` on the relevant handle in
> `destroy_efi_spi_peripheral()` as that will remove all protocols anyway
> and the call is simpler. I can make that change in v6 of the series.

This patch does not correctly interface with the driver model.

What we need is:

* When a SPI flash device is probed successfully this causes the
installation of the protocols and thereby the creation of the handle.
* When trying to remove a SPI flash device this causes the
uninstallation of the protocols.

For interfacing with the driver model you should use the events
EVT_DM_POST_PROBE and EVT_DM_PRE_REMOVE. Cf. efi_bl_init().

When the driver model tries to remove the SPI device you have to call
UninstallMultipleProtocolInterfaces().

efi_delete_handle() does not check if one of the protocols installed on
the handles has been opened (e.g. with BY_DRIVER) and therefore cannot
request those parts of the loaded code that still hold references to the
protocol interfaces to close the protocols. This may lead to crashes.

If UninstallMultipleProtocolInterfaces() returns an error, the remove
event handler must return an error to avoid the removal of the device.

Best regards

Heinrich

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2320 bytes --]

On Sat, Dec 24, 2022 at 12:03:52PM +0000, Paul Barker wrote:
> On 20/12/2022 15:55, Rob Herring wrote:
> > On Wed, Nov 23, 2022 at 05:50:06PM +0000, Paul Barker wrote:
> >> Add properties to the Authenta SPI flash device node to enable access by
> >> a UEFI application using a fixed GUID.
> >>
> >> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
> >> ---
> >>  arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi | 13 ++++++++++---
> >>  arch/arm/dts/am335x-sancloud-bbe-lite.dts         |  2 +-
> >>  2 files changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
> >> index 01c105ebb383..6c4ff67f9a4b 100644
> >> --- a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
> >> +++ b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
> >> @@ -38,7 +38,14 @@
> >>  
> >>  &spi0 {
> >>  	u-boot,dm-pre-reloc;
> >> -	channel@0 {
> >> -		u-boot,dm-pre-reloc;
> >> -	};
> >> +};
> >> +
> >> +&authenta_flash {
> >> +	u-boot,dm-pre-reloc;
> >> +
> >> +	u-boot,uefi-spi-vendor = "micron";
> >> +	u-boot,uefi-spi-part-number = "mt25ql128abb";
> > 
> > Looks like a compatible string. Yet, the flash node compatible string, 
> > micron,spi-authenta, is not documented (though in use for spidev). So 
> > use a compatible string for the flash that is specific to the flash 
> > model. I assume there is some reason the specific model is needed?
> 
> For context, the UEFI Platform Initialization (PI) spec defines
> EFI_SPI_PART, EFI_SPI_PERIPHERAL and EFI_SPI_IO_PROTOCOL structures.
> I'm referencing v1.7 Errata A. See https://uefi.org/specifications for
> downloads.
> 
> The EFI_SPI_PART structure has "Vendor" and "PartNumber" fields. We need
> something to put in those fields and the device tree is the best place
> to store the data. These properties are in the `-u-boot.dtsi` file so
> they won't be submitted to the Linux kernel.

Well, IMHO, this doesn't belong in U-Boot only for forever. Just like
other bindings/properties that we're working on getting merged upstream
and so that there is really Just One Device Tree, this should go
upstream I believe. This might be another case of starts in -u-boot.dtsi
while things get sorted out.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Rob Herring]

On Sat, Dec 24, 2022 at 6:04 AM Paul Barker <paul.barker@sancloud.com> wrote:
>
> On 20/12/2022 15:55, Rob Herring wrote:
> > On Wed, Nov 23, 2022 at 05:50:06PM +0000, Paul Barker wrote:
> >> Add properties to the Authenta SPI flash device node to enable access by
> >> a UEFI application using a fixed GUID.
> >>
> >> Signed-off-by: Paul Barker <paul.barker@sancloud.com>
> >> ---
> >>  arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi | 13 ++++++++++---
> >>  arch/arm/dts/am335x-sancloud-bbe-lite.dts         |  2 +-
> >>  2 files changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
> >> index 01c105ebb383..6c4ff67f9a4b 100644
> >> --- a/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
> >> +++ b/arch/arm/dts/am335x-sancloud-bbe-lite-u-boot.dtsi
> >> @@ -38,7 +38,14 @@
> >>
> >>  &spi0 {
> >>      u-boot,dm-pre-reloc;
> >> -    channel@0 {
> >> -            u-boot,dm-pre-reloc;
> >> -    };
> >> +};
> >> +
> >> +&authenta_flash {
> >> +    u-boot,dm-pre-reloc;
> >> +
> >> +    u-boot,uefi-spi-vendor = "micron";
> >> +    u-boot,uefi-spi-part-number = "mt25ql128abb";
> >
> > Looks like a compatible string. Yet, the flash node compatible string,
> > micron,spi-authenta, is not documented (though in use for spidev). So
> > use a compatible string for the flash that is specific to the flash
> > model. I assume there is some reason the specific model is needed?
>
> For context, the UEFI Platform Initialization (PI) spec defines
> EFI_SPI_PART, EFI_SPI_PERIPHERAL and EFI_SPI_IO_PROTOCOL structures.
> I'm referencing v1.7 Errata A. See https://uefi.org/specifications for
> downloads.
>
> The EFI_SPI_PART structure has "Vendor" and "PartNumber" fields. We need
> something to put in those fields and the device tree is the best place
> to store the data. These properties are in the `-u-boot.dtsi` file so
> they won't be submitted to the Linux kernel.

Can't you just split the `micron,mt25ql128abb` compatible string to
populate those?


> The `micron,spi-authenta` compatible string is unfortunate in my
> opinion. I plan to replace this with two entries, `micron,mt25ql128abb`
> and `jedec,spi-nor`, once we've got MTD support for the Authenta parts
> merged into the mainline kernel. That work is currently on hold, I'll
> be working with Micron in the new year to unblock it. We could submit
> a change to the compatible property in the meantime if you think it's
> worth getting a temporary improvement in before the MTD changes are
> done.

No need to wait on driver changes to add compatible strings. Anyone
that says otherwise is wrong.

Looking at the datasheet, maybe `micron,mt25ql128abb` is too specific.
Perhaps 'abb' or just the last 'b' can be dropped? It's a trade off of
lots of compatible strings vs. whether you might need to distinguish
the parts (only before you do jedec discovery).

>
> >> +    /* GUID in UEFI format: 77126730-a4ca-4386-b341-881fe18e7f7d */
> >> +    u-boot,uefi-spi-io-guid = [30 67 12 77 ca a4 86 43
> >> +                               b3 41 88 1f e1 8e 7f 7d];
> >
> > We need to define first in the DT spec the format for GUIDs. I don't
> > think there are any existing cases though some have been proposed. IMO,
> > I would make this a string instead. The byte array is not that
> > readable with its little endian order. A GUID as a string is readily
> > identifiable as a GUID.
>
> I see there is a `uuid_str_to_bin()` function in u-boot so I could make
> this a string property and convert it to a binary GUID at runtime. This
> would make the dts more readable so I'll make the change for v6 of this
> series.
>
> > Why is this u-boot specific? Another UEFI implementation doesn't need
> > the GUID?
>
> Each EFI_SPI_IO_PROTOCOL instance needs its own GUID so that it can
> be located at runtime. These GUIDs are not pre-defined by the spec,
> instead an arbitrary per-device GUID is used and is stored in the
> SpiPeripheralDriverGuid field of the relevant EFI_SPI_PERIPHERAL
> instance. We could randomly generate these GUIDs at runtime since a UEFI
> application can walk the tree of SPI busses and peripherals, looking
> for a match in the Vendor and PartNumber fields defined above, to get
> the appropriate GUID. However, storing a known value in the device tree
> allows a UEFI application to just lookup the GUID without needing to
> walk the SPI tree.

Okay, then I guess my next question is why is it SPI IO protocol
specific? I'd assume the UEFI spec would use this for any h/w
protocol.

Generating GUIDs at runtime seems like the right way though. We simply
can't populate DT with every client's method of identifying a given
instance. That simply doesn't scale.

Rob

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

[Paul Barker]

On 03/01/2023 19:27, Rob Herring wrote:
> On Sat, Dec 24, 2022 at 6:04 AM Paul Barker <paul.barker@sancloud.com> wrote:
>> The EFI_SPI_PART structure has "Vendor" and "PartNumber" fields. We need
>> something to put in those fields and the device tree is the best place
>> to store the data. These properties are in the `-u-boot.dtsi` file so
>> they won't be submitted to the Linux kernel.
> 
> Can't you just split the `micron,mt25ql128abb` compatible string to
> populate those?

Yes, we can go that route. I'll prepare a patch to add
`micron,mt25ql128` to the compatibile strings for this node and submit
that to both the kernel & u-boot. Then I'll add code to split that into
vendor & part number at runtime.

>>> Why is this u-boot specific? Another UEFI implementation doesn't need
>>> the GUID?
>>
>> Each EFI_SPI_IO_PROTOCOL instance needs its own GUID so that it can
>> be located at runtime. These GUIDs are not pre-defined by the spec,
>> instead an arbitrary per-device GUID is used and is stored in the
>> SpiPeripheralDriverGuid field of the relevant EFI_SPI_PERIPHERAL
>> instance. We could randomly generate these GUIDs at runtime since a UEFI
>> application can walk the tree of SPI busses and peripherals, looking
>> for a match in the Vendor and PartNumber fields defined above, to get
>> the appropriate GUID. However, storing a known value in the device tree
>> allows a UEFI application to just lookup the GUID without needing to
>> walk the SPI tree.
> 
> Okay, then I guess my next question is why is it SPI IO protocol
> specific? I'd assume the UEFI spec would use this for any h/w
> protocol.

Sadly, common sense assumptions and the UEFI spec do not go together.
There at least 3 different ways this is handled in UEFI:

* For SD/MMC, the UEFI spec defines a single GUID for the I/O protocol
  and the API takes a slot number to determine which actual device to
  operate on. The `This` argument is assumed to point to a global
  instance of EFI_SD_MMC_PASS_THRU_PROTOCOL.

* For NVMe, it's the same as above but with a namespace ID instead of a
  slot number.

* For ATA, PCI, SCSI, USB, etc, it looks to be a similar story.

* For I2C, there is a fixed GUID for the I/O protocol but the API takes
  an additional GUID to identify the device to operate on. So a unique
  GUID must be allocated for each device, but only one instance of
  EFI_I2C_IO_PROTOCOL is needed.

* For SPI, the I/O protocol has no fixed GUID and it's expected that
  there is a separate EFI_SPI_IO_PROTOCOL instance with a unique GUID
  for each device a the SPI bus. The API does not take any arguments to
  identify the SPI bus or chip select, instead the `This` argument
  points to the specific instance of EFI_SPI_IO_PROTOCOL for the device
  we want to operate on. There is a separate protocol,
  EFI_SPI_CONFIGURATION_PROTOCOL, with a fixed GUID, that allows the
  SPI bus to be walked.

> Generating GUIDs at runtime seems like the right way though. We simply
> can't populate DT with every client's method of identifying a given
> instance. That simply doesn't scale.

Ok. We can re-write our application code to walk the SPI bus nodes
looking for the appropriate Vendor & PartNumber pair, then lookup the
GUID for that device. That will allow us to generate the GUID
dynamically at runtime. We will then be relying solely on the compatible
property in the device tree to fill in the Vendor & PartNumber fields.

> Rob

Thanks for your feedback Rob. It's been good to see another perspective
on this.

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

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

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

[Paul Barker]

On 24/12/2022 14:09, Heinrich Schuchardt wrote:
> On 12/24/22 13:25, Paul Barker wrote:
>> On 13/12/2022 07:15, Ilias Apalodimas wrote:
>>> Hi Paul,
>>>
>>> Apologies for the delayed reply.
>>>
>>> [...]
>>>
>>>> +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;
>>>> +    efi_handle_t handle = NULL;
>>>> +    struct udevice *dev_bus = dev->parent;
>>>> +    struct spi_slave *target;
>>>> +    const char *name = dev_read_name(dev);
>>>> +    const char *vendor = dev_read_string(dev, "u-boot,uefi-spi-vendor");
>>>> +    const char *part_number = dev_read_string(dev,
>>>> +            "u-boot,uefi-spi-part-number");
>>>> +    efi_guid_t *guid = (efi_guid_t *)dev_read_u8_array_ptr(dev,
>>>> +            "u-boot,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 = efi_convert_string(vendor);
>>>> +    if (!vendor_utf16) {
>>>> +        status = EFI_OUT_OF_RESOURCES;
>>>> +        goto fail_2;
>>>> +    }
>>>> +
>>>> +    part_number_utf16 = efi_convert_string(part_number);
>>>> +    if (!part_number_utf16) {
>>>> +        status = EFI_OUT_OF_RESOURCES;
>>>> +        goto fail_3;
>>>> +    }
>>>> +
>>>> +    name_utf16 = efi_convert_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_install_multiple_protocol_interfaces(&handle, guid,
>>>> +                              &priv->io_protocol,
>>>> +                              NULL);
>>>
>>> There's a protocols installed here as well as in
>>> efi_spi_protocol_register().  But I don't see those being uninstalled
>>> somewhere.  Shouldn't destroy_efi_spi_bus() call
>>> efi_uninstall_multiple_protocol_interfaces() as well ?
>>
>> Yes, `destroy_efi_spi_bus()` and `destroy_efi_spi_peripheral()`
>> should cleanup everything created by `export_spi_bus()` and
>> `export_spi_peripheral()` respectively.
>>
>> I think we can just call `efi_delete_handle()` on the relevant handle in
>> `destroy_efi_spi_peripheral()` as that will remove all protocols anyway
>> and the call is simpler. I can make that change in v6 of the series.
> 
> This patch does not correctly interface with the driver model.
> 
> What we need is:
> 
> * When a SPI flash device is probed successfully this causes the
> installation of the protocols and thereby the creation of the handle.
> * When trying to remove a SPI flash device this causes the
> uninstallation of the protocols.
> 
> For interfacing with the driver model you should use the events
> EVT_DM_POST_PROBE and EVT_DM_PRE_REMOVE. Cf. efi_bl_init().
> 
> When the driver model tries to remove the SPI device you have to call
> UninstallMultipleProtocolInterfaces().
> 
> efi_delete_handle() does not check if one of the protocols installed on
> the handles has been opened (e.g. with BY_DRIVER) and therefore cannot
> request those parts of the loaded code that still hold references to the
> protocol interfaces to close the protocols. This may lead to crashes.
> 
> If UninstallMultipleProtocolInterfaces() returns an error, the remove
> event handler must return an error to avoid the removal of the device.

Hi Heinrich,

I appreciate your feedback, though I have to say I'm disappointed that
this feedback wasn't provided on an earlier iteration of the patch
series.

I've got some other pressing priorities this week but will try to
re-write the relevant functions next week and re-submit.

Thanks,

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

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