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

["Daniel Kochmański" <jackdaniel@hellsgate.pl>]

Ian Campbell writes:

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

I'm not sure what is a reason, but controller can't initialize MMC when
SPL is loaded. MMC works in main u-boot allowing to boot from it and
perform any other option (rescan, part, info, etc.). It is only SPL
relevant.
>> +
>>  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.
>

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

OK, I'll read about it and try to adjust driver.
>
> 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.

This work is mainly based on register guess-work from here:
http://rhombus-tech.net/allwinner_a10/A10_register_guide/A10_NAND/ - so
I can gather names from there and put them in source.
>
>> +	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.
>

Sure.
>> +	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()?
>

Ok, I'll look into that.
>> +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?
>

I'll check this, I think I had problems with that, but if it works w/o
temp_buf on both SPL and main U-Boot builds, then I'll get rid of it.
> Ian.

-- 
Daniel Kochma?ski | Pozna?, Poland
;; aka jackdaniel

"Be the change that you wish to see in the world." - Mahatma Gandhi