Re: [PATCH v5 3/3] arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export
On 03/01/2023 19:27, Rob Herring wrote: > On Sat, Dec 24, 2022 at 6:04 AM Paul Barker wrote: >> The EFI_SPI_PART structure has "Vendor" and "PartNumber" fields. We need >> something to put in those fields and the device tree is the best place >> to store the data. These properties are in the `-u-boot.dtsi` file so >> they won't be submitted to the Linux kernel. > > Can't you just split the `micron,mt25ql128abb` compatible string to > populate those? Yes, we can go that route. I'll prepare a patch to add `micron,mt25ql128` to the compatibile strings for this node and submit that to both the kernel & u-boot. Then I'll add code to split that into vendor & part number at runtime. >>> Why is this u-boot specific? Another UEFI implementation doesn't need >>> the GUID? >> >> Each EFI_SPI_IO_PROTOCOL instance needs its own GUID so that it can >> be located at runtime. These GUIDs are not pre-defined by the spec, >> instead an arbitrary per-device GUID is used and is stored in the >> SpiPeripheralDriverGuid field of the relevant EFI_SPI_PERIPHERAL >> instance. We could randomly generate these GUIDs at runtime since a UEFI >> application can walk the tree of SPI busses and peripherals, looking >> for a match in the Vendor and PartNumber fields defined above, to get >> the appropriate GUID. However, storing a known value in the device tree >> allows a UEFI application to just lookup the GUID without needing to >> walk the SPI tree. > > Okay, then I guess my next question is why is it SPI IO protocol > specific? I'd assume the UEFI spec would use this for any h/w > protocol. Sadly, common sense assumptions and the UEFI spec do not go together. There at least 3 different ways this is handled in UEFI: * For SD/MMC, the UEFI spec defines a single GUID for the I/O protocol and the API takes a slot number to determine which actual device to operate on. The `This` argument is assumed to point to a global instance of EFI_SD_MMC_PASS_THRU_PROTOCOL. * For NVMe, it's the same as above but with a namespace ID instead of a slot number. * For ATA, PCI, SCSI, USB, etc, it looks to be a similar story. * For I2C, there is a fixed GUID for the I/O protocol but the API takes an additional GUID to identify the device to operate on. So a unique GUID must be allocated for each device, but only one instance of EFI_I2C_IO_PROTOCOL is needed. * For SPI, the I/O protocol has no fixed GUID and it's expected that there is a separate EFI_SPI_IO_PROTOCOL instance with a unique GUID for each device a the SPI bus. The API does not take any arguments to identify the SPI bus or chip select, instead the `This` argument points to the specific instance of EFI_SPI_IO_PROTOCOL for the device we want to operate on. There is a separate protocol, EFI_SPI_CONFIGURATION_PROTOCOL, with a fixed GUID, that allows the SPI bus to be walked. > Generating GUIDs at runtime seems like the right way though. We simply > can't populate DT with every client's method of identifying a given > instance. That simply doesn't scale. Ok. We can re-write our application code to walk the SPI bus nodes looking for the appropriate Vendor & PartNumber pair, then lookup the GUID for that device. That will allow us to generate the GUID dynamically at runtime. We will then be relying solely on the compatible property in the device tree to fill in the Vendor & PartNumber fields. > Rob Thanks for your feedback Rob. It's been good to see another perspective on this. -- Paul Barker Principal Software Engineer SanCloud Ltd e: paul.bar...@sancloud.com w: https://sancloud.com/
Re: [PATCH v5 3/3] arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export
On Sat, Dec 24, 2022 at 6:04 AM Paul Barker wrote:
>
> On 20/12/2022 15:55, Rob Herring wrote:
> > On Wed, Nov 23, 2022 at 05:50:06PM +, 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
> >> ---
> >> 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 @@
> >>
> >> {
> >> u-boot,dm-pre-reloc;
> >> -channel@0 {
> >> -u-boot,dm-pre-reloc;
> >> -};
> >> +};
> >> +
> >> +_flash {
> >> +u-boot,dm-pre-reloc;
> >> +
> >> +u-boot,uefi-spi-vendor = "micron";
> >> +u-boot,uefi-spi-part-number = "mt25ql128abb";
> >
> > Looks like a compatible string. Yet, the flash node compatible string,
> > micron,spi-authenta, is not documented (though in use for spidev). So
> > use a compatible string for the flash that is specific to the flash
> > model. I assume there is some reason the specific model is needed?
>
> For context, the UEFI Platform Initialization (PI) spec defines
> EFI_SPI_PART, EFI_SPI_PERIPHERAL and EFI_SPI_IO_PROTOCOL structures.
> I'm referencing v1.7 Errata A. See https://uefi.org/specifications for
> downloads.
>
> The EFI_SPI_PART structure has "Vendor" and "PartNumber" fields. We need
> something to put in those fields and the device tree is the best place
> to store the data. These properties are in the `-u-boot.dtsi` file so
> they won't be submitted to the Linux kernel.
Can't you just split the `micron,mt25ql128abb` compatible string to
populate those?
> The `micron,spi-authenta` compatible string is unfortunate in my
> opinion. I plan to replace this with two entries, `micron,mt25ql128abb`
> and `jedec,spi-nor`, once we've got MTD support for the Authenta parts
> merged into the mainline kernel. That work is currently on hold, I'll
> be working with Micron in the new year to unblock it. We could submit
> a change to the compatible property in the meantime if you think it's
> worth getting a temporary improvement in before the MTD changes are
> done.
No need to wait on driver changes to add compatible strings. Anyone
that says otherwise is wrong.
Looking at the datasheet, maybe `micron,mt25ql128abb` is too specific.
Perhaps 'abb' or just the last 'b' can be dropped? It's a trade off of
lots of compatible strings vs. whether you might need to distinguish
the parts (only before you do jedec discovery).
>
> >> +/* GUID in UEFI format: 77126730-a4ca-4386-b341-881fe18e7f7d */
> >> +u-boot,uefi-spi-io-guid = [30 67 12 77 ca a4 86 43
> >> + b3 41 88 1f e1 8e 7f 7d];
> >
> > We need to define first in the DT spec the format for GUIDs. I don't
> > think there are any existing cases though some have been proposed. IMO,
> > I would make this a string instead. The byte array is not that
> > readable with its little endian order. A GUID as a string is readily
> > identifiable as a GUID.
>
> I see there is a `uuid_str_to_bin()` function in u-boot so I could make
> this a string property and convert it to a binary GUID at runtime. This
> would make the dts more readable so I'll make the change for v6 of this
> series.
>
> > Why is this u-boot specific? Another UEFI implementation doesn't need
> > the GUID?
>
> Each EFI_SPI_IO_PROTOCOL instance needs its own GUID so that it can
> be located at runtime. These GUIDs are not pre-defined by the spec,
> instead an arbitrary per-device GUID is used and is stored in the
> SpiPeripheralDriverGuid field of the relevant EFI_SPI_PERIPHERAL
> instance. We could randomly generate these GUIDs at runtime since a UEFI
> application can walk the tree of SPI busses and peripherals, looking
> for a match in the Vendor and PartNumber fields defined above, to get
> the appropriate GUID. However, storing a known value in the device tree
> allows a UEFI application to just lookup the GUID without needing to
> walk the SPI tree.
Okay, then I guess my next question is why is it SPI IO protocol
specific? I'd assume the UEFI spec would use this for any h/w
protocol.
Generating GUIDs at runtime seems like the right way though. We simply
can't populate DT with every client's method of identifying a given
instance. That simply doesn't scale.
Rob
Re: [PATCH v5 3/3] arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export
On Sat, Dec 24, 2022 at 12:03:52PM +, Paul Barker wrote:
> On 20/12/2022 15:55, Rob Herring wrote:
> > On Wed, Nov 23, 2022 at 05:50:06PM +, 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
> >> ---
> >> 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 @@
> >>
> >> {
> >>u-boot,dm-pre-reloc;
> >> - channel@0 {
> >> - u-boot,dm-pre-reloc;
> >> - };
> >> +};
> >> +
> >> +_flash {
> >> + u-boot,dm-pre-reloc;
> >> +
> >> + u-boot,uefi-spi-vendor = "micron";
> >> + u-boot,uefi-spi-part-number = "mt25ql128abb";
> >
> > Looks like a compatible string. Yet, the flash node compatible string,
> > micron,spi-authenta, is not documented (though in use for spidev). So
> > use a compatible string for the flash that is specific to the flash
> > model. I assume there is some reason the specific model is needed?
>
> For context, the UEFI Platform Initialization (PI) spec defines
> EFI_SPI_PART, EFI_SPI_PERIPHERAL and EFI_SPI_IO_PROTOCOL structures.
> I'm referencing v1.7 Errata A. See https://uefi.org/specifications for
> downloads.
>
> The EFI_SPI_PART structure has "Vendor" and "PartNumber" fields. We need
> something to put in those fields and the device tree is the best place
> to store the data. These properties are in the `-u-boot.dtsi` file so
> they won't be submitted to the Linux kernel.
Well, IMHO, this doesn't belong in U-Boot only for forever. Just like
other bindings/properties that we're working on getting merged upstream
and so that there is really Just One Device Tree, this should go
upstream I believe. This might be another case of starts in -u-boot.dtsi
while things get sorted out.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v5 3/3] arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export
On 20/12/2022 15:55, Rob Herring wrote:
> On Wed, Nov 23, 2022 at 05:50:06PM +, 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
>> ---
>> 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 @@
>>
>> {
>> u-boot,dm-pre-reloc;
>> -channel@0 {
>> -u-boot,dm-pre-reloc;
>> -};
>> +};
>> +
>> +_flash {
>> +u-boot,dm-pre-reloc;
>> +
>> +u-boot,uefi-spi-vendor = "micron";
>> +u-boot,uefi-spi-part-number = "mt25ql128abb";
>
> Looks like a compatible string. Yet, the flash node compatible string,
> micron,spi-authenta, is not documented (though in use for spidev). So
> use a compatible string for the flash that is specific to the flash
> model. I assume there is some reason the specific model is needed?
For context, the UEFI Platform Initialization (PI) spec defines
EFI_SPI_PART, EFI_SPI_PERIPHERAL and EFI_SPI_IO_PROTOCOL structures.
I'm referencing v1.7 Errata A. See https://uefi.org/specifications for
downloads.
The EFI_SPI_PART structure has "Vendor" and "PartNumber" fields. We need
something to put in those fields and the device tree is the best place
to store the data. These properties are in the `-u-boot.dtsi` file so
they won't be submitted to the Linux kernel.
The `micron,spi-authenta` compatible string is unfortunate in my
opinion. I plan to replace this with two entries, `micron,mt25ql128abb`
and `jedec,spi-nor`, once we've got MTD support for the Authenta parts
merged into the mainline kernel. That work is currently on hold, I'll
be working with Micron in the new year to unblock it. We could submit
a change to the compatible property in the meantime if you think it's
worth getting a temporary improvement in before the MTD changes are
done.
>> +/* GUID in UEFI format: 77126730-a4ca-4386-b341-881fe18e7f7d */
>> +u-boot,uefi-spi-io-guid = [30 67 12 77 ca a4 86 43
>> + b3 41 88 1f e1 8e 7f 7d];
>
> We need to define first in the DT spec the format for GUIDs. I don't
> think there are any existing cases though some have been proposed. IMO,
> I would make this a string instead. The byte array is not that
> readable with its little endian order. A GUID as a string is readily
> identifiable as a GUID.
I see there is a `uuid_str_to_bin()` function in u-boot so I could make
this a string property and convert it to a binary GUID at runtime. This
would make the dts more readable so I'll make the change for v6 of this
series.
> Why is this u-boot specific? Another UEFI implementation doesn't need
> the GUID?
Each EFI_SPI_IO_PROTOCOL instance needs its own GUID so that it can
be located at runtime. These GUIDs are not pre-defined by the spec,
instead an arbitrary per-device GUID is used and is stored in the
SpiPeripheralDriverGuid field of the relevant EFI_SPI_PERIPHERAL
instance. We could randomly generate these GUIDs at runtime since a UEFI
application can walk the tree of SPI busses and peripherals, looking
for a match in the Vendor and PartNumber fields defined above, to get
the appropriate GUID. However, storing a known value in the device tree
allows a UEFI application to just lookup the GUID without needing to
walk the SPI tree.
I'm happy to consider alternatives to the current implementation,
however we need to work within the limits of the UEFI PI spec.
Thanks,
--
Paul Barker
Principal Software Engineer
SanCloud Ltd
e: paul.bar...@sancloud.com
w: https://sancloud.com/
Re: [PATCH v5 3/3] arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export
On Wed, Nov 23, 2022 at 05:50:06PM +, 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
> ---
> 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 @@
>
> {
> u-boot,dm-pre-reloc;
> - channel@0 {
> - u-boot,dm-pre-reloc;
> - };
> +};
> +
> +_flash {
> + u-boot,dm-pre-reloc;
> +
> + u-boot,uefi-spi-vendor = "micron";
> + u-boot,uefi-spi-part-number = "mt25ql128abb";
Looks like a compatible string. Yet, the flash node compatible string,
micron,spi-authenta, is not documented (though in use for spidev). So
use a compatible string for the flash that is specific to the flash
model. I assume there is some reason the specific model is needed?
> + /* GUID in UEFI format: 77126730-a4ca-4386-b341-881fe18e7f7d */
> + u-boot,uefi-spi-io-guid = [30 67 12 77 ca a4 86 43
> +b3 41 88 1f e1 8e 7f 7d];
We need to define first in the DT spec the format for GUIDs. I don't
think there are any existing cases though some have been proposed. IMO,
I would make this a string instead. The byte array is not that
readable with its little endian order. A GUID as a string is readily
identifiable as a GUID.
Why is this u-boot specific? Another UEFI implementation doesn't need
the GUID?
Rob
[PATCH v5 3/3] arm: dts: am335x-sancloud-bbe-lite: UEFI SPI export
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
---
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 @@
{
u-boot,dm-pre-reloc;
- channel@0 {
- u-boot,dm-pre-reloc;
- };
+};
+
+_flash {
+ u-boot,dm-pre-reloc;
+
+ u-boot,uefi-spi-vendor = "micron";
+ u-boot,uefi-spi-part-number = "mt25ql128abb";
+ /* GUID in UEFI format: 77126730-a4ca-4386-b341-881fe18e7f7d */
+ u-boot,uefi-spi-io-guid = [30 67 12 77 ca a4 86 43
+ b3 41 88 1f e1 8e 7f 7d];
};
diff --git a/arch/arm/dts/am335x-sancloud-bbe-lite.dts
b/arch/arm/dts/am335x-sancloud-bbe-lite.dts
index 8ffbc72dc57e..3909092a140f 100644
--- a/arch/arm/dts/am335x-sancloud-bbe-lite.dts
+++ b/arch/arm/dts/am335x-sancloud-bbe-lite.dts
@@ -37,7 +37,7 @@
pinctrl-names = "default";
pinctrl-0 = <_spi0_pins>;
- channel@0 {
+ authenta_flash: channel@0 {
#address-cells = <1>;
#size-cells = <0>;
--
2.25.1
