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

login
register
mail settings
Submitter Marek Vasut
Date April 19, 2013, 4:10 a.m.
Message ID <1366344655-8535-2-git-send-email-marex@denx.de>
Download mbox | patch
Permalink /patch/237838/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Marek Vasut - April 19, 2013, 4:10 a.m.
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(-)
Benoît Thébaudeau - April 19, 2013, 8:38 a.m.
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?

Best regards,
Benoît
Benoît Thébaudeau - April 19, 2013, 9:35 a.m.
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?

Also, I don't see any nand_init() (called by spl_nand_load_image()) for m53evk.

Best regards,
Benoît
Marek Vasut - April 19, 2013, 11:14 a.m.
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. 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.

> Also, I don't see any nand_init() (called by spl_nand_load_image()) for
> m53evk.

What do you mean?

Best regards,
Marek Vasut
Benoît Thébaudeau - April 19, 2013, 11:55 a.m.
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.

> > Also, I don't see any nand_init() (called by spl_nand_load_image()) for
> > m53evk.
> 
> What do you mean?

I mean that I don't identify the implementation of nand_init() used in the
context of the m53evk since the builds of drivers/mtd/nand/nand.c and
drivers/mtd/nand/nand_spl_simple.c do not seem to be enabled for this board, and
I don't see another definition.

Best regards,
Benoît
Philip Paeps - April 19, 2013, 1 p.m.
On 2013-04-19 06:10:51 (+0200), Marek Vasut <marex@denx.de> wrote:
> 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.

It sounds like "fixing" these boards is mainly a matter of confirming
that a configuration for them based around CONFIG_SPL_FRAMEWORK will fit
in 2K.  If so, I don't think there is any reason to keep legacy support
around.

> +void nand_init(void) {}
> +void nand_deselect(void) {}

Couldn't you just rename nfc_nand_init() to nand_init()?

 - Philip
Marek Vasut - April 19, 2013, 5:06 p.m.
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.

> > > Also, I don't see any nand_init() (called by spl_nand_load_image()) for
> > > m53evk.
> > 
> > What do you mean?
> 
> I mean that I don't identify the implementation of nand_init() used in the
> context of the m53evk since the builds of drivers/mtd/nand/nand.c and
> drivers/mtd/nand/nand_spl_simple.c do not seem to be enabled for this
> board, and I don't see another definition.

It's there, as you said somewhere in the thread below ;-)
Marek Vasut - April 19, 2013, 5:11 p.m.
Dear Philip Paeps,

> On 2013-04-19 06:10:51 (+0200), Marek Vasut <marex@denx.de> wrote:
> > 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.
> 
> It sounds like "fixing" these boards is mainly a matter of confirming
> that a configuration for them based around CONFIG_SPL_FRAMEWORK will fit
> in 2K.  If so, I don't think there is any reason to keep legacy support
> around.

These boards are too limited, so we can do nothing about them but to keep this 
hack in.

> > +void nand_init(void) {}
> > +void nand_deselect(void) {}
> 
> Couldn't you just rename nfc_nand_init() to nand_init()?

What good would that do?

Best regards,
Marek Vasut
Benoît Thébaudeau - April 20, 2013, 1:06 p.m.
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
Marek Vasut - April 20, 2013, 5:09 p.m.
Dear Benoît Thébaudeau,

> 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.

OK, makes sense.

Patch

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