[U-Boot] fdt: Add warning about CONFIG_OF_EMBED

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
Related show

Commit Message

Simon Glass Dec. 5, 2018, 1:57 p.m.
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(+)

Comments

Simon Goldschmidt Dec. 5, 2018, 2:22 p.m. | #1
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
>
Simon Glass Dec. 5, 2018, 3:57 p.m. | #2
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
Simon Goldschmidt Dec. 5, 2018, 4:08 p.m. | #3
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
Tom Rini Dec. 5, 2018, 4:32 p.m. | #4
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?
Tom Rini Dec. 6, 2018, 1:32 a.m. | #5
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>
Tom Rini Dec. 13, 2018, 12:47 a.m. | #6
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!

Patch

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