[U-Boot] [PATCH 2/6] nand: Add SPL_NAND support to mxc_nand_spl

["Benoît Thébaudeau" <benoit.thebaudeau@advansee.com>]

Dear Marek Vasut,

On Friday, April 19, 2013 7:06:39 PM, Marek Vasut wrote:
> Subject: Re: [PATCH 2/6] nand: Add SPL_NAND support to mxc_nand_spl
> 
> Dear Beno?t Th?baudeau,
> 
> > Dear Marek Vasut,
> > 
> > On Friday, April 19, 2013 1:14:16 PM, Marek Vasut wrote:
> > > Dear Beno?t Th?baudeau,
> > > 
> > > > On Friday, April 19, 2013 10:38:48 AM, Beno?t Th?baudeau wrote:
> > > > > Dear Marek Vasut,
> > > > > 
> > > > > On Friday, April 19, 2013 6:10:51 AM, Marek Vasut wrote:
> > > > > > Add support for generic NAND SPL via the SPL framework into the
> > > > > > mxc_nand_spl driver. This is basically just a simple rename and
> > > > > > publication of the already implemented functions. To avoid the
> > > > > > old function which are used with the nand_spl/ stuff getting in
> > > > > > the way of NAND SPL framework, the macro CONFIG_SPL_NAND_LEGACY
> > > > > > was introduced and two remaining legacy boards were adjusted.
> > > > > > These board need to be either fixed or removed in the long run,
> > > > > > but I don't have either.
> > > > > > 
> > > > > > Also make sure the requested payload is aligned to full pages,
> > > > > > otherwise this simple driver fails to load the last page.
> > > > > > 
> > > > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > > > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > > > > > Cc: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> > > > > > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > > > > > Cc: Scott Wood <scottwood@freescale.com>
> > > > > > Cc: Stefano Babic <sbabic@denx.de>
> > > > > > Cc: Tom Rini <trini@ti.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/mtd/nand/mxc_nand_spl.c | 13 ++++++++++---
> > > > > >  include/configs/mx31pdk.h       |  1 +
> > > > > >  include/configs/tx25.h          |  1 +
> > > > > >  3 files changed, 12 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/mtd/nand/mxc_nand_spl.c
> > > > > > b/drivers/mtd/nand/mxc_nand_spl.c
> > > > > > index 09f23c3..8ff03c9 100644
> > > > > > --- a/drivers/mtd/nand/mxc_nand_spl.c
> > > > > > +++ b/drivers/mtd/nand/mxc_nand_spl.c
> > > > > > @@ -290,7 +290,7 @@ static int is_badblock(int pagenumber)
> > > > > > 
> > > > > >  	return 0;
> > > > > >  
> > > > > >  }
> > > > > > 
> > > > > > -static int nand_load(unsigned int from, unsigned int size,
> > > > > > unsigned char *buf)
> > > > > > +int nand_spl_load_image(uint32_t from, unsigned int size, void
> > > > > > *buf)
> > > > > > 
> > > > > >  {
> > > > > >  
> > > > > >  	int i;
> > > > > >  	unsigned int page;
> > > > > > 
> > > > > > @@ -303,6 +303,7 @@ static int nand_load(unsigned int from,
> > > > > > unsigned int size, unsigned char *buf)
> > > > > > 
> > > > > >  	page = from / CONFIG_SYS_NAND_PAGE_SIZE;
> > > > > >  	i = 0;
> > > > > > 
> > > > > > +	size = roundup(size, CONFIG_SYS_NAND_PAGE_SIZE);
> > > > > > 
> > > > > >  	while (i < size / CONFIG_SYS_NAND_PAGE_SIZE) {
> > > > > >  	
> > > > > >  		if (nfc_read_page(page, buf) < 0)
> > > > > >  		
> > > > > >  			return -1;
> > > > > > 
> > > > > > @@ -332,6 +333,7 @@ static int nand_load(unsigned int from,
> > > > > > unsigned int size, unsigned char *buf)
> > > > > > 
> > > > > >  	return 0;
> > > > > >  
> > > > > >  }
> > > > > > 
> > > > > > +#ifdef CONFIG_SPL_NAND_SUPPORT_LEGACY
> > > > > > 
> > > > > >  /*
> > > > > >  
> > > > > >   * The main entry for NAND booting. It's necessary that SDRAM is
> > > > > >   already * configured and available since this code loads the main
> > > > > >   U-Boot image
> > > > > > 
> > > > > > @@ -345,8 +347,9 @@ void nand_boot(void)
> > > > > > 
> > > > > >  	 * CONFIG_SYS_NAND_U_BOOT_OFFS and CONFIG_SYS_NAND_U_BOOT_SIZE
> > > > > >  	 must * be aligned to full pages
> > > > > >  	 */
> > > > > > 
> > > > > > -	if (!nand_load(CONFIG_SYS_NAND_U_BOOT_OFFS,
> > > > > > CONFIG_SYS_NAND_U_BOOT_SIZE, -		       (uchar
> > > > > > *)CONFIG_SYS_NAND_U_BOOT_DST)) {
> > > > > > +	if (!nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
> > > > > > +			CONFIG_SYS_NAND_U_BOOT_SIZE,
> > > > > > +			(uchar *)CONFIG_SYS_NAND_U_BOOT_DST)) {
> > > > > > 
> > > > > >  		/* Copy from NAND successful, start U-boot */
> > > > > >  		uboot = (void *)CONFIG_SYS_NAND_U_BOOT_START;
> > > > > >  		uboot();
> > > > > > 
> > > > > > @@ -364,3 +367,7 @@ void hang(void)
> > > > > > 
> > > > > >  	/* Loop forever */
> > > > > >  	while (1) ;
> > > > > >  
> > > > > >  }
> > > > > > 
> > > > > > +#endif
> > > > > > +
> > > > > > +void nand_init(void) {}
> > > > > > +void nand_deselect(void) {}
> > > > > > diff --git a/include/configs/mx31pdk.h b/include/configs/mx31pdk.h
> > > > > > index 1754595..217552e 100644
> > > > > > --- a/include/configs/mx31pdk.h
> > > > > > +++ b/include/configs/mx31pdk.h
> > > > > > @@ -50,6 +50,7 @@
> > > > > > 
> > > > > >  #define CONFIG_SPL_LDSCRIPT	"arch/$(ARCH)/cpu/u-boot-spl.lds"
> > > > > >  #define CONFIG_SPL_MAX_SIZE	2048
> > > > > >  #define CONFIG_SPL_NAND_SUPPORT
> > > > > > 
> > > > > > +#define CONFIG_SPL_NAND_SUPPORT_LEGACY
> > > > > > 
> > > > > >  #define CONFIG_SPL_TEXT_BASE	0x87dc0000
> > > > > >  #define CONFIG_SYS_TEXT_BASE	0x87e00000
> > > > > > 
> > > > > > diff --git a/include/configs/tx25.h b/include/configs/tx25.h
> > > > > > index e72f8f6..7c362d0 100644
> > > > > > --- a/include/configs/tx25.h
> > > > > > +++ b/include/configs/tx25.h
> > > > > > @@ -37,6 +37,7 @@
> > > > > > 
> > > > > >  #define CONFIG_SPL_LDSCRIPT		"arch/$(ARCH)/cpu/u-boot-
> > > 
> > > spl.lds"
> > > 
> > > > > >  #define CONFIG_SPL_MAX_SIZE		2048
> > > > > >  #define CONFIG_SPL_NAND_SUPPORT
> > > > > > 
> > > > > > +#define CONFIG_SPL_NAND_SUPPORT_LEGACY
> > > > > > 
> > > > > >  #define CONFIG_SPL_TEXT_BASE		0x810c0000
> > > > > >  #define CONFIG_SYS_TEXT_BASE		0x81200000
> > > > > > 
> > > > > > --
> > > > > > 1.7.11.7
> > > > > 
> > > > > This is not about legacy vs. non-legacy. This is about basic vs. more
> > > > > featured
> > > > > SPL because of SPL size constraints. So what about dropping
> > > > > CONFIG_SPL_NAND_SUPPORT_LEGACY and testing for CONFIG_SPL_FRAMEWORK
> > > > > definition
> > > > > instead?
> > > 
> > > I was thinking about that, but the symbol is unrelated to NAND.
> > 
> > Well, it's CONFIG_SPL_FRAMEWORK + CONFIG_SPL_NAND_SUPPORT that defines the
> > other implementation, and CONFIG_SPL_NAND_SUPPORT triggers the build of
> > mxc_nand_spl.c for SPL, so the common point is CONFIG_SPL_FRAMEWORK.
> > 
> > > I still think
> > > it's either a matter of fixing for new SPL or removing those two boards.
> > > The nand_spl/ stuff shall be removed ASAP.
> > 
> > Removing those boards is not a solution.
> > 
> > Is it really about "new" SPL? The generic SPL is enabled by CONFIG_SPL, and
> > CONFIG_SPL_FRAMEWORK is a sub-option. If the generic SPL rules imposed to
> > use the SPL framework functions, there would be no such sub-option. So it
> > looks like these boards are complying to the new SPL rules.
> > 
> > We could see if using CONFIG_SPL_FRAMEWORK would still allow the SPL to fit
> > in 2 kiB in order to drop this function, but if it does not fit, the new
> > SPL rules should still make it possible to have a solution for any board
> > having SPL size constraints.
> 
> Rather than this, the other option would be to make whatever calls
> nand_boot()
> compatible with the SPL frameworks' implementation of spl_nand, so we don't
> need
> different function calls.

If by that you mean renaming and reorganizing functions without using the main
SPL framework, sure. For hang(), this has already been done by Andreas' pending
series, and for nand_boot(), Tom's plan seems to be to merge the various
existing implementations, and beyond that it would be possible to make the calls
look more like the SPL framework's as you suggest.

Best regards,
Beno?t