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

[Scott Wood]

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.

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

>>> +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?

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

-Scott