[U-Boot] [PATCH v3 3/5] jz4740 nand spl files

[Scott Wood]

On Mon, 6 Dec 2010 19:53:54 +0800
<xiangfu at openmobilefree.net> wrote:

> From: Xiangfu Liu <xiangfu at openmobilefree.net>
> 
> Signed-off-by: Xiangfu Liu <xiangfu at openmobilefree.net>
> ---
>  nand_spl/board/xburst/nanonote/Makefile   |   96 ++++++++
>  nand_spl/board/xburst/nanonote/u-boot.lds |   63 ++++++
>  nand_spl/nand_boot_jz4740.c               |  344 +++++++++++++++++++++++++++++

As I asked last time, is there any reason you can't use the standard
nand_boot.c?  Your non-SPL NAND driver looks like the type that would
work with it.

> +LDSCRIPT= $(TOPDIR)/nand_spl/board/$(BOARDDIR)/u-boot.lds
> +LDFLAGS	= -Bstatic -T $(LDSCRIPT) -Ttext $(TEXT_BASE)
> +AFLAGS	+= -DCONFIG_NAND_SPL
> +CFLAGS	+= -DCONFIG_NAND_SPL -O2

Are you sure you want -O2 and not -Os?  What are the space constraints
for this SPL?

> +$(nandobj)u-boot-spl-16k.bin: $(nandobj)u-boot-spl.bin
> +	dd bs=1024 count=8 if=/dev/zero of=$(nandobj)junk1
> +	cat $< $(nandobj)junk1 > $(nandobj)junk2
> +	dd bs=1024 count=8 if=$(nandobj)junk2 of=$(nandobj)junk3
> +	cat $(nandobj)junk3 $(nandobj)junk3 > $(nandobj)junk4
> +	dd bs=1024 count=256 if=/dev/zero of=$(nandobj)junk5
> +	cat $(nandobj)junk4 $(nandobj)junk5 > $(nandobj)junk6
> +	dd bs=1024 count=256 if=$(nandobj)junk6 of=$@
> +	rm -f $(nandobj)junk*

Again, please explain what's going on here.  Other boards don't do this.

> +static inline void __nand_dev_ready(void)

No underscores.

> +{
> +	unsigned int timeout = 10000;
> +	while ((readl(GPIO_PXPIN(2)) & 0x40000000) && timeout--)
> +		;
> +
> +	while (!(readl(GPIO_PXPIN(2)) & 0x40000000))
> +		;
> +}

From last time:
"Hmm, why a timeout here and not on any of the other spin loops?"

> +static unsigned char oob_buf[128] = {0};

From last time:

"Be careful, this may still go into the BSS even with the assignment.
Is the BSS getting cleared in the SPL boot code on this platform?  Do
you actually rely on it being zero initially?

Either way, the assignment doesn't do much."

> +
> +/*External routines */
> +extern void flush_cache_all(void);
> +extern int serial_init(void);
> +extern void serial_puts(const char *s);
> +extern void sdram_init(void);
> +extern void pll_init(void);
> +extern void usb_boot();

Can't you get these from headers?

> +static void gpio_init(void)
> +{
> +	__gpio_as_sdram_16bit_4720();
> +	__gpio_as_uart0();
> +	__gpio_jtag_to_uart0();
> +}
> +
> +static int is_usb_boot()
> +{
> +	__gpio_as_input(KEY_U_IN);
> +	__gpio_enable_pull(KEY_U_IN);
> +	__gpio_as_output(KEY_U_OUT);
> +	__gpio_clear_pin(KEY_U_OUT);
> +
> +	if (__gpio_get_pin(KEY_U_IN) == 0)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +void nand_boot(void)
> +{
> +	void (*uboot)(void);
> +
> +	gpio_init();
> +	pll_init();
> +	serial_init();
> +	sdram_init();
> +	writel(0x094c4400, EMC_SMCR1);	 	/* Optimize the timing of nand */
> +
> +	serial_puts("\nNAND Boot\n");
> +
> +#if defined(CONFIG_NANONOTE)
> +	if (is_usb_boot()) {
> +		serial_puts("[U] pressed, goto USBBOOT mode\n");
> +		usb_boot();
> +	}
> +#endif

Again, this should go in a board file, which is separate from the NAND
implementation.  Especially if this is the only thing keeping you from
using the generic nand_boot.c.

-Scott