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

[Marek Vasut]

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

> 
> How much of this is board-specific?
> 
> > +inline void spl_copy_uboot(void)
> > +{
> > +	uint8_t *addr = (uint8_t *)CONFIG_SYS_TEXT_BASE;
> > +	struct spl_onenand_data data;
> > +	uint32_t total_pages;
> > +	uint32_t block;
> > +	uint32_t page, rpage;
> > +	int ret;
> > +
> > +	spl_onenand_get_geometry(&data);
> > +
> > +	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> > +	total_pages = CONFIG_SPL_ONENAND_LOAD_SIZE >> 11;
> > +	page = CONFIG_SPL_ONENAND_LOAD_ADDR >> 11;
> > +	if (data.pagesize == 4096) {
> > +		total_pages >>= 1;
> > +		page >>= 1;
> > +	}
> > +
> > +	for (; page <= total_pages; page++) {
> > +		block = page >> 6;
> > +		rpage = page & 0xff;
> > +		ret = spl_onenand_read_page(block, rpage, addr, data.pagesize);
> > +		if (ret)
> > +			total_pages++;
> > +		else
> > +			addr += data.pagesize;
> > +	}
> > +}
> 
> What is so different about this compared to spl_copy_self, that warrants
> such duplication?  Can't you just pass in offset, length, and
> destination as parameters?  Or just have the OneNAND SPL driver export
> nand_spl_load_image(), as any other NAND SPL driver would?

Good idea.

> 
> > +inline void board_init_f(unsigned long unused)
> > +{
> > +	uint32_t tmp;
> > +
> > +	asm volatile("mov %0, pc" : "=r"(tmp));
> > +	tmp >>= 24;
> > +
> > +	/* The code runs from OneNAND RAM, copy SPL to SRAM and execute it. */
> > +	if (tmp == 0) {
> > +		spl_copy_self();
> > +		asm volatile("mov pc, %0" : : "r"(CONFIG_SPL_TEXT_BASE));
> > +	}
> 
> 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.

> 
> > +inline void board_init_r(gd_t *id, ulong dest_addr)
> > +{
> > +	for (;;)
> > +		;
> > +}
> 
> This doesn't seem like a useful board_init_r().  If you don't need it,
> maybe make sure it's not called, and save yourself some bytes in the
> SPL.  Likewise for the other stub functions, where practical.
> 
> > +inline int printf(const char *fmt, ...)
> > +{
> > +	return 0;
> > +}
> > +
> > +inline void __coloured_LED_init(void) {}
> > +inline void __red_LED_on(void) {}
> > +void coloured_LED_init(void)
> > +	__attribute__((weak, alias("__coloured_LED_init")));
> > +void red_LED_on(void)
> > +	__attribute__((weak, alias("__red_LED_on")));
> > +void hang(void) __attribute__ ((noreturn));
> > +void hang(void)
> > +{
> > +	for (;;)
> > +		;
> > +}
> > +
> > +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();

> 
> Why are you not specifying static on things that are not needed outside
> this file?

They are actually needed outside.
> 
> > 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.
> 
> -Scott