[PATCH v2 1/3] efi_loader: Add SPI I/O protocol support
Heinrich Schuchardt
xypron.glpk at gmx.de
Wed Sep 14 09:11:04 CEST 2022
On 9/12/22 18:14, Paul Barker wrote:
> Hi Heinrich,
>
> I'm sending a ping on this as there were a few questions in my response
> to your feedback and I'd like to move this forward. I've highlighted the
> key items with further comments below.
>
> On 25/08/2022 11:58, Paul Barker wrote:
>> Hi Henrich,
>>
>> Many thanks for your review on this patch. I've responded to things
>> inline below. I'm also available on IRC (paulbarker) if you want to
>> discuss further.
>>
>> On 13/08/2022 20:20, Heinrich Schuchardt wrote:
>>> On 8/4/22 12:34, Paul Barker wrote:
>>>> This addition allows UEFI applications running under u-boot to access
>>>> peripherals on SPI busses. It is based on the UEFI Platform
>>>> Initialization (PI) Specification, Version 1.7 Errata A (April 2020).
>>>> Only the core functionality required to discover SPI peripherals and
>>>> communicate with them is currently implemented. Other functionality
>>>> such
>>>> as the legacy SPI controller interface and the ability to update the
>>>> SPI
>>>> peripheral object associated with a particular SPI I/O protocol object
>>>> is currently unimplemented.
>>>>
>>>> Since there are no open source implementations of this protocol to use
>>>> as an example, educated guesses/hacks have been made in cases where the
>>>> UEFI PI specification is unclear and these are documented in comments.
>>>>
>>>> This implementation has been tested on the SanCloud BBE Lite and
>>>> allowed
>>>> a UEFI test application to successfully communicate with a Micron
>>>> Authenta flash device connected via the SPI bus. It has also been
>>>> tested
>>>> with the sandbox target using the included efi_selftest case.
>>>
>>> The commit message should describe which protocols are actually
>>> implemented.
>>
>> I'll add this to the commit message in v3.
>>
>>>
>>> I still have no clue why any PI protocol should be implemented U-Boot.
>>> You neither descibe it here nor in the cover-letter.
>>
>> Implementing the SPI I/O protocol allows a UEFI application running
>> under u-boot to enumerate and communicate with devices on the SPI bus.
>> This protocol fulfils the same purpose as the "SD MMC Pass Thru
>> Protocol" and "NVM Express Pass Through Protocol" described in the
>> main UEFI spec, I'm unsure why SPI bus access was deferred to the PI
>> spec. If these patches are accepted we plan to also implement the
>> SD/MMC and NVMe I/O protocols.
>>
>> The broader goal here is to support better encapsulation of "pre-boot"
>> application code which needs to communicate with a SPI, MMC or NVMe
>> devices before Linux is booted. This pre-boot application can then run
>> on both embedded devices running u-boot and PC-like devices running a
>> UEFI BIOS. In our case the application will interact with the Authenta
>> flash security features to verify integrity and to generate ephemeral
>> keys based on the flash contents. Our application links against
>> mbedtls and so it would be non-trivial (in terms of both
>> implementation and licensing) to integrate this directly into u-boot.
>>
>> We have previously explored using the "standalone" application support
>> (e.g. in examples/standalone in the u-boot source) but as that is not
>> well maintained we have moved to a UEFI implementation.
>>
>> Let me know if you need any more info than that. Once you're happy
>> with the explanation I'll add it to the cover letter in v3.
>
> Could you let me know if this is sufficient?
Thanks to linking this to Authenta flash, cf.
https://www.micron.com/-/media/client/global/documents/products/product-flyer/authenta_technology_solutions_brief.pdf
The other answers where also helpfule to better undestand this patch.
Please, send v3 and the we can add this functionality.
Best regards
Heinrich
>
>>
>>>
>>> I would expect capsule updates to be used to update the SPI flash.
>>
>> Our application is not intended to perform firmware or capsule updates
>> or to make any writes to the flash.
>>
>>>
>>>>
>>>> Signed-off-by: Paul Barker <paul.barker at sancloud.com>
>>>> ---
>>>> MAINTAINERS | 7 +
>>>> arch/sandbox/dts/test.dts | 8 +
>>>> include/efi_loader.h | 4 +
>>>> include/efi_spi_protocol.h | 158 +++++
>>>> lib/efi_loader/Kconfig | 8 +
>>>> lib/efi_loader/Makefile | 1 +
>>>> lib/efi_loader/efi_setup.c | 6 +
>>>> lib/efi_loader/efi_spi_protocol.c | 614
>>>> +++++++++++++++++++
>>>> lib/efi_selftest/Makefile | 1 +
>>>> lib/efi_selftest/efi_selftest_spi_protocol.c | 237 +++++++
>>>> 10 files changed, 1044 insertions(+)
>>>> create mode 100644 include/efi_spi_protocol.h
>>>> create mode 100644 lib/efi_loader/efi_spi_protocol.c
>>>> create mode 100644 lib/efi_selftest/efi_selftest_spi_protocol.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 4d1930f76e44..1b5d9c37576b 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -856,6 +856,13 @@ F: tools/efivar.py
>>>> F: tools/file2include.c
>>>> F: tools/mkeficapsule.c
>>>>
>>>> +EFI SPI SUPPORT
>>>> +M: Paul Barker <paul.barker at sancloud.com>
>>>> +S: Maintained
>>>> +F: include/efi_spi_protocol.h
>>>> +F: lib/efi_loader/efi_spi_protocol.c
>>>> +F: lib/efi_selftest/efi_selftest_spi_protocol.c
>>>> +
>>>> EFI VARIABLES VIA OP-TEE
>>>> M: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>>>> S: Maintained
>>>> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
>>>> index d1a8cc7bfb7e..5ac59140b748 100644
>>>> --- a/arch/sandbox/dts/test.dts
>>>> +++ b/arch/sandbox/dts/test.dts
>>>> @@ -1185,6 +1185,10 @@
>>>> compatible = "spansion,m25p16", "jedec,spi-nor";
>>>> spi-max-frequency = <40000000>;
>>>> sandbox,filename = "spi.bin";
>>>> +
>>>> + uefi-spi-vendor = "spansion";
>>>> + uefi-spi-part-number = "mt25p16";
>>>> + uefi-spi-io-guid = [5deb81b8 92ad 484a 8fdd fa75a8e4c6b8];
>>>> };
>>>> spi.bin at 1 {
>>>> reg = <1>;
>>>> @@ -1193,6 +1197,10 @@
>>>> sandbox,filename = "spi.bin";
>>>> spi-cpol;
>>>> spi-cpha;
>>>> +
>>>> + uefi-spi-vendor = "spansion";
>>>
>>> Does the sandbox provide anything that is related to vendor spansion?
>>> Or could the vendor be "sandbox"?
>>
>> This is based on the "spansion,m25p16" compatible string used above.
>> I've used the same vendor and part number for consistency.
>
> Are you happy with this?
>
>>
>>>
>>>> + uefi-spi-part-number = "mt25p16";
>>>
>>> Does the sandbox really emulate a
>>> "Micron M25P16 Serial Flash Embedded Memory"?
>>
>> As above.
>>
>>>
>>>> + uefi-spi-io-guid = [cd9eb3b6 1f2b 43a6 b8d7 3192d7cf7270];
>>>
>>> This is a byte-string. The byte order in the array differs from the low
>>> endian string representation used in the UEFI specification. To avoid
>>> confusion, please, add a space after each byte and add a comment with
>>> the string representation.
>>
>> I'll make this change for v3.
>>
>>>
>>>> };
>>>> };
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 3a63a1f75fdf..2eb96b08205b 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -537,6 +537,10 @@ efi_status_t efi_rng_register(void);
>>>> efi_status_t efi_tcg2_register(void);
>>>> /* Called by efi_init_obj_list() to install
>>>> RISCV_EFI_BOOT_PROTOCOL */
>>>> efi_status_t efi_riscv_register(void);
>>>> +/* Called by efi_init_obj_list() to install
>>>> EFI_SPI_CONFIGURATION_PROTOCOL &
>>>> + * EFI_SPI_IO_PROTOCOL
>>>> + */
>>>> +efi_status_t efi_spi_protocol_register(void);
>>>> /* Called by efi_init_obj_list() to do initial measurement */
>>>> efi_status_t efi_tcg2_do_initial_measurement(void);
>>>> /* measure the pe-coff image, extend PCR and add Event Log */
>>>> diff --git a/include/efi_spi_protocol.h b/include/efi_spi_protocol.h
>>>> new file mode 100644
>>>> index 000000000000..1a4456bd9349
>>>> --- /dev/null
>>>> +++ b/include/efi_spi_protocol.h
>>>> @@ -0,0 +1,158 @@
>>>> +/* SPDX-License-Identifier: BSD-2-Clause-Patent */
>>>> +/*
>>>> + * Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>>
>>> Is this really a copy from EDK II and not based on the specification?
>>
>> This is copied from EDK II and modified to fit the u-boot coding style.
>>
>>>
>>> Please, add a reference to the
>>> "UEFI Platform Initialization (PI) Specification".
>>> and a description what this include is about.
>>
>> I'll add this in v3.
>>
>>>
>>>
>>>> + */
>>>> +
>>>> +#ifndef _EFI_SPI_PROTOCOL_H
>>>> +#define _EFI_SPI_PROTOCOL_H
>>>> +
>>>> +#include <efi.h>
>>>> +#include <efi_api.h>
>>>> +
>>>> +#define EFI_SPI_CONFIGURATION_GUID \
>>>> + EFI_GUID(0x85a6d3e6, 0xb65b, 0x4afc, \
>>>> + 0xb3, 0x8f, 0xc6, 0xd5, 0x4a, 0xf6, 0xdd, 0xc8)
>>>
>>> Please, add a comment that this GUID is defined in the "UEFI Platform
>>> Initialization (PI) Specification".
>>>
>>> Please, add the GUID to list_guid[]in lib/uuid.c .
>>
>> Will do in v3.
>>
>>>
>>>> +
>>>> +struct efi_spi_peripheral;
>>>> +
>>>> +struct efi_spi_part {
>>>> + efi_string_t vendor;
>>>> + efi_string_t part_number;
>>>
>>> Why did you remove "const" used in the specifications?
>>
>> These structures are const in the spec as the UEFI application code is
>> not expected to modify them. However, we need to be able to modify
>> these fields in u-boot before starting a UEFI application in order to
>> populate them with the correct data.
>
> Are you happy with this explanation?
>
>>
>>>
>>>> + u32 min_clock_hz;
>>>> + u32 max_clock_hz;
>>>> + bool chip_select_polarity;
>>>> +};
>>>> +
>>>> +struct efi_spi_bus {
>>>> + efi_string_t friendly_name;
>>>> + struct efi_spi_peripheral *peripheral_list;
>>>> + struct efi_device_path *controller_path;
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * clock)(const struct efi_spi_peripheral *spi_peripheral,
>>>> + u32 *clock_hz);
>>>> +
>>>> + void *clock_parameter;
>>>> +};
>>>> +
>>>> +struct efi_spi_peripheral {
>>>> + struct efi_spi_peripheral *next_spi_peripheral;
>>>> + efi_string_t friendly_name;
>>>> + efi_guid_t *spi_peripheral_driver_guid;
>>>> + struct efi_spi_part *spi_part;
>>>> + u32 max_clock_hz;
>>>> + bool clock_polarity;
>>>> + bool clock_phase;
>>>> + u32 attributes;
>>>> + void *configuration_data;
>>>> + struct efi_spi_bus *spi_bus;
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * chip_select)(const struct efi_spi_peripheral
>>>> *spi_peripheral,
>>>> + bool pin_value);
>>>> +
>>>> + void *chip_select_parameter;
>>>> +};
>>>> +
>>>> +struct efi_spi_configuration_protocol {
>>>> + u32 bus_count;
>>>> + struct efi_spi_bus **bus_list;
>>>> +};
>>>> +
>>>> +#define EFI_LEGACY_SPI_CONTROLLER_GUID \
>>>> + EFI_GUID(0x39136fc7, 0x1a11, 0x49de, \
>>>> + 0xbf, 0x35, 0x0e, 0x78, 0xdd, 0xb5, 0x24, 0xfc)
>>>
>>> The commit message clear states that the legacy protocol will not be
>>> implemented. Why should we implement any *legacy*?
>> I'd rather ignore the whole thing and use a NULL pointer but it
>> doesn't look like that's an option.
>>
>> `struct efi_spi_io_protocol` contains a pointer to a `struct
>> efi_legacy_spi_controller_protocol` and the spec does not seem to
>> allow this or the function pointers in the legacy protocol object to
>> be NULL. I don't think we can assume that a UEFI application will
>> check for NULL pointers before de-referencing given the wording in the
>> spec, so we instead need to provide some valid functions which at
>> least return an EFI_UNSUPPORTED error condition.
>
> Are you happy with this explanation?
>
>>
>>>
>>>> +
>>>> +struct efi_legacy_spi_controller_protocol;
>>>> +
>>>> +struct efi_legacy_spi_controller_protocol {
>>>> + u32 maximum_offset;
>>>> + u32 maximum_range_bytes;
>>>> + u32 range_register_count;
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * erase_block_opcode)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u8 erase_block_opcode);
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * write_status_prefix)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u8 write_status_prefix);
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * bios_base_address)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u32 bios_base_address);
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * clear_spi_protect)(const struct
>>>> efi_legacy_spi_controller_protocol *this);
>>>> +
>>>> + bool
>>>> + (EFIAPI * is_range_protected)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u32 bios_address,
>>>> + u32 blocks_to_protect);
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * protect_next_range)(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u32 bios_address,
>>>> + u32 blocks_to_protect);
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * lock_controller)(const struct
>>>> efi_legacy_spi_controller_protocol *this);
>>>> +};
>>>> +
>>>> +struct efi_spi_io_protocol;
>>>> +
>>>> +enum efi_spi_transaction_type {
>>>> + SPI_TRANSACTION_FULL_DUPLEX,
>>>> + SPI_TRANSACTION_WRITE_ONLY,
>>>> + SPI_TRANSACTION_READ_ONLY,
>>>> + SPI_TRANSACTION_WRITE_THEN_READ
>>>> +};
>>>> +
>>>> +struct efi_spi_bus_transaction {
>>>> + struct efi_spi_peripheral *spi_peripheral;
>>>> + enum efi_spi_transaction_type transaction_type;
>>>> + bool debug_transaction;
>>>> + u32 bus_width;
>>>> + u32 frame_size;
>>>> + u32 write_bytes;
>>>> + u8 *write_buffer;
>>>> + u32 read_bytes;
>>>> + u8 *read_buffer;
>>>> +};
>>>> +
>>>> +struct efi_spi_io_protocol {
>>>> + struct efi_spi_peripheral *spi_peripheral;
>>>> + struct efi_spi_peripheral *original_spi_peripheral;
>>>> + u32 frame_size_support_mask;
>>>> + u32 maximum_transfer_bytes;
>>>> + u32 attributes;
>>>> + struct efi_legacy_spi_controller_protocol *legacy_spi_protocol;
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * transaction)(const struct efi_spi_io_protocol *this,
>>>> + enum efi_spi_transaction_type transaction_type,
>>>> + bool debug_transaction,
>>>> + u32 clock_hz,
>>>> + u32 bus_width,
>>>> + u32 frame_size,
>>>> + u32 write_bytes,
>>>> + u8 *write_buffer,
>>>> + u32 read_bytes,
>>>> + u8 *read_buffer);
>>>> +
>>>> + efi_status_t
>>>> + (EFIAPI * update_spi_peripheral)(struct efi_spi_io_protocol *this,
>>>> + struct efi_spi_peripheral *spi_peripheral);
>>>> +};
>>>> +
>>>> +struct efi_spi_peripheral_priv {
>>>> + struct efi_spi_peripheral peripheral;
>>>> + struct efi_spi_part part;
>>>> + struct efi_spi_io_protocol io_protocol;
>>>> + efi_handle_t handle;
>>>> + struct spi_slave *target;
>>>> +};
>>>> +
>>>> +efi_status_t efi_spi_protocol_register(void);
>>>> +
>>>> +#endif
>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>>> index e3f2402d0e8e..c5899a6f6e52 100644
>>>> --- a/lib/efi_loader/Kconfig
>>>> +++ b/lib/efi_loader/Kconfig
>>>> @@ -394,4 +394,12 @@ config EFI_RISCV_BOOT_PROTOCOL
>>>> replace the transfer via the device-tree. The latter is not
>>>> possible on systems using ACPI.
>>>>
>>>> +config EFI_SPI_PROTOCOL
>>>> + bool "EFI SPI protocol support"
>>>> + depends on DM_SPI
>>>> + help
>>>> + Provide implementations of EFI_SPI_CONFIGURATION_PROTOCOL and
>>>> + EFI_SPI_IO_PROTOCOL to allow UEFI applications to access devices
>>>> + connected via SPI bus.
>>>> +
>>>> endif
>>>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>>>> index f54c244c3268..9fa0d27927b6 100644
>>>> --- a/lib/efi_loader/Makefile
>>>> +++ b/lib/efi_loader/Makefile
>>>> @@ -74,6 +74,7 @@ obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
>>>> obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
>>>> obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
>>>> obj-$(CONFIG_EFI_RISCV_BOOT_PROTOCOL) += efi_riscv.o
>>>> +obj-$(CONFIG_EFI_SPI_PROTOCOL) += efi_spi_protocol.o
>>>> obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
>>>> obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o
>>>>
>>>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>>>> index 492ecf4cb15c..ef1ee9862b72 100644
>>>> --- a/lib/efi_loader/efi_setup.c
>>>> +++ b/lib/efi_loader/efi_setup.c
>>>> @@ -295,6 +295,12 @@ efi_status_t efi_init_obj_list(void)
>>>> goto out;
>>>> }
>>>>
>>>> + if (IS_ENABLED(CONFIG_EFI_SPI_PROTOCOL)) {
>>>> + ret = efi_spi_protocol_register();
>>>> + if (ret != EFI_SUCCESS)
>>>> + goto out;
>>>> + }
>>>> +
>>>> /* Secure boot */
>>>> ret = efi_init_secure_boot();
>>>> if (ret != EFI_SUCCESS)
>>>> diff --git a/lib/efi_loader/efi_spi_protocol.c
>>>> b/lib/efi_loader/efi_spi_protocol.c
>>>> new file mode 100644
>>>> index 000000000000..c491741cbbf1
>>>> --- /dev/null
>>>> +++ b/lib/efi_loader/efi_spi_protocol.c
>>>> @@ -0,0 +1,614 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Copyright (c) 2022 Micron Technology, Inc.
>>>> + */
>>>> +
>>>> +#define LOG_CATEGORY LOGC_EFI
>>>> +
>>>> +#include <common.h>
>>>> +#include <dm/device.h>
>>>> +#include <dm/device-internal.h>
>>>> +#include <dm/read.h>
>>>> +#include <efi.h>
>>>> +#include <efi_loader.h>
>>>> +#include <efi_spi_protocol.h>
>>>> +#include <malloc.h>
>>>> +#include <spi.h>
>>>> +
>>>> +static efi_string_t convert_efi_string(const char *str)
>>>> +{
>>>> + efi_string_t str_16, tmp;
>>>> + size_t sz_8, sz_16;
>>>> +
>>>> + sz_8 = strlen(str);
>>>> + sz_16 = utf8_utf16_strlen(str);
>>>> + str_16 = calloc(sz_16 + 1, 2);
>>>> + if (!str_16)
>>>> + return NULL;
>>>> +
>>>> + tmp = str_16;
>>>> + utf8_utf16_strcpy(&tmp, str);
>>>> +
>>>> + return str_16;
>>>> +}
>>>> +
>>>> +static void dump_buffer(const char *msg, u32 length, u8 *buffer)
>>>> +{
>>>> + u32 i;
>>>> + EFI_PRINT("%s %d bytes:", msg, length);
>>>> + for (i = 0; i < length; i++)
>>>> + EFI_PRINT(" %02x", buffer[i]);
>>>> + EFI_PRINT("\n");
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +efi_spi_bus_clock(const struct efi_spi_peripheral *spi_peripheral,
>>>> + u32 *clock_hz)
>>>> +{
>>>> + EFI_ENTRY("%p, %p", spi_peripheral, clock_hz);
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +efi_spi_peripheral_chip_select(const struct efi_spi_peripheral
>>>> *spi_peripheral,
>>>> + bool pin_value)
>>>> +{
>>>> + EFI_ENTRY("%p, %d", spi_peripheral, pin_value);
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_erase_block_opcode(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u8 erase_block_opcode)
>>>> +{
>>>> + EFI_ENTRY("%p, %u", this, erase_block_opcode);
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_write_status_prefix(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u8 write_status_prefix)
>>>> +{
>>>> + EFI_ENTRY("%p, %u", this, write_status_prefix);
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_bios_base_address(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u32 bios_base_address)
>>>> +{
>>>> + EFI_ENTRY("%p, %u", this, bios_base_address);
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_clear_spi_protect(const struct
>>>> efi_legacy_spi_controller_protocol *this)
>>>> +{
>>>> + EFI_ENTRY("%p", this);
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static bool EFIAPI
>>>> +legacy_is_range_protected(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u32 bios_address,
>>>> + u32 blocks_to_protect)
>>>> +{
>>>> + EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
>>>> + return EFI_EXIT(false);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_protect_next_range(const struct
>>>> efi_legacy_spi_controller_protocol *this,
>>>> + u32 bios_address,
>>>> + u32 blocks_to_protect)
>>>> +{
>>>> + EFI_ENTRY("%p, %u, %u", this, bios_address, blocks_to_protect);
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +legacy_lock_controller(const struct
>>>> efi_legacy_spi_controller_protocol *this)
>>>> +{
>>>> + EFI_ENTRY("%p", this);
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +efi_spi_io_update_spi_peripheral(struct efi_spi_io_protocol *this,
>>>> + struct efi_spi_peripheral *spi_peripheral)
>>>> +{
>>>> + EFI_ENTRY("%p, %p", this, spi_peripheral);
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI
>>>> +efi_spi_io_transaction(const struct efi_spi_io_protocol *this,
>>>> + enum efi_spi_transaction_type transaction_type,
>>>> + bool debug_transaction,
>>>> + u32 clock_hz,
>>>> + u32 bus_width,
>>>> + u32 frame_size,
>>>> + u32 write_bytes,
>>>> + u8 *write_buffer,
>>>> + u32 read_bytes,
>>>> + u8 *read_buffer)
>>>> +{
>>>> + struct spi_slave *target;
>>>> + efi_status_t status = EFI_SUCCESS;
>>>> + int r;
>>>> +
>>>> + /* We ignore the bus_width and frame_size arguments to this
>>>> function as the
>>>> + * appropriate bus configuration for the connected device will
>>>> be performed
>>>> + * during spi_claim_bus().
>>>> + */
>>>> +
>>>> + /* TODO: Print transaction details if debug_transaction is
>>>> true. */
>>>> +
>>>> + EFI_ENTRY("%p, %u, %u, %u, %u, %u, %u, %p, %u, %p",
>>>> + this, transaction_type, debug_transaction,
>>>> + clock_hz, bus_width, frame_size,
>>>> + write_bytes, write_buffer, read_bytes, read_buffer);
>>>> +
>>>> + if (!this)
>>>> + return EFI_EXIT(EFI_INVALID_PARAMETER);
>>>> +
>>>> + target = container_of(this, struct efi_spi_peripheral_priv,
>>>> io_protocol)->target;
>>>> +
>>>> + if (clock_hz > this->spi_peripheral->max_clock_hz)
>>>> + return EFI_EXIT(EFI_UNSUPPORTED);
>>>> +
>>>> + r = spi_claim_bus(target);
>>>> + if (r != 0)
>>>> + return EFI_EXIT(EFI_DEVICE_ERROR);
>>>> + EFI_PRINT("SPI IO: Bus claimed\n");
>>>> +
>>>> + if (clock_hz) {
>>>> + EFI_PRINT("SPI IO: Setting clock rate to %u Hz\n", clock_hz);
>>>> + spi_get_ops(target->dev->parent)->set_speed(target->dev->parent,
>>>> clock_hz);
>>>> + } else {
>>>> + EFI_PRINT("SPI IO: Using default clock rate\n");
>>>> + }
>>>> +
>>>> + switch (transaction_type) {
>>>> + case SPI_TRANSACTION_FULL_DUPLEX:
>>>> + EFI_PRINT("SPI IO: Full-duplex\n");
>>>> + if (write_bytes != read_bytes || !write_bytes ||
>>>> !write_buffer || !read_buffer) {
>>>> + status = EFI_INVALID_PARAMETER;
>>>> + break;
>>>> + }
>>>> + if (debug_transaction)
>>>> + dump_buffer("SPI IO: write", write_bytes, write_buffer);
>>>> + r = spi_xfer(target, 8 * write_bytes,
>>>> + write_buffer, read_buffer, SPI_XFER_ONCE);
>>>> + EFI_PRINT("SPI IO: xfer returned %d\n", r);
>>>> + if (debug_transaction)
>>>> + dump_buffer("SPI IO: read", read_bytes, read_buffer);
>>>> + status = (r == 0) ? EFI_SUCCESS : EFI_DEVICE_ERROR;
>>>> + break;
>>>> + case SPI_TRANSACTION_READ_ONLY:
>>>> + EFI_PRINT("SPI IO: Read-only\n");
>>>> + if (!read_bytes || !read_buffer) {
>>>> + status = EFI_INVALID_PARAMETER;
>>>> + break;
>>>> + }
>>>> + r = spi_xfer(target, 8 * read_bytes,
>>>> + NULL, read_buffer, SPI_XFER_ONCE);
>>>> + EFI_PRINT("SPI IO: xfer returned %d\n", r);
>>>> + if (debug_transaction)
>>>> + dump_buffer("SPI IO: read", read_bytes, read_buffer);
>>>> + status = (r == 0) ? EFI_SUCCESS : EFI_DEVICE_ERROR;
>>>> + break;
>>>> + case SPI_TRANSACTION_WRITE_ONLY:
>>>> + EFI_PRINT("SPI IO: Write-only\n");
>>>> + if (!write_bytes || !write_buffer) {
>>>> + status = EFI_INVALID_PARAMETER;
>>>> + break;
>>>> + }
>>>> + if (debug_transaction)
>>>> + dump_buffer("SPI IO: write", write_bytes, write_buffer);
>>>> + r = spi_xfer(target, 8 * write_bytes,
>>>> + write_buffer, NULL, SPI_XFER_ONCE);
>>>> + EFI_PRINT("SPI IO: xfer returned %d\n", r);
>>>> + status = (r == 0) ? EFI_SUCCESS : EFI_DEVICE_ERROR;
>>>> + break;
>>>> + case SPI_TRANSACTION_WRITE_THEN_READ:
>>>> + EFI_PRINT("SPI IO: Write-then-read\n");
>>>> + if (!write_bytes || !write_buffer || !read_bytes ||
>>>> !read_buffer) {
>>>> + status = EFI_INVALID_PARAMETER;
>>>> + break;
>>>> + }
>>>> + if (debug_transaction)
>>>> + dump_buffer("SPI IO: write", write_bytes, write_buffer);
>>>> + r = spi_xfer(target, 8 * write_bytes,
>>>> + write_buffer, NULL, SPI_XFER_BEGIN);
>>>> + EFI_PRINT("SPI IO: xfer [1/2] returned %d\n", r);
>>>> + if (r != 0) {
>>>> + status = EFI_DEVICE_ERROR;
>>>> + break;
>>>> + }
>>>> + r = spi_xfer(target, 8 * read_bytes,
>>>> + NULL, read_buffer, SPI_XFER_END);
>>>> + EFI_PRINT("SPI IO: xfer [2/2] returned %d\n", r);
>>>> + if (debug_transaction)
>>>> + dump_buffer("SPI IO: read", read_bytes, read_buffer);
>>>> + status = (r == 0) ? EFI_SUCCESS : EFI_DEVICE_ERROR;
>>>> + break;
>>>> + default:
>>>> + status = EFI_INVALID_PARAMETER;
>>>> + break;
>>>> + }
>>>> +
>>>> + spi_release_bus(target);
>>>> + EFI_PRINT("SPI IO: Released bus\n");
>>>> + return EFI_EXIT(status);
>>>> +}
>>>> +
>>>> +static struct efi_device_path null_device_path = {
>>>> + .type = DEVICE_PATH_TYPE_END,
>>>> + .sub_type = DEVICE_PATH_SUB_TYPE_END,
>>>> + .length = 4
>>>> +};
>>>> +
>>>> +static struct efi_legacy_spi_controller_protocol
>>>> +dummy_legacy_spi_controller_protocol = {
>>>> + .maximum_offset = 0,
>>>> + .maximum_range_bytes = 0,
>>>> + .range_register_count = 0,
>>>> + .erase_block_opcode = legacy_erase_block_opcode,
>>>> + .write_status_prefix = legacy_write_status_prefix,
>>>> + .bios_base_address = legacy_bios_base_address,
>>>> + .clear_spi_protect = legacy_clear_spi_protect,
>>>> + .is_range_protected = legacy_is_range_protected,
>>>> + .protect_next_range = legacy_protect_next_range,
>>>> + .lock_controller = legacy_lock_controller
>>>> +};
>>>
>>> In the commit message you said that the legacy protocol will not be
>>> implemented.
>>>
>>> Why would a *legacy* be implemented?
>>
>> As explained above.
>>
>>>
>>>> +
>>>> +static efi_guid_t efi_spi_configuration_guid =
>>>> EFI_SPI_CONFIGURATION_GUID;
>>>> +
>>>> +static void destroy_efi_spi_peripheral(struct efi_spi_peripheral
>>>> *peripheral)
>>>> +{
>>>> + struct efi_spi_peripheral_priv *priv =
>>>> + container_of(peripheral,
>>>> + struct efi_spi_peripheral_priv,
>>>> + peripheral);
>>>> + free(priv->peripheral.friendly_name);
>>>> + free(priv->part.vendor);
>>>> + free(priv->part.part_number);
>>>> + efi_delete_handle(priv->handle);
>>>> + free(priv);
>>>> +}
>>>> +
>>>> +static void destroy_efi_spi_bus(struct efi_spi_bus *bus)
>>>> +{
>>>> + struct efi_spi_peripheral *peripheral = bus->peripheral_list;
>>>> +
>>>> + while (peripheral) {
>>>> + struct efi_spi_peripheral *next =
>>>> + peripheral->next_spi_peripheral;
>>>> + destroy_efi_spi_peripheral(peripheral);
>>>> + peripheral = next;
>>>> + }
>>>> + free(bus->friendly_name);
>>>> + free(bus);
>>>> +}
>>>> +
>>>> +static efi_status_t efi_spi_new_handle(const efi_guid_t *guid, void
>>>> *proto)
>>>> +{
>>>> + efi_status_t status;
>>>> + efi_handle_t handle;
>>>> +
>>>> + status = efi_create_handle(&handle);
>>>> + if (status != EFI_SUCCESS) {
>>>> + printf("Failed to create EFI handle\n");
>>>> + goto fail_1;
>>>> + }
>>>> +
>>>> + status = efi_add_protocol(handle, guid, proto);
>>>> + if (status != EFI_SUCCESS) {
>>>> + printf("Failed to add protocol\n");
>>>> + goto fail_2;
>>>> + }
>>>> +
>>>> + return EFI_SUCCESS;
>>>> +
>>>> +fail_2:
>>>> + efi_delete_handle(handle);
>>>> +fail_1:
>>>> + return status;
>>>> +}
>>>> +
>>>> +static void
>>>> +efi_spi_init_part(struct efi_spi_part *part,
>>>> + struct spi_slave *target,
>>>> + efi_string_t vendor_utf16,
>>>> + efi_string_t part_number_utf16
>>>> +)
>>>> +{
>>>> + part->vendor = vendor_utf16;
>>>> + part->part_number = part_number_utf16;
>>>> + part->min_clock_hz = 0;
>>>> + part->max_clock_hz = target->max_hz;
>>>> + part->chip_select_polarity =
>>>> + (target->mode & SPI_CS_HIGH) ? true : false;
>>>> +}
>>>> +
>>>> +static void
>>>> +efi_spi_init_peripheral(struct efi_spi_peripheral *peripheral,
>>>> + struct efi_spi_part *part,
>>>> + struct efi_spi_bus *bus,
>>>> + struct spi_slave *target,
>>>> + efi_guid_t *guid,
>>>> + efi_string_t name_utf16
>>>> +)
>>>> +{
>>>> + peripheral->friendly_name = name_utf16;
>>>> + peripheral->spi_part = part;
>>>> + peripheral->next_spi_peripheral = NULL;
>>>> + peripheral->spi_peripheral_driver_guid = guid;
>>>> + peripheral->max_clock_hz = target->max_hz;
>>>> + peripheral->clock_polarity = (target->mode & SPI_CPOL) ? true :
>>>> false;
>>>> + peripheral->clock_phase = (target->mode & SPI_CPHA) ? true :
>>>> false;
>>>> + peripheral->attributes = 0;
>>>> + peripheral->configuration_data = NULL;
>>>> + peripheral->spi_bus = bus;
>>>> + peripheral->chip_select = efi_spi_peripheral_chip_select;
>>>> + peripheral->chip_select_parameter = NULL;
>>>> +}
>>>> +
>>>> +static void
>>>> +efi_spi_append_peripheral(struct efi_spi_peripheral *peripheral,
>>>> + struct efi_spi_bus *bus
>>>> +)
>>>> +{
>>>> + if (bus->peripheral_list) {
>>>> + struct efi_spi_peripheral *tmp = bus->peripheral_list;
>>>> +
>>>> + while (tmp->next_spi_peripheral)
>>>> + tmp = tmp->next_spi_peripheral;
>>>> +
>>>> + tmp->next_spi_peripheral = peripheral;
>>>> + } else {
>>>> + bus->peripheral_list = peripheral;
>>>> + }
>>>> +}
>>>> +
>>>> +static void
>>>> +efi_spi_init_io_protocol(struct efi_spi_io_protocol *io_protocol,
>>>> + struct efi_spi_peripheral *peripheral,
>>>> + struct spi_slave *target
>>>> +)
>>>> +{
>>>> + u32 max_read, max_write;
>>>> +
>>>> + io_protocol->spi_peripheral = peripheral;
>>>> + io_protocol->original_spi_peripheral = peripheral;
>>>> + io_protocol->legacy_spi_protocol =
>>>> &dummy_legacy_spi_controller_protocol;
>>>> + io_protocol->transaction = efi_spi_io_transaction;
>>>> + io_protocol->update_spi_peripheral =
>>>> efi_spi_io_update_spi_peripheral;
>>>> +
>>>> + /* This is a bit of a hack. The EFI data structures do not
>>>> allow us to
>>>> + * represent a frame size greater than 32 bits.
>>>> + */
>>>> + if (target->wordlen <= 32)
>>>> + io_protocol->frame_size_support_mask =
>>>> + 1 << (target->wordlen - 1);
>>>> + else
>>>> + io_protocol->frame_size_support_mask = 0;
>>>> +
>>>> + /* Again, this is a bit of a hack. The EFI data structures only
>>>> allow
>>>> + * for a single maximum transfer size whereas the u-boot spi_slave
>>>> + * structure records maximum read transfer size and maximum write
>>>> + * transfer size separately. So we need to take the minimum of
>>>> these two
>>>> + * values.
>>>> + *
>>>> + * In u-boot, a value of zero for these fields means there is
>>>> no limit
>>>> + * on the transfer size. However in the UEFI PI spec a value of
>>>> zero is
>>>> + * invalid so we'll use 0xFFFFFFFF as a substitute unlimited
>>>> value.
>>>> + */
>>>> + max_write = target->max_write_size ? target->max_write_size :
>>>> 0xFFFFFFFF;
>>>> + max_read = target->max_read_size ? target->max_read_size :
>>>> 0xFFFFFFFF;
>>>> + io_protocol->maximum_transfer_bytes = (max_read > max_write) ?
>>>> max_write : max_read;
>>>> +
>>>> + /* Hack++. Leave attributes set to zero since the flags listed
>>>> in the
>>>> + * UEFI PI spec have no defined numerical values and so cannot
>>>> be used.
>>>> + */
>>>> + io_protocol->attributes = 0;
>>>> +}
>>>> +
>>>> +static efi_status_t
>>>> +export_spi_peripheral(struct efi_spi_bus *bus, struct udevice *dev)
>>>> +{
>>>> + efi_string_t name_utf16, vendor_utf16, part_number_utf16;
>>>> + struct efi_spi_peripheral_priv *priv;
>>>> + efi_status_t status;
>>>> + struct udevice *dev_bus = dev->parent;
>>>> + struct spi_slave *target;
>>>> + const char *name = dev_read_name(dev);
>>>> + const char *vendor = dev_read_string(dev, "uefi-spi-vendor");
>>>> + const char *part_number = dev_read_string(dev,
>>>> "uefi-spi-part-number");
>>>> + efi_guid_t *guid =
>>>> + (efi_guid_t *)dev_read_u8_array_ptr(dev,
>>>> "uefi-spi-io-guid", 16);
>>>> +
>>>> + if (device_get_uclass_id(dev) == UCLASS_SPI_EMUL) {
>>>> + debug("Skipping emulated SPI peripheral %s\n", name);
>>>> + goto fail_1;
>>>> + }
>>>> +
>>>> + if (!vendor || !part_number || !guid) {
>>>> + debug("Skipping SPI peripheral %s\n", name);
>>>> + status = EFI_UNSUPPORTED;
>>>> + goto fail_1;
>>>> + }
>>>> +
>>>> + if (!device_active(dev)) {
>>>> + int ret = device_probe(dev);
>>>> + if (ret) {
>>>> + debug("Skipping SPI peripheral %s, probe failed\n", name);
>>>> + goto fail_1;
>>>> + }
>>>> + }
>>>> +
>>>> + target = dev_get_parent_priv(dev);
>>>> + if (!target) {
>>>> + debug("Skipping uninitialized SPI peripheral %s\n", name);
>>>> + status = EFI_UNSUPPORTED;
>>>> + goto fail_1;
>>>> + }
>>>> +
>>>> + debug("Registering SPI dev %d:%d, name %s\n",
>>>> + dev_bus->seq_, spi_chip_select(dev), name);
>>>> +
>>>> + priv = calloc(1, sizeof(*priv));
>>>> + if (!priv) {
>>>> + status = EFI_OUT_OF_RESOURCES;
>>>> + goto fail_1;
>>>> + }
>>>> +
>>>> + vendor_utf16 = convert_efi_string(vendor);
>>>> + if (!vendor_utf16) {
>>>> + status = EFI_OUT_OF_RESOURCES;
>>>> + goto fail_2;
>>>> + }
>>>> +
>>>> + part_number_utf16 = convert_efi_string(part_number);
>>>> + if (!part_number_utf16) {
>>>> + status = EFI_OUT_OF_RESOURCES;
>>>> + goto fail_3;
>>>> + }
>>>> +
>>>> + name_utf16 = convert_efi_string(name);
>>>> + if (!name_utf16) {
>>>> + status = EFI_OUT_OF_RESOURCES;
>>>> + goto fail_4;
>>>> + }
>>>> +
>>>> + priv->target = target;
>>>> +
>>>> + efi_spi_init_part(&priv->part, target, vendor_utf16,
>>>> part_number_utf16);
>>>> +
>>>> + efi_spi_init_peripheral(&priv->peripheral, &priv->part,
>>>> + bus, target, guid, name_utf16);
>>>> +
>>>> + efi_spi_append_peripheral(&priv->peripheral, bus);
>>>> +
>>>> + efi_spi_init_io_protocol(&priv->io_protocol, &priv->peripheral,
>>>> target);
>>>> +
>>>> + status = efi_spi_new_handle(guid, &priv->io_protocol);
>>>> + if (status != EFI_SUCCESS)
>>>> + goto fail_5;
>>>> +
>>>> + printf("Added EFI_SPI_IO_PROTOCOL for %s with guid "
>>>> +
>>>> "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n",
>>>
>>> This information does not interest any user. Please, use log_debug().
>>
>> I'll change this in v3.
>>
>>>
>>>> + name,
>>>> + guid->b[0], guid->b[1], guid->b[2], guid->b[3],
>>>> + guid->b[4], guid->b[5], guid->b[6], guid->b[7],
>>>> + guid->b[8], guid->b[9], guid->b[10], guid->b[11],
>>>> + guid->b[12], guid->b[13], guid->b[14], guid->b[15]);
>>>> + return EFI_SUCCESS;
>>>> +
>>>> +fail_5:
>>>> + free(name_utf16);
>>>> +fail_4:
>>>> + free(part_number_utf16);
>>>> +fail_3:
>>>> + free(vendor_utf16);
>>>> +fail_2:
>>>> + free(priv);
>>>> +fail_1:
>>>> + return status;
>>>> +}
>>>> +
>>>> +static struct efi_spi_bus *export_spi_bus(int i)
>>>> +{
>>>> + struct efi_spi_bus *bus;
>>>> + struct udevice *dev, *child;
>>>> + const char *name;
>>>> + int r;
>>>> +
>>>> + r = uclass_get_device(UCLASS_SPI, i, &dev);
>>>> + if (r < 0) {
>>>> + printf("Failed to get SPI bus %d\n", i);
>>>> + goto fail_1;
>>>> + }
>>>> +
>>>> + name = dev_read_name(dev);
>>>> + debug("Registering SPI bus %d, name %s\n", i, name);
>>>> +
>>>> + bus = calloc(1, sizeof(*bus));
>>>> + if (!bus)
>>>> + goto fail_1;
>>>> +
>>>> + bus->friendly_name = convert_efi_string(name);
>>>> + if (!bus->friendly_name)
>>>> + goto fail_2;
>>>> +
>>>> + bus->peripheral_list = NULL;
>>>> + bus->clock = efi_spi_bus_clock;
>>>> + bus->clock_parameter = NULL;
>>>> +
>>>> + /* For the purposes of the current implementation, we do not
>>>> need to expose
>>>> + * the hardware device path to users of the SPI I/O protocol.
>>>> + */
>>>> + bus->controller_path = &null_device_path;
>>>> +
>>>> + device_foreach_child(child, dev) {
>>>> + efi_status_t status = export_spi_peripheral(bus, child);
>>>> +
>>>> + if (status == EFI_OUT_OF_RESOURCES)
>>>> + goto fail_3;
>>>> + }
>>>> +
>>>> + return bus;
>>>> +
>>>> +fail_3:
>>>> + destroy_efi_spi_bus(bus);
>>>> +fail_2:
>>>> + free(bus);
>>>> +fail_1:
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +efi_status_t efi_spi_protocol_register(void)
>>>> +{
>>>> + efi_status_t status;
>>>> + struct efi_spi_configuration_protocol *proto;
>>>> + uint i;
>>>> +
>>>> + printf("Registering EFI_SPI_CONFIGURATION_PROTOCOL\n");
>>>> +
>>>> + proto = calloc(1, sizeof(*proto));
>>>> + if (!proto) {
>>>> + status = EFI_OUT_OF_RESOURCES;
>>>> + goto fail_1;
>>>> + }
>>>> +
>>>> + proto->bus_count = uclass_id_count(UCLASS_SPI);
>>>> + proto->bus_list = calloc(proto->bus_count,
>>>> sizeof(*proto->bus_list));
>>>> + if (!proto->bus_list) {
>>>> + status = EFI_OUT_OF_RESOURCES;
>>>> + goto fail_2;
>>>> + }
>>>> +
>>>> + for (i = 0; i < proto->bus_count; i++) {
>>>> + proto->bus_list[i] = export_spi_bus(i);
>>>> + if (!proto->bus_list[i])
>>>> + goto fail_3;
>>>> + }
>>>> +
>>>> + status = efi_spi_new_handle(&efi_spi_configuration_guid, proto);
>>>> + if (status != EFI_SUCCESS)
>>>> + goto fail_3;
>>>> +
>>>> + return EFI_SUCCESS;
>>>> +
>>>> +fail_3:
>>>> + for (i = 0; i < proto->bus_count; i++) {
>>>> + if (proto->bus_list[i])
>>>> + destroy_efi_spi_bus(proto->bus_list[i]);
>>>> + }
>>>> + free(proto->bus_list);
>>>> +fail_2:
>>>> + free(proto);
>>>> +fail_1:
>>>> + return status;
>>>> +}
>>>> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
>>>> index 33536c9ec021..c9b6e4fbb452 100644
>>>> --- a/lib/efi_selftest/Makefile
>>>> +++ b/lib/efi_selftest/Makefile
>>>> @@ -62,6 +62,7 @@ obj-$(CONFIG_EFI_LOADER_HII) += efi_selftest_hii.o
>>>> obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_selftest_rng.o
>>>> obj-$(CONFIG_EFI_GET_TIME) += efi_selftest_rtc.o
>>>> obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_selftest_tcg2.o
>>>> +obj-$(CONFIG_EFI_SPI_PROTOCOL) += efi_selftest_spi_protocol.o
>>>>
>>>> ifeq ($(CONFIG_GENERATE_ACPI_TABLE),)
>>>> obj-y += efi_selftest_fdt.o
>>>> diff --git a/lib/efi_selftest/efi_selftest_spi_protocol.c
>>>> b/lib/efi_selftest/efi_selftest_spi_protocol.c
>>>> new file mode 100644
>>>> index 000000000000..2a3723d3931f
>>>> --- /dev/null
>>>> +++ b/lib/efi_selftest/efi_selftest_spi_protocol.c
>>>> @@ -0,0 +1,237 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Copyright (c) 2022 Micron Technology, Inc.
>>>> + */
>>>> +
>>>> +#include <efi_selftest.h>
>>>> +#include <efi_spi_protocol.h>
>>>> +
>>>> +static struct efi_boot_services *boottime;
>>>> +static efi_guid_t efi_spi_configuration_guid =
>>>> EFI_SPI_CONFIGURATION_GUID;
>>>> +
>>>> +static int setup(const efi_handle_t img_handle,
>>>> + const struct efi_system_table *systable)
>>>> +{
>>>> + boottime = systable->boottime;
>>>> + return EFI_ST_SUCCESS;
>>>> +}
>>>> +
>>>> +static int test_peripheral(struct efi_spi_peripheral *p, struct
>>>> efi_spi_bus *bus)
>>>> +{
>>>> + struct efi_spi_io_protocol *io_protocol;
>>>> + u8 req[5], resp[5];
>>>> + efi_status_t ret;
>>>> +
>>>> + if (!p->friendly_name) {
>>>> + efi_st_error("SPI peripheral lacks a friendly name\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!p->spi_peripheral_driver_guid) {
>>>> + efi_st_error("SPI peripheral lacks driver GUID\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!p->spi_part) {
>>>> + efi_st_error("SPI peripheral lacks SPI part definition\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!p->max_clock_hz) {
>>>> + efi_st_error("SPI peripheral has a max clock rate of zero\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!p->spi_bus) {
>>>> + efi_st_error("SPI peripheral lack pointer to SPI bus\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (p->spi_bus != bus) {
>>>> + efi_st_error("SPI peripheral spi_bus pointer points to the
>>>> wrong bus\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!p->chip_select) {
>>>> + efi_st_error("SPI peripheral lacks chip_select function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!p->spi_part->vendor) {
>>>> + efi_st_error("SPI part lacks vendor string\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!p->spi_part->part_number) {
>>>> + efi_st_error("SPI part lacks part number string\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (p->spi_part->min_clock_hz > p->spi_part->max_clock_hz) {
>>>> + efi_st_error("SPI part min clock rate is greater than max
>>>> clock rate\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (p->spi_part->max_clock_hz != p->max_clock_hz) {
>>>> + efi_st_error("SPI part max clock rate does not match
>>>> peripheral max clock rate\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + ret = boottime->locate_protocol(p->spi_peripheral_driver_guid,
>>>> + NULL, (void **)&io_protocol);
>>>> + if (ret != EFI_SUCCESS) {
>>>> + efi_st_error("SPI IO protocol not available\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (io_protocol->spi_peripheral != p) {
>>>> + efi_st_error("SPI IO protocol spi_peripheral pointer points
>>>> to the wrong peripheral\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (io_protocol->original_spi_peripheral != p) {
>>>> + efi_st_error("SPI IO protocol original_spi_peripheral
>>>> pointer points to the wrong peripheral\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->maximum_transfer_bytes) {
>>>> + efi_st_error("SPI IO protocol has zero maximum transfer
>>>> size\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->legacy_spi_protocol) {
>>>> + efi_st_error("SPI IO protocol lacks legacy SPI protocol\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->transaction) {
>>>> + efi_st_error("SPI IO protocol lacks transaction function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->update_spi_peripheral) {
>>>> + efi_st_error("SPI IO protocol lacks update_spi_peripheral
>>>> function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->legacy_spi_protocol->erase_block_opcode) {
>>>> + efi_st_error("SPI legacy controller protocol lacks
>>>> erase_block_opcode function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->legacy_spi_protocol->write_status_prefix) {
>>>> + efi_st_error("SPI legacy controller protocol lacks
>>>> write_status_prefix function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>
>>> I would have expected that you actually call each of the functions.
>>
>> I can extend this in v3 to call each function and confirm they return
>> EFI_UNSUPPORTED.
>>
>>>
>>>> +
>>>> + if (!io_protocol->legacy_spi_protocol->bios_base_address) {
>>>> + efi_st_error("SPI legacy controller protocol lacks
>>>> bios_base_address function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->legacy_spi_protocol->clear_spi_protect) {
>>>> + efi_st_error("SPI legacy controller protocol lacks
>>>> clear_spi_protect function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->legacy_spi_protocol->is_range_protected) {
>>>> + efi_st_error("SPI legacy controller protocol lacks
>>>> is_range_protected function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->legacy_spi_protocol->protect_next_range) {
>>>> + efi_st_error("SPI legacy controller protocol lacks
>>>> protect_next_range function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!io_protocol->legacy_spi_protocol->lock_controller) {
>>>> + efi_st_error("SPI legacy controller protocol lacks
>>>> lock_controller function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + req[0] = 0x9f;
>>>> + ret = io_protocol->transaction(io_protocol,
>>>> + SPI_TRANSACTION_FULL_DUPLEX,
>>>> + false, 0, 1, 8,
>>>> + sizeof(req), req,
>>>> + sizeof(resp), resp);
>>>> + if (ret != EFI_SUCCESS) {
>>>> + efi_st_error("SPI transaction failed\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if ((resp[0] != 0xff) || (resp[1] != 0x20) || (resp[2] != 0x20)
>>>> || (resp[3] != 0x15)) {
>>>> + efi_st_error("Incorrect response from sandbox SPI flash
>>>> emulator\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + return EFI_ST_SUCCESS;
>>>> +}
>>>> +
>>>> +static int test_bus(struct efi_spi_bus *bus)
>>>> +{
>>>> + struct efi_spi_peripheral *p;
>>>> + int status;
>>>> +
>>>> + if (!bus->friendly_name) {
>>>> + efi_st_error("SPI bus lacks a friendly name\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!bus->peripheral_list) {
>>>> + efi_st_error("SPI bus has zero peripherals\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!bus->clock) {
>>>> + efi_st_error("SPI bus lacks clock function\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + for (p = bus->peripheral_list; p; p = p->next_spi_peripheral) {
>>>> + status = test_peripheral(p, bus);
>>>> + if (status) {
>>>> + efi_st_error("Failed testing SPI peripheral\n");
>>>> + return status;
>>>> + }
>>>> + }
>>>> +
>>>> + return EFI_ST_SUCCESS;
>>>> +}
>>>> +
>>>> +static int execute(void)
>>>> +{
>>>> + struct efi_spi_configuration_protocol *spi;
>>>> + efi_status_t ret;
>>>> + int status;
>>>> + u32 i;
>>>> +
>>>> + ret = boottime->locate_protocol(&efi_spi_configuration_guid,
>>>> + NULL, (void **)&spi);
>>>> + if (ret != EFI_SUCCESS) {
>>>> + efi_st_error("SPI configuration protocol not available\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + if (!spi->bus_count) {
>>>> + efi_st_error("SPI configuration protocol has zero busses\n");
>>>> + return EFI_ST_FAILURE;
>>>> + }
>>>> +
>>>> + for (i = 0; i < spi->bus_count; i++) {
>>>> + status = test_bus(spi->bus_list[i]);
>>>> + if (status) {
>>>> + efi_st_error("Failed testing SPI bus %d\n", i);
>>>> + return status;
>>>> + }
>>>> + }
>>>
>>> This test does not test anything changing any state of SPI flash. Why is
>>> this patch needed if that is all that is usable?
>>
>> The test here is confirming that io_protocol->transaction() works as
>> expected, without re-testing the u-boot spi driver model or sandbox
>> spi drivers as they should be already tested elsewhere.
>>
>> I could extend this to also test the SPI_TRANSACTION_WRITE_THEN_READ
>> transaction type. Testing the other transaction types
>> (SPI_TRANSACTION_WRITE_ONLY & SPI_TRANSACTION_READ_ONLY) would require
>> extending the sandbox spi drivers since there's not really a valid way
>> to use these with an emulated jedec flash device.
>
> Are you happy with this explanation?
>
>>
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>> +
>>>> + return EFI_ST_SUCCESS;
>>>> +}
>>>> +
>>>> +EFI_UNIT_TEST(spi_protocol) = {
>>>> + .name = "SPI protocol",
>>>> + .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
>>>> + .setup = setup,
>>>> + .execute = execute,
>>>> +};
>>>
>>
>
> Thanks,
>
More information about the U-Boot
mailing list