[PATCH v2 0/4] RFC: OMAP GPMC DT bindings

[PATCH v2 0/4] RFC: OMAP GPMC DT bindings

[Daniel Mack]

This is a series of patches to support GPMC peripherals on OMAP boards.

Depends on Linus' master +
omap-next (branch omap-for-v3.8/cleanup-headers-gpmc)

The only supported peripheral for now is NAND, but other types would be
easy to add.

Version 2 addresses details pointed out by Jon Hunter, Afzal Mohammed
and Rob Herring:

 - add "reg" and "ti,hwmod" properties to Documentation
 - use generic of_mtd functions and the property names defined by them,
   namely "nand-bus-width" and "nand-ecc-mode"
 - reduce the default register space size in the Documentation to 8K,
   as found in the hwmod code
 - switch to a DT layout based on ranges and address translation.
   Although this property is not currently looked at as long as the
   handling code still uses the runtime calculation methods, we now
   have these values in the bindings, eventually allowing us to
   switch the implementation with less pain.

Thanks,
Daniel

Daniel Mack (4):
  mtd: omap-nand: pass device_node in platform data
  ARM: OMAP: gpmc: enable hwecc for AM33xx SoCs
  ARM: OMAP: gpmc: don't create devices from initcall on DT
  ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

 Documentation/devicetree/bindings/bus/ti-gpmc.txt  |  73 +++++++++++
 .../devicetree/bindings/mtd/gpmc-nand.txt          |  61 +++++++++
 arch/arm/mach-omap2/gpmc-nand.c                    |   2 +-
 arch/arm/mach-omap2/gpmc.c                         | 146 +++++++++++++++++++++
 drivers/mtd/nand/omap2.c                           |   4 +-
 include/linux/platform_data/mtd-nand-omap2.h       |   2 +
 6 files changed, 286 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/ti-gpmc.txt
 create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt

-- 
1.7.11.7

[PATCH v2 1/4] mtd: omap-nand: pass device_node in platform data

[Daniel Mack]

Pass an optional device_node pointer in the platform data, which in turn
will be put into a mtd_part_parser_data. This way, code that sets up the
platform devices can pass along the node from DT so that the partitions
can be parsed.

For non-DT boards, this change has no effect.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/omap2.c                     | 4 +++-
 include/linux/platform_data/mtd-nand-omap2.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 3282b15..a733f15 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1331,6 +1331,7 @@ static int __devinit omap_nand_probe(struct platform_device *pdev)
 	dma_cap_mask_t mask;
 	unsigned sig;
 	struct resource			*res;
+	struct mtd_part_parser_data	ppdata = {};
 
 	pdata = pdev->dev.platform_data;
 	if (pdata == NULL) {
@@ -1556,7 +1557,8 @@ static int __devinit omap_nand_probe(struct platform_device *pdev)
 		goto out_release_mem_region;
 	}
 
-	mtd_device_parse_register(&info->mtd, NULL, NULL, pdata->parts,
+	ppdata.of_node = pdata->of_node;
+	mtd_device_parse_register(&info->mtd, NULL, &ppdata, pdata->parts,
 				  pdata->nr_parts);
 
 	platform_set_drvdata(pdev, &info->mtd);
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index 24d32ca..5217d6e 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -60,6 +60,8 @@ struct omap_nand_platform_data {
 	int			devsize;
 	enum omap_ecc           ecc_opt;
 	struct gpmc_nand_regs	reg;
+	/* for passing the partitions */
+	struct device_node	*of_node;
 };
 
 #endif
-- 
1.7.11.7

[PATCH v2 2/4] ARM: OMAP: gpmc: enable hwecc for AM33xx SoCs

[Daniel Mack]

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 arch/arm/mach-omap2/gpmc-nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index 8607735..c3616c6 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -92,7 +92,7 @@ static int omap2_nand_gpmc_retime(
 static bool __init gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
 {
 	/* support only OMAP3 class */
-	if (!cpu_is_omap34xx()) {
+	if (!cpu_is_omap34xx() && !soc_is_am33xx()) {
 		pr_err("BCH ecc is not supported on this CPU\n");
 		return 0;
 	}
-- 
1.7.11.7

[PATCH v2 3/4] ARM: OMAP: gpmc: don't create devices from initcall on DT

[Daniel Mack]

On DT driven boards, the gpmc node will match the driver. Hence, there's
no need to do that unconditionally from the initcall.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 arch/arm/mach-omap2/gpmc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 60f1cce..1dcb30c 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -844,6 +844,13 @@ static int __init omap_gpmc_init(void)
 	struct platform_device *pdev;
 	char *oh_name = "gpmc";
 
+	/*
+	 * if the board boots up with a populated DT, do not
+	 * manually add the device from this initcall
+	 */
+	if (of_have_populated_dt())
+		return -ENODEV;
+
 	oh = omap_hwmod_lookup(oh_name);
 	if (!oh) {
 		pr_err("Could not look up %s\n", oh_name);
-- 
1.7.11.7

[PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

This patch adds basic DT bindings for OMAP GPMC.

The actual peripherals are instanciated from child nodes within the GPMC
node, and the only type of device that is currently supported is NAND.

Code was added to parse the generic GPMC timing parameters and some
documentation with examples on how to use them.

Successfully tested on an AM33xx board.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 Documentation/devicetree/bindings/bus/ti-gpmc.txt  |  73 +++++++++++
 .../devicetree/bindings/mtd/gpmc-nand.txt          |  61 +++++++++
 arch/arm/mach-omap2/gpmc.c                         | 139 +++++++++++++++++++++
 3 files changed, 273 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/ti-gpmc.txt
 create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt

diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
new file mode 100644
index 0000000..6f44487
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
@@ -0,0 +1,73 @@
+Device tree bindings for OMAP general purpose memory controllers (GPMC)
+
+The actual devices are instantiated from the child nodes of a GPMC node.
+
+Required properties:
+
+ - compatible:		Should be set to "ti,gpmc"
+ - reg:			A resource specifier for the register space
+			(see the example below)
+ - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
+			completed.
+ - #address-cells:	Must be set to 2 to allow memory address translation
+ - #size-cells:		Must be set to 1 to allow CS address passing
+ - ranges:		Must be set up to reflect the memory layout
+			Note that this property is not currently parsed.
+			Calculated values derived from the contents of
+			GPMC_CS_CONFIG7 as set up by the bootloader. That will
+			change in the future, so be sure to fill the correct
+			values here.
+
+Timing properties for child nodes. All are optional and default to 0.
+
+ - gpmc,sync-clk:	Minimum clock period for synchronous mode, in picoseconds
+
+ Chip-select signal timings corresponding to GPMC_CS_CONFIG2:
+ - gpmc,cs-on:		Assertion time
+ - gpmc,cs-rd-off:	Read deassertion time
+ - gpmc,cs-wr-off:	Write deassertion time
+
+ ADV signal timings corresponding to GPMC_CONFIG3:
+ - gpmc,adv-on:		Assertion time
+ - gpmc,adv-rd-off:	Read deassertion time
+ - gpmc,adv-wr-off:	Write deassertion time
+
+ WE signals timings corresponding to GPMC_CONFIG4:
+ - gpmc,we-on:		Assertion time
+ - gpmc,we-off:		Deassertion time
+
+ OE signals timings corresponding to GPMC_CONFIG4
+ - gpmc,oe-on:		Assertion time
+ - gpmc,oe-off:		Deassertion time
+
+ Access time and cycle time timings corresponding to GPMC_CONFIG5
+ - gpmc,page-burst-access: Multiple access word delay
+ - gpmc,access:		Start-cycle to first data valid delay
+ - gpmc,rd-cycle:	Total read cycle time
+ - gpmc,wr-cycle:	Total write cycle time
+
+The following are only on OMAP3430
+ - gpmc,wr-access
+ - gpmc,wr-data-mux-bus
+
+
+Example for an AM33xx board:
+
+	gpmc: gpmc at 50000000 {
+		compatible = "ti,gpmc";
+		ti,hwmods = "gpmc";
+		reg = <0x50000000 0x2000>;
+		interrupt-parent = <&intc>;
+		interrupts = <100>;
+
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <0 0 0x08000000 0x10000000>; /* CS0 */
+
+		/* child nodes go here */
+	};
+
+
+
+
+
diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
new file mode 100644
index 0000000..e0c1e67
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -0,0 +1,61 @@
+Device tree bindings for GPMC connected NANDs
+
+GPMC connected NAND (found on OMAP boards) are represented as child nodes of
+the GPMC controller with a name of "nand".
+
+All timing relevant properties as well as generic gpmc child properties are
+explained in a separate documents - please refer to
+Documentation/devicetree/bindings/bus/ti-gpmc.txt
+
+For NAND specific properties such as ECC modes or bus width, please refer to
+Documentation/devicetree/bindings/mtd/nand.txt
+
+
+Required properties:
+
+ - reg: The CS line the peripheral is connected to
+
+For inline partiton table parsing (optional):
+
+ - #address-cells: should be set to 1
+ - #size-cells: should be set to 1
+
+Example for an AM33xx board:
+
+	gpmc: gpmc at 50000000 {
+		compatible = "ti,gpmc";
+		ti,hwmods = "gpmc";
+		reg = <0x50000000 0x1000000>;
+		interrupt-parent = <&intc>;
+		interrupts = <100>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <0 0 0x08000000 0x10000000>;	/* CS0: NAND */
+
+		nand at 0,0 {
+			reg = <0 0 0>; /* CS0, offset 0 */
+			nand-bus-width = <16>;
+			nand-ecc-mode = "none";
+
+			gpmc,sync-clk = <0>;
+			gpmc,cs-on = <0>;
+			gpmc,cs-rd-off = <36>;
+			gpmc,cs-wr-off = <36>;
+			gpmc,adv-on = <6>;
+			gpmc,adv-rd-off = <24>;
+			gpmc,adv-wr-off = <36>;
+			gpmc,we-off = <30>;
+			gpmc,oe-off = <48>;
+			gpmc,access = <54>;
+			gpmc,rd-cycle = <72>;
+			gpmc,wr-cycle = <72>;
+			gpmc,wr-access = <30>;
+			gpmc,wr-data-mux-bus = <0>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			/* partitions go here */
+		};
+	};
+
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 1dcb30c..b028225 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -25,6 +25,10 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_mtd.h>
+#include <linux/of_device.h>
+#include <linux/mtd/nand.h>
 
 #include <linux/platform_data/mtd-nand-omap2.h>
 
@@ -37,6 +41,7 @@
 #include "soc.h"
 #include "common.h"
 #include "gpmc.h"
+#include "gpmc-nand.h"
 
 #define	DEVICE_NAME		"omap-gpmc"
 
@@ -751,6 +756,131 @@ static int __devinit gpmc_mem_init(void)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id gpmc_dt_ids[] = {
+	{ .compatible = "ti,gpmc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gpmc_dt_ids);
+
+static void gpmc_read_timings_dt(struct device_node *np,
+				 struct gpmc_timings *gpmc_t)
+{
+	u32 val;
+
+	memset(gpmc_t, 0, sizeof(*gpmc_t));
+
+	/* minimum clock period for syncronous mode */
+	if (!of_property_read_u32(np, "gpmc,sync-clk", &val))
+		gpmc_t->sync_clk = val;
+
+	/* chip select timtings */
+	if (!of_property_read_u32(np, "gpmc,cs-on", &val))
+		gpmc_t->cs_on = val;
+
+	if (!of_property_read_u32(np, "gpmc,cs-rd-off", &val))
+		gpmc_t->cs_rd_off = val;
+
+	if (!of_property_read_u32(np, "gpmc,cs-wr-off", &val))
+		gpmc_t->cs_wr_off = val;
+
+	/* ADV signal timings */
+	if (!of_property_read_u32(np, "gpmc,adv-on", &val))
+		gpmc_t->adv_on = val;
+
+	if (!of_property_read_u32(np, "gpmc,adv-rd-off", &val))
+		gpmc_t->adv_rd_off = val;
+
+	if (!of_property_read_u32(np, "gpmc,adv-wr-off", &val))
+		gpmc_t->adv_wr_off = val;
+
+	/* WE signal timings */
+	if (!of_property_read_u32(np, "gpmc,we-on", &val))
+		gpmc_t->we_on = val;
+
+	if (!of_property_read_u32(np, "gpmc,we-off", &val))
+		gpmc_t->we_off = val;
+
+	/* OE signal timings */
+	if (!of_property_read_u32(np, "gpmc,oe-on", &val))
+		gpmc_t->oe_on = val;
+
+	if (!of_property_read_u32(np, "gpmc,oe-off", &val))
+		gpmc_t->oe_off = val;
+
+	/* access and cycle timings */
+	if (!of_property_read_u32(np, "gpmc,page-burst-access", &val))
+		gpmc_t->page_burst_access = val;
+
+	if (!of_property_read_u32(np, "gpmc,access", &val))
+		gpmc_t->access = val;
+
+	if (!of_property_read_u32(np, "gpmc,rd-cycle", &val))
+		gpmc_t->rd_cycle = val;
+
+	if (!of_property_read_u32(np, "gpmc,wr-cycle", &val))
+		gpmc_t->wr_cycle = val;
+
+	/* only for OMAP3430 */
+	if (!of_property_read_u32(np, "gpmc,wr-access", &val))
+		gpmc_t->wr_access = val;
+
+	if (!of_property_read_u32(np, "gpmc,wr-data-mux-bus", &val))
+		gpmc_t->wr_data_mux_bus = val;
+}
+
+static int gpmc_probe_dt(struct platform_device *pdev)
+{
+	u32 val;
+	struct device_node *child;
+	struct gpmc_timings gpmc_t;
+	const struct of_device_id *of_id =
+		of_match_device(gpmc_dt_ids, &pdev->dev);
+
+	if (!of_id)
+		return 0;
+
+	for_each_node_by_name(child, "nand") {
+		struct omap_nand_platform_data *gpmc_nand_data;
+
+		if (of_property_read_u32(child, "reg", &val) < 0) {
+			dev_err(&pdev->dev, "%s has no 'reg' property\n",
+				child->full_name);
+			continue;
+		}
+
+		gpmc_nand_data = devm_kzalloc(&pdev->dev,
+					      sizeof(*gpmc_nand_data),
+					      GFP_KERNEL);
+		if (!gpmc_nand_data) {
+			dev_err(&pdev->dev, "unable to allocate memory?");
+			return -ENOMEM;
+		}
+
+		gpmc_nand_data->cs = val;
+		gpmc_nand_data->of_node = child;
+
+		val = of_get_nand_ecc_mode(child);
+		if (val >= 0)
+			gpmc_nand_data->ecc_opt = val;
+
+		val = of_get_nand_bus_width(child);
+		if (val == 16)
+			gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
+
+		gpmc_read_timings_dt(child, &gpmc_t);
+		gpmc_nand_init(gpmc_nand_data, &gpmc_t);
+	}
+
+	return 0;
+}
+#else
+static int gpmc_probe_dt(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 static __devinit int gpmc_probe(struct platform_device *pdev)
 {
 	int rc;
@@ -804,6 +934,14 @@ static __devinit int gpmc_probe(struct platform_device *pdev)
 	if (IS_ERR_VALUE(gpmc_setup_irq()))
 		dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
 
+	rc = gpmc_probe_dt(pdev);
+	if (rc < 0) {
+		clk_disable_unprepare(gpmc_l3_clk);
+		clk_put(gpmc_l3_clk);
+		dev_err(gpmc_dev, "failed to probe DT parameters\n");
+		return rc;
+	}
+
 	return 0;
 }
 
@@ -821,6 +959,7 @@ static struct platform_driver gpmc_driver = {
 	.driver		= {
 		.name	= DEVICE_NAME,
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(gpmc_dt_ids),
 	},
 };
 
-- 
1.7.11.7

[PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Jon Hunter]

Hi Daniel,

On 11/01/2012 01:36 PM, Daniel Mack wrote:
> This patch adds basic DT bindings for OMAP GPMC.
> 
> The actual peripherals are instanciated from child nodes within the GPMC
> node, and the only type of device that is currently supported is NAND.
> 
> Code was added to parse the generic GPMC timing parameters and some
> documentation with examples on how to use them.
> 
> Successfully tested on an AM33xx board.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  Documentation/devicetree/bindings/bus/ti-gpmc.txt  |  73 +++++++++++
>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  61 +++++++++
>  arch/arm/mach-omap2/gpmc.c                         | 139 +++++++++++++++++++++
>  3 files changed, 273 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/ti-gpmc.txt
>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> 
> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
> new file mode 100644
> index 0000000..6f44487
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
> @@ -0,0 +1,73 @@
> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
> +
> +The actual devices are instantiated from the child nodes of a GPMC node.
> +
> +Required properties:
> +
> + - compatible:		Should be set to "ti,gpmc"
> + - reg:			A resource specifier for the register space
> +			(see the example below)
> + - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
> +			completed.
> + - #address-cells:	Must be set to 2 to allow memory address translation
> + - #size-cells:		Must be set to 1 to allow CS address passing
> + - ranges:		Must be set up to reflect the memory layout
> +			Note that this property is not currently parsed.
> +			Calculated values derived from the contents of
> +			GPMC_CS_CONFIG7 as set up by the bootloader. That will
> +			change in the future, so be sure to fill the correct
> +			values here.

I still think it would be good to add number of chip-selects and
wait-pins here.

> +Timing properties for child nodes. All are optional and default to 0.
> +
> + - gpmc,sync-clk:	Minimum clock period for synchronous mode, in picoseconds
> +
> + Chip-select signal timings corresponding to GPMC_CS_CONFIG2:
> + - gpmc,cs-on:		Assertion time
> + - gpmc,cs-rd-off:	Read deassertion time
> + - gpmc,cs-wr-off:	Write deassertion time
> +
> + ADV signal timings corresponding to GPMC_CONFIG3:
> + - gpmc,adv-on:		Assertion time
> + - gpmc,adv-rd-off:	Read deassertion time
> + - gpmc,adv-wr-off:	Write deassertion time

Nit-pick, looks like you are mixing GPMC_CS_CONFIGx and GPMC_CONFIGx
naming conventions in the above and below. Would be good to make this
consistent.

> +
> + WE signals timings corresponding to GPMC_CONFIG4:
> + - gpmc,we-on:		Assertion time
> + - gpmc,we-off:		Deassertion time
> +
> + OE signals timings corresponding to GPMC_CONFIG4
> + - gpmc,oe-on:		Assertion time
> + - gpmc,oe-off:		Deassertion time
> +
> + Access time and cycle time timings corresponding to GPMC_CONFIG5
> + - gpmc,page-burst-access: Multiple access word delay
> + - gpmc,access:		Start-cycle to first data valid delay
> + - gpmc,rd-cycle:	Total read cycle time
> + - gpmc,wr-cycle:	Total write cycle time
> +
> +The following are only on OMAP3430
> + - gpmc,wr-access
> + - gpmc,wr-data-mux-bus
> +
> +
> +Example for an AM33xx board:
> +
> +	gpmc: gpmc at 50000000 {
> +		compatible = "ti,gpmc";
> +		ti,hwmods = "gpmc";
> +		reg = <0x50000000 0x2000>;
> +		interrupt-parent = <&intc>;

We should drop interrupt-parent. We are declaring the interrupt-parent
globally in the dts files and so no need to replicate in each individual
binding.

> +		interrupts = <100>;
> +
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0x08000000 0x10000000>; /* CS0 */
> +
> +		/* child nodes go here */
> +	};
> +
> +
> +
> +
> +
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> new file mode 100644
> index 0000000..e0c1e67
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> @@ -0,0 +1,61 @@
> +Device tree bindings for GPMC connected NANDs
> +
> +GPMC connected NAND (found on OMAP boards) are represented as child nodes of
> +the GPMC controller with a name of "nand".
> +
> +All timing relevant properties as well as generic gpmc child properties are
> +explained in a separate documents - please refer to
> +Documentation/devicetree/bindings/bus/ti-gpmc.txt
> +
> +For NAND specific properties such as ECC modes or bus width, please refer to
> +Documentation/devicetree/bindings/mtd/nand.txt
> +
> +
> +Required properties:
> +
> + - reg: The CS line the peripheral is connected to
> +
> +For inline partiton table parsing (optional):
> +
> + - #address-cells: should be set to 1
> + - #size-cells: should be set to 1
> +
> +Example for an AM33xx board:
> +
> +	gpmc: gpmc at 50000000 {
> +		compatible = "ti,gpmc";
> +		ti,hwmods = "gpmc";
> +		reg = <0x50000000 0x1000000>;
> +		interrupt-parent = <&intc>;

Remove interrupt-parent here too.

> +		interrupts = <100>;
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0x08000000 0x10000000>;	/* CS0: NAND */
> +
> +		nand at 0,0 {
> +			reg = <0 0 0>; /* CS0, offset 0 */

The above description says that this is just the chip-select number. Are
the other fields used here? If so, what for?

> +			nand-bus-width = <16>;
> +			nand-ecc-mode = "none";

I am still wondering if the above needs to be mandatory. Or if not then
may be these should be documented as optional and if these a omitted
then what the default configuration would be.

> +
> +			gpmc,sync-clk = <0>;
> +			gpmc,cs-on = <0>;
> +			gpmc,cs-rd-off = <36>;
> +			gpmc,cs-wr-off = <36>;
> +			gpmc,adv-on = <6>;
> +			gpmc,adv-rd-off = <24>;
> +			gpmc,adv-wr-off = <36>;
> +			gpmc,we-off = <30>;
> +			gpmc,oe-off = <48>;
> +			gpmc,access = <54>;
> +			gpmc,rd-cycle = <72>;
> +			gpmc,wr-cycle = <72>;
> +			gpmc,wr-access = <30>;
> +			gpmc,wr-data-mux-bus = <0>;
> +
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			/* partitions go here */
> +		};
> +	};
> +
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 1dcb30c..b028225 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -25,6 +25,10 @@
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
>  #include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_mtd.h>
> +#include <linux/of_device.h>
> +#include <linux/mtd/nand.h>
>  
>  #include <linux/platform_data/mtd-nand-omap2.h>
>  
> @@ -37,6 +41,7 @@
>  #include "soc.h"
>  #include "common.h"
>  #include "gpmc.h"
> +#include "gpmc-nand.h"
>  
>  #define	DEVICE_NAME		"omap-gpmc"
>  
> @@ -751,6 +756,131 @@ static int __devinit gpmc_mem_init(void)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static struct of_device_id gpmc_dt_ids[] = {
> +	{ .compatible = "ti,gpmc" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, gpmc_dt_ids);
> +
> +static void gpmc_read_timings_dt(struct device_node *np,
> +				 struct gpmc_timings *gpmc_t)
> +{
> +	u32 val;
> +
> +	memset(gpmc_t, 0, sizeof(*gpmc_t));
> +
> +	/* minimum clock period for syncronous mode */
> +	if (!of_property_read_u32(np, "gpmc,sync-clk", &val))
> +		gpmc_t->sync_clk = val;
> +
> +	/* chip select timtings */
> +	if (!of_property_read_u32(np, "gpmc,cs-on", &val))
> +		gpmc_t->cs_on = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,cs-rd-off", &val))
> +		gpmc_t->cs_rd_off = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,cs-wr-off", &val))
> +		gpmc_t->cs_wr_off = val;
> +
> +	/* ADV signal timings */
> +	if (!of_property_read_u32(np, "gpmc,adv-on", &val))
> +		gpmc_t->adv_on = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,adv-rd-off", &val))
> +		gpmc_t->adv_rd_off = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,adv-wr-off", &val))
> +		gpmc_t->adv_wr_off = val;
> +
> +	/* WE signal timings */
> +	if (!of_property_read_u32(np, "gpmc,we-on", &val))
> +		gpmc_t->we_on = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,we-off", &val))
> +		gpmc_t->we_off = val;
> +
> +	/* OE signal timings */
> +	if (!of_property_read_u32(np, "gpmc,oe-on", &val))
> +		gpmc_t->oe_on = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,oe-off", &val))
> +		gpmc_t->oe_off = val;
> +
> +	/* access and cycle timings */
> +	if (!of_property_read_u32(np, "gpmc,page-burst-access", &val))
> +		gpmc_t->page_burst_access = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,access", &val))
> +		gpmc_t->access = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,rd-cycle", &val))
> +		gpmc_t->rd_cycle = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,wr-cycle", &val))
> +		gpmc_t->wr_cycle = val;
> +
> +	/* only for OMAP3430 */
> +	if (!of_property_read_u32(np, "gpmc,wr-access", &val))
> +		gpmc_t->wr_access = val;
> +
> +	if (!of_property_read_u32(np, "gpmc,wr-data-mux-bus", &val))
> +		gpmc_t->wr_data_mux_bus = val;
> +}
> +
> +static int gpmc_probe_dt(struct platform_device *pdev)
> +{
> +	u32 val;
> +	struct device_node *child;
> +	struct gpmc_timings gpmc_t;
> +	const struct of_device_id *of_id =
> +		of_match_device(gpmc_dt_ids, &pdev->dev);
> +
> +	if (!of_id)
> +		return 0;
> +
> +	for_each_node_by_name(child, "nand") {
> +		struct omap_nand_platform_data *gpmc_nand_data;
> +
> +		if (of_property_read_u32(child, "reg", &val) < 0) {
> +			dev_err(&pdev->dev, "%s has no 'reg' property\n",
> +				child->full_name);
> +			continue;
> +		}
> +
> +		gpmc_nand_data = devm_kzalloc(&pdev->dev,
> +					      sizeof(*gpmc_nand_data),
> +					      GFP_KERNEL);
> +		if (!gpmc_nand_data) {
> +			dev_err(&pdev->dev, "unable to allocate memory?");
> +			return -ENOMEM;
> +		}
> +
> +		gpmc_nand_data->cs = val;
> +		gpmc_nand_data->of_node = child;
> +
> +		val = of_get_nand_ecc_mode(child);
> +		if (val >= 0)
> +			gpmc_nand_data->ecc_opt = val;
> +
> +		val = of_get_nand_bus_width(child);
> +		if (val == 16)
> +			gpmc_nand_data->devsize = NAND_BUSWIDTH_16;

Do we need any error checking here? I believe we only support 8-bit and
16-bit width devices and so if 16-bit is not set then we default to 8-bit.

> +
> +		gpmc_read_timings_dt(child, &gpmc_t);
> +		gpmc_nand_init(gpmc_nand_data, &gpmc_t);

I believe that you need an "of_node_put()" when you are done with the child.

Thanks
Jon

[PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

Hi Jon,

as all comments so far focussed on patch 4/4, could we agree to merge
1-3 of this series already? These are all small and straight-forward
things that don't depend on 4/4. That way, I only need to resend the
last one under discussion.

On 02.11.2012 11:41, Jon Hunter wrote:
> Hi Daniel,
> 
> On 11/01/2012 01:36 PM, Daniel Mack wrote:
>> This patch adds basic DT bindings for OMAP GPMC.
>>
>> The actual peripherals are instanciated from child nodes within the GPMC
>> node, and the only type of device that is currently supported is NAND.
>>
>> Code was added to parse the generic GPMC timing parameters and some
>> documentation with examples on how to use them.
>>
>> Successfully tested on an AM33xx board.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/bus/ti-gpmc.txt  |  73 +++++++++++
>>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  61 +++++++++
>>  arch/arm/mach-omap2/gpmc.c                         | 139 +++++++++++++++++++++
>>  3 files changed, 273 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>>
>> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>> new file mode 100644
>> index 0000000..6f44487
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>> @@ -0,0 +1,73 @@
>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>> +
>> +The actual devices are instantiated from the child nodes of a GPMC node.
>> +
>> +Required properties:
>> +
>> + - compatible:		Should be set to "ti,gpmc"
>> + - reg:			A resource specifier for the register space
>> +			(see the example below)
>> + - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
>> +			completed.
>> + - #address-cells:	Must be set to 2 to allow memory address translation
>> + - #size-cells:		Must be set to 1 to allow CS address passing
>> + - ranges:		Must be set up to reflect the memory layout
>> +			Note that this property is not currently parsed.
>> +			Calculated values derived from the contents of
>> +			GPMC_CS_CONFIG7 as set up by the bootloader. That will
>> +			change in the future, so be sure to fill the correct
>> +			values here.
> 
> I still think it would be good to add number of chip-selects and
> wait-pins here.

The number of chip-selects can be derived from the ranges property.
Namely, each 4-value entry to this property maps to one chip-select. I
can try and make the more clear in the documentation.

>> +Timing properties for child nodes. All are optional and default to 0.
>> +
>> + - gpmc,sync-clk:	Minimum clock period for synchronous mode, in picoseconds
>> +
>> + Chip-select signal timings corresponding to GPMC_CS_CONFIG2:
>> + - gpmc,cs-on:		Assertion time
>> + - gpmc,cs-rd-off:	Read deassertion time
>> + - gpmc,cs-wr-off:	Write deassertion time
>> +
>> + ADV signal timings corresponding to GPMC_CONFIG3:
>> + - gpmc,adv-on:		Assertion time
>> + - gpmc,adv-rd-off:	Read deassertion time
>> + - gpmc,adv-wr-off:	Write deassertion time
> 
> Nit-pick, looks like you are mixing GPMC_CS_CONFIGx and GPMC_CONFIGx
> naming conventions in the above and below. Would be good to make this
> consistent.

Ok, but these were just copied from arch/arm/mach-omap2/gpmc.h. So it's
wrong there, too.

>> + WE signals timings corresponding to GPMC_CONFIG4:
>> + - gpmc,we-on:		Assertion time
>> + - gpmc,we-off:		Deassertion time
>> +
>> + OE signals timings corresponding to GPMC_CONFIG4
>> + - gpmc,oe-on:		Assertion time
>> + - gpmc,oe-off:		Deassertion time
>> +
>> + Access time and cycle time timings corresponding to GPMC_CONFIG5
>> + - gpmc,page-burst-access: Multiple access word delay
>> + - gpmc,access:		Start-cycle to first data valid delay
>> + - gpmc,rd-cycle:	Total read cycle time
>> + - gpmc,wr-cycle:	Total write cycle time
>> +
>> +The following are only on OMAP3430
>> + - gpmc,wr-access
>> + - gpmc,wr-data-mux-bus
>> +
>> +
>> +Example for an AM33xx board:
>> +
>> +	gpmc: gpmc at 50000000 {
>> +		compatible = "ti,gpmc";
>> +		ti,hwmods = "gpmc";
>> +		reg = <0x50000000 0x2000>;
>> +		interrupt-parent = <&intc>;
> 
> We should drop interrupt-parent. We are declaring the interrupt-parent
> globally in the dts files and so no need to replicate in each individual
> binding.

Right, will remove.

>> +		interrupts = <100>;
>> +		#address-cells = <2>;
>> +		#size-cells = <1>;
>> +		ranges = <0 0 0x08000000 0x10000000>;	/* CS0: NAND */
>> +
>> +		nand at 0,0 {
>> +			reg = <0 0 0>; /* CS0, offset 0 */
> 
> The above description says that this is just the chip-select number. Are
> the other fields used here? If so, what for?

Thanks to the translation done by the 'ranges' mechanism, these just
denote the offset to the address range specified above. So they can be
left 0.

>> +			nand-bus-width = <16>;
>> +			nand-ecc-mode = "none";
> 
> I am still wondering if the above needs to be mandatory. Or if not then
> may be these should be documented as optional and if these a omitted
> then what the default configuration would be.

In my docs, I referred to Documentation/devicetree/bindings/mtd/nand.txt
which states:

- nand-ecc-mode : String, operation mode of the NAND ecc mode.
  Supported values are: "none", "soft", "hw", "hw_syndrome",
"hw_oob_first", "soft_bch".
- nand-bus-width : 8 or 16 bus width if not present 8

So ecc-mode is mandatory, even though the code currently really defaults
to 0 ("none"). nand-bus-width isn't. I don't know if it makes sense to
duplicate the Documentation here.

>> +static int gpmc_probe_dt(struct platform_device *pdev)
>> +{
>> +	u32 val;
>> +	struct device_node *child;
>> +	struct gpmc_timings gpmc_t;
>> +	const struct of_device_id *of_id =
>> +		of_match_device(gpmc_dt_ids, &pdev->dev);
>> +
>> +	if (!of_id)
>> +		return 0;
>> +
>> +	for_each_node_by_name(child, "nand") {
>> +		struct omap_nand_platform_data *gpmc_nand_data;
>> +
>> +		if (of_property_read_u32(child, "reg", &val) < 0) {
>> +			dev_err(&pdev->dev, "%s has no 'reg' property\n",
>> +				child->full_name);
>> +			continue;
>> +		}
>> +
>> +		gpmc_nand_data = devm_kzalloc(&pdev->dev,
>> +					      sizeof(*gpmc_nand_data),
>> +					      GFP_KERNEL);
>> +		if (!gpmc_nand_data) {
>> +			dev_err(&pdev->dev, "unable to allocate memory?");
>> +			return -ENOMEM;
>> +		}
>> +
>> +		gpmc_nand_data->cs = val;
>> +		gpmc_nand_data->of_node = child;
>> +
>> +		val = of_get_nand_ecc_mode(child);
>> +		if (val >= 0)
>> +			gpmc_nand_data->ecc_opt = val;
>> +
>> +		val = of_get_nand_bus_width(child);
>> +		if (val == 16)
>> +			gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
> 
> Do we need any error checking here? I believe we only support 8-bit and
> 16-bit width devices and so if 16-bit is not set then we default to 8-bit.

Yes, that's that the code does. If val != 16, ->devsize is left set to 0
(from kzalloc). There is no NAND_BUSWIDTH_8, so I think that should be ok?

>> +
>> +		gpmc_read_timings_dt(child, &gpmc_t);
>> +		gpmc_nand_init(gpmc_nand_data, &gpmc_t);
> 
> I believe that you need an "of_node_put()" when you are done with the child.

Good point.


Thanks,
Daniel

[PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Jon Hunter]

On 11/02/2012 06:14 AM, Daniel Mack wrote:
> Hi Jon,
> 
> as all comments so far focussed on patch 4/4, could we agree to merge
> 1-3 of this series already? These are all small and straight-forward
> things that don't depend on 4/4. That way, I only need to resend the
> last one under discussion.

Not sure it makes sense to take 3 without 4.

> On 02.11.2012 11:41, Jon Hunter wrote:
>> Hi Daniel,
>>
>> On 11/01/2012 01:36 PM, Daniel Mack wrote:
>>> This patch adds basic DT bindings for OMAP GPMC.
>>>
>>> The actual peripherals are instanciated from child nodes within the GPMC
>>> node, and the only type of device that is currently supported is NAND.
>>>
>>> Code was added to parse the generic GPMC timing parameters and some
>>> documentation with examples on how to use them.
>>>
>>> Successfully tested on an AM33xx board.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/bus/ti-gpmc.txt  |  73 +++++++++++
>>>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  61 +++++++++
>>>  arch/arm/mach-omap2/gpmc.c                         | 139 +++++++++++++++++++++
>>>  3 files changed, 273 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>> new file mode 100644
>>> index 0000000..6f44487
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>> @@ -0,0 +1,73 @@
>>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>>> +
>>> +The actual devices are instantiated from the child nodes of a GPMC node.
>>> +
>>> +Required properties:
>>> +
>>> + - compatible:		Should be set to "ti,gpmc"
>>> + - reg:			A resource specifier for the register space
>>> +			(see the example below)
>>> + - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
>>> +			completed.
>>> + - #address-cells:	Must be set to 2 to allow memory address translation
>>> + - #size-cells:		Must be set to 1 to allow CS address passing
>>> + - ranges:		Must be set up to reflect the memory layout
>>> +			Note that this property is not currently parsed.
>>> +			Calculated values derived from the contents of
>>> +			GPMC_CS_CONFIG7 as set up by the bootloader. That will
>>> +			change in the future, so be sure to fill the correct
>>> +			values here.
>>
>> I still think it would be good to add number of chip-selects and
>> wait-pins here.
> 
> The number of chip-selects can be derived from the ranges property.
> Namely, each 4-value entry to this property maps to one chip-select. I
> can try and make the more clear in the documentation.

Yes but that only tells you how many you are using. The binding should
describe the hardware and so should tell us how many chip-selects we
have. We should get away from using GPMC_CS_NUM in the code.

What about wait-pins?

>>> +Timing properties for child nodes. All are optional and default to 0.
>>> +
>>> + - gpmc,sync-clk:	Minimum clock period for synchronous mode, in picoseconds
>>> +
>>> + Chip-select signal timings corresponding to GPMC_CS_CONFIG2:
>>> + - gpmc,cs-on:		Assertion time
>>> + - gpmc,cs-rd-off:	Read deassertion time
>>> + - gpmc,cs-wr-off:	Write deassertion time
>>> +
>>> + ADV signal timings corresponding to GPMC_CONFIG3:
>>> + - gpmc,adv-on:		Assertion time
>>> + - gpmc,adv-rd-off:	Read deassertion time
>>> + - gpmc,adv-wr-off:	Write deassertion time
>>
>> Nit-pick, looks like you are mixing GPMC_CS_CONFIGx and GPMC_CONFIGx
>> naming conventions in the above and below. Would be good to make this
>> consistent.
> 
> Ok, but these were just copied from arch/arm/mach-omap2/gpmc.h. So it's
> wrong there, too.

Please feel free to submit a patch.

>>> + WE signals timings corresponding to GPMC_CONFIG4:
>>> + - gpmc,we-on:		Assertion time
>>> + - gpmc,we-off:		Deassertion time
>>> +
>>> + OE signals timings corresponding to GPMC_CONFIG4
>>> + - gpmc,oe-on:		Assertion time
>>> + - gpmc,oe-off:		Deassertion time
>>> +
>>> + Access time and cycle time timings corresponding to GPMC_CONFIG5
>>> + - gpmc,page-burst-access: Multiple access word delay
>>> + - gpmc,access:		Start-cycle to first data valid delay
>>> + - gpmc,rd-cycle:	Total read cycle time
>>> + - gpmc,wr-cycle:	Total write cycle time
>>> +
>>> +The following are only on OMAP3430
>>> + - gpmc,wr-access
>>> + - gpmc,wr-data-mux-bus
>>> +
>>> +
>>> +Example for an AM33xx board:
>>> +
>>> +	gpmc: gpmc at 50000000 {
>>> +		compatible = "ti,gpmc";
>>> +		ti,hwmods = "gpmc";
>>> +		reg = <0x50000000 0x2000>;
>>> +		interrupt-parent = <&intc>;
>>
>> We should drop interrupt-parent. We are declaring the interrupt-parent
>> globally in the dts files and so no need to replicate in each individual
>> binding.
> 
> Right, will remove.

Thanks.

>>> +		interrupts = <100>;
>>> +		#address-cells = <2>;
>>> +		#size-cells = <1>;
>>> +		ranges = <0 0 0x08000000 0x10000000>;	/* CS0: NAND */
>>> +
>>> +		nand at 0,0 {
>>> +			reg = <0 0 0>; /* CS0, offset 0 */
>>
>> The above description says that this is just the chip-select number. Are
>> the other fields used here? If so, what for?
> 
> Thanks to the translation done by the 'ranges' mechanism, these just
> denote the offset to the address range specified above. So they can be
> left 0.
>>> +			nand-bus-width = <16>;
>>> +			nand-ecc-mode = "none";
>>
>> I am still wondering if the above needs to be mandatory. Or if not then
>> may be these should be documented as optional and if these a omitted
>> then what the default configuration would be.
> 
> In my docs, I referred to Documentation/devicetree/bindings/mtd/nand.txt
> which states:
> 
> - nand-ecc-mode : String, operation mode of the NAND ecc mode.
>   Supported values are: "none", "soft", "hw", "hw_syndrome",
> "hw_oob_first", "soft_bch".
> - nand-bus-width : 8 or 16 bus width if not present 8
> 
> So ecc-mode is mandatory, even though the code currently really defaults
> to 0 ("none"). nand-bus-width isn't. I don't know if it makes sense to
> duplicate the Documentation here.

Well maybe there should be some reference?

>>> +static int gpmc_probe_dt(struct platform_device *pdev)
>>> +{
>>> +	u32 val;
>>> +	struct device_node *child;
>>> +	struct gpmc_timings gpmc_t;
>>> +	const struct of_device_id *of_id =
>>> +		of_match_device(gpmc_dt_ids, &pdev->dev);
>>> +
>>> +	if (!of_id)
>>> +		return 0;
>>> +
>>> +	for_each_node_by_name(child, "nand") {
>>> +		struct omap_nand_platform_data *gpmc_nand_data;
>>> +
>>> +		if (of_property_read_u32(child, "reg", &val) < 0) {
>>> +			dev_err(&pdev->dev, "%s has no 'reg' property\n",
>>> +				child->full_name);
>>> +			continue;
>>> +		}
>>> +
>>> +		gpmc_nand_data = devm_kzalloc(&pdev->dev,
>>> +					      sizeof(*gpmc_nand_data),
>>> +					      GFP_KERNEL);
>>> +		if (!gpmc_nand_data) {
>>> +			dev_err(&pdev->dev, "unable to allocate memory?");
>>> +			return -ENOMEM;
>>> +		}
>>> +
>>> +		gpmc_nand_data->cs = val;
>>> +		gpmc_nand_data->of_node = child;
>>> +
>>> +		val = of_get_nand_ecc_mode(child);
>>> +		if (val >= 0)
>>> +			gpmc_nand_data->ecc_opt = val;
>>> +
>>> +		val = of_get_nand_bus_width(child);
>>> +		if (val == 16)
>>> +			gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
>>
>> Do we need any error checking here? I believe we only support 8-bit and
>> 16-bit width devices and so if 16-bit is not set then we default to 8-bit.
> 
> Yes, that's that the code does. If val != 16, ->devsize is left set to 0
> (from kzalloc). There is no NAND_BUSWIDTH_8, so I think that should be ok?

Ok.

>>> +
>>> +		gpmc_read_timings_dt(child, &gpmc_t);
>>> +		gpmc_nand_init(gpmc_nand_data, &gpmc_t);
>>
>> I believe that you need an "of_node_put()" when you are done with the child.
> 
> Good point.

No problem.

Cheers
Jon

[PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

Hi Jon,

On 02.11.2012 20:18, Jon Hunter wrote:
> On 11/02/2012 06:14 AM, Daniel Mack wrote:
>> Hi Jon,
>>
>> as all comments so far focussed on patch 4/4, could we agree to merge
>> 1-3 of this series already? These are all small and straight-forward
>> things that don't depend on 4/4. That way, I only need to resend the
>> last one under discussion.
> 
> Not sure it makes sense to take 3 without 4.

Ok, no problem. I already submitted v3 :)

>>>>  Documentation/devicetree/bindings/bus/ti-gpmc.txt  |  73 +++++++++++
>>>>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  61 +++++++++
>>>>  arch/arm/mach-omap2/gpmc.c                         | 139 +++++++++++++++++++++
>>>>  3 files changed, 273 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>> new file mode 100644
>>>> index 0000000..6f44487
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>> @@ -0,0 +1,73 @@
>>>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>>>> +
>>>> +The actual devices are instantiated from the child nodes of a GPMC node.
>>>> +
>>>> +Required properties:
>>>> +
>>>> + - compatible:		Should be set to "ti,gpmc"
>>>> + - reg:			A resource specifier for the register space
>>>> +			(see the example below)
>>>> + - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
>>>> +			completed.
>>>> + - #address-cells:	Must be set to 2 to allow memory address translation
>>>> + - #size-cells:		Must be set to 1 to allow CS address passing
>>>> + - ranges:		Must be set up to reflect the memory layout
>>>> +			Note that this property is not currently parsed.
>>>> +			Calculated values derived from the contents of
>>>> +			GPMC_CS_CONFIG7 as set up by the bootloader. That will
>>>> +			change in the future, so be sure to fill the correct
>>>> +			values here.
>>>
>>> I still think it would be good to add number of chip-selects and
>>> wait-pins here.
>>
>> The number of chip-selects can be derived from the ranges property.
>> Namely, each 4-value entry to this property maps to one chip-select. I
>> can try and make the more clear in the documentation.
> 
> Yes but that only tells you how many you are using. The binding should
> describe the hardware and so should tell us how many chip-selects we
> have. We should get away from using GPMC_CS_NUM in the code.

Maybe I don't get your point, but we only need to care for as many cs
lines as we actually use, right?

> What about wait-pins?

Afaik, their use depends on the driver acting as GPMC client, right?
Could you point me to code that acts conditionally and that should be
reflected in DT?

>>> I am still wondering if the above needs to be mandatory. Or if not then
>>> may be these should be documented as optional and if these a omitted
>>> then what the default configuration would be.
>>
>> In my docs, I referred to Documentation/devicetree/bindings/mtd/nand.txt
>> which states:
>>
>> - nand-ecc-mode : String, operation mode of the NAND ecc mode.
>>   Supported values are: "none", "soft", "hw", "hw_syndrome",
>> "hw_oob_first", "soft_bch".
>> - nand-bus-width : 8 or 16 bus width if not present 8
>>
>> So ecc-mode is mandatory, even though the code currently really defaults
>> to 0 ("none"). nand-bus-width isn't. I don't know if it makes sense to
>> duplicate the Documentation here.
> 
> Well maybe there should be some reference?

Well, it's there already, that what I'm saying :)

Quoting Documentation/devicetree/bindings/mtd/gpmc-nand.txt:

	For NAND specific properties such as ECC modes or bus width,
	please refer to Documentation/devicetree/bindings/mtd/nand.txt


Thanks for your review,

Daniel

[PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Jon Hunter]

On 11/02/2012 02:23 PM, Daniel Mack wrote:
> Hi Jon,
> 
> On 02.11.2012 20:18, Jon Hunter wrote:
>> On 11/02/2012 06:14 AM, Daniel Mack wrote:
>>> Hi Jon,
>>>
>>> as all comments so far focussed on patch 4/4, could we agree to merge
>>> 1-3 of this series already? These are all small and straight-forward
>>> things that don't depend on 4/4. That way, I only need to resend the
>>> last one under discussion.
>>
>> Not sure it makes sense to take 3 without 4.
> 
> Ok, no problem. I already submitted v3 :)
> 
>>>>>  Documentation/devicetree/bindings/bus/ti-gpmc.txt  |  73 +++++++++++
>>>>>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  61 +++++++++
>>>>>  arch/arm/mach-omap2/gpmc.c                         | 139 +++++++++++++++++++++
>>>>>  3 files changed, 273 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>> new file mode 100644
>>>>> index 0000000..6f44487
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>> @@ -0,0 +1,73 @@
>>>>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>>>>> +
>>>>> +The actual devices are instantiated from the child nodes of a GPMC node.
>>>>> +
>>>>> +Required properties:
>>>>> +
>>>>> + - compatible:		Should be set to "ti,gpmc"
>>>>> + - reg:			A resource specifier for the register space
>>>>> +			(see the example below)
>>>>> + - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
>>>>> +			completed.
>>>>> + - #address-cells:	Must be set to 2 to allow memory address translation
>>>>> + - #size-cells:		Must be set to 1 to allow CS address passing
>>>>> + - ranges:		Must be set up to reflect the memory layout
>>>>> +			Note that this property is not currently parsed.
>>>>> +			Calculated values derived from the contents of
>>>>> +			GPMC_CS_CONFIG7 as set up by the bootloader. That will
>>>>> +			change in the future, so be sure to fill the correct
>>>>> +			values here.
>>>>
>>>> I still think it would be good to add number of chip-selects and
>>>> wait-pins here.
>>>
>>> The number of chip-selects can be derived from the ranges property.
>>> Namely, each 4-value entry to this property maps to one chip-select. I
>>> can try and make the more clear in the documentation.
>>
>> Yes but that only tells you how many you are using. The binding should
>> describe the hardware and so should tell us how many chip-selects we
>> have. We should get away from using GPMC_CS_NUM in the code.
> 
> Maybe I don't get your point, but we only need to care for as many cs
> lines as we actually use, right?

But how many does your device have? How many clients can you support?

If we know how many the device has and then we can get rid of "#define
GPMC_CS_NUM". We currently allocate the CS by calling gpmc_cs_request().
Hmmm ... I now see that your patch is not calling this before
configuring the CS and so that needs to be fixed too.

Without knowing the total CS available, how do we ensure we have the CS
available that someone is asking for?

>> What about wait-pins?
> 
> Afaik, their use depends on the driver acting as GPMC client, right?
> Could you point me to code that acts conditionally and that should be
> reflected in DT?

Again we need to know how many the device has. Clients may or may not
use these. However, if a client wants one they need to request one which
is just like a chip-select. This is not in the current driver but Afzal
has a patch for this [1].

Bottom line, for such hardware specific features, device tree is a good
place to describe how many resources we have. Then we can eliminate such
#defines from the driver code.

>>>> I am still wondering if the above needs to be mandatory. Or if not then
>>>> may be these should be documented as optional and if these a omitted
>>>> then what the default configuration would be.
>>>
>>> In my docs, I referred to Documentation/devicetree/bindings/mtd/nand.txt
>>> which states:
>>>
>>> - nand-ecc-mode : String, operation mode of the NAND ecc mode.
>>>   Supported values are: "none", "soft", "hw", "hw_syndrome",
>>> "hw_oob_first", "soft_bch".
>>> - nand-bus-width : 8 or 16 bus width if not present 8
>>>
>>> So ecc-mode is mandatory, even though the code currently really defaults
>>> to 0 ("none"). nand-bus-width isn't. I don't know if it makes sense to
>>> duplicate the Documentation here.
>>
>> Well maybe there should be some reference?
> 
> Well, it's there already, that what I'm saying :)
> 
> Quoting Documentation/devicetree/bindings/mtd/gpmc-nand.txt:
> 
> 	For NAND specific properties such as ECC modes or bus width,
> 	please refer to Documentation/devicetree/bindings/mtd/nand.txt

Ok, thanks I see that now. Looking at other bindings, some also include
these details but not all. Could be worth listing ecc-mode under
mandatory and bus-width under optional with a reference to nand.txt
binding. I don't think it is worth duplicating but listing the actual
property names would be nice.

Cheers
Jon

[1]
http://gitorious.org/x0148406-public/linux-kernel/commit/f7cbee399ece42e64f9ad2205171c3c67c3f1a9e/diffs

[PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

On 02.11.2012 20:57, Jon Hunter wrote:
> On 11/02/2012 02:23 PM, Daniel Mack wrote:
>> On 02.11.2012 20:18, Jon Hunter wrote:
>>> On 11/02/2012 06:14 AM, Daniel Mack wrote:

>>>>>> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..6f44487
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>>> @@ -0,0 +1,73 @@
>>>>>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>>>>>> +
>>>>>> +The actual devices are instantiated from the child nodes of a GPMC node.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +
>>>>>> + - compatible:		Should be set to "ti,gpmc"
>>>>>> + - reg:			A resource specifier for the register space
>>>>>> +			(see the example below)
>>>>>> + - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
>>>>>> +			completed.
>>>>>> + - #address-cells:	Must be set to 2 to allow memory address translation
>>>>>> + - #size-cells:		Must be set to 1 to allow CS address passing
>>>>>> + - ranges:		Must be set up to reflect the memory layout
>>>>>> +			Note that this property is not currently parsed.
>>>>>> +			Calculated values derived from the contents of
>>>>>> +			GPMC_CS_CONFIG7 as set up by the bootloader. That will
>>>>>> +			change in the future, so be sure to fill the correct
>>>>>> +			values here.
>>>>>
>>>>> I still think it would be good to add number of chip-selects and
>>>>> wait-pins here.
>>>>
>>>> The number of chip-selects can be derived from the ranges property.
>>>> Namely, each 4-value entry to this property maps to one chip-select. I
>>>> can try and make the more clear in the documentation.
>>>
>>> Yes but that only tells you how many you are using. The binding should
>>> describe the hardware and so should tell us how many chip-selects we
>>> have. We should get away from using GPMC_CS_NUM in the code.
>>
>> Maybe I don't get your point, but we only need to care for as many cs
>> lines as we actually use, right?
> 
> But how many does your device have? How many clients can you support?

Well, you state that in the ranges property. Even if the chip could in
theory support 8 CS lines - if the actual setup only uses the first one
of them, the code would only need to allocate and set up the one that is
in use. And as the entries in "ranges" are mandatory, there can actually
be no mis-allocation.

I can still add the maximum number as a separate property, but I wanted
to outline my idea here. Is "num-cs" a good name for the property?

> If we know how many the device has and then we can get rid of "#define
> GPMC_CS_NUM". We currently allocate the CS by calling gpmc_cs_request().
> Hmmm ... I now see that your patch is not calling this before
> configuring the CS and so that needs to be fixed too.

It does implicitly, by calling gpmc_nand_init().

> Without knowing the total CS available, how do we ensure we have the CS
> available that someone is asking for?
> 
>>> What about wait-pins?
>>
>> Afaik, their use depends on the driver acting as GPMC client, right?
>> Could you point me to code that acts conditionally and that should be
>> reflected in DT?
> 
> Again we need to know how many the device has. Clients may or may not
> use these. However, if a client wants one they need to request one which
> is just like a chip-select. This is not in the current driver but Afzal
> has a patch for this [1].

Ah, thanks for the pointer to the patch. Ok, I'll add a "num-waitpins"
property. Does that name sound appropriate?

> Bottom line, for such hardware specific features, device tree is a good
> place to describe how many resources we have. Then we can eliminate such
> #defines from the driver code.

Agreed.

>> Quoting Documentation/devicetree/bindings/mtd/gpmc-nand.txt:
>>
>> 	For NAND specific properties such as ECC modes or bus width,
>> 	please refer to Documentation/devicetree/bindings/mtd/nand.txt
> 
> Ok, thanks I see that now. Looking at other bindings, some also include
> these details but not all. Could be worth listing ecc-mode under
> mandatory and bus-width under optional with a reference to nand.txt
> binding. I don't think it is worth duplicating but listing the actual
> property names would be nice.

Ok, I amended my local version. With the details above sorted out and
"num-cs" and "num-waitpins" in place, do you think we're ready for v4?


Thanks,
Daniel

[PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Jon Hunter]

On 11/02/2012 03:23 PM, Daniel Mack wrote:
> On 02.11.2012 20:57, Jon Hunter wrote:
>> On 11/02/2012 02:23 PM, Daniel Mack wrote:
>>> On 02.11.2012 20:18, Jon Hunter wrote:
>>>> On 11/02/2012 06:14 AM, Daniel Mack wrote:
> 
>>>>>>> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..6f44487
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>>>> @@ -0,0 +1,73 @@
>>>>>>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>>>>>>> +
>>>>>>> +The actual devices are instantiated from the child nodes of a GPMC node.
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +
>>>>>>> + - compatible:		Should be set to "ti,gpmc"
>>>>>>> + - reg:			A resource specifier for the register space
>>>>>>> +			(see the example below)
>>>>>>> + - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
>>>>>>> +			completed.
>>>>>>> + - #address-cells:	Must be set to 2 to allow memory address translation
>>>>>>> + - #size-cells:		Must be set to 1 to allow CS address passing
>>>>>>> + - ranges:		Must be set up to reflect the memory layout
>>>>>>> +			Note that this property is not currently parsed.
>>>>>>> +			Calculated values derived from the contents of
>>>>>>> +			GPMC_CS_CONFIG7 as set up by the bootloader. That will
>>>>>>> +			change in the future, so be sure to fill the correct
>>>>>>> +			values here.
>>>>>>
>>>>>> I still think it would be good to add number of chip-selects and
>>>>>> wait-pins here.
>>>>>
>>>>> The number of chip-selects can be derived from the ranges property.
>>>>> Namely, each 4-value entry to this property maps to one chip-select. I
>>>>> can try and make the more clear in the documentation.
>>>>
>>>> Yes but that only tells you how many you are using. The binding should
>>>> describe the hardware and so should tell us how many chip-selects we
>>>> have. We should get away from using GPMC_CS_NUM in the code.
>>>
>>> Maybe I don't get your point, but we only need to care for as many cs
>>> lines as we actually use, right?
>>
>> But how many does your device have? How many clients can you support?
> 
> Well, you state that in the ranges property. Even if the chip could in
> theory support 8 CS lines - if the actual setup only uses the first one
> of them, the code would only need to allocate and set up the one that is
> in use. And as the entries in "ranges" are mandatory, there can actually
> be no mis-allocation.

Ah, I see your point now. Well typically, we have been putting the
device-level peripheral info in the device's *.dtsi (ie. am33xx.dtsi)
and then board specific stuff in the board *.dts file (am335x-bone.dts).
So I would envision that the device-level info (reg, ti,hwmods,
interrupt, num-cs) be in am33xx.dtsi and ranges be in am335x.dts. So it
would still be nice to catch any badly configured ranges property in the
driver by querying in the number of chip-selects.

> I can still add the maximum number as a separate property, but I wanted
> to outline my idea here. Is "num-cs" a good name for the property?

Sounds good.

>> If we know how many the device has and then we can get rid of "#define
>> GPMC_CS_NUM". We currently allocate the CS by calling gpmc_cs_request().
>> Hmmm ... I now see that your patch is not calling this before
>> configuring the CS and so that needs to be fixed too.
> 
> It does implicitly, by calling gpmc_nand_init().

Yes, you are right!

>> Without knowing the total CS available, how do we ensure we have the CS
>> available that someone is asking for?
>>
>>>> What about wait-pins?
>>>
>>> Afaik, their use depends on the driver acting as GPMC client, right?
>>> Could you point me to code that acts conditionally and that should be
>>> reflected in DT?
>>
>> Again we need to know how many the device has. Clients may or may not
>> use these. However, if a client wants one they need to request one which
>> is just like a chip-select. This is not in the current driver but Afzal
>> has a patch for this [1].
> 
> Ah, thanks for the pointer to the patch. Ok, I'll add a "num-waitpins"
> property. Does that name sound appropriate?

Yes, that would be great!

>> Bottom line, for such hardware specific features, device tree is a good
>> place to describe how many resources we have. Then we can eliminate such
>> #defines from the driver code.
> 
> Agreed.
> 
>>> Quoting Documentation/devicetree/bindings/mtd/gpmc-nand.txt:
>>>
>>> 	For NAND specific properties such as ECC modes or bus width,
>>> 	please refer to Documentation/devicetree/bindings/mtd/nand.txt
>>
>> Ok, thanks I see that now. Looking at other bindings, some also include
>> these details but not all. Could be worth listing ecc-mode under
>> mandatory and bus-width under optional with a reference to nand.txt
>> binding. I don't think it is worth duplicating but listing the actual
>> property names would be nice.
> 
> Ok, I amended my local version. With the details above sorted out and
> "num-cs" and "num-waitpins" in place, do you think we're ready for v4?

Yes, thanks for doing this.

Cheers
Jon

[PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

On 05.11.2012 22:46, Jon Hunter wrote:
> 
> On 11/02/2012 03:23 PM, Daniel Mack wrote:
>> On 02.11.2012 20:57, Jon Hunter wrote:
>>> On 11/02/2012 02:23 PM, Daniel Mack wrote:
>>>> On 02.11.2012 20:18, Jon Hunter wrote:
>>>>> On 11/02/2012 06:14 AM, Daniel Mack wrote:
>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..6f44487
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>>>>> @@ -0,0 +1,73 @@
>>>>>>>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>>>>>>>> +
>>>>>>>> +The actual devices are instantiated from the child nodes of a GPMC node.
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +
>>>>>>>> + - compatible:		Should be set to "ti,gpmc"
>>>>>>>> + - reg:			A resource specifier for the register space
>>>>>>>> +			(see the example below)
>>>>>>>> + - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
>>>>>>>> +			completed.
>>>>>>>> + - #address-cells:	Must be set to 2 to allow memory address translation
>>>>>>>> + - #size-cells:		Must be set to 1 to allow CS address passing
>>>>>>>> + - ranges:		Must be set up to reflect the memory layout
>>>>>>>> +			Note that this property is not currently parsed.
>>>>>>>> +			Calculated values derived from the contents of
>>>>>>>> +			GPMC_CS_CONFIG7 as set up by the bootloader. That will
>>>>>>>> +			change in the future, so be sure to fill the correct
>>>>>>>> +			values here.
>>>>>>>
>>>>>>> I still think it would be good to add number of chip-selects and
>>>>>>> wait-pins here.
>>>>>>
>>>>>> The number of chip-selects can be derived from the ranges property.
>>>>>> Namely, each 4-value entry to this property maps to one chip-select. I
>>>>>> can try and make the more clear in the documentation.
>>>>>
>>>>> Yes but that only tells you how many you are using. The binding should
>>>>> describe the hardware and so should tell us how many chip-selects we
>>>>> have. We should get away from using GPMC_CS_NUM in the code.
>>>>
>>>> Maybe I don't get your point, but we only need to care for as many cs
>>>> lines as we actually use, right?
>>>
>>> But how many does your device have? How many clients can you support?
>>
>> Well, you state that in the ranges property. Even if the chip could in
>> theory support 8 CS lines - if the actual setup only uses the first one
>> of them, the code would only need to allocate and set up the one that is
>> in use. And as the entries in "ranges" are mandatory, there can actually
>> be no mis-allocation.
> 
> Ah, I see your point now. Well typically, we have been putting the
> device-level peripheral info in the device's *.dtsi (ie. am33xx.dtsi)
> and then board specific stuff in the board *.dts file (am335x-bone.dts).
> So I would envision that the device-level info (reg, ti,hwmods,
> interrupt, num-cs) be in am33xx.dtsi and ranges be in am335x.dts. So it
> would still be nice to catch any badly configured ranges property in the
> driver by querying in the number of chip-selects.
> 
>> I can still add the maximum number as a separate property, but I wanted
>> to outline my idea here. Is "num-cs" a good name for the property?
> 
> Sounds good.
> 
>>> If we know how many the device has and then we can get rid of "#define
>>> GPMC_CS_NUM". We currently allocate the CS by calling gpmc_cs_request().
>>> Hmmm ... I now see that your patch is not calling this before
>>> configuring the CS and so that needs to be fixed too.
>>
>> It does implicitly, by calling gpmc_nand_init().
> 
> Yes, you are right!
> 
>>> Without knowing the total CS available, how do we ensure we have the CS
>>> available that someone is asking for?
>>>
>>>>> What about wait-pins?
>>>>
>>>> Afaik, their use depends on the driver acting as GPMC client, right?
>>>> Could you point me to code that acts conditionally and that should be
>>>> reflected in DT?
>>>
>>> Again we need to know how many the device has. Clients may or may not
>>> use these. However, if a client wants one they need to request one which
>>> is just like a chip-select. This is not in the current driver but Afzal
>>> has a patch for this [1].
>>
>> Ah, thanks for the pointer to the patch. Ok, I'll add a "num-waitpins"
>> property. Does that name sound appropriate?
> 
> Yes, that would be great!
> 
>>> Bottom line, for such hardware specific features, device tree is a good
>>> place to describe how many resources we have. Then we can eliminate such
>>> #defines from the driver code.
>>
>> Agreed.
>>
>>>> Quoting Documentation/devicetree/bindings/mtd/gpmc-nand.txt:
>>>>
>>>> 	For NAND specific properties such as ECC modes or bus width,
>>>> 	please refer to Documentation/devicetree/bindings/mtd/nand.txt
>>>
>>> Ok, thanks I see that now. Looking at other bindings, some also include
>>> these details but not all. Could be worth listing ecc-mode under
>>> mandatory and bus-width under optional with a reference to nand.txt
>>> binding. I don't think it is worth duplicating but listing the actual
>>> property names would be nice.
>>
>> Ok, I amended my local version. With the details above sorted out and
>> "num-cs" and "num-waitpins" in place, do you think we're ready for v4?
> 
> Yes, thanks for doing this.

I'll integrate the details mentioned by Philip Avinash on top of yours
and then send a v4, hopefully tomorrow!

Thanks for all the feddback, much appreciated!


Daniel