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

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.