Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Raymond,
On Fri, 20 Oct 2023 at 15:15, Raymond Mao wrote:
>
> Hi Simon,
>
> On Thu, 21 Sept 2023 at 04:58, Simon Glass wrote:
> >
> > Standard passage provides for a bloblist to be passed from one firmware
> > phase to the next. That can be used to pass the devicetree along as well.
> > Add an option to support this.
> >
> > Tests for this will be added as part of the Universal Payload work.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > Changes in v2:
> > - No changes as it still seems unclear what should be done
> >
> [...]
> > diff --git a/include/bloblist.h b/include/bloblist.h
> > index 080cc46a1266..e16d122f4fb1 100644
> > --- a/include/bloblist.h
> > +++ b/include/bloblist.h
> > @@ -103,6 +103,11 @@ enum bloblist_tag_t {
> > BLOBLISTT_ACPI_TABLES = 0x104, /* ACPI tables for x86 */
> > BLOBLISTT_SMBIOS_TABLES = 0x105, /* SMBIOS tables for x86 */
> > BLOBLISTT_VBOOT_CTX = 0x106, /* Chromium OS verified boot context */
> > + /*
> > + * Devicetree for use by firmware. On some platforms this is passed to
> > + * the OS also
> > + */
> > + BLOBLISTT_CONTROL_FDT = 0x107,
> Shall we re-define all the `BLOBLISTT_` tags according to the firmware handoff
> spec v0.9?
> According to the latest discussion (in
> https://github.com/FirmwareHandoff/firmware_handoff/pull/19)
> It is preferred that generic ones to start from `0`, and Arm specific ones
> starts
> from `0x100`.
> In this case, the FDT should be re-defined as `1` and other existing non-arm
> ones (e.g. for x86 or Chromium OS) should have their placeholder before
> `0xff_f000`,
> while generic ones (e.g. for TPM) should be placed somewhere between `0`
> and `0x100`.
>
> [...]
>
Sure, if you like. I'm a little unclear when that stuff will land. We
can always update the tag numbering along with the format changes.
Regards,
Simon
Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Simon,
On Thu, 21 Sept 2023 at 04:58, Simon Glass wrote:
>
> Standard passage provides for a bloblist to be passed from one firmware
> phase to the next. That can be used to pass the devicetree along as well.
> Add an option to support this.
>
> Tests for this will be added as part of the Universal Payload work.
>
> Signed-off-by: Simon Glass
> ---
>
> Changes in v2:
> - No changes as it still seems unclear what should be done
>
[...]
> diff --git a/include/bloblist.h b/include/bloblist.h
> index 080cc46a1266..e16d122f4fb1 100644
> --- a/include/bloblist.h
> +++ b/include/bloblist.h
> @@ -103,6 +103,11 @@ enum bloblist_tag_t {
> BLOBLISTT_ACPI_TABLES = 0x104, /* ACPI tables for x86 */
> BLOBLISTT_SMBIOS_TABLES = 0x105, /* SMBIOS tables for x86 */
> BLOBLISTT_VBOOT_CTX = 0x106, /* Chromium OS verified boot context */
> + /*
> + * Devicetree for use by firmware. On some platforms this is passed to
> + * the OS also
> + */
> + BLOBLISTT_CONTROL_FDT = 0x107,
Shall we re-define all the `BLOBLISTT_` tags according to the firmware
handoff
spec v0.9?
According to the latest discussion (in
https://github.com/FirmwareHandoff/firmware_handoff/pull/19)
It is preferred that generic ones to start from `0`, and Arm specific ones
starts
from `0x100`.
In this case, the FDT should be re-defined as `1` and other existing non-arm
ones (e.g. for x86 or Chromium OS) should have their placeholder before
`0xff_f000`,
while generic ones (e.g. for TPM) should be placed somewhere between `0`
and `0x100`.
[...]
Regards,
Raymond
Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Ilias,
On Mon, 2 Oct 2023 at 01:33, Ilias Apalodimas
wrote:
>
> Hi Simon
>
> On Mon, 2 Oct 2023 at 04:23, Simon Glass wrote:
> >
> > Hi Ilias,
> >
> > On Tue, 26 Sept 2023 at 07:13, Ilias Apalodimas
> > wrote:
> > >
> > > Hi Simon,
> > >
> > > [...]
> > >
> > > > > > >
> > > > > > > So, instead of adding OF_BLOBLIST, just move this code under
> > > > > > > OF_BOARD,
> > > > > > > inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required
> > > > > > > and
> > > > > > > the previous stage loader is supposed to provide a DT we can just
> > > > > > > throw an error and stop booting
> > > > > >
> > > > > > This is the bit I don't get.
> > > > > >
> > > > > > The OF_BOARD thing is a hack, in that the board can do what it
> > > > > > likes.
> > > > > > It is our way of handling board-specific mechanisms.
> > > > > >
> > > > > > But I am wanting a standard mechanism, i.e. like 'standard
> > > > > > passage', a
> > > > > > way of passing the DT through the phases.
> > > > > >
> > > > > > If I put this under OF_BOARD, then the board gets to override the
> > > > > > mechanism, so which is it?
> > > > >
> > > > > No, it's the other way around in my head. OF_BOARD description is 'a
> > > > > previous stage loader hands me over the DT', which is a superset of
> > > > > the bloblist.
> > > > > Whether it comes in a firmware handoff format, or a hacky register the
> > > > > previous bootloader filled in is a detail we have to deal with and we
> > > > > need to keep backwards compatibility.
> > > > >
> > > > > Maybe adding a coding snip would help
> > > > > if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > > > if (CONFIG_IS_ENABLED(BLOBLIST)) { <- This instead of OF_BLOBLIST
> > > > > ret = bloblist_maybe_init();
> > > > > if (ret)
> > > > > return ret;
> > > > > /* Dynamically scan for a DT in the bloblist. */
> > > > > gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
> > > > > if (!gd->fdt_blob) {
> > > > > printf("Not FDT found in bloblist\n");
> > > > > bloblist_show_list();
> > > > >// We can choose to not return an error here and keep
> > > > > scanning in case the DT is in a register, but I am fine with both
> > > > > return -ENOENT;
> > > > > }
> > > > >gd->fdt_src = FDTSRC_BLOBLIST;
> > > > >bloblist_show_list();
> > > > >log_debug("Devicetree is in bloblist at %p\n", gd->fdt_blob);
> > > > > // We can also bail out of this entirely if we do find a DT via
> > > > > a bloblist.
> > > > > } else {
> > > > > gd->fdt_blob = board_fdt_blob_setup(&ret);
> > > > > if (ret)
> > > > > return ret;
> > > > > gd->fdt_src = FDTSRC_BOARD;
> > > > > }
> > > > > }
> > > > >
> > > > > I haven't even compiled the code above, but it should give you a
> > > > > better idea of what I am suggesting
> > > >
> > > > OK I see...yes that is along the lines of what I thought you meant.
> > > >
> > > > But OF_BOARD does not mean 'previous stage loader hands me over the
> > > > DT'. I means call board_fdt_blob_setup() which could do anything. If
> > > > some boards use that function to implement getting a DT from the prior
> > > > stage, that's fine, but it isn't limited to that.
> > >
> > > I think it is limited. The help message says 'Provider of DTB for DT
> > > control' and OF_BOARD help message is 'Provided by the board (e.g a
> > > previous loader) at runtime '.
> > > In fact, that's exactly what I tried to clean up with commit
> > > e7fb789612e39. Sandbox had its special OF_HOSTFILE for the DT. We
> > > cleaned that up and then reintroduced it with a different name in
> > > commit 275b4832f6bf91c.
> >
> > That commit adds HAS_PRIOR_STATE, though, not anything to do with sandbox.
> >
> > OK. Perhaps it is just the OF_BOARD name that I object to?
> >
> > >
> > > In any case, if people want to do 'more' we have
> > > CONFIG_OF_BOARD_SETUP/FIXUP which should be used instead for other
> > > platform-specific stuff. While at it OF_PRIOR_STAGE is a much better
> > > name that OF_BOARD. So we could get rid of OF_PRIOR_STAGE and rename
> > > OF_BOARD to that.
> >
> > To do that, we have to work out what to do with board-specific setup.
> >
> > >
> > > The function name that is called in the setup phase is
> > > board_fdt_blob_setup(), so it's explicitly targeting the fdt setup, or
> > > at least that's what the name suggets. Grepping for CONFIG_OF_BOARD
> > > matches in
> > > board/AndesTech/ae350/ae350.c
> > > board/armltd/vexpress64/vexpress64.c
> > > board/raspberrypi/rpi/rpi.c
> > > board/sifive/unleashed/unleashed.c
> > > board/sifive/unmatched/unmatched.c
> > > board/starfive/visionfive2/starfive_visionfive2.c
> > > board/xilinx/common/board.c:
> > > All these just use it to setup the DT, apart from the rpi which
> > > additionally sets up some memory for the DT.
> > >
> > > Apart from that it's used on a few platform drivers to se
Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Simon
On Mon, 2 Oct 2023 at 04:23, Simon Glass wrote:
>
> Hi Ilias,
>
> On Tue, 26 Sept 2023 at 07:13, Ilias Apalodimas
> wrote:
> >
> > Hi Simon,
> >
> > [...]
> >
> > > > > >
> > > > > > So, instead of adding OF_BLOBLIST, just move this code under
> > > > > > OF_BOARD,
> > > > > > inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and
> > > > > > the previous stage loader is supposed to provide a DT we can just
> > > > > > throw an error and stop booting
> > > > >
> > > > > This is the bit I don't get.
> > > > >
> > > > > The OF_BOARD thing is a hack, in that the board can do what it likes.
> > > > > It is our way of handling board-specific mechanisms.
> > > > >
> > > > > But I am wanting a standard mechanism, i.e. like 'standard passage', a
> > > > > way of passing the DT through the phases.
> > > > >
> > > > > If I put this under OF_BOARD, then the board gets to override the
> > > > > mechanism, so which is it?
> > > >
> > > > No, it's the other way around in my head. OF_BOARD description is 'a
> > > > previous stage loader hands me over the DT', which is a superset of
> > > > the bloblist.
> > > > Whether it comes in a firmware handoff format, or a hacky register the
> > > > previous bootloader filled in is a detail we have to deal with and we
> > > > need to keep backwards compatibility.
> > > >
> > > > Maybe adding a coding snip would help
> > > > if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > > if (CONFIG_IS_ENABLED(BLOBLIST)) { <- This instead of OF_BLOBLIST
> > > > ret = bloblist_maybe_init();
> > > > if (ret)
> > > > return ret;
> > > > /* Dynamically scan for a DT in the bloblist. */
> > > > gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
> > > > if (!gd->fdt_blob) {
> > > > printf("Not FDT found in bloblist\n");
> > > > bloblist_show_list();
> > > >// We can choose to not return an error here and keep
> > > > scanning in case the DT is in a register, but I am fine with both
> > > > return -ENOENT;
> > > > }
> > > >gd->fdt_src = FDTSRC_BLOBLIST;
> > > >bloblist_show_list();
> > > >log_debug("Devicetree is in bloblist at %p\n", gd->fdt_blob);
> > > > // We can also bail out of this entirely if we do find a DT via
> > > > a bloblist.
> > > > } else {
> > > > gd->fdt_blob = board_fdt_blob_setup(&ret);
> > > > if (ret)
> > > > return ret;
> > > > gd->fdt_src = FDTSRC_BOARD;
> > > > }
> > > > }
> > > >
> > > > I haven't even compiled the code above, but it should give you a
> > > > better idea of what I am suggesting
> > >
> > > OK I see...yes that is along the lines of what I thought you meant.
> > >
> > > But OF_BOARD does not mean 'previous stage loader hands me over the
> > > DT'. I means call board_fdt_blob_setup() which could do anything. If
> > > some boards use that function to implement getting a DT from the prior
> > > stage, that's fine, but it isn't limited to that.
> >
> > I think it is limited. The help message says 'Provider of DTB for DT
> > control' and OF_BOARD help message is 'Provided by the board (e.g a
> > previous loader) at runtime '.
> > In fact, that's exactly what I tried to clean up with commit
> > e7fb789612e39. Sandbox had its special OF_HOSTFILE for the DT. We
> > cleaned that up and then reintroduced it with a different name in
> > commit 275b4832f6bf91c.
>
> That commit adds HAS_PRIOR_STATE, though, not anything to do with sandbox.
>
> OK. Perhaps it is just the OF_BOARD name that I object to?
>
> >
> > In any case, if people want to do 'more' we have
> > CONFIG_OF_BOARD_SETUP/FIXUP which should be used instead for other
> > platform-specific stuff. While at it OF_PRIOR_STAGE is a much better
> > name that OF_BOARD. So we could get rid of OF_PRIOR_STAGE and rename
> > OF_BOARD to that.
>
> To do that, we have to work out what to do with board-specific setup.
>
> >
> > The function name that is called in the setup phase is
> > board_fdt_blob_setup(), so it's explicitly targeting the fdt setup, or
> > at least that's what the name suggets. Grepping for CONFIG_OF_BOARD
> > matches in
> > board/AndesTech/ae350/ae350.c
> > board/armltd/vexpress64/vexpress64.c
> > board/raspberrypi/rpi/rpi.c
> > board/sifive/unleashed/unleashed.c
> > board/sifive/unmatched/unmatched.c
> > board/starfive/visionfive2/starfive_visionfive2.c
> > board/xilinx/common/board.c:
> > All these just use it to setup the DT, apart from the rpi which
> > additionally sets up some memory for the DT.
> >
> > Apart from that it's used on a few platform drivers to setup so DM
> > flags (most of them set DM_FLAG_PRE_RELOC), but we can easily change
> > that and use The only affected files are
> > drivers/pinctrl/broadcom/pinctrl-bcm283x.c
> > drivers/serial/serial_bcm283x_mu.c
> > drivers/serial/serial_bcm283x_pl011.c
> >
> > So making OF_BOARD do the right thing is trivial and I can work with
Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Ilias,
On Tue, 26 Sept 2023 at 07:13, Ilias Apalodimas
wrote:
>
> Hi Simon,
>
> [...]
>
> > > > >
> > > > > So, instead of adding OF_BLOBLIST, just move this code under OF_BOARD,
> > > > > inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and
> > > > > the previous stage loader is supposed to provide a DT we can just
> > > > > throw an error and stop booting
> > > >
> > > > This is the bit I don't get.
> > > >
> > > > The OF_BOARD thing is a hack, in that the board can do what it likes.
> > > > It is our way of handling board-specific mechanisms.
> > > >
> > > > But I am wanting a standard mechanism, i.e. like 'standard passage', a
> > > > way of passing the DT through the phases.
> > > >
> > > > If I put this under OF_BOARD, then the board gets to override the
> > > > mechanism, so which is it?
> > >
> > > No, it's the other way around in my head. OF_BOARD description is 'a
> > > previous stage loader hands me over the DT', which is a superset of
> > > the bloblist.
> > > Whether it comes in a firmware handoff format, or a hacky register the
> > > previous bootloader filled in is a detail we have to deal with and we
> > > need to keep backwards compatibility.
> > >
> > > Maybe adding a coding snip would help
> > > if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > if (CONFIG_IS_ENABLED(BLOBLIST)) { <- This instead of OF_BLOBLIST
> > > ret = bloblist_maybe_init();
> > > if (ret)
> > > return ret;
> > > /* Dynamically scan for a DT in the bloblist. */
> > > gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
> > > if (!gd->fdt_blob) {
> > > printf("Not FDT found in bloblist\n");
> > > bloblist_show_list();
> > >// We can choose to not return an error here and keep
> > > scanning in case the DT is in a register, but I am fine with both
> > > return -ENOENT;
> > > }
> > >gd->fdt_src = FDTSRC_BLOBLIST;
> > >bloblist_show_list();
> > >log_debug("Devicetree is in bloblist at %p\n", gd->fdt_blob);
> > > // We can also bail out of this entirely if we do find a DT via
> > > a bloblist.
> > > } else {
> > > gd->fdt_blob = board_fdt_blob_setup(&ret);
> > > if (ret)
> > > return ret;
> > > gd->fdt_src = FDTSRC_BOARD;
> > > }
> > > }
> > >
> > > I haven't even compiled the code above, but it should give you a
> > > better idea of what I am suggesting
> >
> > OK I see...yes that is along the lines of what I thought you meant.
> >
> > But OF_BOARD does not mean 'previous stage loader hands me over the
> > DT'. I means call board_fdt_blob_setup() which could do anything. If
> > some boards use that function to implement getting a DT from the prior
> > stage, that's fine, but it isn't limited to that.
>
> I think it is limited. The help message says 'Provider of DTB for DT
> control' and OF_BOARD help message is 'Provided by the board (e.g a
> previous loader) at runtime '.
> In fact, that's exactly what I tried to clean up with commit
> e7fb789612e39. Sandbox had its special OF_HOSTFILE for the DT. We
> cleaned that up and then reintroduced it with a different name in
> commit 275b4832f6bf91c.
That commit adds HAS_PRIOR_STATE, though, not anything to do with sandbox.
OK. Perhaps it is just the OF_BOARD name that I object to?
>
> In any case, if people want to do 'more' we have
> CONFIG_OF_BOARD_SETUP/FIXUP which should be used instead for other
> platform-specific stuff. While at it OF_PRIOR_STAGE is a much better
> name that OF_BOARD. So we could get rid of OF_PRIOR_STAGE and rename
> OF_BOARD to that.
To do that, we have to work out what to do with board-specific setup.
>
> The function name that is called in the setup phase is
> board_fdt_blob_setup(), so it's explicitly targeting the fdt setup, or
> at least that's what the name suggets. Grepping for CONFIG_OF_BOARD
> matches in
> board/AndesTech/ae350/ae350.c
> board/armltd/vexpress64/vexpress64.c
> board/raspberrypi/rpi/rpi.c
> board/sifive/unleashed/unleashed.c
> board/sifive/unmatched/unmatched.c
> board/starfive/visionfive2/starfive_visionfive2.c
> board/xilinx/common/board.c:
> All these just use it to setup the DT, apart from the rpi which
> additionally sets up some memory for the DT.
>
> Apart from that it's used on a few platform drivers to setup so DM
> flags (most of them set DM_FLAG_PRE_RELOC), but we can easily change
> that and use The only affected files are
> drivers/pinctrl/broadcom/pinctrl-bcm283x.c
> drivers/serial/serial_bcm283x_mu.c
> drivers/serial/serial_bcm283x_pl011.c
>
> So making OF_BOARD do the right thing is trivial and I can work with
> you on that. I don't think it's too much effort anyway
What is the right thing you refer to here?
>
> >
> > There is an HAS_PRIOR_STAGE option which sets OF_BOARD at present. But
> > that doesn't seem right in the long term.
> >
> > The goal here is to standardise the passing of DT from a pri
Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Simon,
[...]
> > > >
> > > > So, instead of adding OF_BLOBLIST, just move this code under OF_BOARD,
> > > > inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and
> > > > the previous stage loader is supposed to provide a DT we can just
> > > > throw an error and stop booting
> > >
> > > This is the bit I don't get.
> > >
> > > The OF_BOARD thing is a hack, in that the board can do what it likes.
> > > It is our way of handling board-specific mechanisms.
> > >
> > > But I am wanting a standard mechanism, i.e. like 'standard passage', a
> > > way of passing the DT through the phases.
> > >
> > > If I put this under OF_BOARD, then the board gets to override the
> > > mechanism, so which is it?
> >
> > No, it's the other way around in my head. OF_BOARD description is 'a
> > previous stage loader hands me over the DT', which is a superset of
> > the bloblist.
> > Whether it comes in a firmware handoff format, or a hacky register the
> > previous bootloader filled in is a detail we have to deal with and we
> > need to keep backwards compatibility.
> >
> > Maybe adding a coding snip would help
> > if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > if (CONFIG_IS_ENABLED(BLOBLIST)) { <- This instead of OF_BLOBLIST
> > ret = bloblist_maybe_init();
> > if (ret)
> > return ret;
> > /* Dynamically scan for a DT in the bloblist. */
> > gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
> > if (!gd->fdt_blob) {
> > printf("Not FDT found in bloblist\n");
> > bloblist_show_list();
> >// We can choose to not return an error here and keep
> > scanning in case the DT is in a register, but I am fine with both
> > return -ENOENT;
> > }
> >gd->fdt_src = FDTSRC_BLOBLIST;
> >bloblist_show_list();
> >log_debug("Devicetree is in bloblist at %p\n", gd->fdt_blob);
> > // We can also bail out of this entirely if we do find a DT via
> > a bloblist.
> > } else {
> > gd->fdt_blob = board_fdt_blob_setup(&ret);
> > if (ret)
> > return ret;
> > gd->fdt_src = FDTSRC_BOARD;
> > }
> > }
> >
> > I haven't even compiled the code above, but it should give you a
> > better idea of what I am suggesting
>
> OK I see...yes that is along the lines of what I thought you meant.
>
> But OF_BOARD does not mean 'previous stage loader hands me over the
> DT'. I means call board_fdt_blob_setup() which could do anything. If
> some boards use that function to implement getting a DT from the prior
> stage, that's fine, but it isn't limited to that.
I think it is limited. The help message says 'Provider of DTB for DT
control' and OF_BOARD help message is 'Provided by the board (e.g a
previous loader) at runtime '.
In fact, that's exactly what I tried to clean up with commit
e7fb789612e39. Sandbox had its special OF_HOSTFILE for the DT. We
cleaned that up and then reintroduced it with a different name in
commit 275b4832f6bf91c.
In any case, if people want to do 'more' we have
CONFIG_OF_BOARD_SETUP/FIXUP which should be used instead for other
platform-specific stuff. While at it OF_PRIOR_STAGE is a much better
name that OF_BOARD. So we could get rid of OF_PRIOR_STAGE and rename
OF_BOARD to that.
The function name that is called in the setup phase is
board_fdt_blob_setup(), so it's explicitly targeting the fdt setup, or
at least that's what the name suggets. Grepping for CONFIG_OF_BOARD
matches in
board/AndesTech/ae350/ae350.c
board/armltd/vexpress64/vexpress64.c
board/raspberrypi/rpi/rpi.c
board/sifive/unleashed/unleashed.c
board/sifive/unmatched/unmatched.c
board/starfive/visionfive2/starfive_visionfive2.c
board/xilinx/common/board.c:
All these just use it to setup the DT, apart from the rpi which
additionally sets up some memory for the DT.
Apart from that it's used on a few platform drivers to setup so DM
flags (most of them set DM_FLAG_PRE_RELOC), but we can easily change
that and use The only affected files are
drivers/pinctrl/broadcom/pinctrl-bcm283x.c
drivers/serial/serial_bcm283x_mu.c
drivers/serial/serial_bcm283x_pl011.c
So making OF_BOARD do the right thing is trivial and I can work with
you on that. I don't think it's too much effort anyway
>
> There is an HAS_PRIOR_STAGE option which sets OF_BOARD at present. But
> that doesn't seem right in the long term.
>
> The goal here is to standardise the passing of DT from a prior stage,
> so that all boards (except perhaps the dark child rpi) use standard
> passage (bloblist) to do so. That should not depend on OF_BOARD and in
> fact I would like to make OF_BOARD the exception, used when
> OF_BLOBLIST doesn't suit.
We are on the same page regarding the standardization. But I think
shoehorning an OF_BLOBLIST just papers over the problem.
The board should ideally have 2 options
1. a previous loader gives me the DT in X random ways (one of them is
the bloblist)
2. U-Boot provides it because it was bui
Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Ilias,
On Tue, 26 Sept 2023 at 06:32, Ilias Apalodimas
wrote:
>
> Hi Simon
>
> On Tue, 26 Sept 2023 at 14:37, Simon Glass wrote:
> >
> > Hi Ilias,
> >
> > On Mon, 25 Sept 2023 at 13:48, Ilias Apalodimas
> > wrote:
> > >
> > > Hi Simon,
> > >
> > > I commented on the v1 thread, but let's continue the discussion here
> > >
> > > On Thu, 21 Sept 2023 at 04:58, Simon Glass wrote:
> > > >
> > > > Standard passage provides for a bloblist to be passed from one firmware
> > > > phase to the next. That can be used to pass the devicetree along as
> > > > well.
> > > > Add an option to support this.
> > > >
> > > > Tests for this will be added as part of the Universal Payload work.
> > > >
> > > > Signed-off-by: Simon Glass
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - No changes as it still seems unclear what should be done
> > > >
> > >
> > > [...]
> > >
> > > >
> > > > /* BLOBLISTT_VENDOR_AREA */
> > > > diff --git a/doc/develop/devicetree/control.rst
> > > > b/doc/develop/devicetree/control.rst
> > > > index cbb65c9b177f..56e00090166f 100644
> > > > --- a/doc/develop/devicetree/control.rst
> > > > +++ b/doc/develop/devicetree/control.rst
> > > > @@ -108,6 +108,9 @@ If CONFIG_OF_BOARD is defined, a board-specific
> > > > routine will provide the
> > > > devicetree at runtime, for example if an earlier bootloader stage
> > > > creates
> > > > it and passes it to U-Boot.
> > > >
> > > > +If CONFIG_OF_BLOBLIST is defined, the devicetree comes from a bloblist
> > > > passed
> > > > +from a previous stage.
> > >
> > > What I argued before is that we don't need to be this explicit.
> > > The bloblist can carry a bunch of options that might be used by
> > > U-Boot. It would be better if we had a more generic approach instead
> > > of adding Kconfig options per bloblist entry
> >
> > Yes, fair enough. It would really get out of hand if we had a lot of
> > Kconfig options for everything that could be in a bloblist.
> >
> > I believe someone objected to this in the other thread, so there may
> > be board-specific issues to resolve.
>
> I had the same objection to that first version as well
>
> >
> > >
> > > [...]
> > >
> > > > #ifndef USE_HOSTCC
> > > > +
> > > > +#define LOG_CATEGORY LOGC_DT
> > > > +
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -87,6 +91,7 @@ static const char *const fdt_src_name[] = {
> > > > [FDTSRC_BOARD] = "board",
> > > > [FDTSRC_EMBED] = "embed",
> > > > [FDTSRC_ENV] = "env",
> > > > + [FDTSRC_BLOBLIST] = "bloblist",
> > > > };
> > > >
> > > > const char *fdtdec_get_srcname(void)
> > > > @@ -1666,20 +1671,35 @@ int fdtdec_setup(void)
> > > > int ret;
> > > >
> > > > /* The devicetree is typically appended to U-Boot */
> > > > - if (IS_ENABLED(CONFIG_OF_SEPARATE)) {
> > > > - gd->fdt_blob = fdt_find_separate();
> > > > - gd->fdt_src = FDTSRC_SEPARATE;
> > > > - } else { /* embed dtb in ELF file for testing / development */
> > > > - gd->fdt_blob = dtb_dt_embedded();
> > > > - gd->fdt_src = FDTSRC_EMBED;
> > > > - }
> > > > -
> > > > - /* Allow the board to override the fdt address. */
> > > > - if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > > - gd->fdt_blob = board_fdt_blob_setup(&ret);
> > > > + if (CONFIG_IS_ENABLED(OF_BLOBLIST)) {
> > > > + ret = bloblist_maybe_init();
> > > > if (ret)
> > > > return ret;
> > > > - gd->fdt_src = FDTSRC_BOARD;
> > >
> > > So, instead of adding OF_BLOBLIST, just move this code under OF_BOARD,
> > > inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and
> > > the previous stage loader is supposed to provide a DT we can just
> > > throw an error and stop booting
> >
> > This is the bit I don't get.
> >
> > The OF_BOARD thing is a hack, in that the board can do what it likes.
> > It is our way of handling board-specific mechanisms.
> >
> > But I am wanting a standard mechanism, i.e. like 'standard passage', a
> > way of passing the DT through the phases.
> >
> > If I put this under OF_BOARD, then the board gets to override the
> > mechanism, so which is it?
>
> No, it's the other way around in my head. OF_BOARD description is 'a
> previous stage loader hands me over the DT', which is a superset of
> the bloblist.
> Whether it comes in a firmware handoff format, or a hacky register the
> previous bootloader filled in is a detail we have to deal with and we
> need to keep backwards compatibility.
>
> Maybe adding a coding snip would help
> if (IS_ENABLED(CONFIG_OF_BOARD)) {
> if (CONFIG_IS_ENABLED(BLOBLIST)) { <- This instead of OF_BLOBLIST
> ret = bloblist_maybe_init();
> if (ret)
> return ret;
> /* Dynamically scan for a DT in the bloblist. */
> gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
>
Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Simon
On Tue, 26 Sept 2023 at 14:37, Simon Glass wrote:
>
> Hi Ilias,
>
> On Mon, 25 Sept 2023 at 13:48, Ilias Apalodimas
> wrote:
> >
> > Hi Simon,
> >
> > I commented on the v1 thread, but let's continue the discussion here
> >
> > On Thu, 21 Sept 2023 at 04:58, Simon Glass wrote:
> > >
> > > Standard passage provides for a bloblist to be passed from one firmware
> > > phase to the next. That can be used to pass the devicetree along as well.
> > > Add an option to support this.
> > >
> > > Tests for this will be added as part of the Universal Payload work.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > Changes in v2:
> > > - No changes as it still seems unclear what should be done
> > >
> >
> > [...]
> >
> > >
> > > /* BLOBLISTT_VENDOR_AREA */
> > > diff --git a/doc/develop/devicetree/control.rst
> > > b/doc/develop/devicetree/control.rst
> > > index cbb65c9b177f..56e00090166f 100644
> > > --- a/doc/develop/devicetree/control.rst
> > > +++ b/doc/develop/devicetree/control.rst
> > > @@ -108,6 +108,9 @@ If CONFIG_OF_BOARD is defined, a board-specific
> > > routine will provide the
> > > devicetree at runtime, for example if an earlier bootloader stage creates
> > > it and passes it to U-Boot.
> > >
> > > +If CONFIG_OF_BLOBLIST is defined, the devicetree comes from a bloblist
> > > passed
> > > +from a previous stage.
> >
> > What I argued before is that we don't need to be this explicit.
> > The bloblist can carry a bunch of options that might be used by
> > U-Boot. It would be better if we had a more generic approach instead
> > of adding Kconfig options per bloblist entry
>
> Yes, fair enough. It would really get out of hand if we had a lot of
> Kconfig options for everything that could be in a bloblist.
>
> I believe someone objected to this in the other thread, so there may
> be board-specific issues to resolve.
I had the same objection to that first version as well
>
> >
> > [...]
> >
> > > #ifndef USE_HOSTCC
> > > +
> > > +#define LOG_CATEGORY LOGC_DT
> > > +
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -87,6 +91,7 @@ static const char *const fdt_src_name[] = {
> > > [FDTSRC_BOARD] = "board",
> > > [FDTSRC_EMBED] = "embed",
> > > [FDTSRC_ENV] = "env",
> > > + [FDTSRC_BLOBLIST] = "bloblist",
> > > };
> > >
> > > const char *fdtdec_get_srcname(void)
> > > @@ -1666,20 +1671,35 @@ int fdtdec_setup(void)
> > > int ret;
> > >
> > > /* The devicetree is typically appended to U-Boot */
> > > - if (IS_ENABLED(CONFIG_OF_SEPARATE)) {
> > > - gd->fdt_blob = fdt_find_separate();
> > > - gd->fdt_src = FDTSRC_SEPARATE;
> > > - } else { /* embed dtb in ELF file for testing / development */
> > > - gd->fdt_blob = dtb_dt_embedded();
> > > - gd->fdt_src = FDTSRC_EMBED;
> > > - }
> > > -
> > > - /* Allow the board to override the fdt address. */
> > > - if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > > - gd->fdt_blob = board_fdt_blob_setup(&ret);
> > > + if (CONFIG_IS_ENABLED(OF_BLOBLIST)) {
> > > + ret = bloblist_maybe_init();
> > > if (ret)
> > > return ret;
> > > - gd->fdt_src = FDTSRC_BOARD;
> >
> > So, instead of adding OF_BLOBLIST, just move this code under OF_BOARD,
> > inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and
> > the previous stage loader is supposed to provide a DT we can just
> > throw an error and stop booting
>
> This is the bit I don't get.
>
> The OF_BOARD thing is a hack, in that the board can do what it likes.
> It is our way of handling board-specific mechanisms.
>
> But I am wanting a standard mechanism, i.e. like 'standard passage', a
> way of passing the DT through the phases.
>
> If I put this under OF_BOARD, then the board gets to override the
> mechanism, so which is it?
No, it's the other way around in my head. OF_BOARD description is 'a
previous stage loader hands me over the DT', which is a superset of
the bloblist.
Whether it comes in a firmware handoff format, or a hacky register the
previous bootloader filled in is a detail we have to deal with and we
need to keep backwards compatibility.
Maybe adding a coding snip would help
if (IS_ENABLED(CONFIG_OF_BOARD)) {
if (CONFIG_IS_ENABLED(BLOBLIST)) { <- This instead of OF_BLOBLIST
ret = bloblist_maybe_init();
if (ret)
return ret;
/* Dynamically scan for a DT in the bloblist. */
gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
if (!gd->fdt_blob) {
printf("Not FDT found in bloblist\n");
bloblist_show_list();
// We can choose to not return an error here and keep
scanning in case the DT is in a register, but I am fine with both
return -ENOENT;
}
gd->fdt_src = FDTSRC_BLOBLIST;
bloblist_show_list(
Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Ilias,
On Mon, 25 Sept 2023 at 13:48, Ilias Apalodimas
wrote:
>
> Hi Simon,
>
> I commented on the v1 thread, but let's continue the discussion here
>
> On Thu, 21 Sept 2023 at 04:58, Simon Glass wrote:
> >
> > Standard passage provides for a bloblist to be passed from one firmware
> > phase to the next. That can be used to pass the devicetree along as well.
> > Add an option to support this.
> >
> > Tests for this will be added as part of the Universal Payload work.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > Changes in v2:
> > - No changes as it still seems unclear what should be done
> >
>
> [...]
>
> >
> > /* BLOBLISTT_VENDOR_AREA */
> > diff --git a/doc/develop/devicetree/control.rst
> > b/doc/develop/devicetree/control.rst
> > index cbb65c9b177f..56e00090166f 100644
> > --- a/doc/develop/devicetree/control.rst
> > +++ b/doc/develop/devicetree/control.rst
> > @@ -108,6 +108,9 @@ If CONFIG_OF_BOARD is defined, a board-specific routine
> > will provide the
> > devicetree at runtime, for example if an earlier bootloader stage creates
> > it and passes it to U-Boot.
> >
> > +If CONFIG_OF_BLOBLIST is defined, the devicetree comes from a bloblist
> > passed
> > +from a previous stage.
>
> What I argued before is that we don't need to be this explicit.
> The bloblist can carry a bunch of options that might be used by
> U-Boot. It would be better if we had a more generic approach instead
> of adding Kconfig options per bloblist entry
Yes, fair enough. It would really get out of hand if we had a lot of
Kconfig options for everything that could be in a bloblist.
I believe someone objected to this in the other thread, so there may
be board-specific issues to resolve.
>
> [...]
>
> > #ifndef USE_HOSTCC
> > +
> > +#define LOG_CATEGORY LOGC_DT
> > +
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -87,6 +91,7 @@ static const char *const fdt_src_name[] = {
> > [FDTSRC_BOARD] = "board",
> > [FDTSRC_EMBED] = "embed",
> > [FDTSRC_ENV] = "env",
> > + [FDTSRC_BLOBLIST] = "bloblist",
> > };
> >
> > const char *fdtdec_get_srcname(void)
> > @@ -1666,20 +1671,35 @@ int fdtdec_setup(void)
> > int ret;
> >
> > /* The devicetree is typically appended to U-Boot */
> > - if (IS_ENABLED(CONFIG_OF_SEPARATE)) {
> > - gd->fdt_blob = fdt_find_separate();
> > - gd->fdt_src = FDTSRC_SEPARATE;
> > - } else { /* embed dtb in ELF file for testing / development */
> > - gd->fdt_blob = dtb_dt_embedded();
> > - gd->fdt_src = FDTSRC_EMBED;
> > - }
> > -
> > - /* Allow the board to override the fdt address. */
> > - if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > - gd->fdt_blob = board_fdt_blob_setup(&ret);
> > + if (CONFIG_IS_ENABLED(OF_BLOBLIST)) {
> > + ret = bloblist_maybe_init();
> > if (ret)
> > return ret;
> > - gd->fdt_src = FDTSRC_BOARD;
>
> So, instead of adding OF_BLOBLIST, just move this code under OF_BOARD,
> inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and
> the previous stage loader is supposed to provide a DT we can just
> throw an error and stop booting
This is the bit I don't get.
The OF_BOARD thing is a hack, in that the board can do what it likes.
It is our way of handling board-specific mechanisms.
But I am wanting a standard mechanism, i.e. like 'standard passage', a
way of passing the DT through the phases.
If I put this under OF_BOARD, then the board gets to override the
mechanism, so which is it?
You say 'we can just throw and error' but what is the error? If the DT
is provided via the bloblist, there is no error. If it is not, I
already show an error and halt as you can see in the code below.
I guess I'm just confused about what you are saying here.
>
>
> > + gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
> > + if (!gd->fdt_blob) {
> > + printf("Not FDT found in bloblist\n");
> > + bloblist_show_list();
> > + return -ENOENT;
> > + }
>
> [...]
>
> Regards
> /Ilias
Regards,
Simon
Re: [PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Hi Simon,
I commented on the v1 thread, but let's continue the discussion here
On Thu, 21 Sept 2023 at 04:58, Simon Glass wrote:
>
> Standard passage provides for a bloblist to be passed from one firmware
> phase to the next. That can be used to pass the devicetree along as well.
> Add an option to support this.
>
> Tests for this will be added as part of the Universal Payload work.
>
> Signed-off-by: Simon Glass
> ---
>
> Changes in v2:
> - No changes as it still seems unclear what should be done
>
[...]
>
> /* BLOBLISTT_VENDOR_AREA */
> diff --git a/doc/develop/devicetree/control.rst
> b/doc/develop/devicetree/control.rst
> index cbb65c9b177f..56e00090166f 100644
> --- a/doc/develop/devicetree/control.rst
> +++ b/doc/develop/devicetree/control.rst
> @@ -108,6 +108,9 @@ If CONFIG_OF_BOARD is defined, a board-specific routine
> will provide the
> devicetree at runtime, for example if an earlier bootloader stage creates
> it and passes it to U-Boot.
>
> +If CONFIG_OF_BLOBLIST is defined, the devicetree comes from a bloblist passed
> +from a previous stage.
What I argued before is that we don't need to be this explicit.
The bloblist can carry a bunch of options that might be used by
U-Boot. It would be better if we had a more generic approach instead
of adding Kconfig options per bloblist entry
[...]
> #ifndef USE_HOSTCC
> +
> +#define LOG_CATEGORY LOGC_DT
> +
> #include
> +#include
> #include
> #include
> #include
> @@ -87,6 +91,7 @@ static const char *const fdt_src_name[] = {
> [FDTSRC_BOARD] = "board",
> [FDTSRC_EMBED] = "embed",
> [FDTSRC_ENV] = "env",
> + [FDTSRC_BLOBLIST] = "bloblist",
> };
>
> const char *fdtdec_get_srcname(void)
> @@ -1666,20 +1671,35 @@ int fdtdec_setup(void)
> int ret;
>
> /* The devicetree is typically appended to U-Boot */
> - if (IS_ENABLED(CONFIG_OF_SEPARATE)) {
> - gd->fdt_blob = fdt_find_separate();
> - gd->fdt_src = FDTSRC_SEPARATE;
> - } else { /* embed dtb in ELF file for testing / development */
> - gd->fdt_blob = dtb_dt_embedded();
> - gd->fdt_src = FDTSRC_EMBED;
> - }
> -
> - /* Allow the board to override the fdt address. */
> - if (IS_ENABLED(CONFIG_OF_BOARD)) {
> - gd->fdt_blob = board_fdt_blob_setup(&ret);
> + if (CONFIG_IS_ENABLED(OF_BLOBLIST)) {
> + ret = bloblist_maybe_init();
> if (ret)
> return ret;
> - gd->fdt_src = FDTSRC_BOARD;
So, instead of adding OF_BLOBLIST, just move this code under OF_BOARD,
inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and
the previous stage loader is supposed to provide a DT we can just
throw an error and stop booting
> + gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
> + if (!gd->fdt_blob) {
> + printf("Not FDT found in bloblist\n");
> + bloblist_show_list();
> + return -ENOENT;
> + }
[...]
Regards
/Ilias
