Add DT support for NAND to LCDK via ti-aemif

Hi,

Here is another spin. I could get my hands on an EVM and test the changes on it as well. Doing so, I could also observe that 4bit ECC on 8bits NANDS is currently broken in mailine. Apparently there had already been some alerts on the mailing list some months ago.

Rgds,
Karl

Changes from v1:
- s/cs2/cs3
- kept "ti,.." only nand properties (the adjustments made by nand_davinci_probe are broken)
- replaced v1_1/4:
"memory: ti-aemif: Get a named clock rather than an unnamed one"
with v2_1/4:
"davinci: da8xx-dt: Add ti-aemif lookup for clock matching"
- Make the same improvements for the EVM as well as asked by Sekhar

Changes from v2:
- remove now useless pins of nand_cs3 in v3_3/4

Karl Beldan (4):
ARM: davinci: da8xx-dt: Add ti-aemif lookup for clock matching
ARM: davinci_all_defconfig: Enable AEMIF as a module
ARM: dts: da850,da850-evm: Add an aemif node and use it for the NAND
ARM: dts: da850-lcdk: Add NAND to DT

 arch/arm/boot/dts/da850-evm.dts        |  49 +++++++++++++--
 arch/arm/boot/dts/da850-lcdk.dts       | 108 +++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/da850.dtsi           |  35 +++--------
 arch/arm/configs/davinci_all_defconfig |   2 +
 arch/arm/mach-davinci/da850.c          |   1 +
 arch/arm/mach-davinci/da8xx-dt.c       |   1 +
 6 files changed, 164 insertions(+), 32 deletions(-)

2.9.2

Many davinci boards (da830 and da850 families) don't have their clocks in DT yet and won't be successful in getting an unnamed aemif clock without explicitly registering them via clk_lookups, failing the ti-aemif memory driver probe.

The current aemif lookup entry resolving to the same clock:
'CLK(NULL, "aemif", &aemif_clk)'
remains, as it is currently used (davinci_nand is getting a named clock "aemif").

This change will allow to switch from the mach-davinci aemif code to the ti-aemif memory driver.

---
 arch/arm/mach-davinci/da850.c    | 1 +
 arch/arm/mach-davinci/da8xx-dt.c | 1 +
 2 files changed, 2 insertions(+)

index 0d046ac..ed3d0e9 100644
@@ -501,6 +501,7 @@ static struct clk_lookup da850_clks[] = {
 	CLK("da8xx_lcdc.0",	"fck",		&lcdc_clk),
 	CLK("da830-mmc.0",	NULL,		&mmcsd0_clk),
 	CLK("da830-mmc.1",	NULL,		&mmcsd1_clk),
+	CLK("ti-aemif",		NULL,		&aemif_clk),
 	CLK(NULL,		"aemif",	&aemif_clk),
 	CLK(NULL,		"usb11",	&usb11_clk),
 	CLK(NULL,		"usb20",	&usb20_clk),

index ca99711..c9f7e92 100644
@@ -37,6 +37,7 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("ti,davinci-dm6467-emac", 0x01e20000, "davinci_emac.1",
 		       NULL),
 	OF_DEV_AUXDATA("ti,da830-mcasp-audio", 0x01d00000, "davinci-mcasp.0", NULL),
+	OF_DEV_AUXDATA("ti,da850-aemif", 0x68000000, "ti-aemif", NULL),
 	{}
 };
 

2.9.2

Actually none of DaVinci platforms have clocks in DT yet.

I am happy with the patch itself. But I think the terminology used in the commit message can be made more accurate. clk_get() does not look up a clock by name. It looks up a clock by consumer device and a consumer id (used for multiple clocks used by same consumer device). The AEMIF clock in DaVinci has a name already. Its assigned in da850.c as "aemif". But the clock name itself does not matter in clock lookup.

So, IMO, saying "won't be successful in getting an unnamed aemif clock" is inaccurate.

So the existing look-up does not recognize the AEMIF as a device (NULL device name) and is using a "global" consumer id to look-up "device-less" clocks. As you noted, this entry should remain for non-DT mode and for backward compatibility.

Thanks,
Sekhar

Indeed, I haven't got used to the TI platforms, I mistook some omap SoCs for davinci ones.

I will be happy to send a more accurate comment, here is my case for the record and the feedback, although I try to keep my distance from comments of comments ;).

Checking clk_get:

struct clk *clk_get(struct device *dev, const char *con_id)

{       
	[...]
	if (dev) {
		struct clk *__of_clk_get_by_name(struct device_node *np,
						 const char *dev_id,
						 const char *name)
		clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
		[...]
	}
	return clk_get_sys(dev_id, con_id);
}

In DT case the con_id _is_ the clock name, so the assertion "clk_get() does not look up a clock by name" would be false ?
Also, numerous commits refer to *clk_get(*, NULL) as getting an unnamed clock, although it only really is accurate to the point in DT cases.

You are right, it is innacurate, because in that case it won't try getting such a clock, ie. by name. Instead it will resort to looking it up by dev_id / con_id, connecting back with your point.

I will resend the series with this commit message reworded.

Rgds,
Karl

Wrong.

	clocks = <&provider 0>, <&provider 1>;
	clock-names = "fck", "ick";

	fck = clk_get(dev, "fck");

	ick = clk_get(dev, "ick");

Works just the same. The whole point of the string is to identify an _individual_ _input_ _to_ _the_ _device_ and not a global clock name.

Think what happens if you specify a clock name. Where does that name come from? What if the clock is named differently on another SoC? With that approach, we need to define a new set of APIs to allow a device to determine the global clock name for the clock that it's interested in - which is completely absurd when we've already got that within clk_get().

The whole point of the clk API is to abstract those differences. That's why it takes a _connection_ _id_ and not a clock name.

I really can't understand why people keep getting this wrong in their heads. It makes no sense to me for this to take a global clock name.

Table-driven clkdev copes fine with that, and will try to locate an appropriate table entry with a NULL connection ID, not matching any non-NULL connection ID entry.

In the DT case, it's always the first specified clock.

RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

I hope so.
You are matching con_id with _a_ clock name, that's my point.

Maybe my writing 'the clock name' instead of 'a clock name' made you deduce I was asserting in some way that the clock tree was addressed by some kind of a unique root namespace via the clock-names ?? It was not the case ('the' was contextual if that means anything).

Nope, I am fine with that, it is a convenient scheme.

Yes it does, my change relies on that and I was happy to be able to so.

It is good to see the maintainers like Sekhar and you seizing the opportunity to throw light on such things.

Karl

This enables the use of the memory/ti-aemif.c driver.
ATM most davinci boards use the mach-davinci aemif code which gets in the way of genericity and proper DT boot.

---
 arch/arm/configs/davinci_all_defconfig | 2 ++
 1 file changed, 2 insertions(+)

index d037d3d..2802897 100644
@@ -192,6 +192,8 @@ CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_OMAP=m
 CONFIG_DMADEVICES=y
 CONFIG_TI_EDMA=y
+CONFIG_MEMORY=y
+CONFIG_TI_AEMIF=m
 CONFIG_EXT2_FS=y
 CONFIG_EXT3_FS=y
 CONFIG_XFS_FS=m

2.9.2

Applied to v4.9/defconfig.

Regards,
Sekhar

Currently the davinci da8xx boards use the mach-davinci aemif code. Instantiating an aemif node into the DT allows to use the ti-aemif memory driver and is another step to better DT support.
This change adds an aemif node in the dtsi while retiring the nand_cs3 node. The NAND is now instantiated in the dts as a subnode of the aemif one along with its pins.

---
 arch/arm/boot/dts/da850-evm.dts | 49 ++++++++++++++++++++++++++++++++++++-----
 arch/arm/boot/dts/da850.dtsi    | 35 +++++++----------------------
 2 files changed, 52 insertions(+), 32 deletions(-)

index 1a15db8..eedcc59 100644
@@ -29,6 +29,20 @@
 					0x04 0x00011000 0x000ff000
 				>;
 			};
+			nand_pins: nand_pins {
+				pinctrl-single,bits = <
+					/* EMA_WAIT[0], EMA_OE, EMA_WE, EMA_CS[4], EMA_CS[3] */
+					0x1c 0x10110110  0xf0ff0ff0
+					/*
+					 * EMA_D[0], EMA_D[1], EMA_D[2],
+					 * EMA_D[3], EMA_D[4], EMA_D[5],
+					 * EMA_D[6], EMA_D[7]
+					 */
+					0x24 0x11111111  0xffffffff
+					/* EMA_A[1], EMA_A[2] */
+					0x30 0x01100000  0x0ff00000
+				>;
+			};
 		};
 		serial0: serial at 42000 {
 			status = "okay";

@@ -131,11 +145,6 @@
 			status = "okay";
 		};
 	};
-	nand_cs3 at 62000000 {
-		status = "okay";
-		pinctrl-names = "default";
-		pinctrl-0 = <&nand_cs3_pins>;
-	};
 	vbat: fixedregulator at 0 {
 		compatible = "regulator-fixed";
 		regulator-name = "vbat";

@@ -250,3 +259,33 @@
 &edma1 {
 	ti,edma-reserved-slot-ranges = <32 90>;
 };
+
+&aemif {
+	pinctrl-names = "default";
+	pinctrl-0 = <&nand_pins>;
+	status = "ok";
+	cs3 {
+		#address-cells = <2>;
+		#size-cells = <1>;
+		clock-ranges;
+		ranges;
+
+		ti,cs-chipselect = <3>;
+
+		nand at 2000000,0 {
+			compatible = "ti,davinci-nand";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0 0x02000000 0x02000000
+			       1 0x00000000 0x00008000>;
+
+			ti,davinci-chipselect = <1>;
+			ti,davinci-mask-ale = <0>;
+			ti,davinci-mask-cle = <0>;
+			ti,davinci-mask-chipsel = <0>;
+			ti,davinci-ecc-mode = "hw";
+			ti,davinci-ecc-bits = <4>;
+			ti,davinci-nand-use-bbt;
+		};
+	};
+};

index bc10e7e..9ba018f 100644
@@ -77,22 +77,6 @@
 					0x10 0x00220000 0x00ff0000
 				>;
 			};
-			nand_cs3_pins: pinmux_nand_pins {
-				pinctrl-single,bits = <
-					/* EMA_OE, EMA_WE */
-					0x1c 0x00110000  0x00ff0000
-					/* EMA_CS[4],EMA_CS[3]*/
-					0x1c 0x00000110  0x00000ff0
-					/*
-					 * EMA_D[0], EMA_D[1], EMA_D[2],
-					 * EMA_D[3], EMA_D[4], EMA_D[5],
-					 * EMA_D[6], EMA_D[7]
-					 */
-					0x24 0x11111111  0xffffffff
-					/* EMA_A[1], EMA_A[2] */
-					0x30 0x01100000  0x0ff00000
-				>;
-			};
 			i2c0_pins: pinmux_i2c0_pins {
 				pinctrl-single,bits = <
 					/* I2C0_SDA,I2C0_SCL */

@@ -411,17 +395,14 @@
 			dma-names = "tx", "rx";
 		};
 	};
-	nand_cs3@62000000 {
-		compatible = "ti,davinci-nand";
-		reg = <0x62000000 0x807ff
-		       0x68000000 0x8000>;
-		ti,davinci-chipselect = <1>;
-		ti,davinci-mask-ale = <0>;
-		ti,davinci-mask-cle = <0>;
-		ti,davinci-mask-chipsel = <0>;
-		ti,davinci-ecc-mode = "hw";
-		ti,davinci-ecc-bits = <4>;
-		ti,davinci-nand-use-bbt;
+	aemif: aemif at 68000000 {
+		compatible = "ti,da850-aemif";
+		#address-cells = <2>;
+		#size-cells = <1>;
+
+		reg = <0x68000000 0x00008000>;
+		ranges = <0 0 0x60000000 0x08000000
+			  1 0 0x68000000 0x00008000>;
 		status = "disabled";
 	};
 };

2.9.2

Applied to v4.9/dts branch of my tree.

Regards,
Sekhar

This adds DT support for the NAND connected to the SoC AEMIF. The parameters (timings, ecc) are the same as what the board ships with (default AEMIF timings, 1bit ECC) and improvements will be handled in due course.
This passed elementary tests hashing a 20MB file on top of ubifs on my LCDK but I trust torture tests would show the weakness of the 1bit ECC.

---
 arch/arm/boot/dts/da850-lcdk.dts | 108 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

index dbcca0b..fca5336 100644
@@ -27,6 +27,27 @@
 
 &pmx_core {
 	status = "okay";
+
+	nand_pins: nand_pins {
+		pinctrl-single,bits = <
+			/* EMA_WAIT[0], EMA_OE, EMA_WE, EMA_CS[3] */
+			0x1c 0x10110010  0xf0ff00f0
+			/*
+			 * EMA_D[0], EMA_D[1], EMA_D[2],
+			 * EMA_D[3], EMA_D[4], EMA_D[5],
+			 * EMA_D[6], EMA_D[7]
+			 */
+			0x24 0x11111111  0xffffffff
+			/*
+			 * EMA_D[8],  EMA_D[9],  EMA_D[10],
+			 * EMA_D[11], EMA_D[12], EMA_D[13],
+			 * EMA_D[14], EMA_D[15]
+			 */
+			0x20 0x11111111  0xffffffff
+			/* EMA_A[1], EMA_A[2] */
+			0x30 0x01100000  0x0ff00000
+		>;
+	};
 };
 
 &serial2 {

@@ -68,3 +89,90 @@
 	cd-gpios = <&gpio 64 GPIO_ACTIVE_HIGH>;
 	status = "okay";
 };
+
+&aemif {
+	pinctrl-names = "default";
+	pinctrl-0 = <&nand_pins>;
+	status = "ok";
+	cs3 {
+		#address-cells = <2>;
+		#size-cells = <1>;
+		clock-ranges;
+		ranges;
+
+		ti,cs-chipselect = <3>;
+
+		nand at 2000000,0 {
+			compatible = "ti,davinci-nand";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0 0x02000000 0x02000000
+			       1 0x00000000 0x00008000>;
+
+			ti,davinci-chipselect = <1>;
+			ti,davinci-mask-ale = <0>;
+			ti,davinci-mask-cle = <0>;
+			ti,davinci-mask-chipsel = <0>;
+
+			/*
+			 * nand_ecc_strength_good will emit a warning
+			 * but the LCDK ships with these settings [1].
+			 * Also HW 4bits ECC with 16bits NAND seems to
+			 * require some attention, and it doesn't work
+			 * in mainline for 8bits ones.
+			 *
+			 * ATM nand_davinci_probe handling of nand-ecc-*
+			 * is broken, e.g.
+			 * chip.ecc.strength = pdata->ecc_bits occurs after
+			 * scan_ident(), otherwise I would have used:
+			 * 	nand-ecc-mode = "hw";
+			 * 	nand-ecc-strength = <1>;
+			 * 	nand-ecc-step-size = <512>;
+			 */
+			ti,davinci-nand-buswidth = <16>;
+			ti,davinci-ecc-mode = "hw";
+			ti,davinci-ecc-bits = <1>;
+			ti,davinci-nand-use-bbt;
+
+			/*
+			 * LCDK original partitions:
+			 * 0x000000000000-0x000000020000 : "u-boot env"
+			 * 0x000000020000-0x0000000a0000 : "u-boot"
+			 * 0x0000000a0000-0x0000002a0000 : "kernel"
+			 * 0x0000002a0000-0x000020000000 : "filesystem"
+			 *
+			 * The 1st NAND block being guaranted to be valid w/o ECC (> 1k cycles),
+			 * it makes a perfect candidate as an SPL for the BootROM to jump to.
+			 * However the OMAP-L132/L138 Bootloader doc SPRAB41E reads:
+			 * "To boot from NAND Flash, the AIS should be written to NAND block 1
+			 * (NAND block 0 is not used by default)", which matches the LCDK
+			 * original partitioning.
+			 * Also, the LCDK ships with only the u-boot partition provisioned and
+			 * boots on it in its default configuration while using the MMC for the
+			 * kernel and rootfs, so preserve that one as is for now.
+			 * [1]: Ensuring for example that U-Boot LCDK SPL can handle it properly
+			 * and a proper boot chain ROM->SPL->U-Boot->Linux wrt ECC, would allow
+			 * for a better partitioning.
+			 */
+			partitions {
+				compatible = "fixed-partitions";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				partition at 0 {
+					label = "u-boot env";
+					reg = <0 0x020000>;
+				};
+				partition at 0x020000 {
+					/* The LCDK defaults to booting from this partition */
+					label = "u-boot";
+					reg = <0x020000 0x080000>;
+				};
+				partition at 0x0a0000 {
+					label = "space";
+					reg = <0x0a0000 0>;
+				};
+			};
+		};
+	};
+};

2.9.2

[...]

Tested on DA850-LCDK with UBIFS.

Kevin

I forgot to mention that I had to enable:

CONFIG_UBIFS_FS=y
CONFIG_MTD_UBI=y

in order to test with ubifs. Care to send a patch adding these as modules to davinci_all_defconfig?

Thanks,

Kevin

Sure, I'll add it to the series when resending.

Karl

I just pushed an updated master branch on my tree with all the patches accepted so far. Can you please rebase against that branch while sending?

Regards,
Sekhar

I rebased and sent
[1] [PATCH] ARM: davinci_all_defconfig: Enable some UBI modules
[2] [PATCH v2] ARM: davinci: da8xx-dt:
Add ti-aemif lookup for clock matching

Unfortunately while automating I messed up and sent both twice and [2] as a reply to [1] while I intended to send them separately and preferably
[1] after [2].

You've already applied patches 2/4 and 3/4 of the original series while 3/4 depends on [2] and 4/4 is on hold because of the ECC issue.

Please tell me if you want to deal with these patches or prefer a resend.

Regards,
Karl