[U-Boot] [PATCH 1/4] common: imx: Implement generic u-boot.nand target

[Benoît Thébaudeau]

On Thursday, February 28, 2013 5:21:55 PM, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/28/2013 11:06 AM, Benoît Thébaudeau wrote:
> > Hi Tom,
> > 
> > On Thursday, February 28, 2013 4:24:22 PM, Benoît Thébaudeau
> > wrote:
> >> Hi Tom,
> >> 
> >> On Thursday, February 28, 2013 3:03:09 PM, Tom Rini write:
> >>> On Wed, Feb 27, 2013 at 11:18:48PM +0100, Beno??t Th??baudeau
> >>> wrote:
> >>>> Hi Marek,
> >>>> 
> >>>> On Monday, February 25, 2013 7:19:54 PM, Marek Vasut wrote:
> >>>>> Implement u-boot.nand target that can be reused with a
> >>>>> small amount of churn across all CPU models. The idea is
> >>>>> to delegate the u-boot.nand target out of the main
> >>>>> Makefile and into the CPU's Makefile (very similar to what
> >>>>>  u-boot.imx does now). The main Makefile shall only
> >>>>> contain path to which the u-boot.nand target is delegated.
> >>>>> Hopefully this will not produce too much bloat in the main
> >>>>> Makefile.
> >>>>> 
> >>>>> To demonstrate this implementation, add u-boot.nand target
> >>>>> for i.MX53.
> >>>>> 
> >>>>> Signed-off-by: Marek Vasut <marex at denx.de> Cc: Beno??t
> >>>>> Th??baudeau <benoit.thebaudeau at advansee.com> Cc: Fabio
> >>>>> Estevam <fabio.estevam at freescale.com> Cc: Stefano Babic
> >>>>> <sbabic at denx.de> --- Makefile                     |   18
> >>>>> ++++++++++++++++++ arch/arm/imx-common/Makefile |    6
> >>>>> ++++++ 2 files changed, 24 insertions(+)
> >>>>> 
> >>>>> diff --git a/Makefile b/Makefile index 41054b7..8b1010a
> >>>>> 100644 --- a/Makefile +++ b/Makefile @@ -470,6 +470,23 @@
> >>>>> $(obj)u-boot.img:	$(obj)u-boot.bin $(obj)u-boot.imx:
> >>>>> $(obj)u-boot.bin depend $(MAKE) -C
> >>>>> $(SRCTREE)/arch/arm/imx-common $(obj)u-boot.imx
> >>>>> 
> >>>>> +# +# Generic u-boot.nand target. +# +# Every CPU that
> >>>>> needs u-boot.nand must add a path to an implementation of
> >>>>> +# the actual u-boot.nand generator below. +# +ifdef
> >>>>> CONFIG_MX53 +CONFIG_NAND_TRG_PATH :=
> >>>>> $(SRCTREE)/arch/arm/imx-common +endif +
> >>>>> +$(obj)u-boot.nand: $(obj)u-boot.bin depend +		if [
> >>>>> "X$(CONFIG_NAND_TRG_PATH)X" = "XX" ] ; then		\ +			echo
> >>>>> "This CPU does not support u-boot.nand target!" ;	\ +
> >>>>> exit 1 ;						\ +		fi + $(MAKE) -C $(CONFIG_NAND_TRG_PATH)
> >>>>> $(obj)u-boot.nand + $(obj)u-boot.kwb:
> >>>>> $(obj)u-boot.bin $(obj)tools/mkimage -n
> >>>>> $(CONFIG_SYS_KWD_CONFIG) -T kwbimage \ -a
> >>>>> $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_TEXT_BASE) -d $< $@
> >>>>> @@ -857,6 +874,7 @@ clobber:	tidy @rm -f $(obj)u-boot.kwb
> >>>>> @rm -f $(obj)u-boot.pbl @rm -f $(obj)u-boot.imx +	@rm -f
> >>>>> $(obj)u-boot.nand @rm -f $(obj)u-boot.ubl @rm -f
> >>>>> $(obj)u-boot.ais @rm -f $(obj)u-boot.dtb diff --git
> >>>>> a/arch/arm/imx-common/Makefile
> >>>>> b/arch/arm/imx-common/Makefile index 5d5c5b2..71ea36f
> >>>>> 100644 --- a/arch/arm/imx-common/Makefile +++
> >>>>> b/arch/arm/imx-common/Makefile @@ -50,6 +50,12 @@
> >>>>> $(obj)u-boot.imx: $(OBJTREE)/u-boot.bin
> >>>>> $(OBJTREE)/$(patsubst "%",%,$(CONFIG_IMX
> >>>>> $(OBJTREE)/tools/mkimage -n $(filter-out %.bin,$^) -T
> >>>>> imximage \ -e $(CONFIG_SYS_TEXT_BASE) -d $< $@
> >>>>> 
> >>>>> +$(obj)u-boot.nand: $(obj)u-boot.imx +	(								\ +		echo
> >>>>> -ne '\x00\x00\x00\x00\x46\x43\x42\x20\x01' ;	\
> >>>> ^ It does not work in my environment (Ubuntu 12.10). -ne is
> >>>> interpreted as text, so the FCB is broken. This is because
> >>>> /bin/sh (set to dash) is invoked by default on my machine
> >>>> here. It would work with the /bin/bash set by the main
> >>>> Makefile for SHELL, but this is not passed to the sub-make.
> >>>> 
> >>>> So what do you think we should do: 1) Add "export SHELL" to
> >>>> the main Makefile? 2) Move the SHELL assignment from the
> >>>> main Makefile to the top-level config.mk? 3) Set
> >>>> "SHELL=$(SHELL)" on the command line when invoking the
> >>>> sub-make? 4) Use printf instead of echo? 5) Something else?
> >>>> 
> >>>> I like 1). Do you see possible side effects?
> >>> 
> >>> Wait!  We do 1 already and have for years (cf7a7b99), so
> >>> what's going on?
> >> 
> >> Indeed! But I get anyway SHELL set to /bin/bash in /Makefile,
> >> and to /bin/sh in /arch/arm/imx-common/makefile.
> >> 
> >> I don't see SHELL being set anywhere else, and $(MAKE)
> >> SHELL=$(SHELL) works.
> >> 
> >> That's really weird. It's like SHELL export was ignored, or
> >> applied before the assignment. I will further dig into this.
> > 
> > From "5.3.2 Choosing the Shell" in the make manual, it seems
> > impossible to force make to use SHELL from the environment, even
> > with export. The note about SHELL in "5.7.2 Communicating
> > Variables to a Sub-make" can be misleading however.
> > 
> > Without "export SHELL", the recipes are executed with the set
> > SHELL, but the default value of SHELL is in their environment.
> > 
> > With "export SHELL", the recipes are executed with the set SHELL,
> > which is also in their environment. But since the sub-make ignores
> > its environment for SHELL, it does not help. That's probably why
> > Linux uses CONFIG_SHELL (which means nothing special to make)
> > instead of SHELL.
> > 
> > So 1) is not a solution, and 4) also does not work. So we are left
> > with 2), 3) or 5).
> 
> How about 3?

3) pros:
 - limited change without any side effect

3) cons:
 - the recipe works or not depending on how the sub-make is invoked
 - the same issue might happen again with another recipe and remain unnoticed
   for some time before causing trouble

That's why I'd probably prefer 2), but the risk is side effects in sub-makes.

Best regards,
Benoît