[U-Boot] [PATCH 2/5] nand: sunxi: Add support for booting from internal NAND memory

[Ian Campbell <ijc@hellion.org.uk>]

On Wed, 2015-04-29 at 17:02 +0200, Daniel Kochma?ski wrote:
> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
> index 88e3358..1a30684 100644
> --- a/board/sunxi/Kconfig
> +++ b/board/sunxi/Kconfig
> @@ -239,6 +239,18 @@ config MMC_SUNXI_SLOT_EXTRA
>  	slot or emmc on mmc1 - mmc3. Setting this to 1, 2 or 3 will enable
>  	support for this.
>  
> +config SPL_NAND_SUPPORT
> +	bool "SPL/NAND mode support"
> +	depends on SPL
> +	default n
> +	---help---
> +	  This enables support for booting from NAND internal
> +	  memory. U-Boot SPL doesn't detect where is it load from,
> +	  therefore this option is needed to properly load image from
> +	  flash. Option also disables MMC functionality on U-Boot due to
> +	  initialization errors encountered, when both controllers are
> +	  enabled.

Is this last bit a bug in the s/w or a hardware thing? Does this mean
that MMC is not available in the main u-boot image too when NAND support
is enabled?

> +
>  config USB0_VBUS_PIN
>  	string "Vbus enable pin for usb0 (otg)"
>  	default ""
> diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile
> index 43766e0..7ad7412 100644
> --- a/board/sunxi/Makefile
> +++ b/board/sunxi/Makefile
> @@ -9,6 +9,7 @@
>  # SPDX-License-Identifier:	GPL-2.0+
>  #
>  obj-y	+= board.o
> +obj-$(CONFIG_NAND_SUNXI)	+= nand.o

CONFIG_SUNXI_NAND would be more consistent with the rest I think.

>  obj-$(CONFIG_SUNXI_GMAC)	+= gmac.o
>  obj-$(CONFIG_SUNXI_AHCI)	+= ahci.o

> +void nand_set_clocks(void)
> +{
> +	W32(PORTC_BASE + 0x48, 0x22222222);

u-boot style is to declare a struct which matches the register layout
and to do something like:
        struct nand_ctrl_foo *mcf = (void *)NANCFLASHC_BASE;

And use writel, set_bits, setclr_bits and friends.

Please also try and give sensible names to all the magic numbers, or at
least include a comment explaining that these are magic numbers derived
from $SOMEWHERE and we don't know what they mean (if that is the case).

Both of these apply to several bits of the code too.

> +	W32(PORTC_BASE + 0x4C, 0x22222222);
> +	W32(PORTC_BASE + 0x50, 0x2222222);
> +	W32(PORTC_BASE + 0x54, 0x2);
> +	W32(PORTC_BASE + 0x5C, 0x55555555);
> +	W32(PORTC_BASE + 0x60, 0x15555);
> +	W32(PORTC_BASE + 0x64, 0x5140);
> +	W32(PORTC_BASE + 0x68, 0x4016);
> +
> +	uint32_t val = R32(CCMU_BASE + 0x60);
> +	W32(CCMU_BASE + 0x60, 0x2000 | val);
> +	val = R32(CCMU_BASE + 0x80);
> +	W32(CCMU_BASE + 0x80, val | 0x80000000 | 0x1);
> +}
> +
> +int initialized = 0;
> +void nand_init(void)
> +{
> +	initialized = 1;

Please call this from somewhere during init e.g. board.c rather than
from nand_read with a latch.

> +	uint32_t val;
> +	nand_set_clocks();
> +	val = R32(NANDFLASHC_BASE + 0x00);
> +	/* CTL = (1<<0 <-EN    1<<1 RESET) */
> +	W32(NANDFLASHC_BASE + 0x00, val | 0x3);	/* enable and reset CTL */
> +	do {
> +		val = R32(NANDFLASHC_BASE + 0x00);
> +		if (val & (1<<1))
> +			break;
> +	} while (1);

Potentially infinite loop?

There were some similar instances below which had a t/o. Perhaps combine
them all into a helper similar to the dram code's
mctl_await_completion()?

> +uint32_t ecc_errors = 0;
> +
> +/* read 0x400 bytes from real_addr to temp_buf */
> +void nand_read_block(unsigned int real_addr, int syndrome)
> +{
> +	uint32_t val;
> +	if (!initialized)
> +		nand_init();
> +
> +	memset((void *)temp_buf, 0, 0x400); /* clear temp_buf */

Can we not avoid going via this global temp_buf by setting DMAC_BASE +
0x308 to the correct address on each read?

Ian.