| Message ID | 1368781939-15750-1-git-send-email-ying.zhang@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
On Fri, May 17, 2013 at 05:12:19PM +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> > --- > Compared with the previous version, give up new symbol and delete the line > ifndef CONFIG_SPL_BUILD in common/env_common.c What the heck is going on? First, you seem to be changing a number of checks from !CONFIG_SPL_BUILD to !CONFIG_SPL_NAND_MINIMAL, and then stop defining CONFIG_SPL_NAND_MINIMAL always and only define it for CONFIG_SPL_BUILD. Next, powerpc uses -ffunction-sections/-fdata-sections/--gc-sections so outside of assembler files, we shouldn't need to be using CONFIG_SPL_BUILD to not build something that's a static function. Can you please post the everything as a series, including adding the board(s) that need environment in SPL?
On Fri, May 17, 2013 at 02:10:49PM +0000, Zhang Ying-B40530 wrote: > ________________________________________ > From: Tom Rini [tom.rini@gmail.com] on behalf of Tom Rini [trini@ti.com] > Sent: Friday, May 17, 2013 12:34 PM > To: Zhang Ying-B40530 > Cc: u-boot@lists.denx.de; Wood Scott-B07421; afleming@gmail.com; Xie Xiaobo-R63061; Zhang Ying-B40530 > Subject: Re: [U-Boot] [PATCH v2] spl: Make CONFIG_SPL_BUILD contain more functionality > > On Fri, May 17, 2013 at 05:12:19PM +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> > > --- > > Compared with the previous version, give up new symbol and delete the line > > ifndef CONFIG_SPL_BUILD in common/env_common.c > > What the heck is going on? First, you seem to be changing a number of > checks from !CONFIG_SPL_BUILD to !CONFIG_SPL_NAND_MINIMAL, and then stop > defining CONFIG_SPL_NAND_MINIMAL always and only define it for > CONFIG_SPL_BUILD. > Next, powerpc uses > -ffunction-sections/-fdata-sections/--gc-sections so outside of > assembler files, we shouldn't need to be using CONFIG_SPL_BUILD to not > build something that's a static function. > [Zhang Ying] > First?$B!$ Your understanding is correct. > CONFIG_SPL_NAND_MINIMAL has not been used and it suited to express a > state(CONFIG_SPL_BUILD && CONFIG_SPL_INIT_MINIMAL && CONFIG_NAND) > So, I used !CONFIG_SPL_NAND_MINIMAL to contain some functionality for > SD SPL but not for NAND SPL. > > Second, I tried. If we don't use !CONFIG_SPL_BUILD to build, the SPL size is > increased and the SPL size exceeds 4K Bytes. As you know, the NAND SPL > for mpc85xx can't large than 4K, Now only a few bytes of free space. > > Can you please post the everything as a series, including adding the > board(s) that need environment in SPL? > [Zhang Ying] > The patch is split into several in order to facilitate everyone to review. Right. So please post a 5 or 10 or whatever part series that shows us the end goal as well as each step it takes to go from today to that platform working. Thanks!
On Fri, May 17, 2013 at 02:26:04PM +0000, Zhang Ying-B40530 wrote: > > ________________________________________ > From: Tom Rini [tom.rini@gmail.com] on behalf of Tom Rini [trini@ti.com] > Sent: Friday, May 17, 2013 2:20 PM > To: Zhang Ying-B40530 > Cc: u-boot@lists.denx.de; afleming@gmail.com; Xie@theia.denx.de; Wood Scott-B07421 > Subject: Re: [U-Boot] [PATCH v2] spl: Make CONFIG_SPL_BUILD contain more functionality > > On Fri, May 17, 2013 at 02:10:49PM +0000, Zhang Ying-B40530 wrote: > > > ________________________________________ > > From: Tom Rini [tom.rini@gmail.com] on behalf of Tom Rini [trini@ti.com] > > Sent: Friday, May 17, 2013 12:34 PM > > To: Zhang Ying-B40530 > > Cc: u-boot@lists.denx.de; Wood Scott-B07421; afleming@gmail.com; Xie Xiaobo-R63061; Zhang Ying-B40530 > > Subject: Re: [U-Boot] [PATCH v2] spl: Make CONFIG_SPL_BUILD contain more functionality > > > > On Fri, May 17, 2013 at 05:12:19PM +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> > > > --- > > > Compared with the previous version, give up new symbol and delete the line > > > ifndef CONFIG_SPL_BUILD in common/env_common.c > > > > What the heck is going on? First, you seem to be changing a number of > > checks from !CONFIG_SPL_BUILD to !CONFIG_SPL_NAND_MINIMAL, and then stop > > defining CONFIG_SPL_NAND_MINIMAL always and only define it for > > CONFIG_SPL_BUILD. > > Next, powerpc uses > > -ffunction-sections/-fdata-sections/--gc-sections so outside of > > assembler files, we shouldn't need to be using CONFIG_SPL_BUILD to not > > build something that's a static function. > > [Zhang Ying] > > First, Your understanding is correct. > > CONFIG_SPL_NAND_MINIMAL has not been used and it suited to express a > > state(CONFIG_SPL_BUILD && CONFIG_SPL_INIT_MINIMAL && CONFIG_NAND) > > So, I used !CONFIG_SPL_NAND_MINIMAL to contain some functionality for > > SD SPL but not for NAND SPL. > > > > Second, I tried. If we don't use !CONFIG_SPL_BUILD to build, the SPL size is > > increased and the SPL size exceeds 4K Bytes. As you know, the NAND SPL > > for mpc85xx can't large than 4K, Now only a few bytes of free space. > > > > Can you please post the everything as a series, including adding the > > board(s) that need environment in SPL? > > [Zhang Ying] > > The patch is split into several in order to facilitate everyone to review. > > Right. So please post a 5 or 10 or whatever part series that shows us > the end goal as well as each step it takes to go from today to that > platform working. Thanks! > [Zhang Ying] > So for this patch, are there any problem? > Yes. It doesn't seem to do anything. Which is why I want to see the series of patches that result in new functionality on some boards, so we can evaluate the changes in context.
On 05/17/2013 10:04:12 AM, Zhang Ying-B40530 wrote: > > ________________________________________ > From: Tom Rini [tom.rini@gmail.com] on behalf of Tom Rini > [trini@ti.com] > Sent: Friday, May 17, 2013 2:54 PM > To: Zhang Ying-B40530 > Cc: Xie@theia.denx.de; u-boot@lists.denx.de; afleming@gmail.com; > Wood@theia.denx.de; Wood Scott-B07421 > Subject: Re: [U-Boot] [PATCH v2] spl: Make CONFIG_SPL_BUILD contain > more functionality > > On Fri, May 17, 2013 at 02:26:04PM +0000, Zhang Ying-B40530 wrote: > > [Zhang Ying] > > So for this patch, are there any problem? > > > > Yes. It doesn't seem to do anything. Which is why I want to see the > series of patches that result in new functionality on some boards, so > we > can evaluate the changes in context. > [Zhang Ying] > It seems to be several patches were sent together. No, they weren't. They were sent separately. Use git format-patch to generate all the patches at once, so they're numbered, and use git send-email to send them all at once, so they're threaded properly. -Scott
Oh, I understand. Thanks.
On 05/17/2013 07:34:25 AM, Tom Rini wrote: > On Fri, May 17, 2013 at 05:12:19PM +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> > > --- > > Compared with the previous version, give up new symbol and delete > the line > > ifndef CONFIG_SPL_BUILD in common/env_common.c > > What the heck is going on? First, you seem to be changing a number of > checks from !CONFIG_SPL_BUILD to !CONFIG_SPL_NAND_MINIMAL, and then > stop > defining CONFIG_SPL_NAND_MINIMAL always and only define it for > CONFIG_SPL_BUILD. Next, powerpc uses > -ffunction-sections/-fdata-sections/--gc-sections so outside of > assembler files, we shouldn't need to be using CONFIG_SPL_BUILD to not > build something that's a static function. gc-sections does not work on anonymous strings. It also doesn't work to avoid code that doesn't compile due to some missing dependency (e.g. missing #define). I don't know off the top of my head if this code falls into one of these categories (I don't even know what code we're talking about from the context here, without digging back for the original patch). I'm not sure why being a static function matters. -Scott
On Fri, May 24, 2013 at 02:06:01PM -0500, Scott Wood wrote: > On 05/17/2013 07:34:25 AM, Tom Rini wrote: > >On Fri, May 17, 2013 at 05:12:19PM +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> > >> --- > >> Compared with the previous version, give up new symbol and > >delete the line > >> ifndef CONFIG_SPL_BUILD in common/env_common.c > > > >What the heck is going on? First, you seem to be changing a number of > >checks from !CONFIG_SPL_BUILD to !CONFIG_SPL_NAND_MINIMAL, and > >then stop > >defining CONFIG_SPL_NAND_MINIMAL always and only define it for > >CONFIG_SPL_BUILD. Next, powerpc uses > >-ffunction-sections/-fdata-sections/--gc-sections so outside of > >assembler files, we shouldn't need to be using CONFIG_SPL_BUILD to not > >build something that's a static function. > > gc-sections does not work on anonymous strings. It also doesn't > work to avoid code that doesn't compile due to some missing > dependency (e.g. missing #define). I don't know off the top of my > head if this code falls into one of these categories (I don't even > know what code we're talking about from the context here, without > digging back for the original patch). > > I'm not sure why being a static function matters. Yes, there's a gcc bug (afaict) about strings, and #define-related fail to builds don't work, just unreferenced functions. You do sometimes need/want to whack #ifndef's around static functions for when you have to not define function A because of one of the above fails to be collected, and then you end up with an unused function warning.
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..7ad76c8 100644 --- a/include/configs/MPC8313ERDB.h +++ b/include/configs/MPC8313ERDB.h @@ -40,13 +40,13 @@ #define CONFIG_SPL_INIT_MINIMAL #define CONFIG_SPL_SERIAL_SUPPORT #define CONFIG_SPL_NAND_SUPPORT -#define CONFIG_SPL_NAND_MINIMAL #define CONFIG_SPL_FLUSH_IMAGE #define CONFIG_SPL_TARGET "u-boot-with-spl.bin" #define CONFIG_SPL_MPC83XX_WAIT_FOR_NAND #ifdef CONFIG_SPL_BUILD #define CONFIG_NS16550_MIN_FUNCTIONS +#define CONFIG_SPL_NAND_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..12e5e88 100644 --- a/include/configs/P1022DS.h +++ b/include/configs/P1022DS.h @@ -41,7 +41,6 @@ #define CONFIG_SPL_INIT_MINIMAL #define CONFIG_SPL_SERIAL_SUPPORT #define CONFIG_SPL_NAND_SUPPORT -#define CONFIG_SPL_NAND_MINIMAL #define CONFIG_SPL_FLUSH_IMAGE #define CONFIG_SPL_TARGET "u-boot-with-spl.bin" @@ -55,6 +54,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_NAND_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..59fb9d2 100644 --- a/include/configs/p1_p2_rdb_pc.h +++ b/include/configs/p1_p2_rdb_pc.h @@ -159,7 +159,6 @@ #define CONFIG_SPL_INIT_MINIMAL #define CONFIG_SPL_SERIAL_SUPPORT #define CONFIG_SPL_NAND_SUPPORT -#define CONFIG_SPL_NAND_MINIMAL #define CONFIG_SPL_FLUSH_IMAGE #define CONFIG_SPL_TARGET "u-boot-with-spl.bin" @@ -187,6 +186,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_NAND_MINIMAL +#endif #endif #ifndef CONFIG_SYS_TEXT_BASE