diff mbox

[v3] u-boot: allow to pass a custom configuration file

Message ID 1399904527-8159-1-git-send-email-eric.jarrige@armadeus.org
State Rejected
Headers show

Commit Message

Eric Jarrige May 12, 2014, 2:22 p.m. UTC
Add an option BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE that makes it possible
to override the configuration options in the board header file. This
avoids the need for manipulating the board header file with sed hacks
like is currently done for the BR2_TARGET_UBOOT_NETWORK settings.

Note that this option does not make it possible to add a new board to
U-Boot. That still has to be done by patching the source.

Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
---

Changes v2 -> v3:
 - refactoring based on uClibc/BusyBox model (suggested by Thomas and Arnoult)
 - add missing help text (suggested by Arnoult)
 - copy custom configuration file to legal info (suggested by Yann)
Changes v1 -> v2:
 - Fix typo

 boot/uboot/Config.in |    8 ++++++++
 boot/uboot/uboot.mk  |   18 ++++++++++++++++++
 2 files changed, 26 insertions(+)

Comments

Thomas Petazzoni May 12, 2014, 7:41 p.m. UTC | #1
Dear Eric Jarrige,

On Mon, 12 May 2014 16:22:07 +0200, Eric Jarrige wrote:
> Add an option BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE that makes it possible
> to override the configuration options in the board header file. This
> avoids the need for manipulating the board header file with sed hacks
> like is currently done for the BR2_TARGET_UBOOT_NETWORK settings.
> 
> Note that this option does not make it possible to add a new board to
> U-Boot. That still has to be done by patching the source.
> 
> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
> ---
> 
> Changes v2 -> v3:
>  - refactoring based on uClibc/BusyBox model (suggested by Thomas and Arnoult)
>  - add missing help text (suggested by Arnoult)
>  - copy custom configuration file to legal info (suggested by Yann)
> Changes v1 -> v2:
>  - Fix typo
> 
>  boot/uboot/Config.in |    8 ++++++++
>  boot/uboot/uboot.mk  |   18 ++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index 1c77f6a..299d50f 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -79,6 +79,14 @@ config BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR
>  
>  	  Most users may leave this empty
>  
> +config BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE
> +	string "Configuration file path"
> +	help
> +	  Path to the modified configuration header file. It will be
> +	  copied to include/configs/<boardname>.h in the U-Boot sources.
> +	  Note that this doesn't allow you to create a custom board,
> +	  only to modify some of configuration variables.

I must say I'm still unconvinced by this patch. In U-Boot, the
configuration is completely mixed with the definition of the board, and
defining a board does not only require adding a file in
include/configs/, but also adding an entry in boards.cfg.

Therefore, I really believe that people willing to customize their
U-Boot board or configuration should instead carry a patch against
U-Boot instead of using this non-standard solution.

Moreover, it looks like U-Boot is progressively moving towards a
kbuild/kconfig approach (kbuild step is done, I guess kconfig is the
next step), which should on the long run make U-Boot closer to the
Linux kernel or Barebox in terms of configuration.

Best regards,

Thomas
Thomas Petazzoni June 11, 2014, 8:16 p.m. UTC | #2
Dear Eric Jarrige,

On Mon, 12 May 2014 16:22:07 +0200, Eric Jarrige wrote:
> Add an option BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE that makes it possible
> to override the configuration options in the board header file. This
> avoids the need for manipulating the board header file with sed hacks
> like is currently done for the BR2_TARGET_UBOOT_NETWORK settings.
> 
> Note that this option does not make it possible to add a new board to
> U-Boot. That still has to be done by patching the source.
> 
> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>

We just had a discussion on IRC with Peter Korsgaard, and he also
doesn't like much this solution:

22:16 < Jacmet> kos_tom: I also don't like 348092

He hasn't expressed the details on IRC, but I believe he shares similar
concerns to mine, i.e:

"""
I must say I'm still unconvinced by this patch. In U-Boot, the
configuration is completely mixed with the definition of the board, and
defining a board does not only require adding a file in
include/configs/, but also adding an entry in boards.cfg.

Therefore, I really believe that people willing to customize their
U-Boot board or configuration should instead carry a patch against
U-Boot instead of using this non-standard solution.
"""

Of course, if Peter has different/other reasons, I'm sure he will
give more details in this thread.

As a consequence, I'm marking the patch as Rejected in patchwork.

Thanks,

Thomas
Eric Jarrige July 2, 2014, 11:40 a.m. UTC | #3
Hi Thomas,

On 11 juin 2014, at 22:16, Thomas Petazzoni wrote:

> 
> Dear Eric Jarrige,
> 
> On Mon, 12 May 2014 16:22:07 +0200, Eric Jarrige wrote:
>> Add an option BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE that makes it possible
>> to override the configuration options in the board header file. This
>> avoids the need for manipulating the board header file with sed hacks
>> like is currently done for the BR2_TARGET_UBOOT_NETWORK settings.
>> 
>> Note that this option does not make it possible to add a new board to
>> U-Boot. That still has to be done by patching the source.
>> 
>> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
> 
> We just had a discussion on IRC with Peter Korsgaard, and he also
> doesn't like much this solution:
> 
> 22:16 < Jacmet> kos_tom: I also don't like 348092
> 
> He hasn't expressed the details on IRC, but I believe he shares similar
> concerns to mine, i.e:
> 
> """
> I must say I'm still unconvinced by this patch. In U-Boot, the
> configuration is completely mixed with the definition of the board, and
> defining a board does not only require adding a file in
> include/configs/, but also adding an entry in boards.cfg.
> 
> Therefore, I really believe that people willing to customize their
> U-Boot board or configuration should instead carry a patch against
> U-Boot instead of using this non-standard solution.
> """
> 
> Of course, if Peter has different/other reasons, I'm sure he will
> give more details in this thread.
> 
> As a consequence, I'm marking the patch as Rejected in patchwork.
> 
> Thanks,

Thanks for your feedback.

Regards,
Eric
diff mbox

Patch

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 1c77f6a..299d50f 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -79,6 +79,14 @@  config BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR
 
 	  Most users may leave this empty
 
+config BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE
+	string "Configuration file path"
+	help
+	  Path to the modified configuration header file. It will be
+	  copied to include/configs/<boardname>.h in the U-Boot sources.
+	  Note that this doesn't allow you to create a custom board,
+	  only to modify some of configuration variables.
+
 choice
 	prompt "U-Boot binary format"
 	default BR2_TARGET_UBOOT_FORMAT_BIN
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index da67706..1fa3fb9 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -94,7 +94,25 @@  endef
 UBOOT_POST_PATCH_HOOKS += UBOOT_APPLY_CUSTOM_PATCHES
 endif
 
+UBOOT_CUSTOM_CONFIG = $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE))
+
+ifneq ($(UBOOT_CUSTOM_CONFIG),)
+
+define UBOOT_COPY_CUSTOM_CONFIG_FILE
+	$(INSTALL) -m 0644 $(UBOOT_CUSTOM_CONFIG) \
+		$(@D)/include/configs/$(UBOOT_BOARD_NAME).h;
+endef
+
+define UBOOT_COPY_CUSTOM_CONFIG_FILE_TO_LEGAL_INFO
+	$(INSTALL) -m 0644 $(UBOOT_CUSTOM_CONFIG) \
+		$(REDIST_SOURCES_DIR_TARGET)/$(UBOOT_BOARD_NAME).h;
+endef
+
+UBOOT_POST_LEGAL_INFO_HOOKS += UBOOT_COPY_CUSTOM_CONFIG_FILE_TO_LEGAL_INFO
+endif
+
 define UBOOT_CONFIGURE_CMDS
+	$(UBOOT_COPY_CUSTOM_CONFIG_FILE)
 	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
 		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
 		$(UBOOT_BOARD_NAME)_config