[U-Boot] [PATCH] V2 NAND_SPL support for phycore imx31to Scott Wood

[Maxim Artamonov]

In message on Friday 12 December 2008 04:55:49 Scott Wood wrote:
> On Fri, Dec 05, 2008 at 02:13:51PM +0300, Maxim Artamonov wrote:
> > +#ifdef CONFIG_NAND_SPL
> > +/* somewhat macro to reduce bin size for CONFIG_NAND_SPL*/
> > +.macro FILLREGS begreg, val, count, step
> 
> Why not use this macro always?
Why not? But, I think this 'll lose clearity of code. Let's discuss this.

> 
> > @@ -32,6 +35,15 @@
> >  #include <version.h>
> >  .globl _start
> >  _start: b	reset
> > +#ifdef CONFIG_NAND_SPL
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +#else
> >  #ifdef CONFIG_ONENAND_IPL
> 
> #elif
> 
> Do NAND and OneNAND really have different requirements here?
> 
> > @@ -156,9 +169,9 @@ relocate:				/* relocate U-Boot to RAM	    */
> >  	adr	r0, _start		/* r0 <- current position of code   */
> >  	ldr	r1, _TEXT_BASE		/* test if we run from flash or RAM */
> >  	cmp	r0, r1			/* don't reloc during debug	    */
> > -#ifndef CONFIG_ONENAND_IPL
> > +#if !defined(CONFIG_ONENAND_IPL) && !defined(CONFIG_NAND_SPL)
> 
> Should define one symbol that is set if either of the above are set, to
> simplify the ifdefs.
> 
I don't work with OneNAND flash and reference to "hang" is deeply in source tree, and we have only 2kB on
 NAND in-place of 4kB on OneNand. Unhappily, I don't have anytime to learn about OneNand and to change its code.

> > +#define NFC_BUF_SIZE            (*((volatile u16 *)(NFC_BASE_ADDR + 0xE00)))
> > +#define NFC_BUF_ADDR            (*((volatile u16 *)(NFC_BASE_ADDR + 0xE04)))
>> ...
> > +#define NFC_CONFIG1             (*((volatile u16 *)(NFC_BASE_ADDR + 0xE1A)))
> > +#define NFC_CONFIG2             (*((volatile u16 *)(NFC_BASE_ADDR + 0xE1C)))
> 
> Use I/O accessors, 
What you about, tell me please?

> and move the NFC register layout to a 
> non-arch-specific header file (preferably as a struct), as it's shared
> with other chips.

Which chips? This is a system's integration task, but I wrote only for imx-31 phycore. I don't
understand in this. 
> 
> >  #define	CONFIG_EXTRA_ENV_SETTINGS											\
> > -	"bootargs_base=setenv bootargs console=ttySMX0,115200\0"							\
> > -	"bootargs_nfs=setenv bootargs $(bootargs) root=/dev/nfs ip=dhcp nfsroot=$(serverip):$(nfsrootfs),v3,tcp\0"	\
> > -	"bootargs_flash=setenv bootargs $(bootargs) root=/dev/mtdblock2 rootfstype=jffs2"				\
> > -	"bootargs_mtd=setenv bootargs $(bootargs) $(mtdparts)"								\
> > +	"jffs2=root-pcm037.jffs2\0"											\
> > +	"uimage=uImage-pcm037\0"											\
> > +	"nfsrootfs=/nfsroot\0"												\
> >  	"bootcmd=run bootcmd_net\0"											\
> > +	"mtdids=nor0=phys_mapped_flash\0"										\
> > +	"bootargs_base=setenv bootargs console=ttymxc0,115200\0"							\
> > +	"bootargs_nfs=setenv bootargs $(bootargs) root=/dev/nfs ip=dhcp nfsroot=$(serverip):$(nfsrootfs),v3,tcp\0"	\
> > +	"bootargs_mtd=setenv bootargs $(bootargs) $(mtdparts)\0"							\
> > +	"bootargs_flash=setenv bootargs $(bootargs) root=/dev/mtdblock3 rootfstype=jffs2\0"				\
> 
> There are changes in here that don't seem to be related to nand boot.
> 
Yes, it's so. I'll change code.

> > +static void mx31_wait_ready(void)
> > +{
> > +	while (1)
> > +		if (NFC_CONFIG2 & NFC_INT){
> > +				NFC_CONFIG2 = NFC_CONFIG2 & (~NFC_INT);/*int flag reset*/
> > +				break;
> > +		}
> > +}
> 
> There's an extra indentation level in the if-body.
> 
> Braces around the while-body.  Space before and after braces (here and
> elsewhere).
> 
> > +static void mx31_nand_init(void)
> > +{
> > +	/*unlocking RAM Buff*/
> > +	NFC_CONFIG = 0x2;
> > +
> > +	/*hardware ECC checking and correct*/
> > +	NFC_CONFIG1 = NFC_ECC_EN;
> 
> Space after /*, before */
> 
It will apply.

> > +	if (mx31_nand_check_ecc ())
> > +		while (1){
> > +			break;};	/*!!!!!*/
> 
> What is this?
Ok, let's it be infinitive while-cycle.

> 
> > +	p1 = (u32 *)MAIN_AREA0;
> > +	p2 = (u32 *)buf;
> > +
> > +	/*main copy loop from NAND-buffer to SDRAM memory*/
> > +	for (i=0; i < (CFG_NAND_PAGE_SIZE / 4); i++){		
> > +		*p2 = *p1;
> > +		p1++;
> > +		p2++;
> > +	}
> > +	
> > +	p1 = (u32 *)SPARE_AREA0;
> > +
> > +	/*it is hardware specific code for 8-bit 512kB NAND-flash spare area*/
> > +	p1++;
> > +	a = *p1;
> > +	a = (a & 0x0000ff00) >> 8;
> > +
> > +	if (a != 0xff) /*bad block marker verify*/
> > +		return 1;/*potential bad block*/
> > +	
> > +	return 0;
> > +}
> > +
> > +static int nand_load(unsigned int from, unsigned int size, unsigned char *buf)
> > +{
> > +	int i, bb;
> > +	
> > +	mx31_nand_init ();
> > +	
> > +	/*conver from to page number*/
> > +	from = from / CFG_NAND_PAGE_SIZE;
> > +
> > +	i = 0;
> > +
> > +	while (i < (size/CFG_NAND_PAGE_SIZE)){
> > +		
> > +		if ((from*CFG_NAND_PAGE_SIZE) >= CFG_NAND_CHIP_SIZE)
> > +			return 2;/*memory segment violation*/
> > +
> > +		bb = mx31_read_page (from, buf);
> > +
> > +		/*checking first page of each block*/
> > +		/*if this page has bb marker, then skip whole block*/
> > +		if ((!(from % CFG_NAND_PAGE_COUNT)) && bb){
> > +			from = from + CFG_NAND_PAGE_COUNT;
> 
> CONFIG_NAND_PAGES_PER_BLOCK would probably be a better name.
All right! 

with regards, Maxim