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

[Rob Herring <robh@kernel.org>]

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

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


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

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

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

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

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

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

Rob