fdt: Add warning about CONFIG_OF_EMBED

PATCH In review Simon Glass 4 participants 11 messages Dec 5, 2018 – Dec 31, 2018 Kbuild

lore

Share & tools

lore

Simon Glass

author

2018-12-05 13:57 UTC #0 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

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 <[email protected]>

---

 Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

Makefile +8 -0

index 0d11ff97971..05896598fe3 100644
@@ -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

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Simon Goldschmidt

2018-12-05 14:22 UTC #1 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

Am 05.12.2018 um 14:57 schrieb Simon Glass:

Quoted 5 lines

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

Quoted 27 lines

Signed-off-by Simon Glass <[email protected]>

---

  Makefile | 8 ++++++++
  1 file changed, 8 insertions(+)

Makefile +8 -0

index 0d11ff97971..05896598fe3 100644
@@ -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

Simon Glass

author

2018-12-05 15:57 UTC #2 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

Hi Simon,

Quoted 8 lines · Simon Goldschmidt

Am 05.12.2018 um 14:57 schrieb Simon Glass:
Quoted 5 lines
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

Quoted 7 lines

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.

Quoted 2 lines

- 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

Simon Goldschmidt

2018-12-05 16:08 UTC #3 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

Hi Simon,

Am 05.12.2018 um 16:57 schrieb Simon Glass:

Quoted 20 lines

Hi Simon,
Quoted 8 lines · Simon Goldschmidt
Am 05.12.2018 um 14:57 schrieb Simon Glass:
Quoted 5 lines
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
Quoted 7 lines
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

Quoted 8 lines

Let's fix that then.
Quoted 2 lines
- 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

Tom Rini

2018-12-05 16:32 UTC #4 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

Quoted 28 lines · Simon Goldschmidt

Hi Simon,
Am 05.12.2018 um 16:57 schrieb Simon Glass:
Quoted 25 lines
Hi Simon,
Quoted 8 lines · Simon Goldschmidt
Am 05.12.2018 um 14:57 schrieb Simon Glass:
Quoted 5 lines
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
Quoted 7 lines
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.
Quoted 2 lines
- 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.

Quoted 7 lines

Quoted 3 lines
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?

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181205/80e724b2/attachment.sig>

Lukasz Majewski

2018-12-19 10:11 UTC #5 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

In reply to Simon Goldschmidt

Dear Simon and Simon,

Quoted 10 lines

Am 05.12.2018 um 14:57 schrieb Simon Glass:
Quoted 5 lines
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.

Quoted 7 lines

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.

Quoted 29 lines

Regards,
Simon
Quoted 25 lines
Signed-off-by Simon Glass <[email protected]>
---
  Makefile | 8 ++++++++
  1 file changed, 8 insertions(+)
Makefile +0 -0
index 0d11ff97971..05896598fe3 100644
@@ -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
Makefile +2 -0
+ifeq ($(CONFIG_OF_EMBED),y)
+	@echo "===================== WARNING
======================"
Makefile +5 -0
+	@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

Quoted 8 lines

Quoted 3 lines
@# 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 at 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 at denx.de -------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181219/7750db7f/attachment.sig>

Simon Glass

author

2018-12-27 23:29 UTC #6 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

Hi Lukasz,

Quoted 30 lines · Lukasz Majewski

Dear Simon and Simon,
Quoted 10 lines
Am 05.12.2018 um 14:57 schrieb Simon Glass:
Quoted 5 lines
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.
Quoted 7 lines
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.

Quoted 8 lines

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?).

Quoted 7 lines

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

Lukasz Majewski

2018-12-31 11:26 UTC #7 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

Hi Simon,

Quoted 38 lines

Hi Lukasz,
Quoted 35 lines · Lukasz Majewski
Dear Simon and Simon,
Quoted 10 lines
Am 05.12.2018 um 14:57 schrieb Simon Glass:
Quoted 5 lines
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.
Quoted 8 lines
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.

Quoted 8 lines

Quoted 5 lines
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.

Quoted 7 lines

Quoted 7 lines
Moreover, defining board_fdt_blob_setup() for _each_ IMX6 board which would switch to DM_MMC in SPL seems like an no acceptable solution.

Quoted 5 lines

Quoted 2 lines
[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

Simon Glass

author

2018-12-31 16:23 UTC #8 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

Hi Lukasz,

Quoted 47 lines · Lukasz Majewski

Hi Simon,
Quoted 44 lines
Hi Lukasz,
Quoted 40 lines · Lukasz Majewski
Dear Simon and Simon,
Quoted 10 lines
Am 05.12.2018 um 14:57 schrieb Simon Glass:
Quoted 5 lines
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.
Quoted 8 lines
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) ?

Quoted 8 lines

Quoted 2 lines
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

Quoted 26 lines

Quoted 8 lines
Quoted 8 lines
Moreover, defining board_fdt_blob_setup() for _each_ IMX6 board which would switch to DM_MMC in SPL seems like an no acceptable solution.
Quoted 5 lines
Quoted 2 lines
[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 at denx.de

Tom Rini

2018-12-06 01:32 UTC #9 lore raw

PATCH 1/1 fdt: Add warning about CONFIG_OF_EMBED

In reply to Simon Glass

Quoted 7 lines · 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
Signed-off-by Simon Glass <[email protected]>

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181205/84609add/attachment.sig>

Tom Rini

2018-12-13 00:47 UTC #10 lore raw

In reply to Simon Glass

Quoted 8 lines · 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
Signed-off-by Simon Glass <[email protected]>
Reviewed-by Tom Rini <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181212/e7cd13f0/attachment.sig>