| Message ID | 20181205135708.58002-1-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] fdt: Add warning about CONFIG_OF_EMBED | expand |
Am 05.12.2018 um 14:57 schrieb Simon Glass: > This option has crept into use with some boards. Add a warning to try to > prevent this. > > As an example: > https://lists.denx.de/pipermail/u-boot/2017-September/304966.html We have just discussed this in another thread. There seem to be ~109 defconfigs in the tree that enable OF_EMBED. I doubt all of them do this for fun, so we might want to collect the reasons they do so. I do know two: - socfpga_stratix10_defconfig needs this to get a correct u-boot-spl.hex - I would need it to ensure in SPL, the DTB is in one block with the other readonly parts. Without OF_EMBED, we have '.text', '.bss', DT. Regards, Simon > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Makefile | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/Makefile b/Makefile > index 0d11ff97971..05896598fe3 100644 > --- a/Makefile > +++ b/Makefile > @@ -947,6 +947,14 @@ ifeq ($(CONFIG_LIBATA)$(CONFIG_DM_SCSI)$(CONFIG_MVSATA_IDE),y) > @echo "Failure to update by the deadline may result in board removal." > @echo "See doc/driver-model/MIGRATION.txt for more info." > @echo "====================================================" > +endif > +ifeq ($(CONFIG_OF_EMBED),y) > + @echo "===================== WARNING ======================" > + @echo "CONFIG_OF_EMBED is enabled. This option should only" > + @echo "be used for debugging purposes. Please use" > + @echo "CONFIG_OF_SEPARATE for boards in mainline." > + @echo "See doc/README.fdt-control for more info." > + @echo "====================================================" > endif > @# Check that this build does not use CONFIG options that we do not > @# know about unless they are in Kconfig. All the existing CONFIG >
Hi Simon, On Wed, 5 Dec 2018 at 07:22, Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> wrote: > > Am 05.12.2018 um 14:57 schrieb Simon Glass: > > This option has crept into use with some boards. Add a warning to try to > > prevent this. > > > > As an example: > > https://lists.denx.de/pipermail/u-boot/2017-September/304966.html > > We have just discussed this in another thread. There seem to be ~109 > defconfigs in the tree that enable OF_EMBED. > > I doubt all of them do this for fun, so we might want to collect the > reasons they do so. I do know two: > > - socfpga_stratix10_defconfig needs this to get a correct u-boot-spl.hex Let's fix that then. > - I would need it to ensure in SPL, the DTB is in one block with the > other readonly parts. Without OF_EMBED, we have '.text', '.bss', DT. Similarly, let's change that. CONFIG_OF_EMBED should not be a work-around for missing features that we need. Regards, Simon
Hi Simon, Am 05.12.2018 um 16:57 schrieb Simon Glass: > Hi Simon, > > On Wed, 5 Dec 2018 at 07:22, Simon Goldschmidt > <simon.k.r.goldschmidt@gmail.com> wrote: >> >> Am 05.12.2018 um 14:57 schrieb Simon Glass: >>> This option has crept into use with some boards. Add a warning to try to >>> prevent this. >>> >>> As an example: >>> https://lists.denx.de/pipermail/u-boot/2017-September/304966.html >> >> We have just discussed this in another thread. There seem to be ~109 >> defconfigs in the tree that enable OF_EMBED. >> >> I doubt all of them do this for fun, so we might want to collect the >> reasons they do so. I do know two: >> >> - socfpga_stratix10_defconfig needs this to get a correct u-boot-spl.hex > > Let's fix that then. > >> - I would need it to ensure in SPL, the DTB is in one block with the >> other readonly parts. Without OF_EMBED, we have '.text', '.bss', DT. > > Similarly, let's change that. > > CONFIG_OF_EMBED should not be a work-around for missing features that we need. I'm with you there. Don't get me wrong, I don't want to argue to use OF_EMBED, just wanted to list the use cases I know so that we only fix them once instead of letting everyone fixing them on their own. Regards, Simon
On Wed, Dec 05, 2018 at 05:08:53PM +0100, Simon Goldschmidt wrote: > Hi Simon, > > Am 05.12.2018 um 16:57 schrieb Simon Glass: > >Hi Simon, > > > >On Wed, 5 Dec 2018 at 07:22, Simon Goldschmidt > ><simon.k.r.goldschmidt@gmail.com> wrote: > >> > >>Am 05.12.2018 um 14:57 schrieb Simon Glass: > >>>This option has crept into use with some boards. Add a warning to try to > >>>prevent this. > >>> > >>>As an example: > >>> https://lists.denx.de/pipermail/u-boot/2017-September/304966.html > >> > >>We have just discussed this in another thread. There seem to be ~109 > >>defconfigs in the tree that enable OF_EMBED. > >> > >>I doubt all of them do this for fun, so we might want to collect the > >>reasons they do so. I do know two: > >> > >>- socfpga_stratix10_defconfig needs this to get a correct u-boot-spl.hex > > > >Let's fix that then. > > > >>- I would need it to ensure in SPL, the DTB is in one block with the > >>other readonly parts. Without OF_EMBED, we have '.text', '.bss', DT. > > > >Similarly, let's change that. > > > >CONFIG_OF_EMBED should not be a work-around for missing features that we need. > > I'm with you there. Don't get me wrong, I don't want to argue to use > OF_EMBED, just wanted to list the use cases I know so that we only fix them > once instead of letting everyone fixing them on their own. Yes, please, what is the way to fix say this one board? And can we then from there try and (semi)automate changing over?
On Wed, Dec 05, 2018 at 06:57:08AM -0700, Simon Glass wrote: > This option has crept into use with some boards. Add a warning to try to > prevent this. > > As an example: > https://lists.denx.de/pipermail/u-boot/2017-September/304966.html > > Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Tom Rini <trini@konsulko.com>
On Wed, Dec 05, 2018 at 06:57:08AM -0700, Simon Glass wrote: > This option has crept into use with some boards. Add a warning to try to > prevent this. > > As an example: > https://lists.denx.de/pipermail/u-boot/2017-September/304966.html > > Signed-off-by: Simon Glass <sjg@chromium.org> > Reviewed-by: Tom Rini <trini@konsulko.com> Applied to u-boot/master, thanks!
Dear Simon and Simon, > Am 05.12.2018 um 14:57 schrieb Simon Glass: > > This option has crept into use with some boards. Add a warning to > > try to prevent this. > > > > As an example: > > https://lists.denx.de/pipermail/u-boot/2017-September/304966.html > > We have just discussed this in another thread. There seem to be ~109 > defconfigs in the tree that enable OF_EMBED. > > I doubt all of them do this for fun, so we might want to collect the > reasons they do so. I do know two: > > - socfpga_stratix10_defconfig needs this to get a correct > u-boot-spl.hex > - I would need it to ensure in SPL, the DTB is in one block with the > other readonly parts. Without OF_EMBED, we have '.text', '.bss', DT. Another use case is imx6: When I want to use SPL's embedded DTB for early init, with OF_SEPARATE I get fdt_blob = (ulong *)&__bss_end; ,which points me to 0x18200060, which is in DDR (and there is either garbage nor DDR is not initialized at all). The above looks like the __bss_end is for u-boot proper. The workaround (for now) is to have OF_EMBED, which assigns gd->fdt_blob = __dtb_dt_spl_begin [1], and this is a correct address. Hence, I'm wondering if shall I assign: gd->fdt_blob = __dtb_dt_spl_begin; in my early SPL code (and do not use fdt_setup() function at all in SPL) ? Moreover, defining board_fdt_blob_setup() for _each_ IMX6 board which would switch to DM_MMC in SPL seems like an no acceptable solution. [1] - ./spl/dts/dt-spl.dtb.S:3:.global __dtb_dt_spl_begin --> Looks like __dtb_dt_spl_begin is exactly for this purpose. > > Regards, > Simon > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > Makefile | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/Makefile b/Makefile > > index 0d11ff97971..05896598fe3 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -947,6 +947,14 @@ ifeq > > ($(CONFIG_LIBATA)$(CONFIG_DM_SCSI)$(CONFIG_MVSATA_IDE),y) @echo > > "Failure to update by the deadline may result in board removal." > > @echo "See doc/driver-model/MIGRATION.txt for more info." @echo > > "====================================================" +endif > > +ifeq ($(CONFIG_OF_EMBED),y) > > + @echo "===================== WARNING > > ======================" > > + @echo "CONFIG_OF_EMBED is enabled. This option should only" > > + @echo "be used for debugging purposes. Please use" > > + @echo "CONFIG_OF_SEPARATE for boards in mainline." > > + @echo "See doc/README.fdt-control for more info." > > + @echo > > "====================================================" endif > > @# Check that this build does not use CONFIG options that > > we do not @# know about unless they are in Kconfig. All the > > existing CONFIG > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, On Wed, 19 Dec 2018 at 03:11, Lukasz Majewski <lukma@denx.de> wrote: > > Dear Simon and Simon, > > > Am 05.12.2018 um 14:57 schrieb Simon Glass: > > > This option has crept into use with some boards. Add a warning to > > > try to prevent this. > > > > > > As an example: > > > https://lists.denx.de/pipermail/u-boot/2017-September/304966.html > > > > We have just discussed this in another thread. There seem to be ~109 > > defconfigs in the tree that enable OF_EMBED. > > > > I doubt all of them do this for fun, so we might want to collect the > > reasons they do so. I do know two: > > > > - socfpga_stratix10_defconfig needs this to get a correct > > u-boot-spl.hex > > - I would need it to ensure in SPL, the DTB is in one block with the > > other readonly parts. Without OF_EMBED, we have '.text', '.bss', DT. > > Another use case is imx6: > > When I want to use SPL's embedded DTB for early init, with OF_SEPARATE > I get fdt_blob = (ulong *)&__bss_end; ,which points me to 0x18200060, > which is in DDR (and there is either garbage nor DDR is not > initialized at all). > > The above looks like the __bss_end is for u-boot proper. > > The workaround (for now) is to have OF_EMBED, which assigns > gd->fdt_blob = __dtb_dt_spl_begin [1], and this is a correct address. > > Hence, I'm wondering if shall I assign: > gd->fdt_blob = __dtb_dt_spl_begin; > > in my early SPL code (and do not use fdt_setup() function at all in > SPL) ? Or you could update fdt_setup() to do what you want (perhaps with a new CONFIG?). > > Moreover, defining board_fdt_blob_setup() for _each_ IMX6 board which > would switch to DM_MMC in SPL seems like an no acceptable solution. > > > [1] - ./spl/dts/dt-spl.dtb.S:3:.global __dtb_dt_spl_begin --> Looks > like __dtb_dt_spl_begin is exactly for this purpose. Regards, Simon
Hi Simon, > Hi Lukasz, > > On Wed, 19 Dec 2018 at 03:11, Lukasz Majewski <lukma@denx.de> wrote: > > > > Dear Simon and Simon, > > > > > Am 05.12.2018 um 14:57 schrieb Simon Glass: > > > > This option has crept into use with some boards. Add a warning > > > > to try to prevent this. > > > > > > > > As an example: > > > > https://lists.denx.de/pipermail/u-boot/2017-September/304966.html > > > > > > We have just discussed this in another thread. There seem to be > > > ~109 defconfigs in the tree that enable OF_EMBED. > > > > > > I doubt all of them do this for fun, so we might want to collect > > > the reasons they do so. I do know two: > > > > > > - socfpga_stratix10_defconfig needs this to get a correct > > > u-boot-spl.hex > > > - I would need it to ensure in SPL, the DTB is in one block with > > > the other readonly parts. Without OF_EMBED, we have '.text', > > > '.bss', DT. > > > > Another use case is imx6: > > > > When I want to use SPL's embedded DTB for early init, with > > OF_SEPARATE I get fdt_blob = (ulong *)&__bss_end; ,which points me > > to 0x18200060, which is in DDR (and there is either garbage nor DDR > > is not initialized at all). > > > > The above looks like the __bss_end is for u-boot proper. > > > > The workaround (for now) is to have OF_EMBED, which assigns > > gd->fdt_blob = __dtb_dt_spl_begin [1], and this is a correct > > address. > > > > Hence, I'm wondering if shall I assign: > > gd->fdt_blob = __dtb_dt_spl_begin; > > > > in my early SPL code (and do not use fdt_setup() function at all in > > SPL) ? > > Or you could update fdt_setup() to do what you want (perhaps with a > new CONFIG?). I've found the root cause of this problem.... For the board I had not defined CONFIG_SPL_SEPARATE_BSS. After providing it the OF_SEPARATE can be used (and dtb is appended between SPL binary and bss (in SDRAM) region. > > > > > Moreover, defining board_fdt_blob_setup() for _each_ IMX6 board > > which would switch to DM_MMC in SPL seems like an no acceptable > > solution. > > > > > > [1] - ./spl/dts/dt-spl.dtb.S:3:.global __dtb_dt_spl_begin --> Looks > > like __dtb_dt_spl_begin is exactly for this purpose. > > Regards, > Simon Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, On Mon, 31 Dec 2018 at 04:26, Lukasz Majewski <lukma@denx.de> wrote: > > Hi Simon, > > > Hi Lukasz, > > > > On Wed, 19 Dec 2018 at 03:11, Lukasz Majewski <lukma@denx.de> wrote: > > > > > > Dear Simon and Simon, > > > > > > > Am 05.12.2018 um 14:57 schrieb Simon Glass: > > > > > This option has crept into use with some boards. Add a warning > > > > > to try to prevent this. > > > > > > > > > > As an example: > > > > > https://lists.denx.de/pipermail/u-boot/2017-September/304966.html > > > > > > > > We have just discussed this in another thread. There seem to be > > > > ~109 defconfigs in the tree that enable OF_EMBED. > > > > > > > > I doubt all of them do this for fun, so we might want to collect > > > > the reasons they do so. I do know two: > > > > > > > > - socfpga_stratix10_defconfig needs this to get a correct > > > > u-boot-spl.hex > > > > - I would need it to ensure in SPL, the DTB is in one block with > > > > the other readonly parts. Without OF_EMBED, we have '.text', > > > > '.bss', DT. > > > > > > Another use case is imx6: > > > > > > When I want to use SPL's embedded DTB for early init, with > > > OF_SEPARATE I get fdt_blob = (ulong *)&__bss_end; ,which points me > > > to 0x18200060, which is in DDR (and there is either garbage nor DDR > > > is not initialized at all). > > > > > > The above looks like the __bss_end is for u-boot proper. > > > > > > The workaround (for now) is to have OF_EMBED, which assigns > > > gd->fdt_blob = __dtb_dt_spl_begin [1], and this is a correct > > > address. > > > > > > Hence, I'm wondering if shall I assign: > > > gd->fdt_blob = __dtb_dt_spl_begin; > > > > > > in my early SPL code (and do not use fdt_setup() function at all in > > > SPL) ? > > > > Or you could update fdt_setup() to do what you want (perhaps with a > > new CONFIG?). > > I've found the root cause of this problem.... > > For the board I had not defined CONFIG_SPL_SEPARATE_BSS. After > providing it the OF_SEPARATE can be used (and dtb is appended between > SPL binary and bss (in SDRAM) region. OK that's great news, thank you for digging into it. Regards, Simon > > > > > > > > > Moreover, defining board_fdt_blob_setup() for _each_ IMX6 board > > > which would switch to DM_MMC in SPL seems like an no acceptable > > > solution. > > > > > > > > > [1] - ./spl/dts/dt-spl.dtb.S:3:.global __dtb_dt_spl_begin --> Looks > > > like __dtb_dt_spl_begin is exactly for this purpose. > > > > Regards, > > Simon > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/Makefile b/Makefile index 0d11ff97971..05896598fe3 100644 --- a/Makefile +++ b/Makefile @@ -947,6 +947,14 @@ ifeq ($(CONFIG_LIBATA)$(CONFIG_DM_SCSI)$(CONFIG_MVSATA_IDE),y) @echo "Failure to update by the deadline may result in board removal." @echo "See doc/driver-model/MIGRATION.txt for more info." @echo "====================================================" +endif +ifeq ($(CONFIG_OF_EMBED),y) + @echo "===================== WARNING ======================" + @echo "CONFIG_OF_EMBED is enabled. This option should only" + @echo "be used for debugging purposes. Please use" + @echo "CONFIG_OF_SEPARATE for boards in mainline." + @echo "See doc/README.fdt-control for more info." + @echo "====================================================" endif @# Check that this build does not use CONFIG options that we do not @# know about unless they are in Kconfig. All the existing CONFIG
This option has crept into use with some boards. Add a warning to try to prevent this. As an example: https://lists.denx.de/pipermail/u-boot/2017-September/304966.html Signed-off-by: Simon Glass <sjg@chromium.org> --- Makefile | 8 ++++++++ 1 file changed, 8 insertions(+)