[PATCH 0/4] mtd: nand: atmel: Rework DT representation of NFC/NAND

[PATCH 0/4] mtd: nand: atmel: Rework DT representation of NFC/NAND

[Boris Brezillon]

Hello,

While working on the EBI driver [1] I noticed that the relationship between
the NFC (NAND Flash Controller) and the NAND chip it is attached to would
make things harder to represent when moving the NAND node under the EBI bus
(another useless 'ranges' definition).

Actually this representation might be even more problematic if one wants
decide to connect two NAND chips on his sama5 based board, because the NFC
node is a child of the NAND chip node, and thus can only be attached to a
single NAND chip.

To address this problem the current series moves the NFC node outside of
the NAND chip device which then reference the NFC using the "atmel,nfc"
property.

The series does not implement multi-chip support, but at least the new
representation should make it possible.

Best Regards,

Boris

[1]https://lkml.org/lkml/2014/12/3/806

Boris Brezillon (4):
  mtd: nand: atmel: Rework driver to separate nfc and nand nodes
  mtd: nand: atmel: Update DT documentation after splitting NFC and NAND
  ARM: at91/dt: sama5: move NFC nodes outside of NAND nodes
  ARM: at91/dt: sama5: move NAND nodes into board dts/dtsi

 .../devicetree/bindings/mtd/atmel-nand.txt         | 46 +++++++------
 arch/arm/boot/dts/at91-sama5d3_xplained.dts        | 18 ++++-
 arch/arm/boot/dts/at91-sama5d4ek.dts               | 16 ++++-
 arch/arm/boot/dts/sama5d3.dtsi                     | 36 ++--------
 arch/arm/boot/dts/sama5d3xcm.dtsi                  | 18 ++++-
 arch/arm/boot/dts/sama5d4.dtsi                     | 36 ++--------
 drivers/mtd/nand/atmel_nand.c                      | 76 +++++++++++++++++-----
 7 files changed, 149 insertions(+), 97 deletions(-)

-- 
1.9.1

[PATCH 1/4] mtd: nand: atmel: Rework driver to separate nfc and nand nodes

[Boris Brezillon]

mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

The NAND and NFC (NAND Flash Controller) were linked together with a
parent <-> child relationship.

This model has several drawbacks:
- it does not allow for multiple NAND chip handling while the controller
  support multi-chip (even though the driver is not ready yet)
- it mixes NAND partitions and NFC nodes at the same level (which is a bit
  disturbing)
- the introduction of the EBI bus implies defining NAND chips under the
  EBI node, and the ranges available under the EBI node should be
  restricted to EBI address space, while the NFC references several
  registers outside of these EBI ranges.

Move the NFC node outside of the NAND node, to get a more future-proof
DT representation.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/atmel_nand.c | 76 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 19d1e9d..0239fe6 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -123,6 +123,7 @@ struct atmel_nand_host {
 	struct dma_chan		*dma_chan;
 
 	struct atmel_nfc	*nfc;
+	bool			wait_for_nfc;
 
 	bool			has_pmecc;
 	u8			pmecc_corr_cap;
@@ -1423,6 +1424,12 @@ static void atmel_nand_hwctl(struct mtd_info *mtd, int mode)
 		ecc_writel(host->ecc, CR, ATMEL_ECC_RST);
 }
 
+static const struct of_device_id atmel_nand_nfc_match[] = {
+	{ .compatible = "atmel,sama5d3-nfc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
+
 static int atmel_of_init_port(struct atmel_nand_host *host,
 			      struct device_node *np)
 {
@@ -1431,6 +1438,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
 	int ecc_mode;
 	struct atmel_nand_data *board = &host->board;
 	enum of_gpio_flags flags = 0;
+	struct device_node *child;
 
 	if (of_property_read_u32(np, "atmel,nand-addr-offset", &val) == 0) {
 		if (val >= 32) {
@@ -1467,8 +1475,30 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
 
 	host->has_pmecc = of_property_read_bool(np, "atmel,has-pmecc");
 
-	/* load the nfc driver if there is */
-	of_platform_populate(np, NULL, NULL, host->dev);
+	/*
+	 * Backward compatibility with DTs defining the NFC node as their
+	 * child.
+	 * The new model reference the NFC so that we can define several
+	 * chip controlled by the same controller.
+	 */
+	for_each_available_child_of_node(np, child) {
+		/*
+		 * If we find an NFC node under the NAND node, then populate
+		 * the device so that it can be probed, and wait for it.
+		 */
+		if (of_match_node(atmel_nand_nfc_match, child)) {
+			of_platform_populate(np, NULL, NULL, host->dev);
+			host->wait_for_nfc = true;
+			break;
+		}
+	}
+
+	/*
+	 * If there's an atmel,nfc property we should access the NAND
+	 * through the NFC.
+	 */
+	if (of_property_read_bool(np, "atmel,nfc"))
+		host->wait_for_nfc = true;
 
 	if (!(board->ecc_mode == NAND_ECC_HW) || !host->has_pmecc)
 		return 0;	/* Not using PMECC */
@@ -2032,10 +2062,6 @@ static int atmel_nand_probe(struct platform_device *pdev)
 	if (!host)
 		return -ENOMEM;
 
-	res = platform_driver_register(&atmel_nand_nfc_driver);
-	if (res)
-		dev_err(&pdev->dev, "atmel_nand: can't register NFC driver\n");
-
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	host->io_base = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(host->io_base)) {
@@ -2089,6 +2115,13 @@ static int atmel_nand_probe(struct platform_device *pdev)
 			goto err_nand_ioremap;
 		}
 	} else {
+		/*
+		 * If the NFC is not initialized (or not probed) and the device
+		 * is asking to be accessed through the NFC then defer the
+		 * probe.
+		 */
+		if (host->wait_for_nfc)
+			return -EPROBE_DEFER;
 		res = atmel_nand_set_enable_ready_pins(mtd);
 		if (res)
 			goto err_nand_ioremap;
@@ -2230,8 +2263,6 @@ static int atmel_nand_remove(struct platform_device *pdev)
 	if (host->dma_chan)
 		dma_release_channel(host->dma_chan);
 
-	platform_driver_unregister(&atmel_nand_nfc_driver);
-
 	return 0;
 }
 
@@ -2303,12 +2334,6 @@ static int atmel_nand_nfc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id atmel_nand_nfc_match[] = {
-	{ .compatible = "atmel,sama5d3-nfc" },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
-
 static struct platform_driver atmel_nand_nfc_driver = {
 	.driver = {
 		.name = "atmel_nand_nfc",
@@ -2329,7 +2354,28 @@ static struct platform_driver atmel_nand_driver = {
 	},
 };
 
-module_platform_driver(atmel_nand_driver);
+static int __init atmel_nand_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&atmel_nand_nfc_driver);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&atmel_nand_driver);
+	if (ret)
+		platform_driver_unregister(&atmel_nand_nfc_driver);
+
+	return ret;
+}
+module_init(atmel_nand_init);
+
+static void __exit atmel_nand_exit(void)
+{
+	platform_driver_unregister(&atmel_nand_driver);
+	platform_driver_unregister(&atmel_nand_nfc_driver);
+}
+module_exit(atmel_nand_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Rick Bronson");
-- 
1.9.1

[PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

[Boris Brezillon]

The NAND and NFC (NAND Flash Controller) were linked together with a
parent <-> child relationship.

This model has several drawbacks:
- it does not allow for multiple NAND chip handling while the controller
  support multi-chip (even though the driver is not ready yet)
- it mixes NAND partitions and NFC nodes at the same level (which is a bit
  disturbing)
- the introduction of the EBI bus implies defining NAND chips under the
  EBI node, and the ranges available under the EBI node should be
  restricted to EBI address space, while the NFC references several
  registers outside of these EBI ranges.

Move the NFC node outside of the NAND node, to get a more future-proof
model.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../devicetree/bindings/mtd/atmel-nand.txt         | 46 ++++++++++++----------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
index 6edc3b6..8896850 100644
--- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
@@ -30,15 +30,19 @@ Optional properties:
   sector size 1024.
 - nand-bus-width : 8 or 16 bus width if not present 8
 - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
-- Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
-  - Required properties:
-    - compatible : "atmel,sama5d3-nfc".
-    - reg : should specify the address and size used for NFC command registers,
-            NFC registers and NFC Sram. NFC Sram address and size can be absent
-            if don't want to use it.
-    - clocks: phandle to the peripheral clock
-  - Optional properties:
-    - atmel,write-by-sram: boolean to enable NFC write by sram.
+- atmel,nfc: phandle referencing the NAND Flash Controller, if available.
+
+The NAND Flash Controller (NFC) is an advanced NAND controller and should be
+used in conjunction with the NAND flash device.
+Required properties:
+ - compatible : "atmel,sama5d3-nfc".
+ - reg : should specify the address and size used for NFC command registers,
+         NFC registers and NFC Sram. NFC Sram address and size can be absent
+         if you don't want to use it.
+ - clocks: phandle to the peripheral clock
+
+Optional properties:
+ - atmel,write-by-sram: boolean to enable NFC write by sram.
 
 Examples:
 nand0: nand@40000000,0 {
@@ -89,21 +93,23 @@ nand0: nand@40000000 {
 };
 
 /* for NFC supported chips */
+nfc: nfc@70000000 {
+	compatible = "atmel,sama5d3-nfc";
+	#address-cells = <1>;
+	#size-cells = <1>;
+	clocks = <&hsmc_clk>
+	reg = <
+		0x70000000 0x10000000	/* NFC Command Registers */
+		0xffffc000 0x00000070	/* NFC HSMC regs */
+		0x00200000 0x00100000	/* NFC SRAM banks */
+	>;
+};
+
 nand0: nand@40000000 {
 	compatible = "atmel,at91rm9200-nand";
 	#address-cells = <1>;
 	#size-cells = <1>;
 	ranges;
+	atmel,nfc = <&nfc>;
         ...
-        nfc@70000000 {
-		compatible = "atmel,sama5d3-nfc";
-		#address-cells = <1>;
-		#size-cells = <1>;
-		clocks = <&hsmc_clk>
-		reg = <
-			0x70000000 0x10000000	/* NFC Command Registers */
-			0xffffc000 0x00000070	/* NFC HSMC regs */
-			0x00200000 0x00100000	/* NFC SRAM banks */
-		>;
-	};
 };
-- 
1.9.1

[PATCH 3/4] ARM: at91/dt: sama5: move NFC nodes outside of NAND nodes

[Boris Brezillon]

We have updated the atmel-nand binding to get a better representation of
the NAND/NFC relationship.
Update the DTs defining those NFC nodes accordingly.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 arch/arm/boot/dts/sama5d3.dtsi | 19 ++++++++-----------
 arch/arm/boot/dts/sama5d4.dtsi | 21 +++++++++------------
 2 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index 5f4144d..1749853 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -1414,19 +1414,16 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&pinctrl_nand0_ale_cle>;
 			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
+			atmel,nfc = <&nfc>;
 			status = "disabled";
+		};
 
-			nfc@70000000 {
-				compatible = "atmel,sama5d3-nfc";
-				#address-cells = <1>;
-				#size-cells = <1>;
-				reg = <
-					0x70000000 0x10000000	/* NFC Command Registers */
-					0xffffc000 0x00000070	/* NFC HSMC regs */
-					0x00200000 0x00100000	/* NFC SRAM banks */
-					>;
-				clocks = <&hsmc_clk>;
-			};
+		nfc: nfc@70000000 {
+			compatible = "atmel,sama5d3-nfc";
+			reg = <0x70000000 0x10000000	/* NFC Command Registers */
+			       0xffffc000 0x00000070	/* NFC HSMC regs */
+			       0x00200000 0x00100000>;	/* NFC SRAM banks */
+			clocks = <&hsmc_clk>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index e0157b0..3b5e9f1 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -280,20 +280,17 @@
 			atmel,nand-has-dma;
 			pinctrl-names = "default";
 			pinctrl-0 = <&pinctrl_nand>;
+			atmel,nfc = <&nfc>;
 			status = "disabled";
+		};
 
-			nfc@90000000 {
-				compatible = "atmel,sama5d3-nfc";
-				#address-cells = <1>;
-				#size-cells = <1>;
-				reg = <
-					0x90000000 0x10000000	/* NFC Command Registers */
-					0xfc05c000 0x00000070	/* NFC HSMC regs */
-					0x00100000 0x00100000	/* NFC SRAM banks */
-                                         >;
-				clocks = <&hsmc_clk>;
-				atmel,write-by-sram;
-			};
+		nfc: nfc@90000000 {
+			compatible = "atmel,sama5d3-nfc";
+			reg = <0x90000000 0x10000000	/* NFC Command Registers */
+			       0xfc05c000 0x00000070	/* NFC HSMC regs */
+			       0x00100000 0x00100000>;	/* NFC SRAM banks */
+			clocks = <&hsmc_clk>;
+			atmel,write-by-sram;
 		};
 
 		apb {
-- 
1.9.1

[PATCH 4/4] ARM: at91/dt: sama5: move NAND nodes into board dts/dtsi

[Boris Brezillon]

sama5d3 and sama5d4 SoCs provides several CS to interface with external
memories, and in particular NAND chips.
The NAND flash controller embedded in the these SoCs can connect to any of
the available CS (each CS is assigned a memory range, hence the nand@xxx
you're seeing in the DT), thus the NAND chip definition should be part of
the board description because we cannot guess at the SoC level which CS
will be chosen by the board designer.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 arch/arm/boot/dts/at91-sama5d3_xplained.dts | 18 +++++++++++++++++-
 arch/arm/boot/dts/at91-sama5d4ek.dts        | 16 +++++++++++++++-
 arch/arm/boot/dts/sama5d3.dtsi              | 21 ---------------------
 arch/arm/boot/dts/sama5d3xcm.dtsi           | 18 +++++++++++++++++-
 arch/arm/boot/dts/sama5d4.dtsi              | 19 -------------------
 5 files changed, 49 insertions(+), 43 deletions(-)

diff --git a/arch/arm/boot/dts/at91-sama5d3_xplained.dts b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
index fec1fca..860258b 100644
--- a/arch/arm/boot/dts/at91-sama5d3_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
@@ -213,13 +213,29 @@
 		};
 
 		nand0: nand@60000000 {
+			compatible = "atmel,at91rm9200-nand";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			reg = <	0x60000000 0x01000000	/* EBI CS3 */
+				0xffffc070 0x00000490	/* SMC PMECC regs */
+				0xffffc500 0x00000100	/* SMC PMECC Error Location regs */
+				0x00110000 0x00018000	/* ROM code */
+				>;
+			interrupts = <5 IRQ_TYPE_LEVEL_HIGH 6>;
+			atmel,nand-addr-offset = <21>;
+			atmel,nand-cmd-offset = <22>;
+			atmel,nand-has-dma;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_nand0_ale_cle>;
+			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
+			atmel,nfc = <&nfc>;
 			nand-bus-width = <8>;
 			nand-ecc-mode = "hw";
 			atmel,has-pmecc;
 			atmel,pmecc-cap = <4>;
 			atmel,pmecc-sector-size = <512>;
 			nand-on-flash-bbt;
-			status = "okay";
 
 			at91bootstrap@0 {
 				label = "at91bootstrap";
diff --git a/arch/arm/boot/dts/at91-sama5d4ek.dts b/arch/arm/boot/dts/at91-sama5d4ek.dts
index b5b8400..5de0d2d 100644
--- a/arch/arm/boot/dts/at91-sama5d4ek.dts
+++ b/arch/arm/boot/dts/at91-sama5d4ek.dts
@@ -204,11 +204,25 @@
 		};
 
 		nand0: nand@80000000 {
+			compatible = "atmel,at91rm9200-nand";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			reg = <	0x80000000 0x08000000	/* EBI CS3 */
+				0xfc05c070 0x00000490	/* SMC PMECC regs */
+				0xfc05c500 0x00000100	/* SMC PMECC Error Location regs */
+				>;
+			interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>;
+			atmel,nand-addr-offset = <21>;
+			atmel,nand-cmd-offset = <22>;
+			atmel,nand-has-dma;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_nand>;
+			atmel,nfc = <&nfc>;
 			nand-bus-width = <8>;
 			nand-ecc-mode = "hw";
 			nand-on-flash-bbt;
 			atmel,has-pmecc;
-			status = "okay";
 
 			at91bootstrap@0 {
 				label = "at91bootstrap";
diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index 1749853..0a944e0 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -1397,27 +1397,6 @@
 			status = "disabled";
 		};
 
-		nand0: nand@60000000 {
-			compatible = "atmel,at91rm9200-nand";
-			#address-cells = <1>;
-			#size-cells = <1>;
-			ranges;
-			reg = <	0x60000000 0x01000000	/* EBI CS3 */
-				0xffffc070 0x00000490	/* SMC PMECC regs */
-				0xffffc500 0x00000100	/* SMC PMECC Error Location regs */
-				0x00110000 0x00018000	/* ROM code */
-				>;
-			interrupts = <5 IRQ_TYPE_LEVEL_HIGH 6>;
-			atmel,nand-addr-offset = <21>;
-			atmel,nand-cmd-offset = <22>;
-			atmel,nand-has-dma;
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_nand0_ale_cle>;
-			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
-			atmel,nfc = <&nfc>;
-			status = "disabled";
-		};
-
 		nfc: nfc@70000000 {
 			compatible = "atmel,sama5d3-nfc";
 			reg = <0x70000000 0x10000000	/* NFC Command Registers */
diff --git a/arch/arm/boot/dts/sama5d3xcm.dtsi b/arch/arm/boot/dts/sama5d3xcm.dtsi
index cfcd200..e6c2aec 100644
--- a/arch/arm/boot/dts/sama5d3xcm.dtsi
+++ b/arch/arm/boot/dts/sama5d3xcm.dtsi
@@ -76,13 +76,29 @@
 		};
 
 		nand0: nand@60000000 {
+			compatible = "atmel,at91rm9200-nand";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			reg = <	0x60000000 0x01000000	/* EBI CS3 */
+				0xffffc070 0x00000490	/* SMC PMECC regs */
+				0xffffc500 0x00000100	/* SMC PMECC Error Location regs */
+				0x00110000 0x00018000	/* ROM code */
+				>;
+			interrupts = <5 IRQ_TYPE_LEVEL_HIGH 6>;
+			atmel,nand-addr-offset = <21>;
+			atmel,nand-cmd-offset = <22>;
+			atmel,nand-has-dma;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_nand0_ale_cle>;
+			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
+			atmel,nfc = <&nfc>;
 			nand-bus-width = <8>;
 			nand-ecc-mode = "hw";
 			atmel,has-pmecc;
 			atmel,pmecc-cap = <4>;
 			atmel,pmecc-sector-size = <512>;
 			nand-on-flash-bbt;
-			status = "okay";
 
 			at91bootstrap@0 {
 				label = "at91bootstrap";
diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 3b5e9f1..8647eb3 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -265,25 +265,6 @@
 			cache-level = <2>;
 		};
 
-		nand0: nand@80000000 {
-			compatible = "atmel,at91rm9200-nand";
-			#address-cells = <1>;
-			#size-cells = <1>;
-			ranges;
-			reg = <	0x80000000 0x08000000	/* EBI CS3 */
-				0xfc05c070 0x00000490	/* SMC PMECC regs */
-				0xfc05c500 0x00000100	/* SMC PMECC Error Location regs */
-				>;
-			interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>;
-			atmel,nand-addr-offset = <21>;
-			atmel,nand-cmd-offset = <22>;
-			atmel,nand-has-dma;
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_nand>;
-			atmel,nfc = <&nfc>;
-			status = "disabled";
-		};
-
 		nfc: nfc@90000000 {
 			compatible = "atmel,sama5d3-nfc";
 			reg = <0x90000000 0x10000000	/* NFC Command Registers */
-- 
1.9.1

Re: [PATCH 0/4] mtd: nand: atmel: Rework DT representation of NFC/NAND

[Nicolas Ferre]

Le 04/12/2014 23:30, Boris Brezillon a écrit :
> Hello,
> 
> While working on the EBI driver [1] I noticed that the relationship between
> the NFC (NAND Flash Controller) and the NAND chip it is attached to would
> make things harder to represent when moving the NAND node under the EBI bus
> (another useless 'ranges' definition).
> 
> Actually this representation might be even more problematic if one wants
> decide to connect two NAND chips on his sama5 based board, because the NFC
> node is a child of the NAND chip node, and thus can only be attached to a
> single NAND chip.
> 
> To address this problem the current series moves the NFC node outside of
> the NAND chip device which then reference the NFC using the "atmel,nfc"
> property.
> 
> The series does not implement multi-chip support, but at least the new
> representation should make it possible.
> 
> Best Regards,
> 
> Boris
> 
> [1]https://lkml.org/lkml/2014/12/3/806

Boris,

It sounds promising and for sure we have to think about a way to deal
with SMC (EBI interface) and the NAND controller with its attributes
(PMECC) or flavors (plain SMC or NFC).

Let's consider this as a RFC and discuss how to deal with the DT
modifications and the representation hierarchy all together.

Thanks, bye.

> Boris Brezillon (4):
>   mtd: nand: atmel: Rework driver to separate nfc and nand nodes
>   mtd: nand: atmel: Update DT documentation after splitting NFC and NAND
>   ARM: at91/dt: sama5: move NFC nodes outside of NAND nodes
>   ARM: at91/dt: sama5: move NAND nodes into board dts/dtsi
> 
>  .../devicetree/bindings/mtd/atmel-nand.txt         | 46 +++++++------
>  arch/arm/boot/dts/at91-sama5d3_xplained.dts        | 18 ++++-
>  arch/arm/boot/dts/at91-sama5d4ek.dts               | 16 ++++-
>  arch/arm/boot/dts/sama5d3.dtsi                     | 36 ++--------
>  arch/arm/boot/dts/sama5d3xcm.dtsi                  | 18 ++++-
>  arch/arm/boot/dts/sama5d4.dtsi                     | 36 ++--------
>  drivers/mtd/nand/atmel_nand.c                      | 76 +++++++++++++++++-----
>  7 files changed, 149 insertions(+), 97 deletions(-)
> 


-- 
Nicolas Ferre

Re: [PATCH 1/4] mtd: nand: atmel: Rework driver to separate nfc and nand nodes

[Josh Wu]

Hi, Boris

On 12/5/2014 6:30 AM, Boris Brezillon wrote:
> mtd: nand: atmel: Update DT documentation after splitting NFC and NAND
>
> The NAND and NFC (NAND Flash Controller) were linked together with a
> parent <-> child relationship.
>
> This model has several drawbacks:
> - it does not allow for multiple NAND chip handling while the controller
>    support multi-chip (even though the driver is not ready yet)
> - it mixes NAND partitions and NFC nodes at the same level (which is a bit
>    disturbing)
> - the introduction of the EBI bus implies defining NAND chips under the
>    EBI node, and the ranges available under the EBI node should be
>    restricted to EBI address space, while the NFC references several
>    registers outside of these EBI ranges.
>
> Move the NFC node outside of the NAND node, to get a more future-proof
> DT representation.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
I'm fine with this patch.
Acked-by: Josh Wu <josh.wu@atmel.com>

Best Regards,
Josh Wu

> ---
>   drivers/mtd/nand/atmel_nand.c | 76 ++++++++++++++++++++++++++++++++++---------
>   1 file changed, 61 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 19d1e9d..0239fe6 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -123,6 +123,7 @@ struct atmel_nand_host {
>   	struct dma_chan		*dma_chan;
>   
>   	struct atmel_nfc	*nfc;
> +	bool			wait_for_nfc;
>   
>   	bool			has_pmecc;
>   	u8			pmecc_corr_cap;
> @@ -1423,6 +1424,12 @@ static void atmel_nand_hwctl(struct mtd_info *mtd, int mode)
>   		ecc_writel(host->ecc, CR, ATMEL_ECC_RST);
>   }
>   
> +static const struct of_device_id atmel_nand_nfc_match[] = {
> +	{ .compatible = "atmel,sama5d3-nfc" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
> +
>   static int atmel_of_init_port(struct atmel_nand_host *host,
>   			      struct device_node *np)
>   {
> @@ -1431,6 +1438,7 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
>   	int ecc_mode;
>   	struct atmel_nand_data *board = &host->board;
>   	enum of_gpio_flags flags = 0;
> +	struct device_node *child;
>   
>   	if (of_property_read_u32(np, "atmel,nand-addr-offset", &val) == 0) {
>   		if (val >= 32) {
> @@ -1467,8 +1475,30 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
>   
>   	host->has_pmecc = of_property_read_bool(np, "atmel,has-pmecc");
>   
> -	/* load the nfc driver if there is */
> -	of_platform_populate(np, NULL, NULL, host->dev);
> +	/*
> +	 * Backward compatibility with DTs defining the NFC node as their
> +	 * child.
> +	 * The new model reference the NFC so that we can define several
> +	 * chip controlled by the same controller.
> +	 */
> +	for_each_available_child_of_node(np, child) {
> +		/*
> +		 * If we find an NFC node under the NAND node, then populate
> +		 * the device so that it can be probed, and wait for it.
> +		 */
> +		if (of_match_node(atmel_nand_nfc_match, child)) {
> +			of_platform_populate(np, NULL, NULL, host->dev);
> +			host->wait_for_nfc = true;
> +			break;
> +		}
> +	}
> +
> +	/*
> +	 * If there's an atmel,nfc property we should access the NAND
> +	 * through the NFC.
> +	 */
> +	if (of_property_read_bool(np, "atmel,nfc"))
> +		host->wait_for_nfc = true;
>   
>   	if (!(board->ecc_mode == NAND_ECC_HW) || !host->has_pmecc)
>   		return 0;	/* Not using PMECC */
> @@ -2032,10 +2062,6 @@ static int atmel_nand_probe(struct platform_device *pdev)
>   	if (!host)
>   		return -ENOMEM;
>   
> -	res = platform_driver_register(&atmel_nand_nfc_driver);
> -	if (res)
> -		dev_err(&pdev->dev, "atmel_nand: can't register NFC driver\n");
> -
>   	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	host->io_base = devm_ioremap_resource(&pdev->dev, mem);
>   	if (IS_ERR(host->io_base)) {
> @@ -2089,6 +2115,13 @@ static int atmel_nand_probe(struct platform_device *pdev)
>   			goto err_nand_ioremap;
>   		}
>   	} else {
> +		/*
> +		 * If the NFC is not initialized (or not probed) and the device
> +		 * is asking to be accessed through the NFC then defer the
> +		 * probe.
> +		 */
> +		if (host->wait_for_nfc)
> +			return -EPROBE_DEFER;
>   		res = atmel_nand_set_enable_ready_pins(mtd);
>   		if (res)
>   			goto err_nand_ioremap;
> @@ -2230,8 +2263,6 @@ static int atmel_nand_remove(struct platform_device *pdev)
>   	if (host->dma_chan)
>   		dma_release_channel(host->dma_chan);
>   
> -	platform_driver_unregister(&atmel_nand_nfc_driver);
> -
>   	return 0;
>   }
>   
> @@ -2303,12 +2334,6 @@ static int atmel_nand_nfc_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> -static const struct of_device_id atmel_nand_nfc_match[] = {
> -	{ .compatible = "atmel,sama5d3-nfc" },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, atmel_nand_nfc_match);
> -
>   static struct platform_driver atmel_nand_nfc_driver = {
>   	.driver = {
>   		.name = "atmel_nand_nfc",
> @@ -2329,7 +2354,28 @@ static struct platform_driver atmel_nand_driver = {
>   	},
>   };
>   
> -module_platform_driver(atmel_nand_driver);
> +static int __init atmel_nand_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&atmel_nand_nfc_driver);
> +	if (ret)
> +		return ret;
> +
> +	ret = platform_driver_register(&atmel_nand_driver);
> +	if (ret)
> +		platform_driver_unregister(&atmel_nand_nfc_driver);
> +
> +	return ret;
> +}
> +module_init(atmel_nand_init);
> +
> +static void __exit atmel_nand_exit(void)
> +{
> +	platform_driver_unregister(&atmel_nand_driver);
> +	platform_driver_unregister(&atmel_nand_nfc_driver);
> +}
> +module_exit(atmel_nand_exit);
>   
>   MODULE_LICENSE("GPL");
>   MODULE_AUTHOR("Rick Bronson");

Re: [PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

[Josh Wu]

Hi, Boris

Thanks for the patch.

You need to rebase on the top of current mtd-l2 git tree. As I had some 
change on the binding document.

Best Regards,
Josh Wu

On 12/5/2014 6:30 AM, Boris Brezillon wrote:
> The NAND and NFC (NAND Flash Controller) were linked together with a
> parent <-> child relationship.
>
> This model has several drawbacks:
> - it does not allow for multiple NAND chip handling while the controller
>    support multi-chip (even though the driver is not ready yet)
> - it mixes NAND partitions and NFC nodes at the same level (which is a bit
>    disturbing)
> - the introduction of the EBI bus implies defining NAND chips under the
>    EBI node, and the ranges available under the EBI node should be
>    restricted to EBI address space, while the NFC references several
>    registers outside of these EBI ranges.
>
> Move the NFC node outside of the NAND node, to get a more future-proof
> model.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>   .../devicetree/bindings/mtd/atmel-nand.txt         | 46 ++++++++++++----------
>   1 file changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 6edc3b6..8896850 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -30,15 +30,19 @@ Optional properties:
>     sector size 1024.
>   - nand-bus-width : 8 or 16 bus width if not present 8
>   - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
> -- Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
> -  - Required properties:
> -    - compatible : "atmel,sama5d3-nfc".
> -    - reg : should specify the address and size used for NFC command registers,
> -            NFC registers and NFC Sram. NFC Sram address and size can be absent
> -            if don't want to use it.
> -    - clocks: phandle to the peripheral clock
> -  - Optional properties:
> -    - atmel,write-by-sram: boolean to enable NFC write by sram.
> +- atmel,nfc: phandle referencing the NAND Flash Controller, if available.
> +
> +The NAND Flash Controller (NFC) is an advanced NAND controller and should be
> +used in conjunction with the NAND flash device.
> +Required properties:
> + - compatible : "atmel,sama5d3-nfc".
> + - reg : should specify the address and size used for NFC command registers,
> +         NFC registers and NFC Sram. NFC Sram address and size can be absent
> +         if you don't want to use it.
> + - clocks: phandle to the peripheral clock
> +
> +Optional properties:
> + - atmel,write-by-sram: boolean to enable NFC write by sram.
>   
>   Examples:
>   nand0: nand@40000000,0 {
> @@ -89,21 +93,23 @@ nand0: nand@40000000 {
>   };
>   
>   /* for NFC supported chips */
> +nfc: nfc@70000000 {
> +	compatible = "atmel,sama5d3-nfc";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	clocks = <&hsmc_clk>
> +	reg = <
> +		0x70000000 0x10000000	/* NFC Command Registers */
> +		0xffffc000 0x00000070	/* NFC HSMC regs */
> +		0x00200000 0x00100000	/* NFC SRAM banks */
> +	>;
> +};
> +
>   nand0: nand@40000000 {
>   	compatible = "atmel,at91rm9200-nand";
>   	#address-cells = <1>;
>   	#size-cells = <1>;
>   	ranges;
> +	atmel,nfc = <&nfc>;
>           ...
> -        nfc@70000000 {
> -		compatible = "atmel,sama5d3-nfc";
> -		#address-cells = <1>;
> -		#size-cells = <1>;
> -		clocks = <&hsmc_clk>
> -		reg = <
> -			0x70000000 0x10000000	/* NFC Command Registers */
> -			0xffffc000 0x00000070	/* NFC HSMC regs */
> -			0x00200000 0x00100000	/* NFC SRAM banks */
> -		>;
> -	};
>   };

Re: [PATCH 4/4] ARM: at91/dt: sama5: move NAND nodes into board dts/dtsi

[Josh Wu]

Hi, Boris

On 12/5/2014 6:30 AM, Boris Brezillon wrote:
> sama5d3 and sama5d4 SoCs provides several CS to interface with external
> memories, and in particular NAND chips.
> The NAND flash controller embedded in the these SoCs can connect to any of
> the available CS (each CS is assigned a memory range, hence the nand@xxx
> you're seeing in the DT), thus the NAND chip definition should be part of
> the board description because we cannot guess at the SoC level which CS
> will be chosen by the board designer.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>   arch/arm/boot/dts/at91-sama5d3_xplained.dts | 18 +++++++++++++++++-
>   arch/arm/boot/dts/at91-sama5d4ek.dts        | 16 +++++++++++++++-
>   arch/arm/boot/dts/sama5d3.dtsi              | 21 ---------------------
>   arch/arm/boot/dts/sama5d3xcm.dtsi           | 18 +++++++++++++++++-
>   arch/arm/boot/dts/sama5d4.dtsi              | 19 -------------------
>   5 files changed, 49 insertions(+), 43 deletions(-)
>
> diff --git a/arch/arm/boot/dts/at91-sama5d3_xplained.dts b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
> index fec1fca..860258b 100644
> --- a/arch/arm/boot/dts/at91-sama5d3_xplained.dts
> +++ b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
> @@ -213,13 +213,29 @@
>   		};
>   
>   		nand0: nand@60000000 {
> +			compatible = "atmel,at91rm9200-nand";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
it would be better to leave this part to the sama5d3.dtsi.

> +			reg = <	0x60000000 0x01000000	/* EBI CS3 */
> +				0xffffc070 0x00000490	/* SMC PMECC regs */
> +				0xffffc500 0x00000100	/* SMC PMECC Error Location regs */
> +				0x00110000 0x00018000	/* ROM code */
> +				>;
> +			interrupts = <5 IRQ_TYPE_LEVEL_HIGH 6>;
ditto.
> +			atmel,nand-addr-offset = <21>;
> +			atmel,nand-cmd-offset = <22>;
> +			atmel,nand-has-dma;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_nand0_ale_cle>;
> +			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
ditto.

> +			atmel,nfc = <&nfc>;
>   			nand-bus-width = <8>;
>   			nand-ecc-mode = "hw";
>   			atmel,has-pmecc;
>   			atmel,pmecc-cap = <4>;
>   			atmel,pmecc-sector-size = <512>;
>   			nand-on-flash-bbt;
> -			status = "okay";
>   
>   			at91bootstrap@0 {
>   				label = "at91bootstrap";
> diff --git a/arch/arm/boot/dts/at91-sama5d4ek.dts b/arch/arm/boot/dts/at91-sama5d4ek.dts
> index b5b8400..5de0d2d 100644
> --- a/arch/arm/boot/dts/at91-sama5d4ek.dts
> +++ b/arch/arm/boot/dts/at91-sama5d4ek.dts
> @@ -204,11 +204,25 @@
>   		};
>   
>   		nand0: nand@80000000 {
> +			compatible = "atmel,at91rm9200-nand";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
it would be better to leave this part to the sama5d4.dtsi.

> +			reg = <	0x80000000 0x08000000	/* EBI CS3 */
> +				0xfc05c070 0x00000490	/* SMC PMECC regs */
> +				0xfc05c500 0x00000100	/* SMC PMECC Error Location regs */
> +				>;
> +			interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>;
ditto.

> +			atmel,nand-addr-offset = <21>;
> +			atmel,nand-cmd-offset = <22>;
> +			atmel,nand-has-dma;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_nand>;
> +			atmel,nfc = <&nfc>;
>   			nand-bus-width = <8>;
>   			nand-ecc-mode = "hw";
>   			nand-on-flash-bbt;
>   			atmel,has-pmecc;
> -			status = "okay";
>   
>   			at91bootstrap@0 {
>   				label = "at91bootstrap";
> diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
> index 1749853..0a944e0 100644
> --- a/arch/arm/boot/dts/sama5d3.dtsi
> +++ b/arch/arm/boot/dts/sama5d3.dtsi
> @@ -1397,27 +1397,6 @@
>   			status = "disabled";
>   		};
>   
> -		nand0: nand@60000000 {
> -			compatible = "atmel,at91rm9200-nand";
> -			#address-cells = <1>;
> -			#size-cells = <1>;
> -			ranges;
> -			reg = <	0x60000000 0x01000000	/* EBI CS3 */
> -				0xffffc070 0x00000490	/* SMC PMECC regs */
> -				0xffffc500 0x00000100	/* SMC PMECC Error Location regs */
> -				0x00110000 0x00018000	/* ROM code */
> -				>;
> -			interrupts = <5 IRQ_TYPE_LEVEL_HIGH 6>;
> -			atmel,nand-addr-offset = <21>;
> -			atmel,nand-cmd-offset = <22>;
> -			atmel,nand-has-dma;
> -			pinctrl-names = "default";
> -			pinctrl-0 = <&pinctrl_nand0_ale_cle>;
> -			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
> -			atmel,nfc = <&nfc>;
> -			status = "disabled";
> -		};
> -
>   		nfc: nfc@70000000 {
>   			compatible = "atmel,sama5d3-nfc";
>   			reg = <0x70000000 0x10000000	/* NFC Command Registers */
> diff --git a/arch/arm/boot/dts/sama5d3xcm.dtsi b/arch/arm/boot/dts/sama5d3xcm.dtsi
> index cfcd200..e6c2aec 100644
> --- a/arch/arm/boot/dts/sama5d3xcm.dtsi
> +++ b/arch/arm/boot/dts/sama5d3xcm.dtsi
> @@ -76,13 +76,29 @@
>   		};
>   
>   		nand0: nand@60000000 {
> +			compatible = "atmel,at91rm9200-nand";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
it would be better to leave this part to the sama5d3.dtsi.

> +			reg = <	0x60000000 0x01000000	/* EBI CS3 */
> +				0xffffc070 0x00000490	/* SMC PMECC regs */
> +				0xffffc500 0x00000100	/* SMC PMECC Error Location regs */
> +				0x00110000 0x00018000	/* ROM code */
> +				>;
> +			interrupts = <5 IRQ_TYPE_LEVEL_HIGH 6>;
ditto.
> +			atmel,nand-addr-offset = <21>;
> +			atmel,nand-cmd-offset = <22>;
> +			atmel,nand-has-dma;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_nand0_ale_cle>;
> +			atmel,pmecc-lookup-table-offset = <0x0 0x8000>;
ditto.

> +			atmel,nfc = <&nfc>;
>   			nand-bus-width = <8>;
>   			nand-ecc-mode = "hw";
>   			atmel,has-pmecc;
>   			atmel,pmecc-cap = <4>;
>   			atmel,pmecc-sector-size = <512>;
>   			nand-on-flash-bbt;
> -			status = "okay";
>   
>   			at91bootstrap@0 {
>   				label = "at91bootstrap";
> diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
> index 3b5e9f1..8647eb3 100644
> --- a/arch/arm/boot/dts/sama5d4.dtsi
> +++ b/arch/arm/boot/dts/sama5d4.dtsi
> @@ -265,25 +265,6 @@
>   			cache-level = <2>;
>   		};
>   
> -		nand0: nand@80000000 {
> -			compatible = "atmel,at91rm9200-nand";
> -			#address-cells = <1>;
> -			#size-cells = <1>;
> -			ranges;
> -			reg = <	0x80000000 0x08000000	/* EBI CS3 */
> -				0xfc05c070 0x00000490	/* SMC PMECC regs */
> -				0xfc05c500 0x00000100	/* SMC PMECC Error Location regs */
> -				>;
> -			interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>;
> -			atmel,nand-addr-offset = <21>;
> -			atmel,nand-cmd-offset = <22>;
> -			atmel,nand-has-dma;
> -			pinctrl-names = "default";
> -			pinctrl-0 = <&pinctrl_nand>;
> -			atmel,nfc = <&nfc>;
> -			status = "disabled";
> -		};
> -
>   		nfc: nfc@90000000 {
>   			compatible = "atmel,sama5d3-nfc";
>   			reg = <0x90000000 0x10000000	/* NFC Command Registers */
Best Regards,
Josh Wu

Re: [PATCH 4/4] ARM: at91/dt: sama5: move NAND nodes into board dts/dtsi

[Boris Brezillon]

Hi Josh,

On Fri, 26 Dec 2014 17:45:51 +0800
Josh Wu <josh.wu@atmel.com> wrote:

> Hi, Boris
> 
> On 12/5/2014 6:30 AM, Boris Brezillon wrote:
> > sama5d3 and sama5d4 SoCs provides several CS to interface with external
> > memories, and in particular NAND chips.
> > The NAND flash controller embedded in the these SoCs can connect to any of
> > the available CS (each CS is assigned a memory range, hence the nand@xxx
> > you're seeing in the DT), thus the NAND chip definition should be part of
> > the board description because we cannot guess at the SoC level which CS
> > will be chosen by the board designer.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >   arch/arm/boot/dts/at91-sama5d3_xplained.dts | 18 +++++++++++++++++-
> >   arch/arm/boot/dts/at91-sama5d4ek.dts        | 16 +++++++++++++++-
> >   arch/arm/boot/dts/sama5d3.dtsi              | 21 ---------------------
> >   arch/arm/boot/dts/sama5d3xcm.dtsi           | 18 +++++++++++++++++-
> >   arch/arm/boot/dts/sama5d4.dtsi              | 19 -------------------
> >   5 files changed, 49 insertions(+), 43 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/at91-sama5d3_xplained.dts b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
> > index fec1fca..860258b 100644
> > --- a/arch/arm/boot/dts/at91-sama5d3_xplained.dts
> > +++ b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
> > @@ -213,13 +213,29 @@
> >   		};
> >   
> >   		nand0: nand@60000000 {
> > +			compatible = "atmel,at91rm9200-nand";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			ranges;
> it would be better to leave this part to the sama5d3.dtsi.

Actually I did it on purpose, because nothing prevents anyone from
connecting its NAND chip on a different CS and connect something else
on CS3, hence this NAND node should not be defined at SoC level but in
upper layers.
I know this is currently hardcoded in the NAND driver, but I'd really
like to have the DT part corrected, and defining the NAND node in the
proper dts(i) file is part of the correction.

If you really want to make this node common to all atmel boards
embedding a sama5d3 SoC, then we could create another dtsi (but I
remember that Nicolas was trying to limit the number of dtsi files).

The same goes for the other parts you pointed out.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

[Boris Brezillon]

On Fri, 26 Dec 2014 17:30:05 +0800
Josh Wu <josh.wu@atmel.com> wrote:

> Hi, Boris
> 
> Thanks for the patch.
> 
> You need to rebase on the top of current mtd-l2 git tree. As I had some 
> change on the binding document.

Sure, I'll do it, but I'd like to have feedback from Nicolas, Brian and
at least one DT maintainer first.

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

[Brian Norris]

Hi Boris,

BTW, this series has a few conflicts with other things I have queued, so
you'll need to refresh.

On Thu, Dec 04, 2014 at 11:30:12PM +0100, Boris Brezillon wrote:
> The NAND and NFC (NAND Flash Controller) were linked together with a
> parent <-> child relationship.
> 
> This model has several drawbacks:
> - it does not allow for multiple NAND chip handling while the controller
>   support multi-chip (even though the driver is not ready yet)
> - it mixes NAND partitions and NFC nodes at the same level (which is a bit
>   disturbing)

I agree that this is disturbing. (FWIW, it also seems a bit disturbing
that atmel_nand.c actually registers two different drivers and the tries
to synchronize them; this seems like it could be handled better, but I'm
not sure how at the moment.)

> - the introduction of the EBI bus implies defining NAND chips under the
>   EBI node, and the ranges available under the EBI node should be
>   restricted to EBI address space, while the NFC references several
>   registers outside of these EBI ranges.

That's an interesting bit. I've actually run across this sort of problem
on other SoCs, where we have a relationship between two pieces of
hardware--the NAND chip and the NAND controller--where the former might
be on one bus (like your EBI bus, with chip selects), and the latter is
part of the top-level MMIO register space.

But can you elaborate here a bit more? Does the NAND chip actually need
to be represented under your EBI bus?

> Move the NFC node outside of the NAND node, to get a more future-proof
> model.

I'm curious if an alternative solution might work, maybe one like the
Allwiner NAND (sunxi-nand) DT, which just reverses the roles; the 'NFC'
is the parent of the NAND chip(s). We've seen this pattern in other
contexts too.

> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  .../devicetree/bindings/mtd/atmel-nand.txt         | 46 ++++++++++++----------
>  1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index 6edc3b6..8896850 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -30,15 +30,19 @@ Optional properties:
>    sector size 1024.
>  - nand-bus-width : 8 or 16 bus width if not present 8
>  - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
> -- Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
> -  - Required properties:
> -    - compatible : "atmel,sama5d3-nfc".
> -    - reg : should specify the address and size used for NFC command registers,
> -            NFC registers and NFC Sram. NFC Sram address and size can be absent
> -            if don't want to use it.
> -    - clocks: phandle to the peripheral clock
> -  - Optional properties:
> -    - atmel,write-by-sram: boolean to enable NFC write by sram.
> +- atmel,nfc: phandle referencing the NAND Flash Controller, if available.

So this seems to clarify one question I had: is the NAND flash
controller required? The previous language didn't make it extremely
clear that the NFC sub-node was optional. But it does appear your code
makes it optional.

> +
> +The NAND Flash Controller (NFC) is an advanced NAND controller and should be
> +used in conjunction with the NAND flash device.
> +Required properties:
> + - compatible : "atmel,sama5d3-nfc".
> + - reg : should specify the address and size used for NFC command registers,
> +         NFC registers and NFC Sram. NFC Sram address and size can be absent
> +         if you don't want to use it.
> + - clocks: phandle to the peripheral clock
> +
> +Optional properties:
> + - atmel,write-by-sram: boolean to enable NFC write by sram.
>  
>  Examples:
>  nand0: nand@40000000,0 {
> @@ -89,21 +93,23 @@ nand0: nand@40000000 {
>  };
>  
>  /* for NFC supported chips */
> +nfc: nfc@70000000 {
> +	compatible = "atmel,sama5d3-nfc";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	clocks = <&hsmc_clk>
> +	reg = <
> +		0x70000000 0x10000000	/* NFC Command Registers */
> +		0xffffc000 0x00000070	/* NFC HSMC regs */
> +		0x00200000 0x00100000	/* NFC SRAM banks */
> +	>;
> +};
> +
>  nand0: nand@40000000 {
>  	compatible = "atmel,at91rm9200-nand";
>  	#address-cells = <1>;
>  	#size-cells = <1>;
>  	ranges;
> +	atmel,nfc = <&nfc>;
>          ...
> -        nfc@70000000 {
> -		compatible = "atmel,sama5d3-nfc";
> -		#address-cells = <1>;
> -		#size-cells = <1>;
> -		clocks = <&hsmc_clk>
> -		reg = <
> -			0x70000000 0x10000000	/* NFC Command Registers */
> -			0xffffc000 0x00000070	/* NFC HSMC regs */
> -			0x00200000 0x00100000	/* NFC SRAM banks */
> -		>;
> -	};
>  };


Brian

Re: [PATCH 0/4] mtd: nand: atmel: Rework DT representation of NFC/NAND

[Brian Norris]

On Thu, Dec 04, 2014 at 11:30:10PM +0100, Boris Brezillon wrote:
> While working on the EBI driver [1] I noticed that the relationship between
> the NFC (NAND Flash Controller) and the NAND chip it is attached to would
> make things harder to represent when moving the NAND node under the EBI bus
> (another useless 'ranges' definition).
> 
> Actually this representation might be even more problematic if one wants
> decide to connect two NAND chips on his sama5 based board, because the NFC
> node is a child of the NAND chip node, and thus can only be attached to a
> single NAND chip.
> 
> To address this problem the current series moves the NFC node outside of
> the NAND chip device which then reference the NFC using the "atmel,nfc"
> property.
> 
> The series does not implement multi-chip support, but at least the new
> representation should make it possible.

I made some comments on the DT binding. I'm not as familiar withi the
hardware details here, so I may be off base. At any rate, the code looks
OK, so if we settle the binding issue OK, then I'd be happy with a
rebased version of this patch set.

Regards,
Brian

Re: [PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

[Boris Brezillon]

Hi Brian,

On Sun, 1 Feb 2015 23:57:37 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi Boris,
> 
> BTW, this series has a few conflicts with other things I have queued, so
> you'll need to refresh.

Yes, that's not a problem, but I'd like to be sure this is the way we
want to go before rebasing this series.

> 
> On Thu, Dec 04, 2014 at 11:30:12PM +0100, Boris Brezillon wrote:
> > The NAND and NFC (NAND Flash Controller) were linked together with a
> > parent <-> child relationship.
> > 
> > This model has several drawbacks:
> > - it does not allow for multiple NAND chip handling while the controller
> >   support multi-chip (even though the driver is not ready yet)
> > - it mixes NAND partitions and NFC nodes at the same level (which is a bit
> >   disturbing)
> 
> I agree that this is disturbing. (FWIW, it also seems a bit disturbing
> that atmel_nand.c actually registers two different drivers and the tries
> to synchronize them; this seems like it could be handled better, but I'm
> not sure how at the moment.)

Yep, that's my feeling too, but I'm not sure how this could/should be
done.
My problem here is that the pinmux should be requested by the EBI
device because the EBI manages several type of devices and the data and
address signals are shared by all the devices, hence the idea of
defining the nand chip node under the EBI node.
In the other hand, the NFC is not part of the EBI bus, and thus should
not be defined under the EBI node.

This might lead to the NFC device being probed before the NAND chip,
hence the need for this synchronization.

> 
> > - the introduction of the EBI bus implies defining NAND chips under the
> >   EBI node, and the ranges available under the EBI node should be
> >   restricted to EBI address space, while the NFC references several
> >   registers outside of these EBI ranges.
> 
> That's an interesting bit. I've actually run across this sort of problem
> on other SoCs, where we have a relationship between two pieces of
> hardware--the NAND chip and the NAND controller--where the former might
> be on one bus (like your EBI bus, with chip selects), and the latter is
> part of the top-level MMIO register space.
> 
> But can you elaborate here a bit more? Does the NAND chip actually need
> to be represented under your EBI bus?

Yes, as said above this is all about pinmux conflicts, the NAND
controller has to request the appropriate pinmux for its NAND chips but
it will conflict with the pinmux requested by the EBI bus (data and
address signals are shared by all the devices connected on the EBI).

> 
> > Move the NFC node outside of the NAND node, to get a more future-proof
> > model.
> 
> I'm curious if an alternative solution might work, maybe one like the
> Allwiner NAND (sunxi-nand) DT, which just reverses the roles; the 'NFC'
> is the parent of the NAND chip(s). We've seen this pattern in other
> contexts too.

I would have preferred this solution too, but the EBI/pinmux constraint
explained above prevents this approach.
What I can do though, is reverse the referencing: reference nand chips
from the nand controller node.

Josh, Brian, any idea to solve this EBI/nand-chip/nand-controller
dependency problem is welcome.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

[Josh Wu]

Hi, Boris, Brian

On 2/2/2015 5:42 PM, Boris Brezillon wrote:
> Hi Brian,
>
> On Sun, 1 Feb 2015 23:57:37 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
>
>> Hi Boris,
>>
>> BTW, this series has a few conflicts with other things I have queued, so
>> you'll need to refresh.
> Yes, that's not a problem, but I'd like to be sure this is the way we
> want to go before rebasing this series.
>
>> On Thu, Dec 04, 2014 at 11:30:12PM +0100, Boris Brezillon wrote:
>>> The NAND and NFC (NAND Flash Controller) were linked together with a
>>> parent <-> child relationship.
>>>
>>> This model has several drawbacks:
>>> - it does not allow for multiple NAND chip handling while the controller
>>>    support multi-chip (even though the driver is not ready yet)
>>> - it mixes NAND partitions and NFC nodes at the same level (which is a bit
>>>    disturbing)
>> I agree that this is disturbing. (FWIW, it also seems a bit disturbing
>> that atmel_nand.c actually registers two different drivers and the tries
>> to synchronize them; this seems like it could be handled better, but I'm
>> not sure how at the moment.)
> Yep, that's my feeling too, but I'm not sure how this could/should be
> done.
> My problem here is that the pinmux should be requested by the EBI
> device because the EBI manages several type of devices and the data and
> address signals are shared by all the devices, hence the idea of
> defining the nand chip node under the EBI node.
> In the other hand, the NFC is not part of the EBI bus, and thus should
> not be defined under the EBI node.
>
> This might lead to the NFC device being probed before the NAND chip,
> hence the need for this synchronization.

OMHO, there is another way, which is change the NFC node to many NFC 
properties, just like PMECC.
As NFC, PMECC or hamming ecc HW could be part of current NAND node (in 
sama5, HSMC maybe a better name for this node. )

And this change can avoid the sync problem and avoid two drivers in 
atmel_nand.c.

>
>>> - the introduction of the EBI bus implies defining NAND chips under the
>>>    EBI node, and the ranges available under the EBI node should be
>>>    restricted to EBI address space, while the NFC references several
>>>    registers outside of these EBI ranges.
>> That's an interesting bit. I've actually run across this sort of problem
>> on other SoCs, where we have a relationship between two pieces of
>> hardware--the NAND chip and the NAND controller--where the former might
>> be on one bus (like your EBI bus, with chip selects), and the latter is
>> part of the top-level MMIO register space.
>>
>> But can you elaborate here a bit more? Does the NAND chip actually need
>> to be represented under your EBI bus?
> Yes, as said above this is all about pinmux conflicts, the NAND
> controller has to request the appropriate pinmux for its NAND chips but
> it will conflict with the pinmux requested by the EBI bus (data and
> address signals are shared by all the devices connected on the EBI).
>
>>> Move the NFC node outside of the NAND node, to get a more future-proof
>>> model.
>> I'm curious if an alternative solution might work, maybe one like the
>> Allwiner NAND (sunxi-nand) DT, which just reverses the roles; the 'NFC'
>> is the parent of the NAND chip(s). We've seen this pattern in other
>> contexts too.

I also prefer this. Then the dt node should looks like finally:

nand (SMC may be more correct) node {
     PMECC properties
     NFC properties --> we can make the NFC not a node, just many NFC 
properties.

     pinctrl-nand0
     nand chip 0: {
         partitions...
     }

     pinctrl-nand1
     nand chip 1: {
         partitions...
     }
}

> I would have preferred this solution too, but the EBI/pinmux constraint
> explained above prevents this approach.
I am not very clear about the pinmux constraint.
Maybe we just leave one DT node (either EBI or current nand node) to 
take care the pins.

> What I can do though, is reverse the referencing: reference nand chips
> from the nand controller node.

I guess the dt looks like: (correct me if I am wrong)

EBI node {
     pinctrl-nand0
     nand chip 0: {
         partitions...
     }

     pinctrl-nand1
     nand chip 1: {
         partitions...
     }
}

nand (SMC/HSMC may be more correct) node {
     PMECC properties
     NFC properties --> we can make the NFC not a node, just many NFC 
properties.

     &nand chip0
     &nand chip1
}

This is still looks nice to me.

>
> Josh, Brian, any idea to solve this EBI/nand-chip/nand-controller
> dependency problem is welcome.
>
> Best Regards,
>
> Boris
>
Best Regards,
Josh Wu

Re: [PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

[Boris Brezillon]

On Tue, 3 Feb 2015 16:46:15 +0800
Josh Wu <josh.wu@atmel.com> wrote:

> Hi, Boris, Brian
> 
> On 2/2/2015 5:42 PM, Boris Brezillon wrote:
> > Hi Brian,
> >
> > On Sun, 1 Feb 2015 23:57:37 -0800
> > Brian Norris <computersforpeace@gmail.com> wrote:
> >
> >> Hi Boris,
> >>
> >> BTW, this series has a few conflicts with other things I have queued, so
> >> you'll need to refresh.
> > Yes, that's not a problem, but I'd like to be sure this is the way we
> > want to go before rebasing this series.
> >
> >> On Thu, Dec 04, 2014 at 11:30:12PM +0100, Boris Brezillon wrote:
> >>> The NAND and NFC (NAND Flash Controller) were linked together with a
> >>> parent <-> child relationship.
> >>>
> >>> This model has several drawbacks:
> >>> - it does not allow for multiple NAND chip handling while the controller
> >>>    support multi-chip (even though the driver is not ready yet)
> >>> - it mixes NAND partitions and NFC nodes at the same level (which is a bit
> >>>    disturbing)
> >> I agree that this is disturbing. (FWIW, it also seems a bit disturbing
> >> that atmel_nand.c actually registers two different drivers and the tries
> >> to synchronize them; this seems like it could be handled better, but I'm
> >> not sure how at the moment.)
> > Yep, that's my feeling too, but I'm not sure how this could/should be
> > done.
> > My problem here is that the pinmux should be requested by the EBI
> > device because the EBI manages several type of devices and the data and
> > address signals are shared by all the devices, hence the idea of
> > defining the nand chip node under the EBI node.
> > In the other hand, the NFC is not part of the EBI bus, and thus should
> > not be defined under the EBI node.
> >
> > This might lead to the NFC device being probed before the NAND chip,
> > hence the need for this synchronization.
> 
> OMHO, there is another way, which is change the NFC node to many NFC 
> properties, just like PMECC.
> As NFC, PMECC or hamming ecc HW could be part of current NAND node (in 
> sama5, HSMC maybe a better name for this node. )
> 
> And this change can avoid the sync problem and avoid two drivers in 
> atmel_nand.c.

Sorry I don't get it...
You gave a pseudo DT example in your following answers but I still
don't understand how you'll link the NFC and its associated NAND chips.

> 
> >
> >>> - the introduction of the EBI bus implies defining NAND chips under the
> >>>    EBI node, and the ranges available under the EBI node should be
> >>>    restricted to EBI address space, while the NFC references several
> >>>    registers outside of these EBI ranges.
> >> That's an interesting bit. I've actually run across this sort of problem
> >> on other SoCs, where we have a relationship between two pieces of
> >> hardware--the NAND chip and the NAND controller--where the former might
> >> be on one bus (like your EBI bus, with chip selects), and the latter is
> >> part of the top-level MMIO register space.
> >>
> >> But can you elaborate here a bit more? Does the NAND chip actually need
> >> to be represented under your EBI bus?
> > Yes, as said above this is all about pinmux conflicts, the NAND
> > controller has to request the appropriate pinmux for its NAND chips but
> > it will conflict with the pinmux requested by the EBI bus (data and
> > address signals are shared by all the devices connected on the EBI).
> >
> >>> Move the NFC node outside of the NAND node, to get a more future-proof
> >>> model.
> >> I'm curious if an alternative solution might work, maybe one like the
> >> Allwiner NAND (sunxi-nand) DT, which just reverses the roles; the 'NFC'
> >> is the parent of the NAND chip(s). We've seen this pattern in other
> >> contexts too.
> 
> I also prefer this. Then the dt node should looks like finally:
> 
> nand (SMC may be more correct) node {

This nand node contains nand chip nodes, so 'nand' is definitely not
the appropriate name for this node.
We could name it SMC, but I'd like to keep EBI (External Bus
Interface), because the only thing that can register child devices in
linux are busses (or MFD devices :-)).
The SMC (Static Memory Controller) is just a additional control logic
acting on top of the EBI.

>      PMECC properties
>      NFC properties --> we can make the NFC not a node, just many NFC 
> properties.

But all NAND chips will have to point to the same nfc struct, and I'd
rather represent the NFC IP in the DT than hide it into the driver's
logic.
Moreover, the NFC IP is not part of the EBI memory range, so I'd prefer
to keep it outside of the EBI node (though I'm not sure you're trying to
represent the EBI bus here).

> 
>      pinctrl-nand0
>      nand chip 0: {
>          partitions...
>      }
> 
>      pinctrl-nand1
>      nand chip 1: {
>          partitions...
>      }
> }


Could you give a real DT example instead of a pseudo DT representation,
maybe I'll understand what you're suggesting then.

> 
> > I would have preferred this solution too, but the EBI/pinmux constraint
> > explained above prevents this approach.
> I am not very clear about the pinmux constraint.
> Maybe we just leave one DT node (either EBI or current nand node) to 
> take care the pins.
> 
> > What I can do though, is reverse the referencing: reference nand chips
> > from the nand controller node.
> 
> I guess the dt looks like: (correct me if I am wrong)
> 
> EBI node {
>      pinctrl-nand0
>      nand chip 0: {
>          partitions...
>      }
> 
>      pinctrl-nand1
>      nand chip 1: {
>          partitions...
>      }
> }

Well, that's more someting like:

ebi@xxxx {
	pinctrl-0 = <&ebi_data_bus_pins &ebi_addr_bus_pins
		     &ebi_nand_cs0_pin &ebi_nand_rb0_pin ...>;

	nand@0,xxxx {
		/* ../ */
	};

	nand@1,xxxx {
		/* ../ */
	}
}

> 
> nand (SMC/HSMC may be more correct) node {
>      PMECC properties
>      NFC properties --> we can make the NFC not a node, just many NFC 
> properties.
> 
>      &nand chip0
>      &nand chip1
> }

Okay, I guess I understand what you were talking about in your previous
suggestion, and I'm not a big fan of this representation.

The SMC IP provides a set of registers to configure external devices
timings (and other related stuff).
Here you're representing NAND chip devices under the SMC node, which is
not exactly how I would represent them.
The IP controlling the available NAND chips is actually the NFC (NAND
Flash Controller).
How about this representation instead ?

nfc@xxxxx {
	nand-chips = <&nand0 &nand1>;
}

Josh, could you rework your proposal with a real DT representation so
that I'll be sure to understand what you're suggesting ?

Thanks.

Best Regards,

Boris



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

[Josh Wu]

Hi, Boris

Thanks a lot for your explanation, check my reply for more description 
for my suggestion.

On 2/3/2015 5:37 PM, Boris Brezillon wrote:
> On Tue, 3 Feb 2015 16:46:15 +0800
> Josh Wu <josh.wu@atmel.com> wrote:
>
>> Hi, Boris, Brian
>>
>> On 2/2/2015 5:42 PM, Boris Brezillon wrote:
>>> Hi Brian,
>>>
>>> On Sun, 1 Feb 2015 23:57:37 -0800
>>> Brian Norris <computersforpeace@gmail.com> wrote:
>>>
>>>> Hi Boris,
>>>>
>>>> BTW, this series has a few conflicts with other things I have queued, so
>>>> you'll need to refresh.
>>> Yes, that's not a problem, but I'd like to be sure this is the way we
>>> want to go before rebasing this series.
>>>
>>>> On Thu, Dec 04, 2014 at 11:30:12PM +0100, Boris Brezillon wrote:
>>>>> The NAND and NFC (NAND Flash Controller) were linked together with a
>>>>> parent <-> child relationship.
>>>>>
>>>>> This model has several drawbacks:
>>>>> - it does not allow for multiple NAND chip handling while the controller
>>>>>     support multi-chip (even though the driver is not ready yet)
>>>>> - it mixes NAND partitions and NFC nodes at the same level (which is a bit
>>>>>     disturbing)
>>>> I agree that this is disturbing. (FWIW, it also seems a bit disturbing
>>>> that atmel_nand.c actually registers two different drivers and the tries
>>>> to synchronize them; this seems like it could be handled better, but I'm
>>>> not sure how at the moment.)
>>> Yep, that's my feeling too, but I'm not sure how this could/should be
>>> done.
>>> My problem here is that the pinmux should be requested by the EBI
>>> device because the EBI manages several type of devices and the data and
>>> address signals are shared by all the devices, hence the idea of
>>> defining the nand chip node under the EBI node.
>>> In the other hand, the NFC is not part of the EBI bus, and thus should
>>> not be defined under the EBI node.
>>>
>>> This might lead to the NFC device being probed before the NAND chip,
>>> hence the need for this synchronization.
>> OMHO, there is another way, which is change the NFC node to many NFC
>> properties, just like PMECC.
>> As NFC, PMECC or hamming ecc HW could be part of current NAND node (in
>> sama5, HSMC maybe a better name for this node. )
>>
>> And this change can avoid the sync problem and avoid two drivers in
>> atmel_nand.c.
> Sorry I don't get it...
> You gave a pseudo DT example in your following answers but I still
> don't understand how you'll link the NFC and its associated NAND chips.
>
>>>>> - the introduction of the EBI bus implies defining NAND chips under the
>>>>>     EBI node, and the ranges available under the EBI node should be
>>>>>     restricted to EBI address space, while the NFC references several
>>>>>     registers outside of these EBI ranges.
>>>> That's an interesting bit. I've actually run across this sort of problem
>>>> on other SoCs, where we have a relationship between two pieces of
>>>> hardware--the NAND chip and the NAND controller--where the former might
>>>> be on one bus (like your EBI bus, with chip selects), and the latter is
>>>> part of the top-level MMIO register space.
>>>>
>>>> But can you elaborate here a bit more? Does the NAND chip actually need
>>>> to be represented under your EBI bus?
>>> Yes, as said above this is all about pinmux conflicts, the NAND
>>> controller has to request the appropriate pinmux for its NAND chips but
>>> it will conflict with the pinmux requested by the EBI bus (data and
>>> address signals are shared by all the devices connected on the EBI).
>>>
>>>>> Move the NFC node outside of the NAND node, to get a more future-proof
>>>>> model.
>>>> I'm curious if an alternative solution might work, maybe one like the
>>>> Allwiner NAND (sunxi-nand) DT, which just reverses the roles; the 'NFC'
>>>> is the parent of the NAND chip(s). We've seen this pattern in other
>>>> contexts too.
>> I also prefer this. Then the dt node should looks like finally:
>>
>> nand (SMC may be more correct) node {
> This nand node contains nand chip nodes, so 'nand' is definitely not
> the appropriate name for this node.
> We could name it SMC, but I'd like to keep EBI (External Bus
> Interface), because the only thing that can register child devices in
> linux are busses (or MFD devices :-)).
> The SMC (Static Memory Controller) is just a additional control logic
> acting on top of the EBI.
After further thought, It seems the SMC should be correct name for nand 
chips' parent.

Before SAMA5 chips, the PMECC/ECC registers address is out of SMC address.

In SAMA5 chips, the PMECC/NFC-hw registers address is in SMC address.
take sama5d3 for example:
     NFC regs: 0xffffc000 0x00000070
     PMECC regs: 0xffffc070 0x00000490
     PMECC error regs: 0xffffc500 0x00000100

And the HSMC regs is: 0xffffc000 0x00000700
which include PMECC/NFC-hw registers.

>>       PMECC properties
>>       NFC properties --> we can make the NFC not a node, just many NFC
>> properties.
> But all NAND chips will have to point to the same nfc struct, and I'd
> rather represent the NFC IP in the DT than hide it into the driver's
> logic.
> Moreover, the NFC IP is not part of the EBI memory range, so I'd prefer
> to keep it outside of the EBI node (though I'm not sure you're trying to
> represent the EBI bus here).
>
>>       pinctrl-nand0
>>       nand chip 0: {
>>           partitions...
>>       }
>>
>>       pinctrl-nand1
>>       nand chip 1: {
>>           partitions...
>>       }
>> }
>
> Could you give a real DT example instead of a pseudo DT representation,
> maybe I'll understand what you're suggesting then.
>
>>> I would have preferred this solution too, but the EBI/pinmux constraint
>>> explained above prevents this approach.
>> I am not very clear about the pinmux constraint.
>> Maybe we just leave one DT node (either EBI or current nand node) to
>> take care the pins.
>>
>>> What I can do though, is reverse the referencing: reference nand chips
>>> from the nand controller node.
>> I guess the dt looks like: (correct me if I am wrong)
>>
>> EBI node {
>>       pinctrl-nand0
>>       nand chip 0: {
>>           partitions...
>>       }
>>
>>       pinctrl-nand1
>>       nand chip 1: {
>>           partitions...
>>       }
>> }
> Well, that's more someting like:
>
> ebi@xxxx {
> 	pinctrl-0 = <&ebi_data_bus_pins &ebi_addr_bus_pins
> 		     &ebi_nand_cs0_pin &ebi_nand_rb0_pin ...>;
>
> 	nand@0,xxxx {
> 		/* ../ */
> 	};
>
> 	nand@1,xxxx {
> 		/* ../ */
> 	}
> }
well, so nand driver should be probed after this ebi node probed, since 
ebi will configure the nand pins.
There should be a sync issue to solve. or maybe I miss something?

>> nand (SMC/HSMC may be more correct) node {
>>       PMECC properties
>>       NFC properties --> we can make the NFC not a node, just many NFC
>> properties.
>>
>>       &nand chip0
>>       &nand chip1
>> }
> Okay, I guess I understand what you were talking about in your previous
> suggestion, and I'm not a big fan of this representation.
>
> The SMC IP provides a set of registers to configure external devices
> timings (and other related stuff).
> Here you're representing NAND chip devices under the SMC node, which is
> not exactly how I would represent them.
> The IP controlling the available NAND chips is actually the NFC (NAND
> Flash Controller).
> How about this representation instead ?
>
> nfc@xxxxx {
> 	nand-chips = <&nand0 &nand1>;
> }

This should be ok, but this nfc@xxxxx should be a logic block. As the 
real NFC hardware only appeared since SAMA5 chips.
And we can disabled it. Without the real hardware NFC the nand should 
also works well.

> Josh, could you rework your proposal with a real DT representation so
> that I'll be sure to understand what you're suggesting ?

Okay, first I prefer to remove the atmel_nand_nfc driver, the work that 
be done in atmel_nand_nfc_probe() function will move to atmel_nand_probe().
The dt should looks like:
         nand0: nand@80000000 {
             compatible = "atmel,sama5d4-nand", "atmel,at91rm9200-nand";
             #address-cells = <1>;
             #size-cells = <1>;
             ranges;
             reg = <    0x80000000 0x08000000    /* EBI CS3 */
                 0xfc05c070 0x00000490    /* SMC PMECC regs */
                 0xfc05c500 0x00000100    /* SMC PMECC Error Location 
regs */
                 0x90000000 0x08000000    /* NFC Command Registers */
                 0xfc05c000 0x00000070    /* NFC HSMC regs */
                 0x00100000 0x00100000    /* NFC SRAM banks */
                 >;
             interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>;
             atmel,nand-addr-offset = <21>;
             atmel,nand-cmd-offset = <22>;
             atmel,nand-has-dma;
             pinctrl-names = "default";
             pinctrl-0 = <&pinctrl_nand>;
             status = "disabled";
             clocks = <&hsmc_clk>;
             atmel,write-by-sram;
         };

The &hsmc_clk & atmel,write-by-sram will move to uplayer.
And the hardware NFC can be disabled in menuconfig some options. or add 
some dt properties like atmel,enable-nfc.

Then we can make use of EBI/SMC node,

nfc@xxxxx {
     compatible = "atmel,sama5d4-nand", "atmel,at91rm9200-nand";
     #address-cells = <1>;
     #size-cells = <1>;
     ranges;
     reg = <
                 0xfc05c070 0x00000490    /* SMC PMECC regs */
                 0xfc05c500 0x00000100    /* SMC PMECC Error Location 
regs */
                 0xfc05c000 0x00000070    /* NFC HSMC regs */
         /* all above address will be overlay with smc regs, maybe we 
can use it from smc? */

                 0x00100000 0x00100000    /* NFC SRAM banks */
                 0x90000000 0x08000000    /* NFC Command Registers */
                 >;
     interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>;
     atmel,nand-addr-offset = <21>;
     atmel,nand-cmd-offset = <22>;
     atmel,nand-has-dma;
     clocks = <&hsmc_clk>;    /* needed for all smc components, like 
pmecc, nfc hardware */

     atmel,nfc-disabled;    /* disabled hw NFC */
     atmel,nfc-write-by-sram;
     status = "disabled";

     nand-chips = <&nand0 &nand1>;
  }

I am not familiar with the EBI/SMC dt node, so above should have errors, 
but it's just a draft for us to discuss.

>
> Thanks.
>
> Best Regards,
>
> Boris
>
>
>
Best Regards,
Josh Wu

Re: [PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND

[Boris Brezillon]

Hi Josh,

On Wed, 4 Feb 2015 18:06:37 +0800
Josh Wu <josh.wu@atmel.com> wrote:

> Hi, Boris
> 
> Thanks a lot for your explanation, check my reply for more description 
> for my suggestion.
> 
> On 2/3/2015 5:37 PM, Boris Brezillon wrote:
> > On Tue, 3 Feb 2015 16:46:15 +0800
> > Josh Wu <josh.wu@atmel.com> wrote:
> >
> >> Hi, Boris, Brian
> >>
> >> On 2/2/2015 5:42 PM, Boris Brezillon wrote:
> >>> Hi Brian,
> >>>
> >>> On Sun, 1 Feb 2015 23:57:37 -0800
> >>> Brian Norris <computersforpeace@gmail.com> wrote:
> >>>
> >>>> Hi Boris,
> >>>>
> >>>> BTW, this series has a few conflicts with other things I have queued, so
> >>>> you'll need to refresh.
> >>> Yes, that's not a problem, but I'd like to be sure this is the way we
> >>> want to go before rebasing this series.
> >>>
> >>>> On Thu, Dec 04, 2014 at 11:30:12PM +0100, Boris Brezillon wrote:
> >>>>> The NAND and NFC (NAND Flash Controller) were linked together with a
> >>>>> parent <-> child relationship.
> >>>>>
> >>>>> This model has several drawbacks:
> >>>>> - it does not allow for multiple NAND chip handling while the controller
> >>>>>     support multi-chip (even though the driver is not ready yet)
> >>>>> - it mixes NAND partitions and NFC nodes at the same level (which is a bit
> >>>>>     disturbing)
> >>>> I agree that this is disturbing. (FWIW, it also seems a bit disturbing
> >>>> that atmel_nand.c actually registers two different drivers and the tries
> >>>> to synchronize them; this seems like it could be handled better, but I'm
> >>>> not sure how at the moment.)
> >>> Yep, that's my feeling too, but I'm not sure how this could/should be
> >>> done.
> >>> My problem here is that the pinmux should be requested by the EBI
> >>> device because the EBI manages several type of devices and the data and
> >>> address signals are shared by all the devices, hence the idea of
> >>> defining the nand chip node under the EBI node.
> >>> In the other hand, the NFC is not part of the EBI bus, and thus should
> >>> not be defined under the EBI node.
> >>>
> >>> This might lead to the NFC device being probed before the NAND chip,
> >>> hence the need for this synchronization.
> >> OMHO, there is another way, which is change the NFC node to many NFC
> >> properties, just like PMECC.
> >> As NFC, PMECC or hamming ecc HW could be part of current NAND node (in
> >> sama5, HSMC maybe a better name for this node. )
> >>
> >> And this change can avoid the sync problem and avoid two drivers in
> >> atmel_nand.c.
> > Sorry I don't get it...
> > You gave a pseudo DT example in your following answers but I still
> > don't understand how you'll link the NFC and its associated NAND chips.
> >
> >>>>> - the introduction of the EBI bus implies defining NAND chips under the
> >>>>>     EBI node, and the ranges available under the EBI node should be
> >>>>>     restricted to EBI address space, while the NFC references several
> >>>>>     registers outside of these EBI ranges.
> >>>> That's an interesting bit. I've actually run across this sort of problem
> >>>> on other SoCs, where we have a relationship between two pieces of
> >>>> hardware--the NAND chip and the NAND controller--where the former might
> >>>> be on one bus (like your EBI bus, with chip selects), and the latter is
> >>>> part of the top-level MMIO register space.
> >>>>
> >>>> But can you elaborate here a bit more? Does the NAND chip actually need
> >>>> to be represented under your EBI bus?
> >>> Yes, as said above this is all about pinmux conflicts, the NAND
> >>> controller has to request the appropriate pinmux for its NAND chips but
> >>> it will conflict with the pinmux requested by the EBI bus (data and
> >>> address signals are shared by all the devices connected on the EBI).
> >>>
> >>>>> Move the NFC node outside of the NAND node, to get a more future-proof
> >>>>> model.
> >>>> I'm curious if an alternative solution might work, maybe one like the
> >>>> Allwiner NAND (sunxi-nand) DT, which just reverses the roles; the 'NFC'
> >>>> is the parent of the NAND chip(s). We've seen this pattern in other
> >>>> contexts too.
> >> I also prefer this. Then the dt node should looks like finally:
> >>
> >> nand (SMC may be more correct) node {
> > This nand node contains nand chip nodes, so 'nand' is definitely not
> > the appropriate name for this node.
> > We could name it SMC, but I'd like to keep EBI (External Bus
> > Interface), because the only thing that can register child devices in
> > linux are busses (or MFD devices :-)).
> > The SMC (Static Memory Controller) is just a additional control logic
> > acting on top of the EBI.
> 
> After further thought, It seems the SMC should be correct name for nand 
> chips' parent.
> 
> Before SAMA5 chips, the PMECC/ECC registers address is out of SMC address.
> 
> In SAMA5 chips, the PMECC/NFC-hw registers address is in SMC address.
> take sama5d3 for example:
>      NFC regs: 0xffffc000 0x00000070
>      PMECC regs: 0xffffc070 0x00000490
>      PMECC error regs: 0xffffc500 0x00000100
> 
> And the HSMC regs is: 0xffffc000 0x00000700
> which include PMECC/NFC-hw registers.

Hmm, it only includes part of the NFC registers, AFAIR, another region
is reserved for the NFC IP.

For those shared registers (all embedded in the SMC memory region), I
recommend using the regmap provided by the SMC syscon device, but
that's another story ;-).

My point is that I don't think nand chip nodes should be under the SMC
node, but under the EBI one instead.

Anyway, wherever we decide to put those NAND chip nodes, the problem
remains: we'll have to link the NAND chips with their controller (the
NFC).

> 
> >
> >>       PMECC properties
> >>       NFC properties --> we can make the NFC not a node, just many NFC
> >> properties.
> > But all NAND chips will have to point to the same nfc struct, and I'd
> > rather represent the NFC IP in the DT than hide it into the driver's
> > logic.
> > Moreover, the NFC IP is not part of the EBI memory range, so I'd prefer
> > to keep it outside of the EBI node (though I'm not sure you're trying to
> > represent the EBI bus here).
> >
> >>       pinctrl-nand0
> >>       nand chip 0: {
> >>           partitions...
> >>       }
> >>
> >>       pinctrl-nand1
> >>       nand chip 1: {
> >>           partitions...
> >>       }
> >> }
> >
> > Could you give a real DT example instead of a pseudo DT representation,
> > maybe I'll understand what you're suggesting then.
> >
> >>> I would have preferred this solution too, but the EBI/pinmux constraint
> >>> explained above prevents this approach.
> >> I am not very clear about the pinmux constraint.
> >> Maybe we just leave one DT node (either EBI or current nand node) to
> >> take care the pins.
> >>
> >>> What I can do though, is reverse the referencing: reference nand chips
> >>> from the nand controller node.
> >> I guess the dt looks like: (correct me if I am wrong)
> >>
> >> EBI node {
> >>       pinctrl-nand0
> >>       nand chip 0: {
> >>           partitions...
> >>       }
> >>
> >>       pinctrl-nand1
> >>       nand chip 1: {
> >>           partitions...
> >>       }
> >> }
> > Well, that's more someting like:
> >
> > ebi@xxxx {
> > 	pinctrl-0 = <&ebi_data_bus_pins &ebi_addr_bus_pins
> > 		     &ebi_nand_cs0_pin &ebi_nand_rb0_pin ...>;
> >
> > 	nand@0,xxxx {
> > 		/* ../ */
> > 	};
> >
> > 	nand@1,xxxx {
> > 		/* ../ */
> > 	}
> > }
> 
> well, so nand driver should be probed after this ebi node probed, since 
> ebi will configure the nand pins.
> There should be a sync issue to solve. or maybe I miss something?

Absolutely, we have to synchronize the NAND chips with their NAND
controller.

> 
> 
> >
> >> nand (SMC/HSMC may be more correct) node {
> >>       PMECC properties
> >>       NFC properties --> we can make the NFC not a node, just many NFC
> >> properties.
> >>
> >>       &nand chip0
> >>       &nand chip1
> >> }
> > Okay, I guess I understand what you were talking about in your previous
> > suggestion, and I'm not a big fan of this representation.
> >
> > The SMC IP provides a set of registers to configure external devices
> > timings (and other related stuff).
> > Here you're representing NAND chip devices under the SMC node, which is
> > not exactly how I would represent them.
> > The IP controlling the available NAND chips is actually the NFC (NAND
> > Flash Controller).
> > How about this representation instead ?
> >
> > nfc@xxxxx {
> > 	nand-chips = <&nand0 &nand1>;
> > }
> 
> This should be ok, but this nfc@xxxxx should be a logic block. As the 
> real NFC hardware only appeared since SAMA5 chips.

No need to define a NFC node if the hardware does not embed one...
Or, are you suggesting to define a NAND controller node for all at91
SoCs ?
That might be a good idea if other SoCs also support multiple NAND chips
sharing the same PMECC block.

> And we can disabled it. Without the real hardware NFC the nand should 
> also works well.
> 
> >
> > Josh, could you rework your proposal with a real DT representation so
> > that I'll be sure to understand what you're suggesting ?
> 
> Okay, first I prefer to remove the atmel_nand_nfc driver, the work that 
> be done in atmel_nand_nfc_probe() function will move to atmel_nand_probe().
> The dt should looks like:
>          nand0: nand@80000000 {
>              compatible = "atmel,sama5d4-nand", "atmel,at91rm9200-nand";
>              #address-cells = <1>;
>              #size-cells = <1>;
>              ranges;
>              reg = <    0x80000000 0x08000000    /* EBI CS3 */
>                  0xfc05c070 0x00000490    /* SMC PMECC regs */
>                  0xfc05c500 0x00000100    /* SMC PMECC Error Location 
> regs */
>                  0x90000000 0x08000000    /* NFC Command Registers */
>                  0xfc05c000 0x00000070    /* NFC HSMC regs */
>                  0x00100000 0x00100000    /* NFC SRAM banks */
>                  >;

These registers should be owned by the NFC not each NAND chip.

>              interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>;

I'm not sure, but I think the same goes for this irq.

>              atmel,nand-addr-offset = <21>;
>              atmel,nand-cmd-offset = <22>;
>              atmel,nand-has-dma;
>              pinctrl-names = "default";
>              pinctrl-0 = <&pinctrl_nand>;

I'm trying to move those pinctrl requests into EBI.

>              status = "disabled";
>              clocks = <&hsmc_clk>;
>              atmel,write-by-sram;

Those 2 properties (clocks and atmel,write-by-sram) should be part of
the NFC node too.

>          };
> 
> The &hsmc_clk & atmel,write-by-sram will move to uplayer.
> And the hardware NFC can be disabled in menuconfig some options. or add 
> some dt properties like atmel,enable-nfc.

Hm, how about enabling/disabling it with the status property ?

> 
> Then we can make use of EBI/SMC node,
> 
> nfc@xxxxx {
> 	compatible = "atmel,sama5d4-nand", "atmel,at91rm9200-nand";

How about "atmel,sama5d4-nand-controller" ?

> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	ranges;
> 	reg = <
>                  0xfc05c070 0x00000490    /* SMC PMECC regs */
>                  0xfc05c500 0x00000100    /* SMC PMECC Error Location regs */
>                  0xfc05c000 0x00000070    /* NFC HSMC regs */
> 		/* all above address will be overlay with smc regs, maybe we can use it from smc? */
> 
>                  0x00100000 0x00100000    /* NFC SRAM banks */
>                  0x90000000 0x08000000    /* NFC Command Registers */
>                  >;
> 	interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>;
> 	atmel,nand-addr-offset = <21>;
> 	atmel,nand-cmd-offset = <22>;

Those 2 propoerties (atmel,nand-addr-offset and atmel,nand-cmd-offset)
are purely NAND chip related, and thus should not be part of the NAND
controller node.

> 	atmel,nand-has-dma;
> 	clocks = <&hsmc_clk>;	/* needed for all smc components, like pmecc, nfc hardware */
> 
> 	atmel,nfc-disabled;	/* disabled hw NFC */
> 	atmel,nfc-write-by-sram;
> 	status = "disabled";
> 
> 	nand-chips = <&nand0 &nand1>;
>   }
> 
> I am not familiar with the EBI/SMC dt node, so above should have errors, 
> but it's just a draft for us to discuss.

You can have a look at my EBI series [1] if you want more details on
the proposed DT binding.

Best Regards,

Boris

[1]http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308469.html

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com