[Buildroot] [PATCH 1/1] u-boot: allow to pass a custom configuration file

[Buildroot] [PATCH 1/1] u-boot: allow to pass a custom configuration file

[Eric Jarrige]

Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
---
 boot/uboot/Config.in |   18 ++++++++++++++++++
 boot/uboot/uboot.mk  |    3 +++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 1b98339..1358f4a 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -71,6 +71,24 @@ config BR2_TARGET_UBOOT_CUSTOM_GIT_VERSION
 endif
 
 choice
+	prompt "U-Boot configuration"
+	default BR2_TARGET_UBOOT_USE_DEFCONFIG
+
+config BR2_TARGET_UBOOT_USE_DEFCONFIG
+	bool "Using default configuration"
+
+config BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG
+	bool "Using a custom configuration file"
+
+endchoice
+
+config BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE
+	string "Configuration file path"
+	depends on BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG
+	help
+	  Path to the customized U-Boot configuration file
+
+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 631da6b..8bcf260 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -80,6 +80,9 @@ UBOOT_POST_PATCH_HOOKS += UBOOT_APPLY_CUSTOM_PATCHES
 endif
 
 define UBOOT_CONFIGURE_CMDS
+	$(if $(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),
+		cp -pf $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
+			$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)
 	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
 		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
 		$(UBOOT_BOARD_NAME)_config
-- 
1.7.2.5

[Buildroot] [PATCH 1/1] u-boot: allow to pass a custom configuration file

[Thomas Petazzoni]

Dear Eric Jarrige,

On Thu, 19 Sep 2013 13:10:06 +0200, Eric Jarrige wrote:

>  choice
> +	prompt "U-Boot configuration"
> +	default BR2_TARGET_UBOOT_USE_DEFCONFIG
> +
> +config BR2_TARGET_UBOOT_USE_DEFCONFIG
> +	bool "Using default configuration"

I don't think using the word 'defconfig' for U-Boot is appropriate,
since 'defconfig' really refers to a kconfig terminology and U-Boot,
sadly, doesn't use kconfig.

>  define UBOOT_CONFIGURE_CMDS
> +	$(if $(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),
> +		cp -pf $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
> +			$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)
>  	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
>  		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
>  		$(UBOOT_BOARD_NAME)_config

I am a bit hesitant on the overall feature. The fact that a
include/configs/<something>.h file is really a configuration file is
pretty fuzzy in U-Boot, especially since now boards are supposed to
also be listed in the main boards.cfg file.

Therefore, I'm tempted to say that users who need to do that should
instead use patches against U-Boot (to add their own board, including
the include/configs/<something>.h file).

But on this one, I believe I can be convinced if there are good
arguments :)

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [PATCH 1/1] u-boot: allow to pass a custom configuration file

[Eric Jarrige]

Hi Thomas,

On 19 sept. 2013, at 17:43, Thomas Petazzoni wrote:

> Dear Eric Jarrige,
> 
> On Thu, 19 Sep 2013 13:10:06 +0200, Eric Jarrige wrote:
> 
>> choice
>> +	prompt "U-Boot configuration"
>> +	default BR2_TARGET_UBOOT_USE_DEFCONFIG
>> +
>> +config BR2_TARGET_UBOOT_USE_DEFCONFIG
>> +	bool "Using default configuration"
> 
> I don't think using the word 'defconfig' for U-Boot is appropriate,
> since 'defconfig' really refers to a kconfig terminology and U-Boot,
> sadly, doesn't use kconfig.

Does the alternate terminology DEFAULT_CONFIG could be more
appropriate or acceptable ?

> 
>> define UBOOT_CONFIGURE_CMDS
>> +	$(if $(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),
>> +		cp -pf $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
>> +			$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)
>> 	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
>> 		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
>> 		$(UBOOT_BOARD_NAME)_config
> 
> I am a bit hesitant on the overall feature. The fact that a
> include/configs/<something>.h file is really a configuration file is
> pretty fuzzy in U-Boot, especially since now boards are supposed to
> also be listed in the main boards.cfg file.
> 
> Therefore, I'm tempted to say that users who need to do that should
> instead use patches against U-Boot (to add their own board, including
> the include/configs/<something>.h file).
> 
> But on this one, I believe I can be convinced if there are good
> arguments :)

The file boards.cfg is a source of confusion for me. This file provides
the list of boards with some extra information like status of the board and
maintainer email address.
Nevertheless the concrete config files are the <BOARD>.h files located in
include/config even if U-Boot a C syntax with a list of #define instead of
the common kconfig syntax.

You are right, a customization of the U-Boot config  file can be done
through the use of patches as it could be done for some other packages
like Barebox, Busybox or the linux kernel but the BuildRoot feature to
support custom config files for the main packages is more than
convenient. So the purpose of this patch is to add this feature to the U
Boot option as well.
IMHO such a feature could be useful for some other BuildRoot/U-Boot
user but may be I wrong. Please let me know.

Best regards,
Eric





 
 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 203 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20130919/a3b7e941/attachment-0001.asc>

[Buildroot] [PATCH 1/1] u-boot: allow to pass a custom configuration file

[Thomas Petazzoni]

Dear Eric Jarrige,

On Thu, 19 Sep 2013 22:07:22 +0200, Eric Jarrige wrote:

> > I don't think using the word 'defconfig' for U-Boot is appropriate,
> > since 'defconfig' really refers to a kconfig terminology and U-Boot,
> > sadly, doesn't use kconfig.
> 
> Does the alternate terminology DEFAULT_CONFIG could be more
> appropriate or acceptable ?

I don't think "default" is really meaningful. Just "Path to the board
configuration file" would be sufficient.

> >> define UBOOT_CONFIGURE_CMDS
> >> +	$(if $(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),
> >> +		cp -pf $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
> >> +			$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)
> >> 	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
> >> 		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
> >> 		$(UBOOT_BOARD_NAME)_config
> > 
> > I am a bit hesitant on the overall feature. The fact that a
> > include/configs/<something>.h file is really a configuration file is
> > pretty fuzzy in U-Boot, especially since now boards are supposed to
> > also be listed in the main boards.cfg file.
> > 
> > Therefore, I'm tempted to say that users who need to do that should
> > instead use patches against U-Boot (to add their own board, including
> > the include/configs/<something>.h file).
> > 
> > But on this one, I believe I can be convinced if there are good
> > arguments :)
> 
> The file boards.cfg is a source of confusion for me. This file provides
> the list of boards with some extra information like status of the board and
> maintainer email address.
> Nevertheless the concrete config files are the <BOARD>.h files located in
> include/config even if U-Boot a C syntax with a list of #define instead of
> the common kconfig syntax.

Right, but it looks like nowadays that adding a board
configuration requires both adding a include/configs/<BOARD>.h file and
an entry in boards.cfg (though I agree that was the 'mkconfig' script
is doing is quite obscure).

> You are right, a customization of the U-Boot config  file can be done
> through the use of patches as it could be done for some other packages
> like Barebox, Busybox or the linux kernel but the BuildRoot feature to
> support custom config files for the main packages is more than
> convenient. So the purpose of this patch is to add this feature to the U
> Boot option as well.
> IMHO such a feature could be useful for some other BuildRoot/U-Boot
> user but may be I wrong. Please let me know.

Well again, the problem I see is that the "board configuration" in
U-Boot is not something that is as well isolated as it is in the kernel
or Barebox. In the kernel or Barebox, the configuration file is only
here to describe *how* you would like the support for a particular
board to be built (with network, without, with this or that other
driver).

While in U-Boot, the <BOARD>.h file is an *integral* part of the board
support itself: it not only defines the configuration you want for this
particular build, but also describes the hardware itself. In U-Boot,
whether you want the network support or not is mixed with the
information of which network device your board has, and how it is
connected on the board. So a <BOARD>.h file is typically not
standalone, it comes with a new board/<something>/<boardname>/
directory.

That's the reason I'm skeptical that this is useful in practice.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [PATCH 1/1] u-boot: allow to pass a custom configuration file

[Eric Jarrige]

Hi Thomas,

On 20 sept. 2013, at 07:08, Thomas Petazzoni wrote:

> Dear Eric Jarrige,
> 
> On Thu, 19 Sep 2013 22:07:22 +0200, Eric Jarrige wrote:
> 
>>> I don't think using the word 'defconfig' for U-Boot is appropriate,
>>> since 'defconfig' really refers to a kconfig terminology and U-Boot,
>>> sadly, doesn't use kconfig.
>> 
>> Does the alternate terminology DEFAULT_CONFIG could be more
>> appropriate or acceptable ?
> 
> I don't think "default" is really meaningful. Just "Path to the board
> configuration file" would be sufficient.

ok

> 
>>>> define UBOOT_CONFIGURE_CMDS
>>>> +	$(if $(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),
>>>> +		cp -pf $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
>>>> +			$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)
>>>> 	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
>>>> 		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
>>>> 		$(UBOOT_BOARD_NAME)_config
>>> 
>>> I am a bit hesitant on the overall feature. The fact that a
>>> include/configs/<something>.h file is really a configuration file is
>>> pretty fuzzy in U-Boot, especially since now boards are supposed to
>>> also be listed in the main boards.cfg file.
>>> 
>>> Therefore, I'm tempted to say that users who need to do that should
>>> instead use patches against U-Boot (to add their own board, including
>>> the include/configs/<something>.h file).
>>> 
>>> But on this one, I believe I can be convinced if there are good
>>> arguments :)
>> 
>> The file boards.cfg is a source of confusion for me. This file provides
>> the list of boards with some extra information like status of the board and
>> maintainer email address.
>> Nevertheless the concrete config files are the <BOARD>.h files located in
>> include/config even if U-Boot a C syntax with a list of #define instead of
>> the common kconfig syntax.
> 
> Right, but it looks like nowadays that adding a board
> configuration requires both adding a include/configs/<BOARD>.h file and
> an entry in boards.cfg (though I agree that was the 'mkconfig' script
> is doing is quite obscure).

Right, by providing an external config file it is only possible to customize the
behaviors of an existing board in U-Boot. 

> 
>> You are right, a customization of the U-Boot config  file can be done
>> through the use of patches as it could be done for some other packages
>> like Barebox, Busybox or the linux kernel but the BuildRoot feature to
>> support custom config files for the main packages is more than
>> convenient. So the purpose of this patch is to add this feature to the U
>> Boot option as well.
>> IMHO such a feature could be useful for some other BuildRoot/U-Boot
>> user but may be I wrong. Please let me know.
> 
> Well again, the problem I see is that the "board configuration" in
> U-Boot is not something that is as well isolated as it is in the kernel
> or Barebox. In the kernel or Barebox, the configuration file is only
> here to describe *how* you would like the support for a particular
> board to be built (with network, without, with this or that other
> driver).
> 
> While in U-Boot, the <BOARD>.h file is an *integral* part of the board
> support itself: it not only defines the configuration you want for this
> particular build, but also describes the hardware itself. In U-Boot,
> whether you want the network support or not is mixed with the
> information of which network device your board has, and how it is
> connected on the board. So a <BOARD>.h file is typically not
> standalone, it comes with a new board/<something>/<boardname>/
> directory.

Sorry for the confusion, but the purpose of this patch is more way to
customize one of the U-Boot config than adding a new board to U-Boot.
This will probably change in the future as soon as U-Boot - like Barebox
- fully support the openfirmware device tree architecture. 

> 
> That's the reason I'm skeptical that this is useful in practice.

I apologize if you understood to be able to support any new board by just
adding a new config file.

I hope to have clarified the purpose of this patch and will submit a new 
version of this patch with the change discussed here above in the
meantime.

Best regards,
Eric


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 203 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20130920/fde647ec/attachment-0001.asc>

[Buildroot] [PATCH 1/1] u-boot: allow to pass a custom configuration file

[Eric Jarrige]

Hi Thomas,

Please forget my previous email that was sadly formated.

On 20 sept. 2013, at 07:08, Thomas Petazzoni wrote:

> Dear Eric Jarrige,
> 
> On Thu, 19 Sep 2013 22:07:22 +0200, Eric Jarrige wrote:
> 
>>> I don't think using the word 'defconfig' for U-Boot is appropriate,
>>> since 'defconfig' really refers to a kconfig terminology and U-Boot,
>>> sadly, doesn't use kconfig.
>> 
>> Does the alternate terminology DEFAULT_CONFIG could be more
>> appropriate or acceptable ?
> 
> I don't think "default" is really meaningful. Just "Path to the board
> configuration file" would be sufficient.

ok

> 
>>>> define UBOOT_CONFIGURE_CMDS
>>>> +	$(if $(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),
>>>> +		cp -pf $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
>>>> +			$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)
>>>> 	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
>>>> 		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
>>>> 		$(UBOOT_BOARD_NAME)_config
>>> 
>>> I am a bit hesitant on the overall feature. The fact that a
>>> include/configs/<something>.h file is really a configuration file is
>>> pretty fuzzy in U-Boot, especially since now boards are supposed to
>>> also be listed in the main boards.cfg file.
>>> 
>>> Therefore, I'm tempted to say that users who need to do that should
>>> instead use patches against U-Boot (to add their own board, including
>>> the include/configs/<something>.h file).
>>> 
>>> But on this one, I believe I can be convinced if there are good
>>> arguments :)
>> 
>> The file boards.cfg is a source of confusion for me. This file provides
>> the list of boards with some extra information like status of the board and
>> maintainer email address.
>> Nevertheless the concrete config files are the <BOARD>.h files located in
>> include/config even if U-Boot a C syntax with a list of #define instead of
>> the common kconfig syntax.
> 
> Right, but it looks like nowadays that adding a board
> configuration requires both adding a include/configs/<BOARD>.h file and
> an entry in boards.cfg (though I agree that was the 'mkconfig' script
> is doing is quite obscure).

Right, by providing an external config file it is only possible to customize the
behaviors of an existing board in U-Boot. 

> 
>> You are right, a customization of the U-Boot config  file can be done
>> through the use of patches as it could be done for some other packages
>> like Barebox, Busybox or the linux kernel but the BuildRoot feature to
>> support custom config files for the main packages is more than
>> convenient. So the purpose of this patch is to add this feature to the U
>> Boot option as well.
>> IMHO such a feature could be useful for some other BuildRoot/U-Boot
>> user but may be I wrong. Please let me know.
> 
> Well again, the problem I see is that the "board configuration" in
> U-Boot is not something that is as well isolated as it is in the kernel
> or Barebox. In the kernel or Barebox, the configuration file is only
> here to describe *how* you would like the support for a particular
> board to be built (with network, without, with this or that other
> driver).
> 
> While in U-Boot, the <BOARD>.h file is an *integral* part of the board
> support itself: it not only defines the configuration you want for this
> particular build, but also describes the hardware itself. In U-Boot,
> whether you want the network support or not is mixed with the
> information of which network device your board has, and how it is
> connected on the board. So a <BOARD>.h file is typically not
> standalone, it comes with a new board/<something>/<boardname>/
> directory.

Sorry for the confusion, but the purpose of this patch is more way to
customize one of the U-Boot config than adding a new board to U-Boot.
This will probably change in the future as soon as U-Boot - like Barebox
- fully support the openfirmware device tree architecture.

> 
> That's the reason I'm skeptical that this is useful in practice.

I apologize if you understood to be able to support any new board by just
adding a new config file.

I hope to have clarified the purpose of this patch and will submit a new 
version of this patch with the change discussed here above in the
meantime.

Best regards,
Eric

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 203 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20130920/9ec9b93c/attachment.asc>

[Buildroot] [PATCH v2] u-boot: allow to pass a custom configuration file

[Eric Jarrige]

---
Changes v1 -> v2:
  - Fix typo

 boot/uboot/Config.in |   18 ++++++++++++++++++
 boot/uboot/uboot.mk  |    3 +++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 1b98339..1db0339 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -71,6 +71,24 @@ config BR2_TARGET_UBOOT_CUSTOM_GIT_VERSION
 endif
 
 choice
+	prompt "U-Boot configuration"
+	default BR2_TARGET_UBOOT_USE_DEFCONFIG
+
+config BR2_TARGET_UBOOT_USE_DEFCONFIG
+	bool "Using default board configuration file"
+
+config BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG
+	bool "Using a custom board configuration file"
+
+endchoice
+
+config BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE
+	string "Configuration file path"
+	depends on BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG
+	help
+	  Path to the board configuration file
+
+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 631da6b..8bcf260 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -80,6 +80,9 @@ UBOOT_POST_PATCH_HOOKS += UBOOT_APPLY_CUSTOM_PATCHES
 endif
 
 define UBOOT_CONFIGURE_CMDS
+	$(if $(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),
+		cp -pf $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
+			$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)
 	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
 		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
 		$(UBOOT_BOARD_NAME)_config
-- 
1.7.2.5

[Buildroot] [PATCH v2 1/1] u-boot: allow to pass a custom configuration file

[Eric Jarrige]

Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
---
Changes v1 -> v2:
  - Fix typo

 boot/uboot/Config.in |   18 ++++++++++++++++++
 boot/uboot/uboot.mk  |    3 +++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 1b98339..1db0339 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -71,6 +71,24 @@ config BR2_TARGET_UBOOT_CUSTOM_GIT_VERSION
 endif
 
 choice
+	prompt "U-Boot configuration"
+	default BR2_TARGET_UBOOT_USE_DEFCONFIG
+
+config BR2_TARGET_UBOOT_USE_DEFCONFIG
+	bool "Using default board configuration file"
+
+config BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG
+	bool "Using a custom board configuration file"
+
+endchoice
+
+config BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE
+	string "Configuration file path"
+	depends on BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG
+	help
+	  Path to the board configuration file
+
+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 631da6b..8bcf260 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -80,6 +80,9 @@ UBOOT_POST_PATCH_HOOKS += UBOOT_APPLY_CUSTOM_PATCHES
 endif
 
 define UBOOT_CONFIGURE_CMDS
+	$(if $(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),
+		cp -pf $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
+			$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)
 	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
 		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
 		$(UBOOT_BOARD_NAME)_config
-- 
1.7.2.5

[Buildroot] [PATCH v2 1/1] u-boot: allow to pass a custom configuration file

[Arnout Vandecappelle]

On 20/09/13 13:11, Eric Jarrige wrote:
> 
> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
> ---
> Changes v1 -> v2:
>   - Fix typo
> 
>  boot/uboot/Config.in |   18 ++++++++++++++++++
>  boot/uboot/uboot.mk  |    3 +++
>  2 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index 1b98339..1db0339 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -71,6 +71,24 @@ config BR2_TARGET_UBOOT_CUSTOM_GIT_VERSION
>  endif
>  
>  choice
> +	prompt "U-Boot configuration"
> +	default BR2_TARGET_UBOOT_USE_DEFCONFIG
> +
> +config BR2_TARGET_UBOOT_USE_DEFCONFIG
> +	bool "Using default board configuration file"

 Thomas P. made a remark that you didn't take into account:

>>> I don't think using the word 'defconfig' for U-Boot is appropriate,
>>> since 'defconfig' really refers to a kconfig terminology and U-Boot,
>>> sadly, doesn't use kconfig.
>>
>> Does the alternate terminology DEFAULT_CONFIG could be more
>> appropriate or acceptable ?
> 
> I don't think "default" is really meaningful. Just "Path to the board
> configuration file" would be sufficient.

 Also, it is missing help text. So I propose:

config BR2_TARGET_UBOOT_USE_BUNDLED_CONFIG
	bool "Use <boardname>.h in the U-Boot sources"
	help
	  Choose this option to use the include/configs/<boardname>.h
	  file in the U-Boot sources as the configuration.

config BR2_TARGET_UBOOT_USE_MODIFIED_CONFIG
	bool "Use a modified configuration header file"
	help
	  Choose this option to provide a path to a customized header
	  file with the configuration option. Note that this doesn't
	  allow you to create a custom board, only to modify some of
	  configuration variables.

> +
> +config BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG
> +	bool "Using a custom board configuration file"
> +
> +endchoice
> +
> +config BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE
> +	string "Configuration file path"
> +	depends on BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG
> +	help
> +	  Path to the board configuration file

	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.


 Also, instead of adding a choice to choose between bundled or modified
configuration file, you could just have the string option and use the
default if it is empty. The first way is what is used in kernel and
barebox, the second way is what is done in busybox and uClibc. Well,
actually, the latter don't use the default when it's empty, instead they
encode the default in the Config.in - but I personally don't like that :-)

 Peter, which approach do you prefer?


> +
> +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 631da6b..8bcf260 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -80,6 +80,9 @@ UBOOT_POST_PATCH_HOOKS += UBOOT_APPLY_CUSTOM_PATCHES
>  endif
>  
>  define UBOOT_CONFIGURE_CMDS
> +	$(if $(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),
> +		cp -pf $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
> +			$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)

 I prefer the following idiom:

ifeq ($(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),y)
define UBOOT_COPY_CUSTOM_CONFIG_FILE
	...
endef
endif

define UBOOT_CONFIGURE_CMDS
	$(UBOOT_COPY_CUSTOM_CONFIG_FILE)
	...

 Peter, please confirm?


 Also, I don't like the -f option of cp much because most people don't
know what it means. And finally, I really wouldn't give it a -p.
Actually, if it's not recursive, we usually use $(INSTALL). Therefore:

ifeq ($(BR2_TARGET_UBOOT_USE_CUSTOM_CONFIG),y)
define UBOOT_COPY_CUSTOM_CONFIG_FILE
	(INSTALL) -m 0644 $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_CONFIG_FILE)) \
		$(@D)/include/configs/$(UBOOT_BOARD_NAME).h)
endef
endif



 And finally, to completely defuse Thomas's remarks, you could put the
following in the commit log.

"""
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.
"""

 Regards,
 Arnout


>  	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
>  		$(MAKE) -C $(@D) $(UBOOT_MAKE_OPTS)		\
>  		$(UBOOT_BOARD_NAME)_config
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F