[PATCH v4 0/3] OMAP GPMC DT bindings

[PATCH v4 0/3] 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.

Version 3 includes fixes pointed out by Jon Hunter:

 - better documentation of the 'ranges' property to describe the
   fact that it's representing the CS lines
 - GPMC_CS_CONFIGx -> GPMC_CONFIGx in comments
 - drop interrupt-parent from example bindings
 - add of_node_put() at the end of the child iteration

Version 4 fixes compilation for !CONFIG_MTD_NAND and includes more
details from Jon Hunter and Avinash, Philip:

 - Add "num-cs" and "num-waitpins" properties, which will eventually
   be used to get rid of GPMC_CS_NUM
 - Better description of generic nand DT properties
 - Dropped patch 3/4 as an equivalent fix was already merged
 - Added ti,nand-ecc-use-elm property

Daniel Mack (3):
  mtd: omap-nand: pass device_node in platform data
  ARM: OMAP: gpmc: enable hwecc for AM33xx SoCs
  ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

 .../devicetree/bindings/mtd/gpmc-nand.txt          |  84 ++++++++++
 arch/arm/mach-omap2/gpmc-nand.c                    |   9 +-
 arch/arm/mach-omap2/gpmc.c                         | 170 +++++++++++++++++++++
 drivers/mtd/nand/omap2.c                           |   4 +-
 include/linux/platform_data/mtd-nand-omap2.h       |   2 +
 5 files changed, 264 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt

-- 
1.7.11.7

[PATCH v4 1/3] 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 5c8978e..1e1d6ef 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1333,6 +1333,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) {
@@ -1558,7 +1559,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 v4 2/3] 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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index 8607735..e23d561 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -92,17 +92,18 @@ 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;
 	}
 
 	/*
-	 * For now, assume 4-bit mode is only supported on OMAP3630 ES1.x, x>=1.
-	 * Other chips may be added if confirmed to work.
+	 * For now, assume 4-bit mode is only supported on OMAP3630 ES1.x, x>=1
+	 * and AM33xx derivates. Other chips may be added if confirmed to work.
 	 */
 	if ((ecc_opt == OMAP_ECC_BCH4_CODE_HW) &&
-	    (!cpu_is_omap3630() || (GET_OMAP_REVISION() == 0))) {
+	    (!cpu_is_omap3630() || (GET_OMAP_REVISION() == 0)) &&
+	    (!soc_is_am33xx())) {
 		pr_err("BCH 4-bit mode is not supported on this CPU\n");
 		return 0;
 	}
-- 
1.7.11.7

[PATCH v4 3/3] 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>
---
 .../devicetree/bindings/mtd/gpmc-nand.txt          |  84 ++++++++++
 arch/arm/mach-omap2/gpmc.c                         | 170 +++++++++++++++++++++
 2 files changed, 254 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
new file mode 100644
index 0000000..20aa5b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -0,0 +1,84 @@
+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
+
+Optional properties:
+
+ - nand-bus-width: 		Set this numeric value to 16 if the hardware
+				is wired that way. If not specified, a bus
+				width of 8 is assumed.
+ - ti,nand-ecc-opt:		A string setting the ECC layout to use. One of:
+
+	Layouts for 1-bit ecc: stored at end of spare area:
+
+		"software"	Software method (default)
+		"hw"		Hardware method (let gpmc do the error detection)
+
+	Layouts for 1-bit ecc: stored at beginning of spare area as romcode:
+
+		"hw-romcode"	gpmc method & romcode layout
+		"bch4"		4-bit BCH ecc code
+		"bch8"		8-bit BCH ecc code
+
+ - ti,nand-ecc-use-elm:		Property without value to specify that the
+				hardware error correction mode should be used.
+
+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>;
+		interrupts = <100>;
+		num-cs = <8>;
+		num-waitpins = <8>;
+		#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>;
+			ti,nand-ecc-opt = "none";
+			ti,nand-ecc-use-elm;
+
+			gpmc,sync-clk = <0>;
+			gpmc,cs-on = <0>;
+			gpmc,cs-rd-off = <44>;
+			gpmc,cs-wr-off = <44>;
+			gpmc,adv-on = <6>;
+			gpmc,adv-rd-off = <34>;
+			gpmc,adv-wr-off = <44>;
+			gpmc,we-off = <40>;
+			gpmc,oe-off = <54>;
+			gpmc,access = <64>;
+			gpmc,rd-cycle = <82>;
+			gpmc,wr-cycle = <82>;
+			gpmc,wr-access = <40>;
+			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 7660a56..971a2bf 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>
 
@@ -34,6 +38,7 @@
 #include "common.h"
 #include "omap_device.h"
 #include "gpmc.h"
+#include "gpmc-nand.h"
 
 #define	DEVICE_NAME		"omap-gpmc"
 
@@ -748,6 +753,162 @@ 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 __maybe_unused 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;
+}
+
+#ifdef CONFIG_MTD_NAND
+
+static const char *nand_ecc_opts[] = {
+	[OMAP_ECC_HAMMING_CODE_DEFAULT]		= "software",
+	[OMAP_ECC_HAMMING_CODE_HW]		= "hw",
+	[OMAP_ECC_HAMMING_CODE_HW_ROMCODE]	= "hw-romcode",
+	[OMAP_ECC_BCH4_CODE_HW]			= "bch4",
+	[OMAP_ECC_BCH8_CODE_HW]			= "bch8",
+};
+
+static int __devinit gpmc_probe_nand_child(struct platform_device *pdev,
+					   struct device_node *child)
+{
+	u32 val;
+	const char *s;
+	struct gpmc_timings gpmc_t;
+	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);
+		return -ENODEV;
+	}
+
+	gpmc_nand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_nand_data),
+				      GFP_KERNEL);
+	if (!gpmc_nand_data)
+		return -ENOMEM;
+
+	gpmc_nand_data->cs = val;
+	gpmc_nand_data->of_node = child;
+
+	if (!of_property_read_string(child, "ti,nand-ecc-opt", &s))
+		for (val = 0; val < ARRAY_SIZE(nand_ecc_opts); val++)
+			if (!strcasecmp(s, nand_ecc_opts[val]))
+				gpmc_nand_data->ecc_opt = val;
+
+	if (of_get_property(child, "ti,nand-ecc-use-elm", NULL))
+		gpmc_nand_data->is_elm_used = 1;
+
+	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 __devinit gpmc_probe_nand_child(struct platform_device *pdev,
+					   struct device_node *child)
+{
+	return 0;
+}
+#endif
+
+static int __devinit gpmc_probe_dt(struct platform_device *pdev)
+{
+	int ret;
+	struct device_node *child;
+	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") {
+		ret = gpmc_probe_nand_child(pdev, child);
+		of_node_put(child);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+#else
+static int __devinit gpmc_probe_dt(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 static __devinit int gpmc_probe(struct platform_device *pdev)
 {
 	int rc;
@@ -801,6 +962,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;
 }
 
@@ -818,6 +987,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 v4 3/3] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Peter Korsgaard]

>>>>> "D" == Daniel Mack <zonque@gmail.com> writes:

Hi,

 D> This patch adds basic DT bindings for OMAP GPMC.
 D> The actual peripherals are instanciated from child nodes within the GPMC

s/instanciated/instantiated/

 D> node, and the only type of device that is currently supported is NAND.

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

 D> Successfully tested on an AM33xx board.

 D> Signed-off-by: Daniel Mack <zonque@gmail.com>
 D> ---
 D>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  84 ++++++++++
 D>  arch/arm/mach-omap2/gpmc.c                         | 170 +++++++++++++++++++++
 D>  2 files changed, 254 insertions(+)
 D>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt

 D> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
 D> new file mode 100644
 D> index 0000000..20aa5b9
 D> --- /dev/null
 D> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
 D> @@ -0,0 +1,84 @@
 D> +Device tree bindings for GPMC connected NANDs
 D> +
 D> +GPMC connected NAND (found on OMAP boards) are represented as child nodes of
 D> +the GPMC controller with a name of "nand".
 D> +
 D> +All timing relevant properties as well as generic gpmc child properties are
 D> +explained in a separate documents - please refer to
 D> +Documentation/devicetree/bindings/bus/ti-gpmc.txt

Which this patch seems to be missing?


 D> +
 D> +For NAND specific properties such as ECC modes or bus width, please refer to
 D> +Documentation/devicetree/bindings/mtd/nand.txt
 D> +
 D> +
 D> +Required properties:
 D> +
 D> + - reg:		The CS line the peripheral is connected to
 D> +
 D> +Optional properties:
 D> +
 D> + - nand-bus-width: 		Set this numeric value to 16 if the hardware
 D> +				is wired that way. If not specified, a bus
 D> +				width of 8 is assumed.
 D> + - ti,nand-ecc-opt:		A string setting the ECC layout to use. One of:
 D> +
 D> +	Layouts for 1-bit ecc: stored at end of spare area:
 D> +
 D> +		"software"	Software method (default)
 D> +		"hw"		Hardware method (let gpmc do the error detection)

Nit: why are you spelling out 'software' when everything else is shortened?

 D> +
 D> +	Layouts for 1-bit ecc: stored at beginning of spare area as romcode:
 D> +
 D> +		"hw-romcode"	gpmc method & romcode layout
 D> +		"bch4"		4-bit BCH ecc code
 D> +		"bch8"		8-bit BCH ecc code
 D> +

These are not 1-bit - Well, romcode might be depending on SoC. Looking at
omap2.c it seems to be an alias for hw, so that isn't really helpful on
E.G. am33xx where you should select bch8 to have something compatible
with the romcode.


 D> + - ti,nand-ecc-use-elm:		Property without value to specify that the
 D> +				hardware error correction mode should be used.

It's called the error location module. Like you pointed out yourself,
this property isn't really nice. I haven't looked at the elm bindings in
detail, but couldn't the nand driver automatically use the elm if the
device tree provides a node for it? Perhaps the elm should be a subnode
of the nand one?

-- 
Bye, Peter Korsgaard

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

[Daniel Mack]

Hi Peter,

On 19.11.2012 21:52, Peter Korsgaard wrote:
>>>>>> "D" == Daniel Mack <zonque@gmail.com> writes:
> 
> Hi,
> 
>  D> This patch adds basic DT bindings for OMAP GPMC.
>  D> The actual peripherals are instanciated from child nodes within the GPMC
> 
> s/instanciated/instantiated/

Thanks, fixed.

> 
>  D> node, and the only type of device that is currently supported is NAND.
> 
>  D> Code was added to parse the generic GPMC timing parameters and some
>  D> documentation with examples on how to use them.
> 
>  D> Successfully tested on an AM33xx board.
> 
>  D> Signed-off-by: Daniel Mack <zonque@gmail.com>
>  D> ---
>  D>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  84 ++++++++++
>  D>  arch/arm/mach-omap2/gpmc.c                         | 170 +++++++++++++++++++++
>  D>  2 files changed, 254 insertions(+)
>  D>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> 
>  D> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>  D> new file mode 100644
>  D> index 0000000..20aa5b9
>  D> --- /dev/null
>  D> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>  D> @@ -0,0 +1,84 @@
>  D> +Device tree bindings for GPMC connected NANDs
>  D> +
>  D> +GPMC connected NAND (found on OMAP boards) are represented as child nodes of
>  D> +the GPMC controller with a name of "nand".
>  D> +
>  D> +All timing relevant properties as well as generic gpmc child properties are
>  D> +explained in a separate documents - please refer to
>  D> +Documentation/devicetree/bindings/bus/ti-gpmc.txt
> 
> Which this patch seems to be missing?

True - sorry for that. Will be back at v5.

>  D> +
>  D> +For NAND specific properties such as ECC modes or bus width, please refer to
>  D> +Documentation/devicetree/bindings/mtd/nand.txt
>  D> +
>  D> +
>  D> +Required properties:
>  D> +
>  D> + - reg:		The CS line the peripheral is connected to
>  D> +
>  D> +Optional properties:
>  D> +
>  D> + - nand-bus-width: 		Set this numeric value to 16 if the hardware
>  D> +				is wired that way. If not specified, a bus
>  D> +				width of 8 is assumed.
>  D> + - ti,nand-ecc-opt:		A string setting the ECC layout to use. One of:
>  D> +
>  D> +	Layouts for 1-bit ecc: stored at end of spare area:
>  D> +
>  D> +		"software"	Software method (default)
>  D> +		"hw"		Hardware method (let gpmc do the error detection)
> 
> Nit: why are you spelling out 'software' when everything else is shortened?

Right. Fixed.

> 
>  D> +
>  D> +	Layouts for 1-bit ecc: stored at beginning of spare area as romcode:
>  D> +
>  D> +		"hw-romcode"	gpmc method & romcode layout
>  D> +		"bch4"		4-bit BCH ecc code
>  D> +		"bch8"		8-bit BCH ecc code
>  D> +
> 
> These are not 1-bit - Well, romcode might be depending on SoC. Looking at
> omap2.c it seems to be an alias for hw, so that isn't really helpful on
> E.G. am33xx where you should select bch8 to have something compatible
> with the romcode.

Not sure whether I follow you here. drivers/mtd/nand/omap2.c handles
cases for pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW, so the DT bindings
need to offer a way to set it.

Or are you purely referring to the comments only?

>  D> + - ti,nand-ecc-use-elm:		Property without value to specify that the
>  D> +				hardware error correction mode should be used.
> 
> It's called the error location module. Like you pointed out yourself,
> this property isn't really nice. I haven't looked at the elm bindings in
> detail, but couldn't the nand driver automatically use the elm if the
> device tree provides a node for it? Perhaps the elm should be a subnode
> of the nand one?

That is to be discussed for the elm driver then. I'm not sure though
whether the elm should always be used when its DT node is present.

For now, I would leave it the way it is and have an optional
"ti,nand-ecc-use-elm". I just added some more lines of documenation to
describe the fact that the elm driver is needed in order to make it work.



Daniel

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

[Peter Korsgaard]

>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes:

Hi,

 D> +	Layouts for 1-bit ecc: stored at beginning of spare area as romcode:
 D> +
 D> +		"hw-romcode"	gpmc method & romcode layout
 D> +		"bch4"		4-bit BCH ecc code
 D> +		"bch8"		8-bit BCH ecc code
 D> +
 >> 
 >> These are not 1-bit - Well, romcode might be depending on SoC. Looking at
 >> omap2.c it seems to be an alias for hw, so that isn't really helpful on
 >> E.G. am33xx where you should select bch8 to have something compatible
 >> with the romcode.

 Daniel> Not sure whether I follow you here. drivers/mtd/nand/omap2.c handles
 Daniel> cases for pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW, so the DT bindings
 Daniel> need to offer a way to set it.

 Daniel> Or are you purely referring to the comments only?

Yes, but the document states (the first line I'm quotinge) 'Layouts for
1-bit ecc'. The other comment was about hw-romcode not being a very good
name, as it apparently means the 1bit hamming code and ECC layout used
on the older omap3, and not E.G. the bch8/elm layout used by
E.G. am335x.


 D> + - ti,nand-ecc-use-elm:		Property without value to specify that the
 D> +				hardware error correction mode should be used.
 >> 
 >> It's called the error location module. Like you pointed out yourself,
 >> this property isn't really nice. I haven't looked at the elm bindings in
 >> detail, but couldn't the nand driver automatically use the elm if the
 >> device tree provides a node for it? Perhaps the elm should be a subnode
 >> of the nand one?

 Daniel> That is to be discussed for the elm driver then. I'm not sure though
 Daniel> whether the elm should always be used when its DT node is present.

 Daniel> For now, I would leave it the way it is and have an optional
 Daniel> "ti,nand-ecc-use-elm". I just added some more lines of documenation to
 Daniel> describe the fact that the elm driver is needed in order to make it work.

As discussed elsewhere, the elm also changes the ECC layout, so why not
just have bchN (software bchN layout) / bchN-elm selections instead of
the seperate ti,nand-ecc-use-elm?

-- 
Bye, Peter Korsgaard

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

[Daniel Mack]

On 23.11.2012 11:47, Peter Korsgaard wrote:
>>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes:
> 
> Hi,
> 
>  D> +	Layouts for 1-bit ecc: stored at beginning of spare area as romcode:
>  D> +
>  D> +		"hw-romcode"	gpmc method & romcode layout
>  D> +		"bch4"		4-bit BCH ecc code
>  D> +		"bch8"		8-bit BCH ecc code
>  D> +
>  >> 
>  >> These are not 1-bit - Well, romcode might be depending on SoC. Looking at
>  >> omap2.c it seems to be an alias for hw, so that isn't really helpful on
>  >> E.G. am33xx where you should select bch8 to have something compatible
>  >> with the romcode.
> 
>  Daniel> Not sure whether I follow you here. drivers/mtd/nand/omap2.c handles
>  Daniel> cases for pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW, so the DT bindings
>  Daniel> need to offer a way to set it.
> 
>  Daniel> Or are you purely referring to the comments only?
> 
> Yes, but the document states (the first line I'm quotinge) 'Layouts for
> 1-bit ecc'.

Ok, I guess I'll just remove these comments then.

> The other comment was about hw-romcode not being a very good
> name, as it apparently means the 1bit hamming code and ECC layout used
> on the older omap3, and not E.G. the bch8/elm layout used by
> E.G. am335x.

So which name would you pick? "bch1"?

> As discussed elsewhere, the elm also changes the ECC layout, so why not
> just have bchN (software bchN layout) / bchN-elm selections instead of
> the seperate ti,nand-ecc-use-elm?

Avinash - what do you think?


Thanks for the feedback - I'd also like to get these bindings right in
the first place.


Daniel

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

[Peter Korsgaard]

>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes:

Hi,

 >> The other comment was about hw-romcode not being a very good
 >> name, as it apparently means the 1bit hamming code and ECC layout used
 >> on the older omap3, and not E.G. the bch8/elm layout used by
 >> E.G. am335x.

 Daniel> So which name would you pick? "bch1"?

No, it's hamming code, isn't it? To me it seems similar to the elm
discussion, E.G. we have a mix of ECC algorithm + ECC layout + use of hw
acceleration or not.

The choice of hardware acceleration or not should preferably be
transparent to this binding (E.G. it is purely an optimization, ECC
strength/format shouldn't change), so the driver should be free to
use it if it is available.


 Daniel> Thanks for the feedback - I'd also like to get these bindings
 Daniel> right in the first place.

Me too!

-- 
Bye, Peter Korsgaard

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

[Philip, Avinash]

On Mon, Nov 19, 2012 at 20:59:16, Daniel Mack wrote:
> This patch adds basic DT bindings for OMAP GPMC.
> 
...
> +
> +	if (of_get_property(child, "ti,nand-ecc-use-elm", NULL))
> +		gpmc_nand_data->is_elm_used = 1;

Can you set to bool value (true) here.

Thanks
Avinash

[PATCH v4 0/3] OMAP GPMC DT bindings

[Jon Hunter]

On 11/19/2012 09:29 AM, Daniel Mack wrote:
> 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.
> 
> Version 3 includes fixes pointed out by Jon Hunter:
> 
>  - better documentation of the 'ranges' property to describe the
>    fact that it's representing the CS lines
>  - GPMC_CS_CONFIGx -> GPMC_CONFIGx in comments
>  - drop interrupt-parent from example bindings
>  - add of_node_put() at the end of the child iteration
> 
> Version 4 fixes compilation for !CONFIG_MTD_NAND and includes more
> details from Jon Hunter and Avinash, Philip:
> 
>  - Add "num-cs" and "num-waitpins" properties, which will eventually
>    be used to get rid of GPMC_CS_NUM
>  - Better description of generic nand DT properties
>  - Dropped patch 3/4 as an equivalent fix was already merged
>  - Added ti,nand-ecc-use-elm property
> 
> Daniel Mack (3):
>   mtd: omap-nand: pass device_node in platform data
>   ARM: OMAP: gpmc: enable hwecc for AM33xx SoCs
>   ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND
> 
>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  84 ++++++++++
>  arch/arm/mach-omap2/gpmc-nand.c                    |   9 +-
>  arch/arm/mach-omap2/gpmc.c                         | 170 +++++++++++++++++++++
>  drivers/mtd/nand/omap2.c                           |   4 +-
>  include/linux/platform_data/mtd-nand-omap2.h       |   2 +
>  5 files changed, 264 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt

Thanks for sending out the update. Am I missing something or did you
drop the gpmc.txt binding documentation?

Cheers
Jon