[PATCH v2 1/3] efi_loader: Add SPI I/O protocol support
Paul Barker
paul.barker at sancloud.com
Mon Sep 12 18:14:19 CEST 2022
Hi Heinrich,
I'm sending a ping on this as there were a few questions in my response
to your feedback and I'd like to move this forward. I've highlighted the
key items with further comments below.
On 25/08/2022 11:58, Paul Barker wrote:
> Hi Henrich,
>
> Many thanks for your review on this patch. I've responded to things
> inline below. I'm also available on IRC (paulbarker) if you want to
> discuss further.
>
> On 13/08/2022 20:20, Heinrich Schuchardt wrote:
>> On 8/4/22 12:34, Paul Barker wrote:
>>> This addition allows UEFI applications running under u-boot to access
>>> peripherals on SPI busses. It is based on the UEFI Platform
>>> Initialization (PI) Specification, Version 1.7 Errata A (April 2020).
>>> Only the core functionality required to discover SPI peripherals and
>>> communicate with them is currently implemented. Other functionality such
>>> as the legacy SPI controller interface and the ability to update the SPI
>>> peripheral object associated with a particular SPI I/O protocol object
>>> is currently unimplemented.
>>>
>>> Since there are no open source implementations of this protocol to use
>>> as an example, educated guesses/hacks have been made in cases where the
>>> UEFI PI specification is unclear and these are documented in comments.
>>>
>>> This implementation has been tested on the SanCloud BBE Lite and allowed
>>> a UEFI test application to successfully communicate with a Micron
>>> Authenta flash device connected via the SPI bus. It has also been tested
>>> with the sandbox target using the included efi_selftest case.
>>
>> The commit message should describe which protocols are actually
>> implemented.
>
> I'll add this to the commit message in v3.
>
>>
>> I still have no clue why any PI protocol should be implemented U-Boot.
>> You neither descibe it here nor in the cover-letter.
>
> Implementing the SPI I/O protocol allows a UEFI application running
> under u-boot to enumerate and communicate with devices on the SPI bus.
> This protocol fulfils the same purpose as the "SD MMC Pass Thru
> Protocol" and "NVM Express Pass Through Protocol" described in the main
> UEFI spec, I'm unsure why SPI bus access was deferred to the PI spec. If
> these patches are accepted we plan to also implement the SD/MMC and NVMe
> I/O protocols.
>
> The broader goal here is to support better encapsulation of "pre-boot"
> application code which needs to communicate with a SPI, MMC or NVMe
> devices before Linux is booted. This pre-boot application can then run
> on both embedded devices running u-boot and PC-like devices running a
> UEFI BIOS. In our case the application will interact with the Authenta
> flash security features to verify integrity and to generate ephemeral
> keys based on the flash contents. Our application links against mbedtls
> and so it would be non-trivial (in terms of both implementation and
> licensing) to integrate this directly into u-boot.
>
> We have previously explored using the "standalone" application support
> (e.g. in examples/standalone in the u-boot source) but as that is not
> well maintained we have moved to a UEFI implementation.
>
> Let me know if you need any more info than that. Once you're happy with
> the explanation I'll add it to the cover letter in v3.
Could you let me know if this is sufficient?
>
>>
>> I would expect capsule updates to be used to update the SPI flash.
>
> Our application is not intended to perform firmware or capsule updates
> or to make any writes to the flash.
>
>>
>>>
>>> Signed-off-by: Paul Barker <paul.barker 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,
--
Paul Barker
Principal Software Engineer
SanCloud Ltd
e: paul.barker at sancloud.com
w: https://sancloud.com/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xA67255DFCCE62ECD.asc
Type: application/pgp-keys
Size: 7526 bytes
Desc: OpenPGP public key
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20220912/09b65faf/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20220912/09b65faf/attachment.sig>
More information about the U-Boot
mailing list