Patchwork [U-Boot,5/6] spl: Make CONFIG_SPL_BUILD contain more functionality

login
register
mail settings
Submitter ying.zhang@freescale.com
Date May 20, 2013, 6:07 a.m.
Message ID <1369030048-26130-5-git-send-email-ying.zhang@freescale.com>
Download mbox | patch
Permalink /patch/244849/
State Superseded
Delegated to: Andy Fleming
Headers show

Comments

ying.zhang@freescale.com - May 20, 2013, 6:07 a.m.
From: Ying Zhang <b40530@freescale.com>

There was some functionality will be used in the SPL. They
had been excluded by ifndef CONFIG_SPL_BUILD. Now, put it
into the SPL.

Signed-off-by: Ying Zhang <b40530@freescale.com>
---
 arch/powerpc/cpu/mpc85xx/tlb.c |    2 +-
 arch/powerpc/cpu/mpc8xxx/law.c |    4 ++--
 common/env_common.c            |    2 --
 include/configs/MPC8313ERDB.h  |    2 ++
 include/configs/P1022DS.h      |    2 ++
 include/configs/p1_p2_rdb_pc.h |    2 ++
 6 files changed, 9 insertions(+), 5 deletions(-)
Scott Wood - May 21, 2013, 7:42 p.m.
Please change the title and the rest of the changelog to describe what  
functionality you're adding and why.

On 05/20/2013 01:07:27 AM, ying.zhang@freescale.com wrote:
> diff --git a/common/env_common.c b/common/env_common.c
> index 906b41f..8cb81e9 100644
> --- a/common/env_common.c
> +++ b/common/env_common.c
> @@ -156,7 +156,6 @@ int set_default_vars(int nvars, char * const  
> vars[])
>  				H_NOCLEAR | H_INTERACTIVE, nvars, vars);
>  }
> 
> -#ifndef CONFIG_SPL_BUILD
>  /*
>   * Check if CRC is valid and (if yes) import the environment.
>   * Note that "buf" may or may not be aligned.
> @@ -188,7 +187,6 @@ int env_import(const char *buf, int check)
> 
>  	return 0;
>  }
> -#endif

This ifndef was introduced by Ilya Yanok in commit  
7ac2fe2da21d292aeaf3af74e5c80de9ce9dab56.

Ilya, what are the consequences of removing this?  Is there some other  
symbol we can use here?

> diff --git a/include/configs/MPC8313ERDB.h  
> b/include/configs/MPC8313ERDB.h
> index c28dfe0..a2bdcff 100644
> --- a/include/configs/MPC8313ERDB.h
> +++ b/include/configs/MPC8313ERDB.h
> @@ -40,7 +40,9 @@
>  #define CONFIG_SPL_INIT_MINIMAL
>  #define CONFIG_SPL_SERIAL_SUPPORT
>  #define CONFIG_SPL_NAND_SUPPORT
> +#ifdef CONFIG_SPL_BUILD
>  #define CONFIG_SPL_NAND_MINIMAL
> +#endif
>  #define CONFIG_SPL_FLUSH_IMAGE
>  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
>  #define CONFIG_SPL_MPC83XX_WAIT_FOR_NAND
> diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
> index 8b13b10..5bdd44a 100644
> --- a/include/configs/P1022DS.h
> +++ b/include/configs/P1022DS.h
> @@ -41,7 +41,9 @@
>  #define CONFIG_SPL_INIT_MINIMAL
>  #define CONFIG_SPL_SERIAL_SUPPORT
>  #define CONFIG_SPL_NAND_SUPPORT
> +#ifdef CONFIG_SPL_BUILD
>  #define CONFIG_SPL_NAND_MINIMAL
> +#endif
>  #define CONFIG_SPL_FLUSH_IMAGE
>  #define CONFIG_SPL_TARGET              "u-boot-with-spl.bin"
> 
> diff --git a/include/configs/p1_p2_rdb_pc.h  
> b/include/configs/p1_p2_rdb_pc.h
> index 7ed634b..bc48d62 100644
> --- a/include/configs/p1_p2_rdb_pc.h
> +++ b/include/configs/p1_p2_rdb_pc.h
> @@ -159,7 +159,9 @@
>  #define CONFIG_SPL_INIT_MINIMAL
>  #define CONFIG_SPL_SERIAL_SUPPORT
>  #define CONFIG_SPL_NAND_SUPPORT
> +#ifdef CONFIG_SPL_BUILD
>  #define CONFIG_SPL_NAND_MINIMAL
> +#endif
>  #define CONFIG_SPL_FLUSH_IMAGE
>  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"

Are you sure this belongs in this patch?

-Scott
Zhang Ying-B40530 - May 22, 2013, 2:15 a.m.
> diff --git a/include/configs/MPC8313ERDB.h  
> b/include/configs/MPC8313ERDB.h
> index c28dfe0..a2bdcff 100644
> --- a/include/configs/MPC8313ERDB.h
> +++ b/include/configs/MPC8313ERDB.h
> @@ -40,7 +40,9 @@
>  #define CONFIG_SPL_INIT_MINIMAL
>  #define CONFIG_SPL_SERIAL_SUPPORT
>  #define CONFIG_SPL_NAND_SUPPORT
> +#ifdef CONFIG_SPL_BUILD
>  #define CONFIG_SPL_NAND_MINIMAL
> +#endif
>  #define CONFIG_SPL_FLUSH_IMAGE
>  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
>  #define CONFIG_SPL_MPC83XX_WAIT_FOR_NAND
> diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
> index 8b13b10..5bdd44a 100644
> --- a/include/configs/P1022DS.h
> +++ b/include/configs/P1022DS.h
> @@ -41,7 +41,9 @@
>  #define CONFIG_SPL_INIT_MINIMAL
>  #define CONFIG_SPL_SERIAL_SUPPORT
>  #define CONFIG_SPL_NAND_SUPPORT
> +#ifdef CONFIG_SPL_BUILD
>  #define CONFIG_SPL_NAND_MINIMAL
> +#endif
>  #define CONFIG_SPL_FLUSH_IMAGE
>  #define CONFIG_SPL_TARGET              "u-boot-with-spl.bin"
> 
> diff --git a/include/configs/p1_p2_rdb_pc.h  
> b/include/configs/p1_p2_rdb_pc.h
> index 7ed634b..bc48d62 100644
> --- a/include/configs/p1_p2_rdb_pc.h
> +++ b/include/configs/p1_p2_rdb_pc.h
> @@ -159,7 +159,9 @@
>  #define CONFIG_SPL_INIT_MINIMAL
>  #define CONFIG_SPL_SERIAL_SUPPORT
>  #define CONFIG_SPL_NAND_SUPPORT
> +#ifdef CONFIG_SPL_BUILD
>  #define CONFIG_SPL_NAND_MINIMAL
> +#endif
>  #define CONFIG_SPL_FLUSH_IMAGE
>  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"

Are you sure this belongs in this patch?
[Zhang Ying] 
Yes, it is necessary. Because CONFIG_SPL_NAND_MINIMAL has been used in the file law.c and tlb.c in this patch.
Scott Wood - May 22, 2013, 3:45 p.m.
On 05/21/2013 09:15:08 PM, Zhang Ying-B40530 wrote:
> > diff --git a/include/configs/MPC8313ERDB.h
> > b/include/configs/MPC8313ERDB.h
> > index c28dfe0..a2bdcff 100644
> > --- a/include/configs/MPC8313ERDB.h
> > +++ b/include/configs/MPC8313ERDB.h
> > @@ -40,7 +40,9 @@
> >  #define CONFIG_SPL_INIT_MINIMAL
> >  #define CONFIG_SPL_SERIAL_SUPPORT
> >  #define CONFIG_SPL_NAND_SUPPORT
> > +#ifdef CONFIG_SPL_BUILD
> >  #define CONFIG_SPL_NAND_MINIMAL
> > +#endif
> >  #define CONFIG_SPL_FLUSH_IMAGE
> >  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
> >  #define CONFIG_SPL_MPC83XX_WAIT_FOR_NAND
> > diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
> > index 8b13b10..5bdd44a 100644
> > --- a/include/configs/P1022DS.h
> > +++ b/include/configs/P1022DS.h
> > @@ -41,7 +41,9 @@
> >  #define CONFIG_SPL_INIT_MINIMAL
> >  #define CONFIG_SPL_SERIAL_SUPPORT
> >  #define CONFIG_SPL_NAND_SUPPORT
> > +#ifdef CONFIG_SPL_BUILD
> >  #define CONFIG_SPL_NAND_MINIMAL
> > +#endif
> >  #define CONFIG_SPL_FLUSH_IMAGE
> >  #define CONFIG_SPL_TARGET              "u-boot-with-spl.bin"
> >
> > diff --git a/include/configs/p1_p2_rdb_pc.h
> > b/include/configs/p1_p2_rdb_pc.h
> > index 7ed634b..bc48d62 100644
> > --- a/include/configs/p1_p2_rdb_pc.h
> > +++ b/include/configs/p1_p2_rdb_pc.h
> > @@ -159,7 +159,9 @@
> >  #define CONFIG_SPL_INIT_MINIMAL
> >  #define CONFIG_SPL_SERIAL_SUPPORT
> >  #define CONFIG_SPL_NAND_SUPPORT
> > +#ifdef CONFIG_SPL_BUILD
> >  #define CONFIG_SPL_NAND_MINIMAL
> > +#endif
> >  #define CONFIG_SPL_FLUSH_IMAGE
> >  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
> 
> Are you sure this belongs in this patch?
> [Zhang Ying]
> Yes, it is necessary. Because CONFIG_SPL_NAND_MINIMAL has been used  
> in the file law.c and tlb.c in this patch.

What I mean is that it should probably have been done earlier, when you  
introduced CONFIG_SPL_NAND_MINIMAL.

-Scott
Zhang Ying-B40530 - May 23, 2013, 1:26 a.m.
-----Original Message-----
From: Wood Scott-B07421 
Sent: Wednesday, May 22, 2013 11:46 PM
To: Zhang Ying-B40530
Cc: Wood Scott-B07421; u-boot@lists.denx.de; afleming@gmail.com; Xie Xiaobo-R63061; Ilya Yanok
Subject: Re: [PATCH 5/6] spl: Make CONFIG_SPL_BUILD contain more functionality

On 05/21/2013 09:15:08 PM, Zhang Ying-B40530 wrote:
> > diff --git a/include/configs/MPC8313ERDB.h
> > b/include/configs/MPC8313ERDB.h
> > index c28dfe0..a2bdcff 100644
> > --- a/include/configs/MPC8313ERDB.h
> > +++ b/include/configs/MPC8313ERDB.h
> > @@ -40,7 +40,9 @@
> >  #define CONFIG_SPL_INIT_MINIMAL
> >  #define CONFIG_SPL_SERIAL_SUPPORT
> >  #define CONFIG_SPL_NAND_SUPPORT
> > +#ifdef CONFIG_SPL_BUILD
> >  #define CONFIG_SPL_NAND_MINIMAL
> > +#endif
> >  #define CONFIG_SPL_FLUSH_IMAGE
> >  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
> >  #define CONFIG_SPL_MPC83XX_WAIT_FOR_NAND
> > diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
> > index 8b13b10..5bdd44a 100644
> > --- a/include/configs/P1022DS.h
> > +++ b/include/configs/P1022DS.h
> > @@ -41,7 +41,9 @@
> >  #define CONFIG_SPL_INIT_MINIMAL
> >  #define CONFIG_SPL_SERIAL_SUPPORT
> >  #define CONFIG_SPL_NAND_SUPPORT
> > +#ifdef CONFIG_SPL_BUILD
> >  #define CONFIG_SPL_NAND_MINIMAL
> > +#endif
> >  #define CONFIG_SPL_FLUSH_IMAGE
> >  #define CONFIG_SPL_TARGET              "u-boot-with-spl.bin"
> >
> > diff --git a/include/configs/p1_p2_rdb_pc.h
> > b/include/configs/p1_p2_rdb_pc.h
> > index 7ed634b..bc48d62 100644
> > --- a/include/configs/p1_p2_rdb_pc.h
> > +++ b/include/configs/p1_p2_rdb_pc.h
> > @@ -159,7 +159,9 @@
> >  #define CONFIG_SPL_INIT_MINIMAL
> >  #define CONFIG_SPL_SERIAL_SUPPORT
> >  #define CONFIG_SPL_NAND_SUPPORT
> > +#ifdef CONFIG_SPL_BUILD
> >  #define CONFIG_SPL_NAND_MINIMAL
> > +#endif
> >  #define CONFIG_SPL_FLUSH_IMAGE
> >  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
> 
> Are you sure this belongs in this patch?
> [Zhang Ying]
> Yes, it is necessary. Because CONFIG_SPL_NAND_MINIMAL has been used  
> in the file law.c and tlb.c in this patch.

What I mean is that it should probably have been done earlier, when you  
introduced CONFIG_SPL_NAND_MINIMAL.
[Zhang Ying] 
I can understand you mean. Because the symbol "CONFIG_SPL_NAND_MINIMAL" has been useless, I think no need to split out a separate patch.
Tom Rini - May 24, 2013, 4:07 p.m.
On Tue, May 21, 2013 at 02:42:02PM -0500, Scott Wood wrote:

> Please change the title and the rest of the changelog to describe
> what functionality you're adding and why.
> 
> On 05/20/2013 01:07:27 AM, ying.zhang@freescale.com wrote:
> >diff --git a/common/env_common.c b/common/env_common.c
> >index 906b41f..8cb81e9 100644
> >--- a/common/env_common.c
> >+++ b/common/env_common.c
> >@@ -156,7 +156,6 @@ int set_default_vars(int nvars, char * const
> >vars[])
> > 				H_NOCLEAR | H_INTERACTIVE, nvars, vars);
> > }
> >
> >-#ifndef CONFIG_SPL_BUILD
> > /*
> >  * Check if CRC is valid and (if yes) import the environment.
> >  * Note that "buf" may or may not be aligned.
> >@@ -188,7 +187,6 @@ int env_import(const char *buf, int check)
> >
> > 	return 0;
> > }
> >-#endif
> 
> This ifndef was introduced by Ilya Yanok in commit
> 7ac2fe2da21d292aeaf3af74e5c80de9ce9dab56.
> 
> Ilya, what are the consequences of removing this?  Is there some
> other symbol we can use here?

This was likely done so that we could fit the combination of USB RNDIS
networking in SPL into the size constraint we have and the need for more
than just unused section discarding to get rid of codepaths we know
won't be taken (but couldn't compile-time determine).
Tom Rini - May 24, 2013, 4:11 p.m.
On Mon, May 20, 2013 at 02:07:27PM +0800, ying.zhang@freescale.com wrote:

> From: Ying Zhang <b40530@freescale.com>
> 
> There was some functionality will be used in the SPL. They
> had been excluded by ifndef CONFIG_SPL_BUILD. Now, put it
> into the SPL.
> 
> Signed-off-by: Ying Zhang <b40530@freescale.com>
> ---
>  arch/powerpc/cpu/mpc85xx/tlb.c |    2 +-
>  arch/powerpc/cpu/mpc8xxx/law.c |    4 ++--

In these cases can we not just always build them, aside from when
CONFIG_NAND_SPL is set and rely on link-time discard here?  Otherwise:


> -#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_BUILD)
> +#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_NAND_MINIMAL)

Should become, I believe (and this isn't whitespaced properly):
#if !defined(CONFIG_NAND_SPL) && !(defined(CONFIG_SPL_BUILD) &&
defined(CONFIG_SPL_NAND_MINIMAL))

So that:

> diff --git a/include/configs/MPC8313ERDB.h b/include/configs/MPC8313ERDB.h
[snip]
> diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
[snip]
> diff --git a/include/configs/p1_p2_rdb_pc.h b/include/configs/p1_p2_rdb_pc.h

Can then all be dropped.
Tom Rini - May 24, 2013, 7:18 p.m.
On Fri, May 24, 2013 at 02:08:07PM -0500, Scott Wood wrote:
> On 05/24/2013 11:11:01 AM, Tom Rini wrote:
> >On Mon, May 20, 2013 at 02:07:27PM +0800, ying.zhang@freescale.com
> >wrote:
> >
> >> From: Ying Zhang <b40530@freescale.com>
> >>
> >> There was some functionality will be used in the SPL. They
> >> had been excluded by ifndef CONFIG_SPL_BUILD. Now, put it
> >> into the SPL.
> >>
> >> Signed-off-by: Ying Zhang <b40530@freescale.com>
> >> ---
> >>  arch/powerpc/cpu/mpc85xx/tlb.c |    2 +-
> >>  arch/powerpc/cpu/mpc8xxx/law.c |    4 ++--
> >
> >In these cases can we not just always build them, aside from when
> >CONFIG_NAND_SPL is set and rely on link-time discard here?  Otherwise:
> >
> >
> >> -#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_BUILD)
> >> +#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_NAND_MINIMAL)
> >
> >Should become, I believe (and this isn't whitespaced properly):
> >#if !defined(CONFIG_NAND_SPL) && !(defined(CONFIG_SPL_BUILD) &&
> >defined(CONFIG_SPL_NAND_MINIMAL))
> >
> >So that:
> >
> >> diff --git a/include/configs/MPC8313ERDB.h
> >b/include/configs/MPC8313ERDB.h
> >[snip]
> >> diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
> >[snip]
> >> diff --git a/include/configs/p1_p2_rdb_pc.h
> >b/include/configs/p1_p2_rdb_pc.h
> >
> >Can then all be dropped.
> 
> Possibly, but it would be nice to limit SPL symbols to only be
> defined for the SPL part of the build, so we don't have to add
> checks for CONFIG_SPL_BUILD all over the place.  Currently this
> won't work for symbols that makefiles look at, though there was a
> patch to fix that, which I referred to elsewhere in the these
> threads.

At the high level, yes, I agree it would be good to clean up everyones
configs around CONFIG_SPL/CONFIG_SPL_BUILD.
Zhang Ying-B40530 - May 27, 2013, 7:43 a.m.
So, about this patch, what need I do?


-----Original Message-----
From: Tom Rini [mailto:tom.rini@gmail.com] On Behalf Of Tom Rini
Sent: Saturday, May 25, 2013 3:18 AM
To: Wood Scott-B07421
Cc: u-boot@lists.denx.de; Zhang Ying-B40530; afleming@gmail.com; Xie Xiaobo-R63061; Zhang Ying-B40530
Subject: Re: [U-Boot] [PATCH 5/6] spl: Make CONFIG_SPL_BUILD contain more functionality

On Fri, May 24, 2013 at 02:08:07PM -0500, Scott Wood wrote:
> On 05/24/2013 11:11:01 AM, Tom Rini wrote:
> >On Mon, May 20, 2013 at 02:07:27PM +0800, ying.zhang@freescale.com
> >wrote:
> >
> >> From: Ying Zhang <b40530@freescale.com>
> >>
> >> There was some functionality will be used in the SPL. They
> >> had been excluded by ifndef CONFIG_SPL_BUILD. Now, put it
> >> into the SPL.
> >>
> >> Signed-off-by: Ying Zhang <b40530@freescale.com>
> >> ---
> >>  arch/powerpc/cpu/mpc85xx/tlb.c |    2 +-
> >>  arch/powerpc/cpu/mpc8xxx/law.c |    4 ++--
> >
> >In these cases can we not just always build them, aside from when
> >CONFIG_NAND_SPL is set and rely on link-time discard here?  Otherwise:
> >
> >
> >> -#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_BUILD)
> >> +#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_NAND_MINIMAL)
> >
> >Should become, I believe (and this isn't whitespaced properly):
> >#if !defined(CONFIG_NAND_SPL) && !(defined(CONFIG_SPL_BUILD) &&
> >defined(CONFIG_SPL_NAND_MINIMAL))
> >
> >So that:
> >
> >> diff --git a/include/configs/MPC8313ERDB.h
> >b/include/configs/MPC8313ERDB.h
> >[snip]
> >> diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
> >[snip]
> >> diff --git a/include/configs/p1_p2_rdb_pc.h
> >b/include/configs/p1_p2_rdb_pc.h
> >
> >Can then all be dropped.
> 
> Possibly, but it would be nice to limit SPL symbols to only be
> defined for the SPL part of the build, so we don't have to add
> checks for CONFIG_SPL_BUILD all over the place.  Currently this
> won't work for symbols that makefiles look at, though there was a
> patch to fix that, which I referred to elsewhere in the these
> threads.

At the high level, yes, I agree it would be good to clean up everyones
configs around CONFIG_SPL/CONFIG_SPL_BUILD.
Zhang Ying-B40530 - May 29, 2013, 2:11 a.m.
-----Original Message-----
From: Wood Scott-B07421 
Sent: Wednesday, May 29, 2013 6:34 AM
To: Zhang Ying-B40530
Cc: Wood Scott-B07421; u-boot@lists.denx.de; afleming@gmail.com; Xie Xiaobo-R63061; Ilya Yanok
Subject: Re: [PATCH 5/6] spl: Make CONFIG_SPL_BUILD contain more functionality

On 05/22/2013 08:26:31 PM, Zhang Ying-B40530 wrote:
> 
> 
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, May 22, 2013 11:46 PM
> To: Zhang Ying-B40530
> Cc: Wood Scott-B07421; u-boot@lists.denx.de; afleming@gmail.com; Xie  
> Xiaobo-R63061; Ilya Yanok
> Subject: Re: [PATCH 5/6] spl: Make CONFIG_SPL_BUILD contain more  
> functionality
> 
> On 05/21/2013 09:15:08 PM, Zhang Ying-B40530 wrote:
> > > diff --git a/include/configs/MPC8313ERDB.h
> > > b/include/configs/MPC8313ERDB.h
> > > index c28dfe0..a2bdcff 100644
> > > --- a/include/configs/MPC8313ERDB.h
> > > +++ b/include/configs/MPC8313ERDB.h
> > > @@ -40,7 +40,9 @@
> > >  #define CONFIG_SPL_INIT_MINIMAL
> > >  #define CONFIG_SPL_SERIAL_SUPPORT
> > >  #define CONFIG_SPL_NAND_SUPPORT
> > > +#ifdef CONFIG_SPL_BUILD
> > >  #define CONFIG_SPL_NAND_MINIMAL
> > > +#endif
> > >  #define CONFIG_SPL_FLUSH_IMAGE
> > >  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
> > >  #define CONFIG_SPL_MPC83XX_WAIT_FOR_NAND
> > > diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
> > > index 8b13b10..5bdd44a 100644
> > > --- a/include/configs/P1022DS.h
> > > +++ b/include/configs/P1022DS.h
> > > @@ -41,7 +41,9 @@
> > >  #define CONFIG_SPL_INIT_MINIMAL
> > >  #define CONFIG_SPL_SERIAL_SUPPORT
> > >  #define CONFIG_SPL_NAND_SUPPORT
> > > +#ifdef CONFIG_SPL_BUILD
> > >  #define CONFIG_SPL_NAND_MINIMAL
> > > +#endif
> > >  #define CONFIG_SPL_FLUSH_IMAGE
> > >  #define CONFIG_SPL_TARGET              "u-boot-with-spl.bin"
> > >
> > > diff --git a/include/configs/p1_p2_rdb_pc.h
> > > b/include/configs/p1_p2_rdb_pc.h
> > > index 7ed634b..bc48d62 100644
> > > --- a/include/configs/p1_p2_rdb_pc.h
> > > +++ b/include/configs/p1_p2_rdb_pc.h
> > > @@ -159,7 +159,9 @@
> > >  #define CONFIG_SPL_INIT_MINIMAL
> > >  #define CONFIG_SPL_SERIAL_SUPPORT
> > >  #define CONFIG_SPL_NAND_SUPPORT
> > > +#ifdef CONFIG_SPL_BUILD
> > >  #define CONFIG_SPL_NAND_MINIMAL
> > > +#endif
> > >  #define CONFIG_SPL_FLUSH_IMAGE
> > >  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
> >
> > Are you sure this belongs in this patch?
> > [Zhang Ying]
> > Yes, it is necessary. Because CONFIG_SPL_NAND_MINIMAL has been used
> > in the file law.c and tlb.c in this patch.
> 
> What I mean is that it should probably have been done earlier, when  
> you
> introduced CONFIG_SPL_NAND_MINIMAL.
> [Zhang Ying]
> I can understand you mean. Because the symbol  
> "CONFIG_SPL_NAND_MINIMAL" has been useless, I think no need to split  
> out a separate patch.

OK, it looks like CONFIG_SPL_NAND_MINIMAL was there before your  
patchset.  I think that was an accidental left-over and should have  
been removed (it was replaced with CONFIG_SPL_NAND_DRIVERS, _BASE, and  
_ECC).

Could you describe what specifically you're using it to mean here?
[Zhang Ying] 
CONFIG_SPL_NAND_MINIMAL is effective only for mpc85xx NAND SPL. If it is set, said the SPL as small as possible and the size is not more than 4K.
Scott Wood - May 29, 2013, 10:54 p.m.
On 05/28/2013 09:11:17 PM, Zhang Ying-B40530 wrote:
> 
> 
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, May 29, 2013 6:34 AM
> To: Zhang Ying-B40530
> Cc: Wood Scott-B07421; u-boot@lists.denx.de; afleming@gmail.com; Xie  
> Xiaobo-R63061; Ilya Yanok
> Subject: Re: [PATCH 5/6] spl: Make CONFIG_SPL_BUILD contain more  
> functionality
> 
> On 05/22/2013 08:26:31 PM, Zhang Ying-B40530 wrote:
> >
> >
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, May 22, 2013 11:46 PM
> > To: Zhang Ying-B40530
> > Cc: Wood Scott-B07421; u-boot@lists.denx.de; afleming@gmail.com; Xie
> > Xiaobo-R63061; Ilya Yanok
> > Subject: Re: [PATCH 5/6] spl: Make CONFIG_SPL_BUILD contain more
> > functionality
> >
> > On 05/21/2013 09:15:08 PM, Zhang Ying-B40530 wrote:
> > > > diff --git a/include/configs/MPC8313ERDB.h
> > > > b/include/configs/MPC8313ERDB.h
> > > > index c28dfe0..a2bdcff 100644
> > > > --- a/include/configs/MPC8313ERDB.h
> > > > +++ b/include/configs/MPC8313ERDB.h
> > > > @@ -40,7 +40,9 @@
> > > >  #define CONFIG_SPL_INIT_MINIMAL
> > > >  #define CONFIG_SPL_SERIAL_SUPPORT
> > > >  #define CONFIG_SPL_NAND_SUPPORT
> > > > +#ifdef CONFIG_SPL_BUILD
> > > >  #define CONFIG_SPL_NAND_MINIMAL
> > > > +#endif
> > > >  #define CONFIG_SPL_FLUSH_IMAGE
> > > >  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
> > > >  #define CONFIG_SPL_MPC83XX_WAIT_FOR_NAND
> > > > diff --git a/include/configs/P1022DS.h  
> b/include/configs/P1022DS.h
> > > > index 8b13b10..5bdd44a 100644
> > > > --- a/include/configs/P1022DS.h
> > > > +++ b/include/configs/P1022DS.h
> > > > @@ -41,7 +41,9 @@
> > > >  #define CONFIG_SPL_INIT_MINIMAL
> > > >  #define CONFIG_SPL_SERIAL_SUPPORT
> > > >  #define CONFIG_SPL_NAND_SUPPORT
> > > > +#ifdef CONFIG_SPL_BUILD
> > > >  #define CONFIG_SPL_NAND_MINIMAL
> > > > +#endif
> > > >  #define CONFIG_SPL_FLUSH_IMAGE
> > > >  #define CONFIG_SPL_TARGET              "u-boot-with-spl.bin"
> > > >
> > > > diff --git a/include/configs/p1_p2_rdb_pc.h
> > > > b/include/configs/p1_p2_rdb_pc.h
> > > > index 7ed634b..bc48d62 100644
> > > > --- a/include/configs/p1_p2_rdb_pc.h
> > > > +++ b/include/configs/p1_p2_rdb_pc.h
> > > > @@ -159,7 +159,9 @@
> > > >  #define CONFIG_SPL_INIT_MINIMAL
> > > >  #define CONFIG_SPL_SERIAL_SUPPORT
> > > >  #define CONFIG_SPL_NAND_SUPPORT
> > > > +#ifdef CONFIG_SPL_BUILD
> > > >  #define CONFIG_SPL_NAND_MINIMAL
> > > > +#endif
> > > >  #define CONFIG_SPL_FLUSH_IMAGE
> > > >  #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
> > >
> > > Are you sure this belongs in this patch?
> > > [Zhang Ying]
> > > Yes, it is necessary. Because CONFIG_SPL_NAND_MINIMAL has been  
> used
> > > in the file law.c and tlb.c in this patch.
> >
> > What I mean is that it should probably have been done earlier, when
> > you
> > introduced CONFIG_SPL_NAND_MINIMAL.
> > [Zhang Ying]
> > I can understand you mean. Because the symbol
> > "CONFIG_SPL_NAND_MINIMAL" has been useless, I think no need to split
> > out a separate patch.
> 
> OK, it looks like CONFIG_SPL_NAND_MINIMAL was there before your
> patchset.  I think that was an accidental left-over and should have
> been removed (it was replaced with CONFIG_SPL_NAND_DRIVERS, _BASE, and
> _ECC).
> 
> Could you describe what specifically you're using it to mean here?
> [Zhang Ying]
> CONFIG_SPL_NAND_MINIMAL is effective only for mpc85xx NAND SPL.

No, it's just a mistake that should be removed.  It doesn't mean  
anything.  There are other SPL targets that use minimal NAND drivers  
(e.g. mxc_nand_spl.c) that don't define this.

-Scott

Patch

diff --git a/arch/powerpc/cpu/mpc85xx/tlb.c b/arch/powerpc/cpu/mpc85xx/tlb.c
index 0dff37f..a44a89c 100644
--- a/arch/powerpc/cpu/mpc85xx/tlb.c
+++ b/arch/powerpc/cpu/mpc85xx/tlb.c
@@ -55,7 +55,7 @@  void init_tlbs(void)
 	return ;
 }
 
-#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_BUILD)
+#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_NAND_MINIMAL)
 void read_tlbcam_entry(int idx, u32 *valid, u32 *tsize, unsigned long *epn,
 		       phys_addr_t *rpn)
 {
diff --git a/arch/powerpc/cpu/mpc8xxx/law.c b/arch/powerpc/cpu/mpc8xxx/law.c
index 6f9d568..0df62b6 100644
--- a/arch/powerpc/cpu/mpc8xxx/law.c
+++ b/arch/powerpc/cpu/mpc8xxx/law.c
@@ -92,7 +92,7 @@  void disable_law(u8 idx)
 	return;
 }
 
-#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_BUILD)
+#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_NAND_MINIMAL)
 static int get_law_entry(u8 i, struct law_entry *e)
 {
 	u32 lawar;
@@ -122,7 +122,7 @@  int set_next_law(phys_addr_t addr, enum law_size sz, enum law_trgt_if id)
 	return idx;
 }
 
-#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_BUILD)
+#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_NAND_MINIMAL)
 int set_last_law(phys_addr_t addr, enum law_size sz, enum law_trgt_if id)
 {
 	u32 idx;
diff --git a/common/env_common.c b/common/env_common.c
index 906b41f..8cb81e9 100644
--- a/common/env_common.c
+++ b/common/env_common.c
@@ -156,7 +156,6 @@  int set_default_vars(int nvars, char * const vars[])
 				H_NOCLEAR | H_INTERACTIVE, nvars, vars);
 }
 
-#ifndef CONFIG_SPL_BUILD
 /*
  * Check if CRC is valid and (if yes) import the environment.
  * Note that "buf" may or may not be aligned.
@@ -188,7 +187,6 @@  int env_import(const char *buf, int check)
 
 	return 0;
 }
-#endif
 
 void env_relocate(void)
 {
diff --git a/include/configs/MPC8313ERDB.h b/include/configs/MPC8313ERDB.h
index c28dfe0..a2bdcff 100644
--- a/include/configs/MPC8313ERDB.h
+++ b/include/configs/MPC8313ERDB.h
@@ -40,7 +40,9 @@ 
 #define CONFIG_SPL_INIT_MINIMAL
 #define CONFIG_SPL_SERIAL_SUPPORT
 #define CONFIG_SPL_NAND_SUPPORT
+#ifdef CONFIG_SPL_BUILD
 #define CONFIG_SPL_NAND_MINIMAL
+#endif
 #define CONFIG_SPL_FLUSH_IMAGE
 #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"
 #define CONFIG_SPL_MPC83XX_WAIT_FOR_NAND
diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
index 8b13b10..5bdd44a 100644
--- a/include/configs/P1022DS.h
+++ b/include/configs/P1022DS.h
@@ -41,7 +41,9 @@ 
 #define CONFIG_SPL_INIT_MINIMAL
 #define CONFIG_SPL_SERIAL_SUPPORT
 #define CONFIG_SPL_NAND_SUPPORT
+#ifdef CONFIG_SPL_BUILD
 #define CONFIG_SPL_NAND_MINIMAL
+#endif
 #define CONFIG_SPL_FLUSH_IMAGE
 #define CONFIG_SPL_TARGET              "u-boot-with-spl.bin"
 
diff --git a/include/configs/p1_p2_rdb_pc.h b/include/configs/p1_p2_rdb_pc.h
index 7ed634b..bc48d62 100644
--- a/include/configs/p1_p2_rdb_pc.h
+++ b/include/configs/p1_p2_rdb_pc.h
@@ -159,7 +159,9 @@ 
 #define CONFIG_SPL_INIT_MINIMAL
 #define CONFIG_SPL_SERIAL_SUPPORT
 #define CONFIG_SPL_NAND_SUPPORT
+#ifdef CONFIG_SPL_BUILD
 #define CONFIG_SPL_NAND_MINIMAL
+#endif
 #define CONFIG_SPL_FLUSH_IMAGE
 #define CONFIG_SPL_TARGET		"u-boot-with-spl.bin"