diff mbox series

[v1] board: mpfs_icicle: implement board_fdt_blob_setup()

Message ID 20240625090806.1787287-2-conor.dooley@microchip.com
State New
Delegated to: Andes
Headers show
Series [v1] board: mpfs_icicle: implement board_fdt_blob_setup() | expand

Commit Message

Conor Dooley June 25, 2024, 9:08 a.m. UTC
The firmware on the Icicle is capable of providing a devicetree in a1 to
U-Boot, but until now the devicetree has been packaged in a "payload" [1]
alongside U-Boot (or other bootloaders/RTOSes) and appended to the image.
The address of this appended devicetree is placed in a1 by the firmware.
This meant that the mechanism used by OF_SEPARATE to locate the
devicetree at the end of the image would pick up the one provided by the
firmware when u-boot-nodtb.bin was in the payload and U-Boot's devicetree
when u-boot.bin was.

The firmware is now going to be capable of providing a minimal devicetree
(quite cut down due to severe space constraints), but this devicetree is
linked into the firmware that runs out of the L2 rather than at the end
of the U-Boot image. Implement board_fdt_blob_setup() so that this
devicetree can be optionally used, and the devicetree provided in the
"payload" can be used without relying on "happening" to implement the
same strategy as OF_SEPARATE expects in combination with
u-boot-nodtb.bin. Unlike other RISC-V boards, the firmware provided
devicetree is only used when OF_BOARD is set, so that the almost
certainly more complete devicetree in U-Boot will be used unless
explicitly requested otherwise.

Link: https://github.com/polarfire-soc/hart-software-services/blob/master/tools/hss-payload-generator/README.md [1]
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
CC: Ivan Griffin <ivan.griffin@microchip.com>
CC: Padmarao Begari <padmarao.begari@microchip.com>
CC: Cyril Jean <cyril.jean@microchip.com>
CC: Tom Rini <trini@konsulko.com>
CC: Conor Dooley <conor.dooley@microchip.com>
CC: u-boot@lists.denx.de
---
 board/microchip/mpfs_icicle/mpfs_icicle.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Tom Rini June 25, 2024, 2:34 p.m. UTC | #1
On Tue, Jun 25, 2024 at 10:08:06AM +0100, Conor Dooley wrote:

> The firmware on the Icicle is capable of providing a devicetree in a1 to
> U-Boot, but until now the devicetree has been packaged in a "payload" [1]
> alongside U-Boot (or other bootloaders/RTOSes) and appended to the image.
> The address of this appended devicetree is placed in a1 by the firmware.
> This meant that the mechanism used by OF_SEPARATE to locate the
> devicetree at the end of the image would pick up the one provided by the
> firmware when u-boot-nodtb.bin was in the payload and U-Boot's devicetree
> when u-boot.bin was.
> 
> The firmware is now going to be capable of providing a minimal devicetree
> (quite cut down due to severe space constraints), but this devicetree is
> linked into the firmware that runs out of the L2 rather than at the end
> of the U-Boot image. Implement board_fdt_blob_setup() so that this
> devicetree can be optionally used, and the devicetree provided in the
> "payload" can be used without relying on "happening" to implement the
> same strategy as OF_SEPARATE expects in combination with
> u-boot-nodtb.bin. Unlike other RISC-V boards, the firmware provided
> devicetree is only used when OF_BOARD is set, so that the almost
> certainly more complete devicetree in U-Boot will be used unless
> explicitly requested otherwise.
> 
> Link: https://github.com/polarfire-soc/hart-software-services/blob/master/tools/hss-payload-generator/README.md [1]
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> CC: Ivan Griffin <ivan.griffin@microchip.com>
> CC: Padmarao Begari <padmarao.begari@microchip.com>
> CC: Cyril Jean <cyril.jean@microchip.com>
> CC: Tom Rini <trini@konsulko.com>
> CC: Conor Dooley <conor.dooley@microchip.com>
> CC: u-boot@lists.denx.de
> ---
>  board/microchip/mpfs_icicle/mpfs_icicle.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/board/microchip/mpfs_icicle/mpfs_icicle.c b/board/microchip/mpfs_icicle/mpfs_icicle.c
> index 4d7d843dfa3..2c1f7175f0e 100644
> --- a/board/microchip/mpfs_icicle/mpfs_icicle.c
> +++ b/board/microchip/mpfs_icicle/mpfs_icicle.c
> @@ -9,6 +9,7 @@
>  #include <init.h>
>  #include <asm/global_data.h>
>  #include <asm/io.h>
> +#include <asm/sections.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -50,6 +51,24 @@ static void read_device_serial_number(u8 *response, u8 response_size)
>  		response_buf[idx] = readb(MPFS_SYS_SERVICE_MAILBOX + idx);
>  }
>  
> +void *board_fdt_blob_setup(int *err)
> +{
> +	*err = 0;
> +	/*
> +	 * The devicetree provided by the previous stage is very minimal due to
> +	 * severe space constraints. The firmware performs no fixups etc.
> +	 * U-Boot, if providing a devicetree, almost certainly has a better
> +	 * more complete one than the firmware so that provided by the firmware
> +	 * is ignored for OF_SEPARATE.
> +	 */
> +	if (IS_ENABLED(CONFIG_OF_BOARD)) {
> +		if (gd->arch.firmware_fdt_addr)
> +			return (ulong *)(uintptr_t)gd->arch.firmware_fdt_addr;
> +	}
> +
> +	return (ulong *)_end;
> +}
> +
>  int board_init(void)
>  {
>  	/* For now nothing to do here. */

I'm adding in Simon and Ilias as this touches on one of those frequent
topics about how device trees can/should be passed along to us.
Simon Glass June 27, 2024, 8:36 a.m. UTC | #2
Hi,

On Tue, 25 Jun 2024 at 15:34, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jun 25, 2024 at 10:08:06AM +0100, Conor Dooley wrote:
>
> > The firmware on the Icicle is capable of providing a devicetree in a1 to
> > U-Boot, but until now the devicetree has been packaged in a "payload" [1]
> > alongside U-Boot (or other bootloaders/RTOSes) and appended to the image.
> > The address of this appended devicetree is placed in a1 by the firmware.
> > This meant that the mechanism used by OF_SEPARATE to locate the
> > devicetree at the end of the image would pick up the one provided by the
> > firmware when u-boot-nodtb.bin was in the payload and U-Boot's devicetree
> > when u-boot.bin was.
> >
> > The firmware is now going to be capable of providing a minimal devicetree
> > (quite cut down due to severe space constraints), but this devicetree is
> > linked into the firmware that runs out of the L2 rather than at the end
> > of the U-Boot image. Implement board_fdt_blob_setup() so that this
> > devicetree can be optionally used, and the devicetree provided in the
> > "payload" can be used without relying on "happening" to implement the
> > same strategy as OF_SEPARATE expects in combination with
> > u-boot-nodtb.bin. Unlike other RISC-V boards, the firmware provided
> > devicetree is only used when OF_BOARD is set, so that the almost
> > certainly more complete devicetree in U-Boot will be used unless
> > explicitly requested otherwise.
> >
> > Link: https://github.com/polarfire-soc/hart-software-services/blob/master/tools/hss-payload-generator/README.md [1]
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> > CC: Ivan Griffin <ivan.griffin@microchip.com>
> > CC: Padmarao Begari <padmarao.begari@microchip.com>
> > CC: Cyril Jean <cyril.jean@microchip.com>
> > CC: Tom Rini <trini@konsulko.com>
> > CC: Conor Dooley <conor.dooley@microchip.com>
> > CC: u-boot@lists.denx.de
> > ---
> >  board/microchip/mpfs_icicle/mpfs_icicle.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/board/microchip/mpfs_icicle/mpfs_icicle.c b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > index 4d7d843dfa3..2c1f7175f0e 100644
> > --- a/board/microchip/mpfs_icicle/mpfs_icicle.c
> > +++ b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > @@ -9,6 +9,7 @@
> >  #include <init.h>
> >  #include <asm/global_data.h>
> >  #include <asm/io.h>
> > +#include <asm/sections.h>
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -50,6 +51,24 @@ static void read_device_serial_number(u8 *response, u8 response_size)
> >               response_buf[idx] = readb(MPFS_SYS_SERVICE_MAILBOX + idx);
> >  }
> >
> > +void *board_fdt_blob_setup(int *err)
> > +{
> > +     *err = 0;
> > +     /*
> > +      * The devicetree provided by the previous stage is very minimal due to
> > +      * severe space constraints. The firmware performs no fixups etc.
> > +      * U-Boot, if providing a devicetree, almost certainly has a better
> > +      * more complete one than the firmware so that provided by the firmware
> > +      * is ignored for OF_SEPARATE.
> > +      */
> > +     if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > +             if (gd->arch.firmware_fdt_addr)
> > +                     return (ulong *)(uintptr_t)gd->arch.firmware_fdt_addr;
> > +     }
> > +
> > +     return (ulong *)_end;
> > +}
> > +
> >  int board_init(void)
> >  {
> >       /* For now nothing to do here. */
>
> I'm adding in Simon and Ilias as this touches on one of those frequent
> topics about how device trees can/should be passed along to us.

The only thing I can think of is implementing bloblist in  the a1 (?)
firmware, then passing the DT in that.

It seems that you still need to be able to turn that on and off in
U-Boot though. So far we have not agreed the mechanism to do that, I
have the same problem, with a pending patch here[1]

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20240626155945.278640-12-sjg@chromium.org/
Conor Dooley June 27, 2024, 9:38 a.m. UTC | #3
On Thu, Jun 27, 2024 at 09:36:49AM +0100, Simon Glass wrote:
> 
> On Tue, 25 Jun 2024 at 15:34, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 10:08:06AM +0100, Conor Dooley wrote:
> >
> > > The firmware on the Icicle is capable of providing a devicetree in a1 to
> > > U-Boot, but until now the devicetree has been packaged in a "payload" [1]
> > > alongside U-Boot (or other bootloaders/RTOSes) and appended to the image.
> > > The address of this appended devicetree is placed in a1 by the firmware.
> > > This meant that the mechanism used by OF_SEPARATE to locate the
> > > devicetree at the end of the image would pick up the one provided by the
> > > firmware when u-boot-nodtb.bin was in the payload and U-Boot's devicetree
> > > when u-boot.bin was.
> > >
> > > The firmware is now going to be capable of providing a minimal devicetree
> > > (quite cut down due to severe space constraints), but this devicetree is
> > > linked into the firmware that runs out of the L2 rather than at the end
> > > of the U-Boot image. Implement board_fdt_blob_setup() so that this
> > > devicetree can be optionally used, and the devicetree provided in the
> > > "payload" can be used without relying on "happening" to implement the
> > > same strategy as OF_SEPARATE expects in combination with
> > > u-boot-nodtb.bin. Unlike other RISC-V boards, the firmware provided
> > > devicetree is only used when OF_BOARD is set, so that the almost
> > > certainly more complete devicetree in U-Boot will be used unless
> > > explicitly requested otherwise.
> > >
> > > Link: https://github.com/polarfire-soc/hart-software-services/blob/master/tools/hss-payload-generator/README.md [1]
> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > ---
> > > CC: Ivan Griffin <ivan.griffin@microchip.com>
> > > CC: Padmarao Begari <padmarao.begari@microchip.com>
> > > CC: Cyril Jean <cyril.jean@microchip.com>
> > > CC: Tom Rini <trini@konsulko.com>
> > > CC: Conor Dooley <conor.dooley@microchip.com>
> > > CC: u-boot@lists.denx.de
> > > ---
> > >  board/microchip/mpfs_icicle/mpfs_icicle.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/board/microchip/mpfs_icicle/mpfs_icicle.c b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > index 4d7d843dfa3..2c1f7175f0e 100644
> > > --- a/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > +++ b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > @@ -9,6 +9,7 @@
> > >  #include <init.h>
> > >  #include <asm/global_data.h>
> > >  #include <asm/io.h>
> > > +#include <asm/sections.h>
> > >
> > >  DECLARE_GLOBAL_DATA_PTR;
> > >
> > > @@ -50,6 +51,24 @@ static void read_device_serial_number(u8 *response, u8 response_size)
> > >               response_buf[idx] = readb(MPFS_SYS_SERVICE_MAILBOX + idx);
> > >  }
> > >
> > > +void *board_fdt_blob_setup(int *err)
> > > +{
> > > +     *err = 0;
> > > +     /*
> > > +      * The devicetree provided by the previous stage is very minimal due to
> > > +      * severe space constraints. The firmware performs no fixups etc.
> > > +      * U-Boot, if providing a devicetree, almost certainly has a better
> > > +      * more complete one than the firmware so that provided by the firmware
> > > +      * is ignored for OF_SEPARATE.
> > > +      */
> > > +     if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > +             if (gd->arch.firmware_fdt_addr)
> > > +                     return (ulong *)(uintptr_t)gd->arch.firmware_fdt_addr;
> > > +     }
> > > +
> > > +     return (ulong *)_end;
> > > +}
> > > +
> > >  int board_init(void)
> > >  {
> > >       /* For now nothing to do here. */
> >
> > I'm adding in Simon and Ilias as this touches on one of those frequent
> > topics about how device trees can/should be passed along to us.
> 
> The only thing I can think of is implementing bloblist in  the a1 (?)
> firmware, then passing the DT in that.

a1 is the register that is used on riscv to pass the dtb, I think the
corresponding thing on arm64 is x0.

Re-reading the firware handoff spec, it's difficult to see what benefits
it actually provides us when we only ever have a single dtb which the
firmware does not interact with/use.
We are super space constrained in the firmware even carving out 4.5 KiB
for a devicetree blob is a stretch and requires disabling other features
and ripping out anything in the DT not required for U-Boot to load the OS.
Even the ~1 KiB mentioned in bloblist.h for handling a bloblist would be a
challenge.

I think the only way a bloblist could work is if it was created at build
time and linked into the firmware, since the on-disk format seems pretty
minimal. Is there tooling for generating a bloblist at build time that I
could use to check whether or not a bloblist is viable? 
I'd also have to investigate how that would interact with OpenSBI, since
it's integrated into the firmware and involved with loading U-Boot.

> It seems that you still need to be able to turn that on and off in
> U-Boot though. So far we have not agreed the mechanism to do that, I
> have the same problem, with a pending patch here[1]

It seems your patch is trying to do some runtime determination of
whether to examine the bloblist or not, but the ?existing? build-time
check for BLOBLIST being enabled would work equally well/poorly as the
OF_BOARD check the code I am adding. I'm not even really sure what runtime
option could be used here here to check if the passed dtb/bloblist was to
be used. U-Boot only runs here as supervisor mode U-Boot proper and always
has a more complete devicetree. Whether to use the one passed to U-Boot
just depends on what the person with the board wants to do - which, given
this is an FPGA, could be vary significantly.

Cheers,
Conor.
Simon Glass June 27, 2024, 10:50 a.m. UTC | #4
Hi Connor,

On Thu, 27 Jun 2024 at 10:38, Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Thu, Jun 27, 2024 at 09:36:49AM +0100, Simon Glass wrote:
> >
> > On Tue, 25 Jun 2024 at 15:34, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Jun 25, 2024 at 10:08:06AM +0100, Conor Dooley wrote:
> > >
> > > > The firmware on the Icicle is capable of providing a devicetree in a1 to
> > > > U-Boot, but until now the devicetree has been packaged in a "payload" [1]
> > > > alongside U-Boot (or other bootloaders/RTOSes) and appended to the image.
> > > > The address of this appended devicetree is placed in a1 by the firmware.
> > > > This meant that the mechanism used by OF_SEPARATE to locate the
> > > > devicetree at the end of the image would pick up the one provided by the
> > > > firmware when u-boot-nodtb.bin was in the payload and U-Boot's devicetree
> > > > when u-boot.bin was.
> > > >
> > > > The firmware is now going to be capable of providing a minimal devicetree
> > > > (quite cut down due to severe space constraints), but this devicetree is
> > > > linked into the firmware that runs out of the L2 rather than at the end
> > > > of the U-Boot image. Implement board_fdt_blob_setup() so that this
> > > > devicetree can be optionally used, and the devicetree provided in the
> > > > "payload" can be used without relying on "happening" to implement the
> > > > same strategy as OF_SEPARATE expects in combination with
> > > > u-boot-nodtb.bin. Unlike other RISC-V boards, the firmware provided
> > > > devicetree is only used when OF_BOARD is set, so that the almost
> > > > certainly more complete devicetree in U-Boot will be used unless
> > > > explicitly requested otherwise.
> > > >
> > > > Link: https://github.com/polarfire-soc/hart-software-services/blob/master/tools/hss-payload-generator/README.md [1]
> > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > > ---
> > > > CC: Ivan Griffin <ivan.griffin@microchip.com>
> > > > CC: Padmarao Begari <padmarao.begari@microchip.com>
> > > > CC: Cyril Jean <cyril.jean@microchip.com>
> > > > CC: Tom Rini <trini@konsulko.com>
> > > > CC: Conor Dooley <conor.dooley@microchip.com>
> > > > CC: u-boot@lists.denx.de
> > > > ---
> > > >  board/microchip/mpfs_icicle/mpfs_icicle.c | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/board/microchip/mpfs_icicle/mpfs_icicle.c b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > index 4d7d843dfa3..2c1f7175f0e 100644
> > > > --- a/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > +++ b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > @@ -9,6 +9,7 @@
> > > >  #include <init.h>
> > > >  #include <asm/global_data.h>
> > > >  #include <asm/io.h>
> > > > +#include <asm/sections.h>
> > > >
> > > >  DECLARE_GLOBAL_DATA_PTR;
> > > >
> > > > @@ -50,6 +51,24 @@ static void read_device_serial_number(u8 *response, u8 response_size)
> > > >               response_buf[idx] = readb(MPFS_SYS_SERVICE_MAILBOX + idx);
> > > >  }
> > > >
> > > > +void *board_fdt_blob_setup(int *err)
> > > > +{
> > > > +     *err = 0;
> > > > +     /*
> > > > +      * The devicetree provided by the previous stage is very minimal due to
> > > > +      * severe space constraints. The firmware performs no fixups etc.
> > > > +      * U-Boot, if providing a devicetree, almost certainly has a better
> > > > +      * more complete one than the firmware so that provided by the firmware
> > > > +      * is ignored for OF_SEPARATE.
> > > > +      */
> > > > +     if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > > +             if (gd->arch.firmware_fdt_addr)
> > > > +                     return (ulong *)(uintptr_t)gd->arch.firmware_fdt_addr;
> > > > +     }
> > > > +
> > > > +     return (ulong *)_end;
> > > > +}
> > > > +
> > > >  int board_init(void)
> > > >  {
> > > >       /* For now nothing to do here. */
> > >
> > > I'm adding in Simon and Ilias as this touches on one of those frequent
> > > topics about how device trees can/should be passed along to us.
> >
> > The only thing I can think of is implementing bloblist in  the a1 (?)
> > firmware, then passing the DT in that.
>
> a1 is the register that is used on riscv to pass the dtb, I think the
> corresponding thing on arm64 is x0.
>
> Re-reading the firware handoff spec, it's difficult to see what benefits
> it actually provides us when we only ever have a single dtb which the
> firmware does not interact with/use.
> We are super space constrained in the firmware even carving out 4.5 KiB
> for a devicetree blob is a stretch and requires disabling other features
> and ripping out anything in the DT not required for U-Boot to load the OS.
> Even the ~1 KiB mentioned in bloblist.h for handling a bloblist would be a
> challenge.
>
> I think the only way a bloblist could work is if it was created at build
> time and linked into the firmware, since the on-disk format seems pretty
> minimal. Is there tooling for generating a bloblist at build time that I
> could use to check whether or not a bloblist is viable?
> I'd also have to investigate how that would interact with OpenSBI, since
> it's integrated into the firmware and involved with loading U-Boot.

There is not such a tool, but it would be easy enough to write. If you
think that would help, I could give it a try.

The confusing thing for me is that the 'firmware' is very
constrained...but are you saying that U-Boot is not? Why pass U-Boot
the DT? Perhaps you could explain the boot sequence so I can
understand it better?

>
> > It seems that you still need to be able to turn that on and off in
> > U-Boot though. So far we have not agreed the mechanism to do that, I
> > have the same problem, with a pending patch here[1]
>
> It seems your patch is trying to do some runtime determination of
> whether to examine the bloblist or not, but the ?existing? build-time
> check for BLOBLIST being enabled would work equally well/poorly as the
> OF_BOARD check the code I am adding. I'm not even really sure what runtime
> option could be used here here to check if the passed dtb/bloblist was to
> be used. U-Boot only runs here as supervisor mode U-Boot proper and always
> has a more complete devicetree. Whether to use the one passed to U-Boot
> just depends on what the person with the board wants to do - which, given
> this is an FPGA, could be vary significantly.

OK...so does this platform use SPL?

Regards,
Simon
Conor Dooley June 27, 2024, 8:27 p.m. UTC | #5
On Thu, Jun 27, 2024 at 11:50:33AM +0100, Simon Glass wrote:
> On Thu, 27 Jun 2024 at 10:38, Conor Dooley <conor.dooley@microchip.com> wrote:
> > On Thu, Jun 27, 2024 at 09:36:49AM +0100, Simon Glass wrote:
> > > On Tue, 25 Jun 2024 at 15:34, Tom Rini <trini@konsulko.com> wrote:
> > > > On Tue, Jun 25, 2024 at 10:08:06AM +0100, Conor Dooley wrote:
> > > > > The firmware on the Icicle is capable of providing a devicetree in a1 to
> > > > > U-Boot, but until now the devicetree has been packaged in a "payload" [1]
> > > > > alongside U-Boot (or other bootloaders/RTOSes) and appended to the image.
> > > > > The address of this appended devicetree is placed in a1 by the firmware.
> > > > > This meant that the mechanism used by OF_SEPARATE to locate the
> > > > > devicetree at the end of the image would pick up the one provided by the
> > > > > firmware when u-boot-nodtb.bin was in the payload and U-Boot's devicetree
> > > > > when u-boot.bin was.
> > > > >
> > > > > The firmware is now going to be capable of providing a minimal devicetree
> > > > > (quite cut down due to severe space constraints), but this devicetree is
> > > > > linked into the firmware that runs out of the L2 rather than at the end
> > > > > of the U-Boot image. Implement board_fdt_blob_setup() so that this
> > > > > devicetree can be optionally used, and the devicetree provided in the
> > > > > "payload" can be used without relying on "happening" to implement the
> > > > > same strategy as OF_SEPARATE expects in combination with
> > > > > u-boot-nodtb.bin. Unlike other RISC-V boards, the firmware provided
> > > > > devicetree is only used when OF_BOARD is set, so that the almost
> > > > > certainly more complete devicetree in U-Boot will be used unless
> > > > > explicitly requested otherwise.
> > > > >
> > > > > Link: https://github.com/polarfire-soc/hart-software-services/blob/master/tools/hss-payload-generator/README.md [1]
> > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > ---
> > > > > CC: Ivan Griffin <ivan.griffin@microchip.com>
> > > > > CC: Padmarao Begari <padmarao.begari@microchip.com>
> > > > > CC: Cyril Jean <cyril.jean@microchip.com>
> > > > > CC: Tom Rini <trini@konsulko.com>
> > > > > CC: Conor Dooley <conor.dooley@microchip.com>
> > > > > CC: u-boot@lists.denx.de
> > > > > ---
> > > > >  board/microchip/mpfs_icicle/mpfs_icicle.c | 19 +++++++++++++++++++
> > > > >  1 file changed, 19 insertions(+)
> > > > >
> > > > > diff --git a/board/microchip/mpfs_icicle/mpfs_icicle.c b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > > index 4d7d843dfa3..2c1f7175f0e 100644
> > > > > --- a/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > > +++ b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > > @@ -9,6 +9,7 @@
> > > > >  #include <init.h>
> > > > >  #include <asm/global_data.h>
> > > > >  #include <asm/io.h>
> > > > > +#include <asm/sections.h>
> > > > >
> > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > >
> > > > > @@ -50,6 +51,24 @@ static void read_device_serial_number(u8 *response, u8 response_size)
> > > > >               response_buf[idx] = readb(MPFS_SYS_SERVICE_MAILBOX + idx);
> > > > >  }
> > > > >
> > > > > +void *board_fdt_blob_setup(int *err)
> > > > > +{
> > > > > +     *err = 0;
> > > > > +     /*
> > > > > +      * The devicetree provided by the previous stage is very minimal due to
> > > > > +      * severe space constraints. The firmware performs no fixups etc.
> > > > > +      * U-Boot, if providing a devicetree, almost certainly has a better
> > > > > +      * more complete one than the firmware so that provided by the firmware
> > > > > +      * is ignored for OF_SEPARATE.
> > > > > +      */
> > > > > +     if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > > > +             if (gd->arch.firmware_fdt_addr)
> > > > > +                     return (ulong *)(uintptr_t)gd->arch.firmware_fdt_addr;
> > > > > +     }
> > > > > +
> > > > > +     return (ulong *)_end;
> > > > > +}
> > > > > +
> > > > >  int board_init(void)
> > > > >  {
> > > > >       /* For now nothing to do here. */
> > > >
> > > > I'm adding in Simon and Ilias as this touches on one of those frequent
> > > > topics about how device trees can/should be passed along to us.
> > >
> > > The only thing I can think of is implementing bloblist in  the a1 (?)
> > > firmware, then passing the DT in that.
> >
> > a1 is the register that is used on riscv to pass the dtb, I think the
> > corresponding thing on arm64 is x0.
> >
> > Re-reading the firware handoff spec, it's difficult to see what benefits
> > it actually provides us when we only ever have a single dtb which the
> > firmware does not interact with/use.
> > We are super space constrained in the firmware even carving out 4.5 KiB
> > for a devicetree blob is a stretch and requires disabling other features
> > and ripping out anything in the DT not required for U-Boot to load the OS.
> > Even the ~1 KiB mentioned in bloblist.h for handling a bloblist would be a
> > challenge.
> >
> > I think the only way a bloblist could work is if it was created at build
> > time and linked into the firmware, since the on-disk format seems pretty
> > minimal. Is there tooling for generating a bloblist at build time that I
> > could use to check whether or not a bloblist is viable?
> > I'd also have to investigate how that would interact with OpenSBI, since
> > it's integrated into the firmware and involved with loading U-Boot.
> 
> There is not such a tool, but it would be easy enough to write. If you
> think that would help, I could give it a try.

I mean I could also just do it myself, I just wanted to know if it
existed, since that'd make investigating this pretty straightforward to
do.

Either way, the firmware already is capable of doing this with a
devicetree blob, so I figure even if we manage to get bloblist stuff
going, there's little harm in supporting what's already there?

> The confusing thing for me is that the 'firmware' is very
> constrained...but are you saying that U-Boot is not?

Correct. The firmware is in eNVM with a size limit of 128 KiB, but it
can load U-Boot from mmc or flash etc.

> Why pass U-Boot the DT?

Primarily to allow using a generic image to be used across multiple
boards, using the compatible in the devicetree to determine what the
boot the OS with. Another idea that was floated was using vendor SBI
extension to request the information - but I felt that using DT was
better than conjuring up something specific to us.

> Perhaps you could explain the boot sequence so I can
> understand it better?

Ivan, correct me if I go off the rails here please... The firmware is
stored in eNVM on the FPGA, the FPGA's "system controller" sets the
reset vector for the CPUs to the location of the firmware. (There are
some other boot modes, but they all work similarly). The firmware
then decompresses itself into the L2 and runs primarily on hart 0,
which is a "monitor core", supporting fewer ISA extensions etc than the
4 cores on which user applications/OSes run on.

The firmware then loads a "payload" from a storage device, that contains
the next stage (or next stages if not running in SMP). The "payload" also
describes the privilege levels for the next stage(s) and the locations
in memory to load the next stage(s) to. OpenSBI is built into the firmware,
although we can't actually fit the whole libsbi library that it provides.
The firmware loads the next stage to whatever location has been provided
to it in the "payload". For U-Boot, we're always running in S-Mode, so
OpenSBI is used run the application once it has been loaded into memory
and to provide the SBI ecall interface etc.

I hope that makes some sense, it probably doesn't...

There's some info on how the payloads configurations are constructed in
the firmware repo here:
https://github.com/polarfire-soc/hart-software-services/tree/master/tools/hss-payload-generator

> > > It seems that you still need to be able to turn that on and off in
> > > U-Boot though. So far we have not agreed the mechanism to do that, I
> > > have the same problem, with a pending patch here[1]
> >
> > It seems your patch is trying to do some runtime determination of
> > whether to examine the bloblist or not, but the ?existing? build-time
> > check for BLOBLIST being enabled would work equally well/poorly as the
> > OF_BOARD check the code I am adding. I'm not even really sure what runtime
> > option could be used here here to check if the passed dtb/bloblist was to
> > be used. U-Boot only runs here as supervisor mode U-Boot proper and always
> > has a more complete devicetree. Whether to use the one passed to U-Boot
> > just depends on what the person with the board wants to do - which, given
> > this is an FPGA, could be vary significantly.
> 
> OK...so does this platform use SPL?

No :)

Thanks,
Conor.
Ilias Apalodimas June 28, 2024, 5:53 a.m. UTC | #6
Hi Conor,


On Thu, 27 Jun 2024 at 23:27, Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Jun 27, 2024 at 11:50:33AM +0100, Simon Glass wrote:
> > On Thu, 27 Jun 2024 at 10:38, Conor Dooley <conor.dooley@microchip.com> wrote:
> > > On Thu, Jun 27, 2024 at 09:36:49AM +0100, Simon Glass wrote:
> > > > On Tue, 25 Jun 2024 at 15:34, Tom Rini <trini@konsulko.com> wrote:
> > > > > On Tue, Jun 25, 2024 at 10:08:06AM +0100, Conor Dooley wrote:
> > > > > > The firmware on the Icicle is capable of providing a devicetree in a1 to
> > > > > > U-Boot, but until now the devicetree has been packaged in a "payload" [1]
> > > > > > alongside U-Boot (or other bootloaders/RTOSes) and appended to the image.
> > > > > > The address of this appended devicetree is placed in a1 by the firmware.
> > > > > > This meant that the mechanism used by OF_SEPARATE to locate the
> > > > > > devicetree at the end of the image would pick up the one provided by the
> > > > > > firmware when u-boot-nodtb.bin was in the payload and U-Boot's devicetree
> > > > > > when u-boot.bin was.
> > > > > >
> > > > > > The firmware is now going to be capable of providing a minimal devicetree
> > > > > > (quite cut down due to severe space constraints), but this devicetree is
> > > > > > linked into the firmware that runs out of the L2 rather than at the end
> > > > > > of the U-Boot image. Implement board_fdt_blob_setup() so that this
> > > > > > devicetree can be optionally used, and the devicetree provided in the
> > > > > > "payload" can be used without relying on "happening" to implement the
> > > > > > same strategy as OF_SEPARATE expects in combination with
> > > > > > u-boot-nodtb.bin. Unlike other RISC-V boards, the firmware provided
> > > > > > devicetree is only used when OF_BOARD is set, so that the almost
> > > > > > certainly more complete devicetree in U-Boot will be used unless
> > > > > > explicitly requested otherwise.
> > > > > >
> > > > > > Link: https://github.com/polarfire-soc/hart-software-services/blob/master/tools/hss-payload-generator/README.md [1]
> > > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > > ---
> > > > > > CC: Ivan Griffin <ivan.griffin@microchip.com>
> > > > > > CC: Padmarao Begari <padmarao.begari@microchip.com>
> > > > > > CC: Cyril Jean <cyril.jean@microchip.com>
> > > > > > CC: Tom Rini <trini@konsulko.com>
> > > > > > CC: Conor Dooley <conor.dooley@microchip.com>
> > > > > > CC: u-boot@lists.denx.de
> > > > > > ---
> > > > > >  board/microchip/mpfs_icicle/mpfs_icicle.c | 19 +++++++++++++++++++
> > > > > >  1 file changed, 19 insertions(+)
> > > > > >
> > > > > > diff --git a/board/microchip/mpfs_icicle/mpfs_icicle.c b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > > > index 4d7d843dfa3..2c1f7175f0e 100644
> > > > > > --- a/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > > > +++ b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > > > @@ -9,6 +9,7 @@
> > > > > >  #include <init.h>
> > > > > >  #include <asm/global_data.h>
> > > > > >  #include <asm/io.h>
> > > > > > +#include <asm/sections.h>
> > > > > >
> > > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > > >
> > > > > > @@ -50,6 +51,24 @@ static void read_device_serial_number(u8 *response, u8 response_size)
> > > > > >               response_buf[idx] = readb(MPFS_SYS_SERVICE_MAILBOX + idx);
> > > > > >  }
> > > > > >
> > > > > > +void *board_fdt_blob_setup(int *err)
> > > > > > +{
> > > > > > +     *err = 0;
> > > > > > +     /*
> > > > > > +      * The devicetree provided by the previous stage is very minimal due to
> > > > > > +      * severe space constraints. The firmware performs no fixups etc.
> > > > > > +      * U-Boot, if providing a devicetree, almost certainly has a better
> > > > > > +      * more complete one than the firmware so that provided by the firmware
> > > > > > +      * is ignored for OF_SEPARATE.
> > > > > > +      */
> > > > > > +     if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > > > > +             if (gd->arch.firmware_fdt_addr)
> > > > > > +                     return (ulong *)(uintptr_t)gd->arch.firmware_fdt_addr;
> > > > > > +     }
> > > > > > +
> > > > > > +     return (ulong *)_end;
> > > > > > +}
> > > > > > +
> > > > > >  int board_init(void)
> > > > > >  {
> > > > > >       /* For now nothing to do here. */
> > > > >
> > > > > I'm adding in Simon and Ilias as this touches on one of those frequent
> > > > > topics about how device trees can/should be passed along to us.
> > > >
> > > > The only thing I can think of is implementing bloblist in  the a1 (?)
> > > > firmware, then passing the DT in that.
> > >
> > > a1 is the register that is used on riscv to pass the dtb, I think the
> > > corresponding thing on arm64 is x0.
> > >
> > > Re-reading the firware handoff spec, it's difficult to see what benefits
> > > it actually provides us when we only ever have a single dtb which the
> > > firmware does not interact with/use.
> > > We are super space constrained in the firmware even carving out 4.5 KiB
> > > for a devicetree blob is a stretch and requires disabling other features
> > > and ripping out anything in the DT not required for U-Boot to load the OS.
> > > Even the ~1 KiB mentioned in bloblist.h for handling a bloblist would be a
> > > challenge.
> > >
> > > I think the only way a bloblist could work is if it was created at build
> > > time and linked into the firmware, since the on-disk format seems pretty
> > > minimal. Is there tooling for generating a bloblist at build time that I
> > > could use to check whether or not a bloblist is viable?
> > > I'd also have to investigate how that would interact with OpenSBI, since
> > > it's integrated into the firmware and involved with loading U-Boot.
> >
> > There is not such a tool, but it would be easy enough to write. If you
> > think that would help, I could give it a try.
>
> I mean I could also just do it myself, I just wanted to know if it
> existed, since that'd make investigating this pretty straightforward to
> do.

Someone is already working on it.
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/29253
this will help!

>
> Either way, the firmware already is capable of doing this with a
> devicetree blob, so I figure even if we manage to get bloblist stuff
> going, there's little harm in supporting what's already there?

There isn't, but OTOH the bloblist support is already in u-boot. I
would prefer to try that out first. Up to you

Cheers
/Ilias

>
> > The confusing thing for me is that the 'firmware' is very
> > constrained...but are you saying that U-Boot is not?
>
> Correct. The firmware is in eNVM with a size limit of 128 KiB, but it
> can load U-Boot from mmc or flash etc.
>
> > Why pass U-Boot the DT?
>
> Primarily to allow using a generic image to be used across multiple
> boards, using the compatible in the devicetree to determine what the
> boot the OS with. Another idea that was floated was using vendor SBI
> extension to request the information - but I felt that using DT was
> better than conjuring up something specific to us.
>
> > Perhaps you could explain the boot sequence so I can
> > understand it better?
>
> Ivan, correct me if I go off the rails here please... The firmware is
> stored in eNVM on the FPGA, the FPGA's "system controller" sets the
> reset vector for the CPUs to the location of the firmware. (There are
> some other boot modes, but they all work similarly). The firmware
> then decompresses itself into the L2 and runs primarily on hart 0,
> which is a "monitor core", supporting fewer ISA extensions etc than the
> 4 cores on which user applications/OSes run on.
>
> The firmware then loads a "payload" from a storage device, that contains
> the next stage (or next stages if not running in SMP). The "payload" also
> describes the privilege levels for the next stage(s) and the locations
> in memory to load the next stage(s) to. OpenSBI is built into the firmware,
> although we can't actually fit the whole libsbi library that it provides.
> The firmware loads the next stage to whatever location has been provided
> to it in the "payload". For U-Boot, we're always running in S-Mode, so
> OpenSBI is used run the application once it has been loaded into memory
> and to provide the SBI ecall interface etc.
>
> I hope that makes some sense, it probably doesn't...
>
> There's some info on how the payloads configurations are constructed in
> the firmware repo here:
> https://github.com/polarfire-soc/hart-software-services/tree/master/tools/hss-payload-generator
>
> > > > It seems that you still need to be able to turn that on and off in
> > > > U-Boot though. So far we have not agreed the mechanism to do that, I
> > > > have the same problem, with a pending patch here[1]
> > >
> > > It seems your patch is trying to do some runtime determination of
> > > whether to examine the bloblist or not, but the ?existing? build-time
> > > check for BLOBLIST being enabled would work equally well/poorly as the
> > > OF_BOARD check the code I am adding. I'm not even really sure what runtime
> > > option could be used here here to check if the passed dtb/bloblist was to
> > > be used. U-Boot only runs here as supervisor mode U-Boot proper and always
> > > has a more complete devicetree. Whether to use the one passed to U-Boot
> > > just depends on what the person with the board wants to do - which, given
> > > this is an FPGA, could be vary significantly.
> >
> > OK...so does this platform use SPL?
>
> No :)
>
> Thanks,
> Conor.
Simon Glass June 28, 2024, 6:22 a.m. UTC | #7
Hi Conor,

On Thu, 27 Jun 2024 at 21:27, Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Jun 27, 2024 at 11:50:33AM +0100, Simon Glass wrote:
> > On Thu, 27 Jun 2024 at 10:38, Conor Dooley <conor.dooley@microchip.com> wrote:
> > > On Thu, Jun 27, 2024 at 09:36:49AM +0100, Simon Glass wrote:
> > > > On Tue, 25 Jun 2024 at 15:34, Tom Rini <trini@konsulko.com> wrote:
> > > > > On Tue, Jun 25, 2024 at 10:08:06AM +0100, Conor Dooley wrote:
> > > > > > The firmware on the Icicle is capable of providing a devicetree in a1 to
> > > > > > U-Boot, but until now the devicetree has been packaged in a "payload" [1]
> > > > > > alongside U-Boot (or other bootloaders/RTOSes) and appended to the image.
> > > > > > The address of this appended devicetree is placed in a1 by the firmware.
> > > > > > This meant that the mechanism used by OF_SEPARATE to locate the
> > > > > > devicetree at the end of the image would pick up the one provided by the
> > > > > > firmware when u-boot-nodtb.bin was in the payload and U-Boot's devicetree
> > > > > > when u-boot.bin was.
> > > > > >
> > > > > > The firmware is now going to be capable of providing a minimal devicetree
> > > > > > (quite cut down due to severe space constraints), but this devicetree is
> > > > > > linked into the firmware that runs out of the L2 rather than at the end
> > > > > > of the U-Boot image. Implement board_fdt_blob_setup() so that this
> > > > > > devicetree can be optionally used, and the devicetree provided in the
> > > > > > "payload" can be used without relying on "happening" to implement the
> > > > > > same strategy as OF_SEPARATE expects in combination with
> > > > > > u-boot-nodtb.bin. Unlike other RISC-V boards, the firmware provided
> > > > > > devicetree is only used when OF_BOARD is set, so that the almost
> > > > > > certainly more complete devicetree in U-Boot will be used unless
> > > > > > explicitly requested otherwise.
> > > > > >
> > > > > > Link: https://github.com/polarfire-soc/hart-software-services/blob/master/tools/hss-payload-generator/README.md [1]
> > > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > > ---
> > > > > > CC: Ivan Griffin <ivan.griffin@microchip.com>
> > > > > > CC: Padmarao Begari <padmarao.begari@microchip.com>
> > > > > > CC: Cyril Jean <cyril.jean@microchip.com>
> > > > > > CC: Tom Rini <trini@konsulko.com>
> > > > > > CC: Conor Dooley <conor.dooley@microchip.com>
> > > > > > CC: u-boot@lists.denx.de
> > > > > > ---
> > > > > >  board/microchip/mpfs_icicle/mpfs_icicle.c | 19 +++++++++++++++++++
> > > > > >  1 file changed, 19 insertions(+)
> > > > > >
> > > > > > diff --git a/board/microchip/mpfs_icicle/mpfs_icicle.c b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > > > index 4d7d843dfa3..2c1f7175f0e 100644
> > > > > > --- a/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > > > +++ b/board/microchip/mpfs_icicle/mpfs_icicle.c
> > > > > > @@ -9,6 +9,7 @@
> > > > > >  #include <init.h>
> > > > > >  #include <asm/global_data.h>
> > > > > >  #include <asm/io.h>
> > > > > > +#include <asm/sections.h>
> > > > > >
> > > > > >  DECLARE_GLOBAL_DATA_PTR;
> > > > > >
> > > > > > @@ -50,6 +51,24 @@ static void read_device_serial_number(u8 *response, u8 response_size)
> > > > > >               response_buf[idx] = readb(MPFS_SYS_SERVICE_MAILBOX + idx);
> > > > > >  }
> > > > > >
> > > > > > +void *board_fdt_blob_setup(int *err)
> > > > > > +{
> > > > > > +     *err = 0;
> > > > > > +     /*
> > > > > > +      * The devicetree provided by the previous stage is very minimal due to
> > > > > > +      * severe space constraints. The firmware performs no fixups etc.
> > > > > > +      * U-Boot, if providing a devicetree, almost certainly has a better
> > > > > > +      * more complete one than the firmware so that provided by the firmware
> > > > > > +      * is ignored for OF_SEPARATE.
> > > > > > +      */
> > > > > > +     if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > > > > +             if (gd->arch.firmware_fdt_addr)
> > > > > > +                     return (ulong *)(uintptr_t)gd->arch.firmware_fdt_addr;
> > > > > > +     }
> > > > > > +
> > > > > > +     return (ulong *)_end;
> > > > > > +}
> > > > > > +
> > > > > >  int board_init(void)
> > > > > >  {
> > > > > >       /* For now nothing to do here. */
> > > > >
> > > > > I'm adding in Simon and Ilias as this touches on one of those frequent
> > > > > topics about how device trees can/should be passed along to us.
> > > >
> > > > The only thing I can think of is implementing bloblist in  the a1 (?)
> > > > firmware, then passing the DT in that.
> > >
> > > a1 is the register that is used on riscv to pass the dtb, I think the
> > > corresponding thing on arm64 is x0.
> > >
> > > Re-reading the firware handoff spec, it's difficult to see what benefits
> > > it actually provides us when we only ever have a single dtb which the
> > > firmware does not interact with/use.
> > > We are super space constrained in the firmware even carving out 4.5 KiB
> > > for a devicetree blob is a stretch and requires disabling other features
> > > and ripping out anything in the DT not required for U-Boot to load the OS.
> > > Even the ~1 KiB mentioned in bloblist.h for handling a bloblist would be a
> > > challenge.
> > >
> > > I think the only way a bloblist could work is if it was created at build
> > > time and linked into the firmware, since the on-disk format seems pretty
> > > minimal. Is there tooling for generating a bloblist at build time that I
> > > could use to check whether or not a bloblist is viable?
> > > I'd also have to investigate how that would interact with OpenSBI, since
> > > it's integrated into the firmware and involved with loading U-Boot.
> >
> > There is not such a tool, but it would be easy enough to write. If you
> > think that would help, I could give it a try.
>
> I mean I could also just do it myself, I just wanted to know if it
> existed, since that'd make investigating this pretty straightforward to
> do.
>
> Either way, the firmware already is capable of doing this with a
> devicetree blob, so I figure even if we manage to get bloblist stuff
> going, there's little harm in supporting what's already there?
>
> > The confusing thing for me is that the 'firmware' is very
> > constrained...but are you saying that U-Boot is not?
>
> Correct. The firmware is in eNVM with a size limit of 128 KiB, but it
> can load U-Boot from mmc or flash etc.
>
> > Why pass U-Boot the DT?
>
> Primarily to allow using a generic image to be used across multiple
> boards, using the compatible in the devicetree to determine what the
> boot the OS with. Another idea that was floated was using vendor SBI
> extension to request the information - but I felt that using DT was
> better than conjuring up something specific to us.
>
> > Perhaps you could explain the boot sequence so I can
> > understand it better?
>
> Ivan, correct me if I go off the rails here please... The firmware is
> stored in eNVM on the FPGA, the FPGA's "system controller" sets the
> reset vector for the CPUs to the location of the firmware. (There are
> some other boot modes, but they all work similarly). The firmware
> then decompresses itself into the L2 and runs primarily on hart 0,
> which is a "monitor core", supporting fewer ISA extensions etc than the
> 4 cores on which user applications/OSes run on.
>
> The firmware then loads a "payload" from a storage device, that contains
> the next stage (or next stages if not running in SMP). The "payload" also
> describes the privilege levels for the next stage(s) and the locations
> in memory to load the next stage(s) to. OpenSBI is built into the firmware,
> although we can't actually fit the whole libsbi library that it provides.
> The firmware loads the next stage to whatever location has been provided
> to it in the "payload". For U-Boot, we're always running in S-Mode, so
> OpenSBI is used run the application once it has been loaded into memory
> and to provide the SBI ecall interface etc.
>
> I hope that makes some sense, it probably doesn't...
>
> There's some info on how the payloads configurations are constructed in
> the firmware repo here:
> https://github.com/polarfire-soc/hart-software-services/tree/master/tools/hss-payload-generator
>
> > > > It seems that you still need to be able to turn that on and off in
> > > > U-Boot though. So far we have not agreed the mechanism to do that, I
> > > > have the same problem, with a pending patch here[1]
> > >
> > > It seems your patch is trying to do some runtime determination of
> > > whether to examine the bloblist or not, but the ?existing? build-time
> > > check for BLOBLIST being enabled would work equally well/poorly as the
> > > OF_BOARD check the code I am adding. I'm not even really sure what runtime
> > > option could be used here here to check if the passed dtb/bloblist was to
> > > be used. U-Boot only runs here as supervisor mode U-Boot proper and always
> > > has a more complete devicetree. Whether to use the one passed to U-Boot
> > > just depends on what the person with the board wants to do - which, given
> > > this is an FPGA, could be vary significantly.
> >
> > OK...so does this platform use SPL?
>
> No :)

OK, thanks for all the details. I suppose, to me, 128KB does not sound
that constrained :-) But I can see that messing with a bloblist can
add code. A static tool would help with that.

Regards,
Simon
Conor Dooley June 28, 2024, 6:29 a.m. UTC | #8
On Fri, Jun 28, 2024 at 07:22:35AM +0100, Simon Glass wrote:
> On Thu, 27 Jun 2024 at 21:27, Conor Dooley <conor@kernel.org> wrote:
> 
> OK, thanks for all the details. I suppose, to me, 128KB does not sound
> that constrained :-) But I can see that messing with a bloblist can
> add code. A static tool would help with that.

It's all relative - when libsbi.a is > 3 MiB 128KiB is a problem!
Conor Dooley June 28, 2024, 6:34 a.m. UTC | #9
On Fri, Jun 28, 2024 at 08:53:11AM +0300, Ilias Apalodimas wrote:
> On Thu, 27 Jun 2024 at 23:27, Conor Dooley <conor@kernel.org> wrote:
> > On Thu, Jun 27, 2024 at 11:50:33AM +0100, Simon Glass wrote:
> > > On Thu, 27 Jun 2024 at 10:38, Conor Dooley <conor.dooley@microchip.com> wrote:
> > > > On Thu, Jun 27, 2024 at 09:36:49AM +0100, Simon Glass wrote:

> > > > I think the only way a bloblist could work is if it was created at build
> > > > time and linked into the firmware, since the on-disk format seems pretty
> > > > minimal. Is there tooling for generating a bloblist at build time that I
> > > > could use to check whether or not a bloblist is viable?
> > > > I'd also have to investigate how that would interact with OpenSBI, since
> > > > it's integrated into the firmware and involved with loading U-Boot.
> > >
> > > There is not such a tool, but it would be easy enough to write. If you
> > > think that would help, I could give it a try.
> >
> > I mean I could also just do it myself, I just wanted to know if it
> > existed, since that'd make investigating this pretty straightforward to
> > do.
> 
> Someone is already working on it.
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/29253
> this will help!

Eww, gerrit. Thanks for the link.

> > Either way, the firmware already is capable of doing this with a
> > devicetree blob, so I figure even if we manage to get bloblist stuff
> > going, there's little harm in supporting what's already there?
> 
> There isn't, but OTOH the bloblist support is already in u-boot.

Devicetree in a1 support is already in U-Boot too, it's already been
nicely placed in gd->arch.firmware_fdt_addr by the arch code :)

> I would prefer to try that out first. Up to you

If it is up to me, I am going to say go with what I have already done,
as it's much easier to update U-Boot on a disk that you can write to
from Linux than it is to change the firmware in envm and something
functional already exists.
diff mbox series

Patch

diff --git a/board/microchip/mpfs_icicle/mpfs_icicle.c b/board/microchip/mpfs_icicle/mpfs_icicle.c
index 4d7d843dfa3..2c1f7175f0e 100644
--- a/board/microchip/mpfs_icicle/mpfs_icicle.c
+++ b/board/microchip/mpfs_icicle/mpfs_icicle.c
@@ -9,6 +9,7 @@ 
 #include <init.h>
 #include <asm/global_data.h>
 #include <asm/io.h>
+#include <asm/sections.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -50,6 +51,24 @@  static void read_device_serial_number(u8 *response, u8 response_size)
 		response_buf[idx] = readb(MPFS_SYS_SERVICE_MAILBOX + idx);
 }
 
+void *board_fdt_blob_setup(int *err)
+{
+	*err = 0;
+	/*
+	 * The devicetree provided by the previous stage is very minimal due to
+	 * severe space constraints. The firmware performs no fixups etc.
+	 * U-Boot, if providing a devicetree, almost certainly has a better
+	 * more complete one than the firmware so that provided by the firmware
+	 * is ignored for OF_SEPARATE.
+	 */
+	if (IS_ENABLED(CONFIG_OF_BOARD)) {
+		if (gd->arch.firmware_fdt_addr)
+			return (ulong *)(uintptr_t)gd->arch.firmware_fdt_addr;
+	}
+
+	return (ulong *)_end;
+}
+
 int board_init(void)
 {
 	/* For now nothing to do here. */