diff mbox

[U-Boot] Add the symbol for the minimal SPL used to eliminate unused code

Message ID 1368601339-8089-1-git-send-email-ying.zhang@freescale.com
State Superseded
Delegated to: Andy Fleming
Headers show

Commit Message

ying.zhang@freescale.com May 15, 2013, 7:02 a.m. UTC 
From: Ying Zhang <b40530@freescale.com>

Add the symbol CONFIG_SPL_BUILD_MINIMAL for the minimal SPL. It used to
eliminate code unused in the minimal SPL but used in the SPL.

This patch is on top of the following patch:
1. common/Makefile: Add new symbol CONFIG_SPL_ENV_SUPPORT for
environment in SPL.
2. powerpc/mpc85xx: support application without resetvec segment in the
linker script.

Signed-off-by: Ying Zhang <b40530@freescale.com>
---
 README                         |    4 ++++
 arch/powerpc/cpu/mpc85xx/tlb.c |    2 +-
 arch/powerpc/cpu/mpc8xxx/law.c |    4 ++--
 common/env_common.c            |    2 +-
 include/configs/MPC8313ERDB.h  |    1 +
 include/configs/P1022DS.h      |    3 +++
 include/configs/p1_p2_rdb_pc.h |    3 +++
 7 files changed, 15 insertions(+), 4 deletions(-)

Comments

Andy Fleming May 15, 2013, 10:45 p.m. UTC | #1 
On Wed, May 15, 2013 at 2:02 AM, <ying.zhang@freescale.com> wrote:

> From: Ying Zhang <b40530@freescale.com>
>
> Add the symbol CONFIG_SPL_BUILD_MINIMAL for the minimal SPL. It used to
> eliminate code unused in the minimal SPL but used in the SPL.
>
> This patch is on top of the following patch:
> 1. common/Makefile: Add new symbol CONFIG_SPL_ENV_SUPPORT for
> environment in SPL.
> 2. powerpc/mpc85xx: support application without resetvec segment in the
> linker script.
>
> Signed-off-by: Ying Zhang <b40530@freescale.com>
>


I'm a bit confused by this patch. As far as I can tell, despite the
description, what this patch *actually* does is make CONFIG_SPL_BUILD take
up more space, while CONFIG_SPL_BUILD_MINIMAL will take up the same space
as CONFIG_SPL_BUILD used to.

It's important to make sure the commit message describes the purpose of the
patch, and that the patch actually does what it says...

If the goal was to make CONFIG_SPL_BUILD contain more functionality, then
say that in the commit message. If not, then please point out the error in
my understanding of the patch.

Andy
Tom Rini May 15, 2013, 11:58 p.m. UTC | #2 
On Wed, May 15, 2013 at 03:02:19PM +0800, ying.zhang@freescale.com wrote:

> From: Ying Zhang <b40530@freescale.com>
> 
> Add the symbol CONFIG_SPL_BUILD_MINIMAL for the minimal SPL. It used to
> eliminate code unused in the minimal SPL but used in the SPL.
[snip]
> diff --git a/common/env_common.c b/common/env_common.c
> index 906b41f..5d82ea0 100644
> --- a/common/env_common.c
> +++ b/common/env_common.c
> @@ -156,7 +156,7 @@ int set_default_vars(int nvars, char * const vars[])
>  				H_NOCLEAR | H_INTERACTIVE, nvars, vars);
>  }
>  
> -#ifndef CONFIG_SPL_BUILD
> +#ifndef CONFIG_SPL_BUILD_MINIMAL
>  /*
>   * Check if CRC is valid and (if yes) import the environment.
>   * Note that "buf" may or may not be aligned.

Hold up, this ignores the whole world of CONFIG_SPL_FRAMEWORK and
probably breaks the ARM targets.   You need to instal an ARM toolchain
(either one of the Linaro ones or ELDK 5.x) and make sure that MAKEALL
-a arm is also fine after your SPL changes.
Scott Wood May 16, 2013, 12:01 a.m. UTC | #3 
On 05/15/2013 06:58:44 PM, Tom Rini wrote:
> On Wed, May 15, 2013 at 03:02:19PM +0800, ying.zhang@freescale.com  
> wrote:
> 
> > From: Ying Zhang <b40530@freescale.com>
> >
> > Add the symbol CONFIG_SPL_BUILD_MINIMAL for the minimal SPL. It  
> used to
> > eliminate code unused in the minimal SPL but used in the SPL.
> [snip]
> > diff --git a/common/env_common.c b/common/env_common.c
> > index 906b41f..5d82ea0 100644
> > --- a/common/env_common.c
> > +++ b/common/env_common.c
> > @@ -156,7 +156,7 @@ int set_default_vars(int nvars, char * const  
> vars[])
> >  				H_NOCLEAR | H_INTERACTIVE, nvars, vars);
> >  }
> >
> > -#ifndef CONFIG_SPL_BUILD
> > +#ifndef CONFIG_SPL_BUILD_MINIMAL
> >  /*
> >   * Check if CRC is valid and (if yes) import the environment.
> >   * Note that "buf" may or may not be aligned.
> 
> Hold up, this ignores the whole world of CONFIG_SPL_FRAMEWORK and
> probably breaks the ARM targets.   You need to instal an ARM toolchain
> (either one of the Linaro ones or ELDK 5.x) and make sure that MAKEALL
> -a arm is also fine after your SPL changes.

Not to mention that the "minimal init" concept is currently only  
well-defined for mpc85xx, so this really doesn't belong in common/.   
Minimal 85xx SPLs don't even include libcommon, so why is this change  
needed at all?

-SCott
Zhang Ying-B40530 May 16, 2013, 10:08 a.m. UTC | #4 
-----Original Message-----
From: Wood Scott-B07421 
Sent: Thursday, May 16, 2013 8:01 AM
To: Tom Rini
Cc: Zhang Ying-B40530; u-boot@lists.denx.de; afleming@gmail.com; Xie Xiaobo-R63061; Zhang Ying-B40530
Subject: Re: [U-Boot] [PATCH] Add the symbol for the minimal SPL used to eliminate unused code

On 05/15/2013 06:58:44 PM, Tom Rini wrote:
> On Wed, May 15, 2013 at 03:02:19PM +0800, ying.zhang@freescale.com  
> wrote:
> 
> > From: Ying Zhang <b40530@freescale.com>
> >
> > Add the symbol CONFIG_SPL_BUILD_MINIMAL for the minimal SPL. It  
> used to
> > eliminate code unused in the minimal SPL but used in the SPL.
> [snip]
> > diff --git a/common/env_common.c b/common/env_common.c
> > index 906b41f..5d82ea0 100644
> > --- a/common/env_common.c
> > +++ b/common/env_common.c
> > @@ -156,7 +156,7 @@ int set_default_vars(int nvars, char * const  
> vars[])
> >  				H_NOCLEAR | H_INTERACTIVE, nvars, vars);
> >  }
> >
> > -#ifndef CONFIG_SPL_BUILD
> > +#ifndef CONFIG_SPL_BUILD_MINIMAL
> >  /*
> >   * Check if CRC is valid and (if yes) import the environment.
> >   * Note that "buf" may or may not be aligned.
> 
> Hold up, this ignores the whole world of CONFIG_SPL_FRAMEWORK and
> probably breaks the ARM targets.   You need to instal an ARM toolchain
> (either one of the Linaro ones or ELDK 5.x) and make sure that MAKEALL
> -a arm is also fine after your SPL changes.

Not to mention that the "minimal init" concept is currently only  
well-defined for mpc85xx, so this really doesn't belong in common/.   
Minimal 85xx SPLs don't even include libcommon, so why is this change  
needed at all?
[Zhang Ying]
Just because some functionality is necessary for SPL booting from SD. 
They were excluded from the SPL by CONFIG_SPL_BUILD now. Of course, 
including the function env_import() in common/env_common.c. 

Andy was right, the goal of this patch was to make CONFIG_SPL_BUILD
contain more functionality.
Tom Rini May 16, 2013, 12:36 p.m. UTC | #5 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/16/2013 06:08 AM, Zhang Ying-B40530 wrote:

> Andy was right, the goal of this patch was to make CONFIG_SPL_BUILD
> contain more functionality.

OK, here's the confusion.  CONFIG_SPL_BUILD is _only_ for telling if
we're building U-Boot or SPL.  Everything else should be covered by
some other CONFIG symbol.  You either want to use CONFIG_SPL_FRAMEWORK
(if you have the initial space for it), or add more options that are
compatible with CONFIG_SPL_INIT_MINIMAL.

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

iQIcBAEBAgAGBQJRlNLhAAoJENk4IS6UOR1WueYP/0t/XtPoBOcEwcZPvzAaDLkm
GULtLaJI7LVJDakVacMvIkOL8aVfvuKJQKFigR2NLvNKDsDQAuUwFsgGtn3xjub8
asG9QyjeO+CZdKSQ3QJO2qb0+XThn8f3+w4dj0E+Fdnpjm4EFDSZ/bTvjKfxm7jD
kTtDewo05sk434dUthsS3b/IM0NTAxnUSa4FGMvRQxK2+ORMXI23mDhy9yEOFOpJ
1IKlxYuI1yt/1BEuLkWxBP6cP60HeOIRP2YS41HjO5sTMhIBRmQWE/95VZvcqDLu
SGjPCLUJ0dDM9w10lebYIr5UJe0j4FVtcy250MZRsiIlA6uYRD9Wq/B7A7QTGqVz
R3igsPSfoh0NsObtx1HsswifYWnpyT7pycvj9dWTpY375dZKL2ENLRXIU9hQ9DNV
dGpdQdr+xsCAI5y9cUKvUj9BvLU0oUGn+fanHqpg+89gAB0Zs4ZW08vnWdO2kmbs
9XcFXYec5ElSx2WMQ2cJpSzDto0As2Xa7A5Ld4Jw3LY6x5K9uBvwopu5UvRx4lM2
KaDRHtYx+omyfaIRNcC43swTaCvRNft+QwtKg22pS3gyX5Er+AOk5xW++MleiiPz
yXnjRTtwcTilsj/txxNaLeU7H+sJNfi3HEk+jNgVWaGM3AZNhyeWW9noXZA7x/TY
K6WYgBsV2wu4if75NlWA
=r7f7
-----END PGP SIGNATURE-----
diff mbox

Patch

diff --git a/README b/README
index daf1fa1..79ccfc9 100644
--- a/README
+++ b/README
@@ -3976,6 +3976,10 @@  Low Level (hardware related) configuration options:
 		that is executed before the actual U-Boot. E.g. when
 		compiling a NAND SPL.
 
+- CONFIG_SPL_BUILD_MINIMAL
+		Add for the minimal SPL, used to eliminate unused code to
+		save the code space.
+
 - CONFIG_SYS_MPC85XX_NO_RESETVEC
 		Only for 85xx systems. If this variable is specified, the section
 		.resetvec is not kept and the section .bootpg is placed in the
diff --git a/arch/powerpc/cpu/mpc85xx/tlb.c b/arch/powerpc/cpu/mpc85xx/tlb.c
index 0dff37f..d21b324 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_BUILD_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..979c3c2 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_BUILD_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_BUILD_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..5d82ea0 100644
--- a/common/env_common.c
+++ b/common/env_common.c
@@ -156,7 +156,7 @@  int set_default_vars(int nvars, char * const vars[])
 				H_NOCLEAR | H_INTERACTIVE, nvars, vars);
 }
 
-#ifndef CONFIG_SPL_BUILD
+#ifndef CONFIG_SPL_BUILD_MINIMAL
 /*
  * Check if CRC is valid and (if yes) import the environment.
  * Note that "buf" may or may not be aligned.
diff --git a/include/configs/MPC8313ERDB.h b/include/configs/MPC8313ERDB.h
index c28dfe0..9b917a3 100644
--- a/include/configs/MPC8313ERDB.h
+++ b/include/configs/MPC8313ERDB.h
@@ -47,6 +47,7 @@ 
 
 #ifdef CONFIG_SPL_BUILD
 #define CONFIG_NS16550_MIN_FUNCTIONS
+#define CONFIG_SPL_BUILD_MINIMAL
 #endif
 
 #define CONFIG_SYS_TEXT_BASE	0x00100000 /* CONFIG_SYS_NAND_U_BOOT_DST */
diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
index 8b13b10..1cefed9 100644
--- a/include/configs/P1022DS.h
+++ b/include/configs/P1022DS.h
@@ -55,6 +55,9 @@ 
 #define CONFIG_SYS_NAND_U_BOOT_START   0x00200000
 #define CONFIG_SYS_NAND_U_BOOT_OFFS    0
 #define CONFIG_SYS_LDSCRIPT            "arch/powerpc/cpu/mpc85xx/u-boot-nand.lds"
+#ifdef CONFIG_SPL_BUILD
+#define CONFIG_SPL_BUILD_MINIMAL
+#endif
 #endif
 
 /* High Level Configuration Options */
diff --git a/include/configs/p1_p2_rdb_pc.h b/include/configs/p1_p2_rdb_pc.h
index 7ed634b..d3d3bad 100644
--- a/include/configs/p1_p2_rdb_pc.h
+++ b/include/configs/p1_p2_rdb_pc.h
@@ -187,6 +187,9 @@ 
 #define CONFIG_SYS_NAND_U_BOOT_SIZE	((512 << 10) - 0x2000)
 #define CONFIG_SYS_NAND_U_BOOT_OFFS	0
 #define CONFIG_SYS_LDSCRIPT		"arch/powerpc/cpu/mpc85xx/u-boot-nand.lds"
+#ifdef CONFIG_SPL_BUILD
+#define CONFIG_SPL_BUILD_MINIMAL
+#endif
 #endif
 
 #ifndef CONFIG_SYS_TEXT_BASE