diff mbox

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

Message ID 1361816397-8661-1-git-send-email-marex@denx.de
State Superseded
Delegated to: Stefano Babic
Headers show

Commit Message

Marek Vasut Feb. 25, 2013, 6:19 p.m. UTC
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@denx.de>
Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
---
 Makefile                     |   18 ++++++++++++++++++
 arch/arm/imx-common/Makefile |    6 ++++++
 2 files changed, 24 insertions(+)

Comments

Benoît Thébaudeau Feb. 25, 2013, 6:51 p.m. UTC | #1
Dear Marek Vasut,

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@denx.de>
> Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Stefano Babic <sbabic@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

What about calling it rather CONFIG_NAND_TGT_PATH? TRG looks more like trigger.

> +endif
> +
> +$(obj)u-boot.nand: $(obj)u-boot.bin depend
                                       ^
$(obj)u-boot.bin already depends on depend through $(obj)u-boot, so it's useless
here.

> +		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
                                                   ^
                                                   $(OBJTREE)/

> +
>  $(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' ;	\
> +		dd if=/dev/zero bs=1015 count=1 2>/dev/null ) |		\
> +	cat - $< > $@

Is that all? According to 7.5.2.2 (i.MX53 RM), the boot ROM switches to serial
mode if anything goes wrong with the NAND. Hence, for reliable NAND boot, you
have to choose either DBBT or SPL (or both, but that would be waste).

Populating the DBBT would be complicated, so I'd go for SPL. You could just use
my u-boot-with-nand-spl.imx and change the dummy header to a true FCB with the
fingerprint like you did in your header above. u-boot.nand then becomes useless.

> +
>  $(obj)SPL: $(OBJTREE)/spl/u-boot-spl.bin $(OBJTREE)/$(patsubst
>  "%",%,$(CONFIG_IMX_CONFIG)).cfgtmp
>  	$(OBJTREE)/tools/mkimage -n $(filter-out %.bin,$^) -T imximage \
>  	-e $(CONFIG_SPL_TEXT_BASE) -d $< $@


Best regards,
Benoît
Marek Vasut Feb. 25, 2013, 9:09 p.m. UTC | #2
Dear Benoît Thébaudeau,

> Dear Marek Vasut,
> 
> 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@denx.de>
> > Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > Cc: Stefano Babic <sbabic@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
> 
> What about calling it rather CONFIG_NAND_TGT_PATH? TRG looks more like
> trigger.
> 
> > +endif
> > +
> > +$(obj)u-boot.nand: $(obj)u-boot.bin depend
> 
>                                        ^
> $(obj)u-boot.bin already depends on depend through $(obj)u-boot, so it's
> useless here.
> 
> > +		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
> 
>                                                    ^
>                                                    $(OBJTREE)/
> 
> > +
> > 
> >  $(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' ;	\
> > +		dd if=/dev/zero bs=1015 count=1 2>/dev/null ) |		\
> > +	cat - $< > $@
> 
> Is that all?

That's all for now, that's how it boots and how others do it as well. In the 
long run though, I'd prefer to bend mxsboot to generate the DBBT.

> According to 7.5.2.2 (i.MX53 RM), the boot ROM switches to
> serial mode if anything goes wrong with the NAND. Hence, for reliable NAND
> boot, you have to choose either DBBT or SPL (or both, but that would be
> waste).
> 
> Populating the DBBT would be complicated, so I'd go for SPL. You could just
> use my u-boot-with-nand-spl.imx and change the dummy header to a true FCB
> with the fingerprint like you did in your header above. u-boot.nand then
> becomes useless.
> 
> > +
> > 
> >  $(obj)SPL: $(OBJTREE)/spl/u-boot-spl.bin $(OBJTREE)/$(patsubst
> >  "%",%,$(CONFIG_IMX_CONFIG)).cfgtmp
> >  
> >  	$(OBJTREE)/tools/mkimage -n $(filter-out %.bin,$^) -T imximage \
> >  	-e $(CONFIG_SPL_TEXT_BASE) -d $< $@
> 
> Best regards,
> Benoît

Best regards,
Marek Vasut
Benoît Thébaudeau Feb. 25, 2013, 9:13 p.m. UTC | #3
Dear Marek Vasut,

On Monday, February 25, 2013 10:09:07 PM, Marek Vasut wrote:
> Dear Benoît Thébaudeau,
> 
> > Dear Marek Vasut,
> > 
> > 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@denx.de>
> > > Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> > > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > > Cc: Stefano Babic <sbabic@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
> > 
> > What about calling it rather CONFIG_NAND_TGT_PATH? TRG looks more like
> > trigger.
> > 
> > > +endif
> > > +
> > > +$(obj)u-boot.nand: $(obj)u-boot.bin depend
> > 
> >                                        ^
> > $(obj)u-boot.bin already depends on depend through $(obj)u-boot, so it's
> > useless here.
> > 
> > > +		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
> > 
> >                                                    ^
> >                                                    $(OBJTREE)/
> > 
> > > +
> > > 
> > >  $(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' ;	\
> > > +		dd if=/dev/zero bs=1015 count=1 2>/dev/null ) |		\
> > > +	cat - $< > $@
> > 
> > Is that all?
> 
> That's all for now, that's how it boots and how others do it as well. In the
> long run though, I'd prefer to bend mxsboot to generate the DBBT.

After a quick look at mxsboot, it seems to just generate an empty DBBT, so this
does not help for boot reliability.

> > According to 7.5.2.2 (i.MX53 RM), the boot ROM switches to
> > serial mode if anything goes wrong with the NAND. Hence, for reliable NAND
> > boot, you have to choose either DBBT or SPL (or both, but that would be
> > waste).
> > 
> > Populating the DBBT would be complicated, so I'd go for SPL. You could just
> > use my u-boot-with-nand-spl.imx and change the dummy header to a true FCB
> > with the fingerprint like you did in your header above. u-boot.nand then
> > becomes useless.
> > 
> > > +
> > > 
> > >  $(obj)SPL: $(OBJTREE)/spl/u-boot-spl.bin $(OBJTREE)/$(patsubst
> > >  "%",%,$(CONFIG_IMX_CONFIG)).cfgtmp
> > >  
> > >  	$(OBJTREE)/tools/mkimage -n $(filter-out %.bin,$^) -T imximage \
> > >  	-e $(CONFIG_SPL_TEXT_BASE) -d $< $@

Best regards,
Benoît
wanxs Feb. 26, 2013, 1:02 a.m. UTC | #4
Dear Benoît Thébaude,
	I git the u-boot-imx from denx.And I patch your patches,but I get error
as:
git apply
U-Boot-v7-14-19-imx-Fix-automatic-make-targets-for-imx-images.patch

error:
error: patch failed: Makefile:486
error: Makefile: patch does not apply
How can i do?
                                                   thanks
                                                                wanxs
Benoît Thébaudeau Feb. 27, 2013, 10:18 p.m. UTC | #5
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@denx.de>
> Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Stefano Babic <sbabic@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?

I will add a patch for that in my v8 once we have made a choice if this choice
is a global fix (i.e. 1 or 2).

> +		dd if=/dev/zero bs=1015 count=1 2>/dev/null ) |		\
> +	cat - $< > $@
> +
>  $(obj)SPL: $(OBJTREE)/spl/u-boot-spl.bin $(OBJTREE)/$(patsubst
>  "%",%,$(CONFIG_IMX_CONFIG)).cfgtmp
>  	$(OBJTREE)/tools/mkimage -n $(filter-out %.bin,$^) -T imximage \
>  	-e $(CONFIG_SPL_TEXT_BASE) -d $< $@

Best regards,
Benoît
Tom Rini Feb. 27, 2013, 11:44 p.m. UTC | #6
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/27/2013 05:18 PM, 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@denx.de> Cc: Benoît Thébaudeau
>> <benoit.thebaudeau@advansee.com> Cc: Fabio Estevam
>> <fabio.estevam@freescale.com> Cc: Stefano Babic <sbabic@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?

It looks like in these cases the kernel does $(shell echo -...), or is
that also not working on your setup?

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRLppcAAoJENk4IS6UOR1WiXoP/1wy8CU/bEgreOP/aPu2xsEA
7oh6CRRVLqGhioTo766IjaaDWD21uNmUpI3VK8wF8FBrPmmgS+IU9icYjd39ZsoL
m0riocPX3TS6SJFqs/xJpgsCFSCnw7MAjz8Eo/RphV8pohMnsqUrKX4TiPiZKxeS
2wky4E9PcMDdf07ahJEUC3Y73BNBXrml+MGh59sKrAKpvES7OutbUHCHD+ZeX3WC
Tk3nbFis3Sb+VY8vnMllyqVyCZQAI+MhJxm9zHTW2GzkX3/FsQ8tUND+DT8JmKv9
RQMkVCY92cdO5+0y+8RGO7WUwY0YghMdzfIkkr3ZQGlu9ccghcwvm21d2zVt7zCa
UfRyl1NK3xgFIfAsyBWVF8q75WkQUVT06YJo2SJTfcvIa/hlGH6FvN9fODcxeris
WAIan5Z3haZ7EiIGjn/KHq4dHYhS7c3iKqsi4FIQcoGsbwI9oFVqIQ98JW1UbWpS
174DvASSobba487fuAYTKgCwa2Qm9LHmy7XvJfb7CQTjrJuzsuRBWuhy3DD70AOC
Mo/Ewq4b1juvS/vTwT+d/eJcnBklDYvvJ31FMuf+P+EBwyz3AmIAmzBKRhRP37SV
EIKjB7GKHSuxrvJV6SjPAJW6suY4TiDSUh/myeXnGs/1+KDeGj91I5Dja4Cidl0o
TpP52BmyQrPVs2hS1PpC
=UQTu
-----END PGP SIGNATURE-----
Benoît Thébaudeau Feb. 27, 2013, 11:47 p.m. UTC | #7
Hi Tom,

On Thursday, February 28, 2013 12:44:28 AM, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/27/2013 05:18 PM, 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@denx.de> Cc: Benoît Thébaudeau
> >> <benoit.thebaudeau@advansee.com> Cc: Fabio Estevam
> >> <fabio.estevam@freescale.com> Cc: Stefano Babic <sbabic@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?
> 
> It looks like in these cases the kernel does $(shell echo -...), or is
> that also not working on your setup?

I'll test that tomorrow, but I don't see how it could work better since
$(shell ...) will very likely invoke $(SHELL) -c "...".

/bin/echo works though, but is this portable enough for U-Boot?

Best regards,
Benoît
Benoît Thébaudeau Feb. 28, 2013, 12:18 p.m. UTC | #8
Hi Tom,

On Thursday, February 28, 2013 12:47:46 AM, Benoît Thébaudeau wrote:
> Hi Tom,
> 
> On Thursday, February 28, 2013 12:44:28 AM, Tom Rini wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > On 02/27/2013 05:18 PM, 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@denx.de> Cc: Benoît Thébaudeau
> > >> <benoit.thebaudeau@advansee.com> Cc: Fabio Estevam
> > >> <fabio.estevam@freescale.com> Cc: Stefano Babic <sbabic@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?
> > 
> > It looks like in these cases the kernel does $(shell echo -...), or is
> > that also not working on your setup?
> 
> I'll test that tomorrow, but I don't see how it could work better since
> $(shell ...) will very likely invoke $(SHELL) -c "...".
> 
> /bin/echo works though, but is this portable enough for U-Boot?

No, your suggestion does not work on my setup.

I've looked at what Linux does, and this is not what you said: When it uses
$(shell echo -...), this is not to make the arguments work, but this is because
$(shell ...) is required in the context, e.g. for make variable assignments or
conditional parts of Makefiles.

U-Boot sets SHELL in its main Makefile and does not export it, whereas Linux
sets CONFIG_SHELL and exports it, using it explicitly.

Best regards,
Benoît
Tom Rini Feb. 28, 2013, 2:03 p.m. UTC | #9
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@denx.de>
> > Cc: Beno??t Th??baudeau <benoit.thebaudeau@advansee.com>
> > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > Cc: Stefano Babic <sbabic@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?
Benoît Thébaudeau Feb. 28, 2013, 3:24 p.m. UTC | #10
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@denx.de>
> > > Cc: Beno??t Th??baudeau <benoit.thebaudeau@advansee.com>
> > > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > > Cc: Stefano Babic <sbabic@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.

Best regards,
Benoît
Benoît Thébaudeau Feb. 28, 2013, 4:06 p.m. UTC | #11
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@denx.de>
> > > > Cc: Beno??t Th??baudeau <benoit.thebaudeau@advansee.com>
> > > > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > > > Cc: Stefano Babic <sbabic@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).

Best regards,
Benoît
Tom Rini Feb. 28, 2013, 4:21 p.m. UTC | #12
-----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@denx.de> Cc: Beno??t 
>>>>> Th??baudeau <benoit.thebaudeau@advansee.com> Cc: Fabio 
>>>>> Estevam <fabio.estevam@freescale.com> Cc: Stefano Babic 
>>>>> <sbabic@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?

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRL4QiAAoJENk4IS6UOR1W9HMP/RvkNPQoxdf9JIxcK0U7vBZC
3JDTnzCheGxI58GE58c2NvK9Swj6h2Zc31zlacYOUH2Xgb7TbM3UyrgD/e+RxrgW
0/6tVPkKyUGouJA+56OO/mcFpmiyFsiJyCKxAMUroBMZrBvXaPnOZYX5T+vQ7SI8
N2RqtFzmkQlSJdNR7Pxy4OC8cFLCUe5I0AffFFOgDBDSEqc7emaK8e4Y+mRIUg9+
Tz9Eai24GvF/5mYqbeenLXtbGnZZBxbYKcD7gUH61DuUlNHeW7kY0jaWwoSLT/yS
mdqy7nsJJenJgs6bjpDzLtL3pdF5Bmlu0BAedMa4U4cfKEYQAAG181CP4AGAA80Y
DE4lj5SXP89nNSiqqNrAdiXjniBpkvcUWZR1oMOgQcAMRnYODSUES8ktCMEAHboO
u3jtVTYWdH6kh4zGo2dY6g/gtYIbXrKtHpARZXeJ2YYnhXPlt641M1HNV1kkHr0x
BIOXGpDsAdIxSZceki7Mm8yLBz0SPiDpkXOpUaUbGAVjsQT+qbRZLazOMzkD/vYB
dkH7HnxXS2orQ3ZJXmShBivMHHOCE1FaGPP+p98y9aZiRuH6e4lg02ft7Oa9p1JE
6hTspGCv+RmsvGYHkkitxjZ8hmuxw+Qg6GxcDsnbAl87qi5gnOeg7QI6G2tGzYZU
CQ0S0MwlUHF8Gk8qQx1v
=qSEN
-----END PGP SIGNATURE-----
Benoît Thébaudeau Feb. 28, 2013, 4:29 p.m. UTC | #13
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@denx.de> Cc: Beno??t
> >>>>> Th??baudeau <benoit.thebaudeau@advansee.com> Cc: Fabio
> >>>>> Estevam <fabio.estevam@freescale.com> Cc: Stefano Babic
> >>>>> <sbabic@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
Tom Rini Feb. 28, 2013, 4:42 p.m. UTC | #14
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/28/2013 11:29 AM, Benoît Thébaudeau wrote:
> 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@denx.de> Cc: Beno??t
>>>>>>>  Th??baudeau <benoit.thebaudeau@advansee.com> Cc: Fabio
>>>>>>>  Estevam <fabio.estevam@freescale.com> Cc: Stefano 
>>>>>>> Babic <sbabic@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.

Yeah, it would be good to not have to whack every place we do $(MAKE)
now with passing SHELL around, to avoid new problems in the future.
So lets try 2 and test it out a bit.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRL4j8AAoJENk4IS6UOR1WV9QQAI5F2A9kqoAjUpeW+EJUu2tC
fAzE6hmOHD2dqglYy08c1kSdD/a4PupILsoX52LHpPO7nedea1fQEgu74I0tOTiI
pRWMZRGLgB584BJvb3R1dyYvzbKeSPCc7CshKt4iNfduLbLSEE8wBzbFa8hGa5Xi
S2LMfeOVf4KX3gBPV+qMERpjcqKAPbrysLLKuV5W2WSBGiwWoHndWxFDqJ56CLRE
GejSv7EhbEEk7WtiwY9maiNlQXOpVcZI5ne9fFwIfRiJ1qddWlfGvbmI8Mi5gac6
J/xMF7cBVT+qpz30Fj9sQz4LQKLgYqIVOo1jm/aKKiQfv67uHdzHM6als0p7mzSj
gYV0LSTUr2CDFEpVZ1wZRlnFg4H7xrxkHfQFffu1MP0BXhPVJQRHCI/73BU9yiZY
WntxbQbgyS7CDFuecNlVxpIZZc+vY4jr7gCRvhEmDa7LJtpNBbF5v4tIaH1sm4ex
VpsgcT93fVkNy7PDkv1JgTH1A5AqI9Yw5WWmyycH6EHF4QxxYtmU7vl2/ot6tS+F
rclLnsCrPuQFTOu57AJmomVcW97ULai2CLaxAqwMm+575RfHaHjT8Uiw6XkxezfA
TFTOFXipSpsNIkYWtG0Y8gT2mhWyOOKNqB62x1tyYv6ypiKn/KGDM3CbIeJPgK9v
96hWXW+AA9LkH/GR7H9r
=9TFg
-----END PGP SIGNATURE-----
diff mbox

Patch

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' ;	\
+		dd if=/dev/zero bs=1015 count=1 2>/dev/null ) |		\
+	cat - $< > $@
+
 $(obj)SPL: $(OBJTREE)/spl/u-boot-spl.bin $(OBJTREE)/$(patsubst "%",%,$(CONFIG_IMX_CONFIG)).cfgtmp
 	$(OBJTREE)/tools/mkimage -n $(filter-out %.bin,$^) -T imximage \
 	-e $(CONFIG_SPL_TEXT_BASE) -d $< $@