[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