| Message ID | 20211026002344.405160-27-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fdt: Make OF_BOARD a boolean option | expand |
Hi Simon,
As I said here [1], this is moving on an entirely different direction I had
in mind. I'd much prefer starting the discussions for a solution that
allows us to scale.
FWIW I think the current code is still not clean for my taste. Commit
3b595da441cf ("fdtdec: allow board to provide fdt for CONFIG_OF_SEPARATE")
allowed this function to be used regardless of the config options. IMHO we
should have 2 clear options:
- U-Boot provides the DTB
- It's somehow passed over to U-Boot
On Mon, Oct 25, 2021 at 06:23:44PM -0600, Simon Glass wrote:
> At present this override function is called even when OF_BOARD Is not
> enabled. This makes it impossible to disable this feature and in fact
> makes the OF_BOARD option useless.
>
> Reinstate its intended purpose, so that it is possible to switch between
> the appended devicetree and one provided by the board's custom function.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v5:
> - Add new patches to clean up fdtdec_setup() and surrounds
>
> include/fdtdec.h | 7 +++++--
> lib/fdtdec.c | 17 +++++++++++------
> 2 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index 386f6611294..b2faa84008e 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -1170,8 +1170,11 @@ int fdtdec_resetup(int *rescan);
>
> /**
> * Board-specific FDT initialization. Returns the address to a device tree blob.
> - * Called when CONFIG_OF_BOARD is defined, or if CONFIG_OF_SEPARATE is defined
> - * and the board implements it.
> + * Called when CONFIG_OF_BOARD is defined.
> + *
> + * The existing devicetree is available at gd->fdt_blob
> + *
> + * @returns new devicetree blob pointer
> */
> void *board_fdt_blob_setup(void);
>
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 067c27d0aa3..da36dffec62 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1203,11 +1203,12 @@ static int uncompress_blob(const void *src, ulong sz_src, void **dstp)
> return 0;
> }
>
> -/*
> - * For CONFIG_OF_SEPARATE, the board may optionally implement this to
> - * provide and/or fixup the fdt.
> +/**
> + * fdt_find_separate() - Find a devicetree at the end of the image
> + *
> + * @return pointer to FDT blob
> */
> -__weak void *board_fdt_blob_setup(void)
> +static void *fdt_find_separate(void)
> {
> void *fdt_blob = NULL;
> #ifdef CONFIG_SPL_BUILD
> @@ -1623,11 +1624,15 @@ int fdtdec_setup(void)
> int ret;
>
> /* The devicetree is typically appended to U-Boot */
> - if (IS_ENABLED(CONFIG_OF_SEPARATE) || IS_ENABLED(CONFIG_OF_BOARD))
> - gd->fdt_blob = board_fdt_blob_setup();
> + if (IS_ENABLED(CONFIG_OF_SEPARATE))
> + gd->fdt_blob = fdt_find_separate();
> else /* embed dtb in ELF file for testing / development */
> gd->fdt_blob = dtb_dt_embedded();
>
> + /* Allow the board to override the fdt address. */
> + if (IS_ENABLED(CONFIG_OF_BOARD))
> + gd->fdt_blob = board_fdt_blob_setup();
> +
> if (!IS_ENABLED(CONFIG_SPL_BUILD)) {
> /* Allow the early environment to override the fdt address */
> gd->fdt_blob = map_sysmem(env_get_ulong("fdtcontroladdr", 16,
> --
> 2.33.0.1079.g6e70778dc9-goog
>
[1] https://lore.kernel.org/u-boot/YXekTkeL73NM0UOU@apalos.home/
Regards
/Ilias
Hi Ilias, On Tue, 26 Oct 2021 at 07:56, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > As I said here [1], this is moving on an entirely different direction I had > in mind. I'd much prefer starting the discussions for a solution that > allows us to scale. I am missing the point here. Is there something in the plans that I don't know about? > FWIW I think the current code is still not clean for my taste. Commit > 3b595da441cf ("fdtdec: allow board to provide fdt for CONFIG_OF_SEPARATE") > allowed this function to be used regardless of the config options. IMHO we > should have 2 clear options: > - U-Boot provides the DTB Supported with: OF_SEPARATE > - It's somehow passed over to U-Boot Supported with: OF_SEPARATE + OF_BOARD I actually hit this problem with my qemu testing, in that it is actually not possible to use U-Boot's devicetree without hacking the code. We need to be able to select this feature explicitly, or turn it off, at least for development. Regards, Simon > > On Mon, Oct 25, 2021 at 06:23:44PM -0600, Simon Glass wrote: > > At present this override function is called even when OF_BOARD Is not > > enabled. This makes it impossible to disable this feature and in fact > > makes the OF_BOARD option useless. > > > > Reinstate its intended purpose, so that it is possible to switch between > > the appended devicetree and one provided by the board's custom function. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > Changes in v5: > > - Add new patches to clean up fdtdec_setup() and surrounds > > > > include/fdtdec.h | 7 +++++-- > > lib/fdtdec.c | 17 +++++++++++------ > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/include/fdtdec.h b/include/fdtdec.h > > index 386f6611294..b2faa84008e 100644 > > --- a/include/fdtdec.h > > +++ b/include/fdtdec.h > > @@ -1170,8 +1170,11 @@ int fdtdec_resetup(int *rescan); > > > > /** > > * Board-specific FDT initialization. Returns the address to a device tree blob. > > - * Called when CONFIG_OF_BOARD is defined, or if CONFIG_OF_SEPARATE is defined > > - * and the board implements it. > > + * Called when CONFIG_OF_BOARD is defined. > > + * > > + * The existing devicetree is available at gd->fdt_blob > > + * > > + * @returns new devicetree blob pointer > > */ > > void *board_fdt_blob_setup(void); > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > index 067c27d0aa3..da36dffec62 100644 > > --- a/lib/fdtdec.c > > +++ b/lib/fdtdec.c > > @@ -1203,11 +1203,12 @@ static int uncompress_blob(const void *src, ulong sz_src, void **dstp) > > return 0; > > } > > > > -/* > > - * For CONFIG_OF_SEPARATE, the board may optionally implement this to > > - * provide and/or fixup the fdt. > > +/** > > + * fdt_find_separate() - Find a devicetree at the end of the image > > + * > > + * @return pointer to FDT blob > > */ > > -__weak void *board_fdt_blob_setup(void) > > +static void *fdt_find_separate(void) > > { > > void *fdt_blob = NULL; > > #ifdef CONFIG_SPL_BUILD > > @@ -1623,11 +1624,15 @@ int fdtdec_setup(void) > > int ret; > > > > /* The devicetree is typically appended to U-Boot */ > > - if (IS_ENABLED(CONFIG_OF_SEPARATE) || IS_ENABLED(CONFIG_OF_BOARD)) > > - gd->fdt_blob = board_fdt_blob_setup(); > > + if (IS_ENABLED(CONFIG_OF_SEPARATE)) > > + gd->fdt_blob = fdt_find_separate(); > > else /* embed dtb in ELF file for testing / development */ > > gd->fdt_blob = dtb_dt_embedded(); > > > > + /* Allow the board to override the fdt address. */ > > + if (IS_ENABLED(CONFIG_OF_BOARD)) > > + gd->fdt_blob = board_fdt_blob_setup(); > > + > > if (!IS_ENABLED(CONFIG_SPL_BUILD)) { > > /* Allow the early environment to override the fdt address */ > > gd->fdt_blob = map_sysmem(env_get_ulong("fdtcontroladdr", 16, > > -- > > 2.33.0.1079.g6e70778dc9-goog > > > > [1] https://lore.kernel.org/u-boot/YXekTkeL73NM0UOU@apalos.home/ > > Regards > /Ilias
Hi Simon, On Tue, 26 Oct 2021 at 18:27, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Tue, 26 Oct 2021 at 07:56, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Simon, > > > > As I said here [1], this is moving on an entirely different direction I had > > in mind. I'd much prefer starting the discussions for a solution that > > allows us to scale. > > I am missing the point here. Is there something in the plans that I > don't know about? I have some ideas, but haven't found time to code it and send patches yet. > > > FWIW I think the current code is still not clean for my taste. Commit > > 3b595da441cf ("fdtdec: allow board to provide fdt for CONFIG_OF_SEPARATE") > > allowed this function to be used regardless of the config options. IMHO we > > should have 2 clear options: > > - U-Boot provides the DTB > > Supported with: OF_SEPARATE > > > - It's somehow passed over to U-Boot > > Supported with: OF_SEPARATE + OF_BOARD That's exactly what I don't personally like. In your example OF_BOARD means "U-Boot has a DTB and here's a way to override it". In my head that options means "U-Boot was handed over a DTB on a register and it must be able to deal with it". The latter is obviously less restrictive to previous bootloaders. > > I actually hit this problem with my qemu testing, in that it is > actually not possible to use U-Boot's devicetree without hacking the > code. We need to be able to select this feature explicitly, or turn it > off, at least for development. Alternatively we can just keep U-Boot config node in a .dtbo and apply it on the fly after QEMU has handed us over the DTB it created. Regards /Ilias > > Regards, > Simon > > > > > > On Mon, Oct 25, 2021 at 06:23:44PM -0600, Simon Glass wrote: > > > At present this override function is called even when OF_BOARD Is not > > > enabled. This makes it impossible to disable this feature and in fact > > > makes the OF_BOARD option useless. > > > > > > Reinstate its intended purpose, so that it is possible to switch between > > > the appended devicetree and one provided by the board's custom function. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > --- > > > > > > Changes in v5: > > > - Add new patches to clean up fdtdec_setup() and surrounds > > > > > > include/fdtdec.h | 7 +++++-- > > > lib/fdtdec.c | 17 +++++++++++------ > > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > > > diff --git a/include/fdtdec.h b/include/fdtdec.h > > > index 386f6611294..b2faa84008e 100644 > > > --- a/include/fdtdec.h > > > +++ b/include/fdtdec.h > > > @@ -1170,8 +1170,11 @@ int fdtdec_resetup(int *rescan); > > > > > > /** > > > * Board-specific FDT initialization. Returns the address to a device tree blob. > > > - * Called when CONFIG_OF_BOARD is defined, or if CONFIG_OF_SEPARATE is defined > > > - * and the board implements it. > > > + * Called when CONFIG_OF_BOARD is defined. > > > + * > > > + * The existing devicetree is available at gd->fdt_blob > > > + * > > > + * @returns new devicetree blob pointer > > > */ > > > void *board_fdt_blob_setup(void); > > > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > > index 067c27d0aa3..da36dffec62 100644 > > > --- a/lib/fdtdec.c > > > +++ b/lib/fdtdec.c > > > @@ -1203,11 +1203,12 @@ static int uncompress_blob(const void *src, ulong sz_src, void **dstp) > > > return 0; > > > } > > > > > > -/* > > > - * For CONFIG_OF_SEPARATE, the board may optionally implement this to > > > - * provide and/or fixup the fdt. > > > +/** > > > + * fdt_find_separate() - Find a devicetree at the end of the image > > > + * > > > + * @return pointer to FDT blob > > > */ > > > -__weak void *board_fdt_blob_setup(void) > > > +static void *fdt_find_separate(void) > > > { > > > void *fdt_blob = NULL; > > > #ifdef CONFIG_SPL_BUILD > > > @@ -1623,11 +1624,15 @@ int fdtdec_setup(void) > > > int ret; > > > > > > /* The devicetree is typically appended to U-Boot */ > > > - if (IS_ENABLED(CONFIG_OF_SEPARATE) || IS_ENABLED(CONFIG_OF_BOARD)) > > > - gd->fdt_blob = board_fdt_blob_setup(); > > > + if (IS_ENABLED(CONFIG_OF_SEPARATE)) > > > + gd->fdt_blob = fdt_find_separate(); > > > else /* embed dtb in ELF file for testing / development */ > > > gd->fdt_blob = dtb_dt_embedded(); > > > > > > + /* Allow the board to override the fdt address. */ > > > + if (IS_ENABLED(CONFIG_OF_BOARD)) > > > + gd->fdt_blob = board_fdt_blob_setup(); > > > + > > > if (!IS_ENABLED(CONFIG_SPL_BUILD)) { > > > /* Allow the early environment to override the fdt address */ > > > gd->fdt_blob = map_sysmem(env_get_ulong("fdtcontroladdr", 16, > > > -- > > > 2.33.0.1079.g6e70778dc9-goog > > > > > > > [1] https://lore.kernel.org/u-boot/YXekTkeL73NM0UOU@apalos.home/ > > > > Regards > > /Ilias
On Wed, Oct 27, 2021 at 10:17:05AM +0300, Ilias Apalodimas wrote: > Hi Simon, > > On Tue, 26 Oct 2021 at 18:27, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Tue, 26 Oct 2021 at 07:56, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > As I said here [1], this is moving on an entirely different direction I had > > > in mind. I'd much prefer starting the discussions for a solution that > > > allows us to scale. > > > > I am missing the point here. Is there something in the plans that I > > don't know about? > > I have some ideas, but haven't found time to code it and send patches yet. > > > > > > FWIW I think the current code is still not clean for my taste. Commit > > > 3b595da441cf ("fdtdec: allow board to provide fdt for CONFIG_OF_SEPARATE") > > > allowed this function to be used regardless of the config options. IMHO we > > > should have 2 clear options: > > > - U-Boot provides the DTB > > > > Supported with: OF_SEPARATE > > > > > - It's somehow passed over to U-Boot > > > > Supported with: OF_SEPARATE + OF_BOARD > > That's exactly what I don't personally like. In your example OF_BOARD > means "U-Boot has a DTB and here's a way to override it". In my head In some ways this is just highlighting a long standing problem of working with DTB. There's certainly been some places where splitting the data from the driver has helped. But in some other places, my goodness has it made the development cycle worse. Doing this on QEMU probably is on one of the most annoying cases too since yes, you start by dumping the dtb you would get, then iterating the dts outside of the rest of the scope of your build. In the kernel you can "make dtbs" at least and get things spit out. I'm just repeating over here that the development cycle of working on device trees is at times not great.
Hi Tom Le mer. 27 oct. 2021 à 21:12, Tom Rini <trini@konsulko.com> a écrit : > On Wed, Oct 27, 2021 at 10:17:05AM +0300, Ilias Apalodimas wrote: > > Hi Simon, > > > > On Tue, 26 Oct 2021 at 18:27, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Ilias, > > > > > > On Tue, 26 Oct 2021 at 07:56, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Simon, > > > > > > > > As I said here [1], this is moving on an entirely different > direction I had > > > > in mind. I'd much prefer starting the discussions for a solution that > > > > allows us to scale. > > > > > > I am missing the point here. Is there something in the plans that I > > > don't know about? > > > > I have some ideas, but haven't found time to code it and send patches > yet. > > > > > > > > > FWIW I think the current code is still not clean for my taste. > Commit > > > > 3b595da441cf ("fdtdec: allow board to provide fdt for > CONFIG_OF_SEPARATE") > > > > allowed this function to be used regardless of the config options. > IMHO we > > > > should have 2 clear options: > > > > - U-Boot provides the DTB > > > > > > Supported with: OF_SEPARATE > > > > > > > - It's somehow passed over to U-Boot > > > > > > Supported with: OF_SEPARATE + OF_BOARD > > > > That's exactly what I don't personally like. In your example OF_BOARD > > means "U-Boot has a DTB and here's a way to override it". In my head > > In some ways this is just highlighting a long standing problem of > working with DTB. There's certainly been some places where splitting > the data from the driver has helped. But in some other places, my > goodness has it made the development cycle worse. Doing this on QEMU > probably is on one of the most annoying cases too since yes, you start > by dumping the dtb you would get, then iterating the dts outside of the > rest of the scope of your build. In the kernel you can "make dtbs" at > least and get things spit out. I'm just repeating over here that the > development cycle of working on device trees is at times not great. In general you do this only once, unless you use the virt machine and in that case there is no documentation about DT generated for each device added on the command line. I had lots of difficulties getting all UART possibilities: virtio <1.0, 1.0,1.1; mmio or pci. But once you get the command line, the dTB is fixed. Of course it would be better to get Qemu document DT generation of its devices. > > > -- > Tom >
On Wed, Oct 27, 2021 at 11:33:53PM +0200, François Ozog wrote: > Hi Tom > > Le mer. 27 oct. 2021 à 21:12, Tom Rini <trini@konsulko.com> a écrit : > > > On Wed, Oct 27, 2021 at 10:17:05AM +0300, Ilias Apalodimas wrote: > > > Hi Simon, > > > > > > On Tue, 26 Oct 2021 at 18:27, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Ilias, > > > > > > > > On Tue, 26 Oct 2021 at 07:56, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > As I said here [1], this is moving on an entirely different > > direction I had > > > > > in mind. I'd much prefer starting the discussions for a solution that > > > > > allows us to scale. > > > > > > > > I am missing the point here. Is there something in the plans that I > > > > don't know about? > > > > > > I have some ideas, but haven't found time to code it and send patches > > yet. > > > > > > > > > > > > FWIW I think the current code is still not clean for my taste. > > Commit > > > > > 3b595da441cf ("fdtdec: allow board to provide fdt for > > CONFIG_OF_SEPARATE") > > > > > allowed this function to be used regardless of the config options. > > IMHO we > > > > > should have 2 clear options: > > > > > - U-Boot provides the DTB > > > > > > > > Supported with: OF_SEPARATE > > > > > > > > > - It's somehow passed over to U-Boot > > > > > > > > Supported with: OF_SEPARATE + OF_BOARD > > > > > > That's exactly what I don't personally like. In your example OF_BOARD > > > means "U-Boot has a DTB and here's a way to override it". In my head > > > > In some ways this is just highlighting a long standing problem of > > working with DTB. There's certainly been some places where splitting > > the data from the driver has helped. But in some other places, my > > goodness has it made the development cycle worse. Doing this on QEMU > > probably is on one of the most annoying cases too since yes, you start > > by dumping the dtb you would get, then iterating the dts outside of the > > rest of the scope of your build. In the kernel you can "make dtbs" at > > least and get things spit out. I'm just repeating over here that the > > development cycle of working on device trees is at times not great. > > In general you do this only once, unless you use the virt machine and in > that case there is no documentation about DT generated for each device > added on the command line. > I had lots of difficulties getting all UART possibilities: virtio <1.0, > 1.0,1.1; mmio or pci. > But once you get the command line, the dTB is fixed. > Of course it would be better to get Qemu document DT generation of its > devices. In a lot of ways QEMU isn't even the real problem, unless it's applying rand() to some parts of where devices end up, etc. The cycle there isn't any worse than where the device tree resides in hardware. The device tree development cycle is a pain. QEMU is actually not so bad in this regard really since it's just dumpdtb and pass back the modified one, it takes care of then ensuring it's in the correct spot. The part where every blob must be in the right place is another part of that horror show that binman and fconf both try and address.
diff --git a/include/fdtdec.h b/include/fdtdec.h index 386f6611294..b2faa84008e 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -1170,8 +1170,11 @@ int fdtdec_resetup(int *rescan); /** * Board-specific FDT initialization. Returns the address to a device tree blob. - * Called when CONFIG_OF_BOARD is defined, or if CONFIG_OF_SEPARATE is defined - * and the board implements it. + * Called when CONFIG_OF_BOARD is defined. + * + * The existing devicetree is available at gd->fdt_blob + * + * @returns new devicetree blob pointer */ void *board_fdt_blob_setup(void); diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 067c27d0aa3..da36dffec62 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1203,11 +1203,12 @@ static int uncompress_blob(const void *src, ulong sz_src, void **dstp) return 0; } -/* - * For CONFIG_OF_SEPARATE, the board may optionally implement this to - * provide and/or fixup the fdt. +/** + * fdt_find_separate() - Find a devicetree at the end of the image + * + * @return pointer to FDT blob */ -__weak void *board_fdt_blob_setup(void) +static void *fdt_find_separate(void) { void *fdt_blob = NULL; #ifdef CONFIG_SPL_BUILD @@ -1623,11 +1624,15 @@ int fdtdec_setup(void) int ret; /* The devicetree is typically appended to U-Boot */ - if (IS_ENABLED(CONFIG_OF_SEPARATE) || IS_ENABLED(CONFIG_OF_BOARD)) - gd->fdt_blob = board_fdt_blob_setup(); + if (IS_ENABLED(CONFIG_OF_SEPARATE)) + gd->fdt_blob = fdt_find_separate(); else /* embed dtb in ELF file for testing / development */ gd->fdt_blob = dtb_dt_embedded(); + /* Allow the board to override the fdt address. */ + if (IS_ENABLED(CONFIG_OF_BOARD)) + gd->fdt_blob = board_fdt_blob_setup(); + if (!IS_ENABLED(CONFIG_SPL_BUILD)) { /* Allow the early environment to override the fdt address */ gd->fdt_blob = map_sysmem(env_get_ulong("fdtcontroladdr", 16,
At present this override function is called even when OF_BOARD Is not enabled. This makes it impossible to disable this feature and in fact makes the OF_BOARD option useless. Reinstate its intended purpose, so that it is possible to switch between the appended devicetree and one provided by the board's custom function. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v5: - Add new patches to clean up fdtdec_setup() and surrounds include/fdtdec.h | 7 +++++-- lib/fdtdec.c | 17 +++++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-)