[U-Boot] [PATCH] powerpc/p1022ds: boot from SD card/SPI flash with SPL

[Scott Wood <scottwood@freescale.com>]

On 05/10/2013 03:44:29 AM, Zhang Ying-B40530 wrote:
> 
> 
> 
> This patch needs to be broken into several patches that each take  
> care of one logical problem; it's too hard to properly review (and  
> have the right people pay attention to certain parts) in its current  
> state.
> [Zhang Ying]
> It only can be broken to two patches, one for SD boot, another for  
> SPI boot.

You can also separate out logical changes in support of your eventual  
goal (as it looks like you already started doing with the  
CONFIG_SPL_ENV_SUPPORT patch and such).

> > @@ -83,5 +107,6 @@ SECTIONS
> >  		*(.sbss*)
> >  		*(.bss*)
> >  	}
> > +	. = ALIGN(4);
> >  	__bss_end = .;
> >  }
> 
> This seems unrelated.
> [Zhang Ying]
> It is necessary. In the function clear_bss(), the address of bss is  
> on the basis of the __bss_start, and then to four bytes of  
> incremental growth, finally the last stop is based on the address and  
> __bss_end is equal or not.

It may be necessary, but I don't see what it has to do with SD/SPI boot  
other than by chance.  Make this a separate patch with a changelog that  
explains the problem.

> > diff --git a/board/freescale/common/sdhc_boot.c
> > b/board/freescale/common/sdhc_boot.c
> > index e432318..96b0680 100644
> > --- a/board/freescale/common/sdhc_boot.c
> > +++ b/board/freescale/common/sdhc_boot.c
> 
> > +	val = *(u16 *)(tmp_buf + ESDHC_BOOT_IMAGE_SIGN_ADDR);
> > +	if ((u16)ESDHC_BOOT_IMAGE_SIGN != val) {
> > +		free(tmp_buf);
> > +		return;
> > +	}
> 
> Why do you need this cast?
> [Zhang Ying]
> The offset 0x1FE of the config data sector should contain the value  
> 0x55AA. If the value in this location doesn't match 0x55AA, it means  
> that the SD/MMC card doesn't contain a valid user code.

But why do you need to cast ESDHC_BOOT_IMAGE_SIGN to (u16)?

And the other cast probably violates C99 aliasing rules.

> > + *
> > + * You should have received a copy of the GNU General Public  
> License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + */
> > +
> > +#ifndef __SDHC_BOOT_H_
> > +#define __SDHC_BOOT_H_    1
> > +
> > +
> > +int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr); void
> > +mmc_get_env(void); void mmc_boot(void);
> > +
> > +#endif  /* __SDHC_BOOT_H_ */
> 
> Does this stuff really belong in board/freescale?  Should probably at  
> least be in arch/powerpc/cpu/mpc85xx, if not more generic.
> [Zhang Ying]
> Ok, whether we can handle like this: sdhc_boot.c and spi_boot.c will  
> be deleted. All this stuff in the sdhc_boot.c will be moved to  
> drivers/mmc/fsl_esdhc.c, and the functions in the spi_boot.c will be  
> moved to drivers/mmc/fsl_espi.c.

Maybe drivers/mmc/fsl_espi_spl.c and such?

> > +void hang(void)
> > +{
> > +	puts("### ERROR ### Please RESET the board ###\n");
> > +	for (;;)
> > +		;
> > +}
> 
> Whitespace
> [Zhang Ying]
> ?

I'm not sure what I meant here. :-P

Maybe I meant to put this comment elsewhere, or trimmed the wrong  
context...

> > diff --git a/common/Makefile b/common/Makefile index  
> 0e0fff1..bc80414
> > 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -225,6 +225,11 @@ COBJS-$(CONFIG_SPL_NET_SUPPORT) += env_common.o
> >  COBJS-$(CONFIG_SPL_NET_SUPPORT) += env_flags.o
> >  COBJS-$(CONFIG_SPL_NET_SUPPORT) += env_nowhere.o
> >  COBJS-$(CONFIG_SPL_NET_SUPPORT) += miiphyutil.o
> > +COBJS-$(CONFIG_SPL_HWCONFIG_SUPPORT) += hwconfig.o
> > +COBJS-$(CONFIG_SPL_INIT_DDR_SUPPORT) += ddr_spd.o
> > +COBJS-$(CONFIG_SPL_ENV_SUPPORT) += env_attr.o
> > +COBJS-$(CONFIG_SPL_ENV_SUPPORT) += env_flags.o
> > +COBJS-$(CONFIG_SPL_ENV_SUPPORT) += env_callback.o
> 
> CONFIG_SPL_ENV_SUPPORT should replace CONFIG_SPL_NET_SUPPORT here  
> (and add it to the boards that already have CONFIG_SPL_NET_SUPPORT).   
> This sort of refactoring needs to be a separate patch, BTW.
> 
> Can ddr_spd.o and hwconfig just use their normal CONFIG symbols (i.e.
> move the existing makefile line out of the !SPL ifdef)?  It's getting  
> a bit excessive with all the new SPL symbols.
> [Zhang Ying]
> If do it, the SPL's size will increase. I fear this will affect the  
> other SPL no CONFIG_SPL_NET_SUPPORT is defined.

Which SPL in particular are you concerned about, that uses  
common/Makefile at all, and have CONFIG_SPD and/or CONFIG_HWCONFIG?   
How much will they grow, after gc-sections (mainly all that's left is  
anonymous strings)?

> > +Where $file is the target file. Also keep in mind the u-boot.bin
> > file needs
> > +to be the u-boot built for your particular platform and target  
> media.
> > +
> > +Hint: To generate a u-boot.bin for a P1022DS booting from SD I  
> would
> > run the
> > +following in the u-boot repository:
> > +
> > +	$ make P1022DS_SDCARD
> 
> s/Hint/Example/ and s/from SD I would run/from SD, run/
> [Zhang Ying]
> run bootsd? This is another independent requirement. If we want, we  
> can do it in another project.

Hmm?  I was just fixing up the wording -- change "Hint" to "Example"  
and change "from SD I would run" to "from SD, run".

That said, yes I would like to see "run bootsd". :-)

-Scott