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

[Scott Wood]

On Thu, 11 Nov 2010 11:37:35 +0800
Xiangfu Liu <xiangfu at openmobilefree.net> wrote:

> +$(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*

Why do you need all this?

> +#define __nand_enable()		(REG_EMC_NFCSR |= EMC_NFCSR_NFE1 | EMC_NFCSR_NFCE1)
> +#define __nand_disable()	(REG_EMC_NFCSR &= ~(EMC_NFCSR_NFCE1))
> +#define __nand_ecc_rs_encoding() \
> +	(REG_EMC_NFECR = EMC_NFECR_ECCE | EMC_NFECR_ERST | EMC_NFECR_RS | EMC_NFECR_RS_ENCODING)
> +#define __nand_ecc_rs_decoding() \
> +	(REG_EMC_NFECR = EMC_NFECR_ECCE | EMC_NFECR_ERST | EMC_NFECR_RS | EMC_NFECR_RS_DECODING)
> +#define __nand_ecc_disable()	(REG_EMC_NFECR &= ~EMC_NFECR_ECCE)
> +#define __nand_ecc_encode_sync() while (!(REG_EMC_NFINTS & EMC_NFINTS_ENCF))
> +#define __nand_ecc_decode_sync() while (!(REG_EMC_NFINTS & EMC_NFINTS_DECF))
> +#define __nand_cmd(n)		(REG8(NAND_COMMPORT) = (n))
> +#define __nand_addr(n)		(REG8(NAND_ADDRPORT) = (n))
> +#define __nand_data8()		REG8(NAND_DATAPORT)
> +#define __nand_data16()		REG16(NAND_DATAPORT)

Drop the leading underscores.

Use I/O accessors.

Consider making some of these inline functions -- or just open-coding
for the really simple ones.

> +#if (JZ4740_NANDBOOT_CFG == JZ4740_NANDBOOT_B8R3)
> +	#define NAND_BUS_WIDTH 8
> +	#define NAND_ROW_CYCLE 3
> +#elif (JZ4740_NANDBOOT_CFG == JZ4740_NANDBOOT_B8R2)
> +	#define NAND_BUS_WIDTH 8
> +	#define NAND_ROW_CYCLE 2
> +#elif (JZ4740_NANDBOOT_CFG == JZ4740_NANDBOOT_B16R3)
> +	#define NAND_BUS_WIDTH 16
> +	#define NAND_ROW_CYCLE 3
> +#elif (JZ4740_NANDBOOT_CFG == JZ4740_NANDBOOT_B16R2)
> +	#define NAND_BUS_WIDTH 16
> +	#define NAND_ROW_CYCLE 2
> +#endif
> +
> +static inline void __nand_dev_ready(void)
> +{
> +	unsigned int timeout = 10000;
> +	while ((REG_GPIO_PXPIN(2) & 0x40000000) && timeout--);
> +	while (!(REG_GPIO_PXPIN(2) & 0x40000000));
> +}

Hmm, why a timeout here and not on any of the other spin loops?

> +/*
> + * NAND flash parameters
> + */
> +static int page_size = 2048;
> +static int oob_size = 64;
> +static int ecc_count = 4;
> +static int page_per_block = 64;
> +static int bad_block_pos = 0;
> +static int block_size = 131072;

You initialize these here, and then you set them to other constant
values in nand_boot().  And then you test them for specific values
later.

What's going on?  If you're not dynamically detecting these, then have
them be constants (either #defines or "static const int").  It will
allow the compiler to generate smaller code.

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

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();

Get these from the appropriate header file.

> +			/* Error occurred */
> +			/* serial_puts("Error occurred\n"); */
> +			if (stat & EMC_NFINTS_UNCOR) {
> +				/* Uncorrectable error occurred */
> +				/* serial_puts("Uncorrectable error occurred\n"); */

Why is this commented out?

If it's for space concerns, the strings could be shrunk and still get a
message out.

> +			} else {
> +				unsigned int errcnt, index, mask;
> +
> +				errcnt = (stat & EMC_NFINTS_ERRCNT_MASK) >> EMC_NFINTS_ERRCNT_BIT;
> +				switch (errcnt) {
> +				case 4:
> +					index = (REG_EMC_NFERR3 & EMC_NFERR_INDEX_MASK) >> EMC_NFERR_INDEX_BIT;
> +					mask = (REG_EMC_NFERR3 & EMC_NFERR_MASK_MASK) >> EMC_NFERR_MASK_BIT;
> +					rs_correct(tmpbuf, index, mask);
> +					/* FALL-THROUGH */
> +				case 3:
> +					index = (REG_EMC_NFERR2 & EMC_NFERR_INDEX_MASK) >> EMC_NFERR_INDEX_BIT;
> +					mask = (REG_EMC_NFERR2 & EMC_NFERR_MASK_MASK) >> EMC_NFERR_MASK_BIT;
> +					rs_correct(tmpbuf, index, mask);
> +					/* FALL-THROUGH */
> +				case 2:
> +					index = (REG_EMC_NFERR1 & EMC_NFERR_INDEX_MASK) >> EMC_NFERR_INDEX_BIT;
> +					mask = (REG_EMC_NFERR1 & EMC_NFERR_MASK_MASK) >> EMC_NFERR_MASK_BIT;
> +					rs_correct(tmpbuf, index, mask);
> +					/* FALL-THROUGH */
> +				case 1:
> +					index = (REG_EMC_NFERR0 & EMC_NFERR_INDEX_MASK) >> EMC_NFERR_INDEX_BIT;
> +					mask = (REG_EMC_NFERR0 & EMC_NFERR_MASK_MASK) >> EMC_NFERR_MASK_BIT;
> +					rs_correct(tmpbuf, index, mask);
> +					break;
> +				default:
> +					break;
> +				}

Please keep lines under 80 columns.  Consider factoring this chunk out
to its own function.

> +			}
> +		}
> +		tmpbuf += CONFIG_SYS_NAND_ECCSIZE;
> +	}
> +
> +	return 0;
> +}
> +
> +#ifndef CONFIG_SYS_NAND_BADBLOCK_PAGE
> +#define CONFIG_SYS_NAND_BADBLOCK_PAGE 0 /* NAND bad block was marked at this page in a block, starting from 0 */
> +#endif

This isn't really an aspect of the controller, but of the flash chip.
Many flash chips can have bad block markers in the first or second page
of a block.

> +static void nand_load(int offs, int uboot_size, uchar *dst)
> +{
> +	int page;
> +	int pagecopy_count;
> +
> +	__nand_enable();
> +
> +	page = offs / page_size;
> +	pagecopy_count = 0;
> +	while (pagecopy_count < (uboot_size / page_size)) {

Round up if uboot_size is not aligned?  Any reason not to take the
basic loop logic from nand_boot.c?

> +		if (page % page_per_block == 0) {
> +			nand_read_oob(page + CONFIG_SYS_NAND_BADBLOCK_PAGE, oob_buf, oob_size);
> +			if (oob_buf[bad_block_pos] != 0xff) {
> +				page += page_per_block;
> +				/* Skip bad block */
> +				continue;
> +			}

On 16-bit NAND isn't the bad-block marker 16-bit?

> +		}
> +		/* Load this page to dst, do the ECC */
> +		nand_read_page(page, dst, oob_buf);
> +
> +		dst += page_size;
> +		page++;
> +		pagecopy_count++;
> +	}
> +
> +	__nand_disable();
> +}
> +
> +static void jz_nand_init(void) {

Brace on its own line.

> + 	/* Optimize the timing of nand */
> +	REG_EMC_SMCR1 = 0x094c4400;
> +}
> +
> +static void gpio_init(void)
> +{
> +	/*
> +	 * Initialize SDRAM pins
> +	 */
> +	__gpio_as_sdram_16bit_4720();
> +
> +	/*
> +	 * Initialize UART0 pins
> +	 */
> +	__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);
> +
> +	/*
> +	 * Init hardware
> +	 */
> +	gpio_init();
> +	pll_init();
> +
> +	serial_init();
> +	sdram_init();
> +	jz_nand_init();
> +
> +	serial_puts("\nNAND Boot\n");
> +
> +#if defined(CONFIG_NANONOTE)
> +	if(is_usb_boot()) {

Space after "if".

> +		serial_puts("[U] pressed, goto USBBOOT mode\n");
> +		usb_boot();
> +	}
> +#endif

I wonder how much of the above stuff belongs in nand_boot(), versus a
board file.  This file should focus on the NAND controller.

> +	page_size = CONFIG_SYS_NAND_PAGE_SIZE;
> +	block_size = CONFIG_SYS_NAND_BLOCK_SIZE;
> +	page_per_block = CONFIG_SYS_NAND_BLOCK_SIZE / CONFIG_SYS_NAND_PAGE_SIZE;
> +	bad_block_pos = (page_size == 512) ? 5 : 0;
> +	oob_size = page_size / 32;
> +	ecc_count = page_size / CONFIG_SYS_NAND_ECCSIZE;
> +
> +	/*
> +	 * Load U-Boot image from NAND into RAM
> +	 */
> +	nand_load(CONFIG_SYS_NAND_U_BOOT_OFFS, CONFIG_SYS_NAND_U_BOOT_SIZE,
> +		  (uchar *)CONFIG_SYS_NAND_U_BOOT_DST);

Consider adding support for CONFIG_NAND_ENV_DST.

-Scott