[PATCH v1] board: mpfs_icicle: implement board_fdt_blob_setup()
Simon Glass
sjg at chromium.org
Fri Jun 28 08:22:35 CEST 2024
Hi Conor,
On Thu, 27 Jun 2024 at 21:27, Conor Dooley <conor at 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 at 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 at 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 at microchip.com>
> > > > > > ---
> > > > > > CC: Ivan Griffin <ivan.griffin at microchip.com>
> > > > > > CC: Padmarao Begari <padmarao.begari at microchip.com>
> > > > > > CC: Cyril Jean <cyril.jean at microchip.com>
> > > > > > CC: Tom Rini <trini at konsulko.com>
> > > > > > CC: Conor Dooley <conor.dooley at microchip.com>
> > > > > > CC: u-boot at 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
More information about the U-Boot
mailing list