[U-Boot] [PATCH 4/4] PXA: Adapt Voipac PXA270 to OneNAND SPL

[Marek Vasut]

> On 11/01/2011 05:12 PM, Marek Vasut wrote:
> >> On 10/31/2011 08:23 AM, Marek Vasut wrote:
> >>> Signed-off-by: Marek Vasut <marek.vasut at gmail.com>
> >>> Cc: Albert ARIBAUD <albert.u.boot at aribaud.net>
> >>> ---
> > 
> > [...]
> > 
> >>> +	for (page = 0; page <= total_pages; page++) {
> >>> +		ret = spl_onenand_read_page(0, page, addr, data.pagesize);
> >>> +		if (ret)
> >>> +			total_pages++;
> >>> +		else
> >>> +			addr += data.pagesize;
> >>> +	}
> >>> +}
> >> 
> >> You want to skip to the next block if spl_onenand_read_page() fails
> >> (which can occur after you've already read some of the block).
> > 
> > I want to skip to next page, not next block.
> 
> That's not how we normally do things, and is not what the current
> OneNAND IPL does.
> 
> Bad block markers apply to the entire block -- unless this is a
> difference I'm not aware of between NAND and OneNAND.

Well then it will fail reading the whole block and continue onwards ... it's a 
bit slower like this.

> 
> >> Is it not possible to use a simple memcpy for spl_copy_self()?  If the
> >> CPU can run the code, you'd think it could read it.
> > 
> > Not exactly. The OneNAND only exposes first 1kb of the contents (aka 1
> > half of the page 0 in my case). That's why I link all of the relevant
> > code there and the rest of the SPL is aligned beyond that. Then I copy
> > the whole SPL to SRAM and execute it again. Then I init DRAM, copy
> > U-Boot there and run it. Simple, isn't it.
> 
> Where do you ensure that the stuff used so far is within the 1K?  What
> parts are not within the 1K?
> 
> I don't see a linker script.

Is in V2, missing.

> 
> >>> +inline void icache_disable(void) {}
> >>> +inline void dcache_disable(void) {}
> >> 
> >> Why are you specifying inline on just about everything, even functions
> >> that are not used in this file?
> > 
> > They are, by dram_init();
> 
> There's no point marking something inline if it's not used later on in
> the same file -- functions aren't inlined across file boundaries.
> You've got inline functions at the very end of the file.
> 
> For that matter, there's not much point marking anything inline that
> isn't a static inline in a header file (where the compiler must not
> generate a non-inline version) -- the compiler has heuristics for
> inlining things, and excessive inlining tends to make things bigger
> rather than smaller.
> 
> >> Why are you not specifying static on things that are not needed outside
> >> this file?
> > 
> > They are actually needed outside.
> 
> All of them, including spl_copy_uboot and spl_copy_self?
> 
> >>> diff --git a/board/vpac270/vpac270.c b/board/vpac270/vpac270.c
> >>> index 43bbdff..f146009 100644
> >>> --- a/board/vpac270/vpac270.c
> >>> +++ b/board/vpac270/vpac270.c
> >>> @@ -56,7 +56,9 @@ struct serial_device *default_serial_console(void)
> >>> 
> >>>  extern void pxa_dram_init(void);
> >>>  int dram_init(void)
> >>>  {
> >>> 
> >>> +#ifndef	CONFIG_ONENAND
> >>> 
> >>>  	pxa_dram_init();
> >>> 
> >>> +#endif
> >>> 
> >>>  	gd->ram_size = PHYS_SDRAM_1_SIZE;
> >>>  	return 0;
> >>>  
> >>>  }
> >> 
> >> Should this really be about whether OneNAND support is present, or
> >> should it be based on whether you're using the OneNAND SPL?
> > 
> > Basically, on this board this is the same thing.
> 
> If you can turn off onenand at all, that suggests there's another boot
> source.  Is it not possible to access onenand when using that other boot
> source?

No, they are mutually exclusive.

> 
> In any case, best to use the symbol that most closely matches the reason
> you're skipping it, which is something SPL-related.
> 
> -Scott