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

[PATCH v3 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.

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

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  |  76 +++++++++++
 .../devicetree/bindings/mtd/gpmc-nand.txt          |  60 +++++++++
 arch/arm/mach-omap2/gpmc-nand.c                    |   2 +-
 arch/arm/mach-omap2/gpmc.c                         | 148 +++++++++++++++++++++
 drivers/mtd/nand/omap2.c                           |   4 +-
 include/linux/platform_data/mtd-nand-omap2.h       |   2 +
 6 files changed, 290 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 v3 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 v3 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 v3 2/4] ARM: OMAP: gpmc: enable hwecc for AM33xx SoCs

[Philip, Avinash]

On Fri, Nov 02, 2012 at 20:55:54, Daniel Mack wrote:
> 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;
>  	}

Please add '!soc_is_am33xx()' check for BCH4 failure case also.
BCH4 support tested in AM335x.
https://lkml.org/lkml/2012/10/31/82

Thanks
Avinash

> -- 
> 1.7.11.7
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v3 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 v3 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  |  76 +++++++++++
 .../devicetree/bindings/mtd/gpmc-nand.txt          |  60 +++++++++
 arch/arm/mach-omap2/gpmc.c                         | 141 +++++++++++++++++++++
 3 files changed, 277 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..4f4a602
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
@@ -0,0 +1,76 @@
+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 with four
+			integer values for each chip-select line in use:
+
+			   <cs-number> 0 <physical address of mapping> <size>
+
+			Note that this property is not currently parsed.
+			Calculated values derived from the contents of the
+			per-CS register GPMC_CONFIG7 (as set up by the
+			bootloader) are used. 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_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>;
+		interrupts = <100>;
+
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <0 0 0x08000000 0x10000000>; /* CS0 @addr 0x8000000, size 0x10000000 */
+
+		/* 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..d78bf49
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -0,0 +1,60 @@
+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>;
+		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..7ebe4fb 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,133 @@ 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);
+
+		of_node_put(child);
+	}
+
+	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 +936,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 +961,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 v3 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Philip, Avinash]

On Fri, Nov 02, 2012 at 20:55:56, 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>
[...]
> +
> +		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>;
> +

Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm
So the timings can be directly used to populate GPMC timings. Timings can found at

http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;
h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e

[...]
> +static int gpmc_probe_dt(struct platform_device *pdev)

Can you take care of the following section mismatch.
WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference
from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init().

[...]
> +
> +		val = of_get_nand_ecc_mode(child);
> +		if (val >= 0)
> +			gpmc_nand_data->ecc_opt = val;

This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection
option between for BCH4 & BCH8 also.
Can you use the of_property_read_u32 (as done early) to pass the ecc selection
from dt file. This will help selection of BCH4 & BCH8 ecc options.

Thanks
Avinash

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

[Daniel Mack]

On 05.11.2012 12:03, Philip, Avinash wrote:
> On Fri, Nov 02, 2012 at 20:55:56, 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>
> [...]
>> +
>> +		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>;
>> +
> 
> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm
> So the timings can be directly used to populate GPMC timings. Timings can found at
> 
> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;
> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e
> 
> [...]
>> +static int gpmc_probe_dt(struct platform_device *pdev)
> 
> Can you take care of the following section mismatch.
> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference
> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init().

Sore, both fixed for v4.

> [...]
>> +
>> +		val = of_get_nand_ecc_mode(child);
>> +		if (val >= 0)
>> +			gpmc_nand_data->ecc_opt = val;
> 
> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection
> option between for BCH4 & BCH8 also.
> Can you use the of_property_read_u32 (as done early) to pass the ecc selection
> from dt file. This will help selection of BCH4 & BCH8 ecc options.

Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and
bring the enum in sync?


Daniel

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

[Philip, Avinash]

On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote:
> On 05.11.2012 12:03, Philip, Avinash wrote:
> > On Fri, Nov 02, 2012 at 20:55:56, 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>
> > [...]
> >> +
> >> +		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>;
> >> +
> > 
> > Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm
> > So the timings can be directly used to populate GPMC timings. Timings can found at
> > 
> > http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;
> > h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e
> > 
> > [...]
> >> +static int gpmc_probe_dt(struct platform_device *pdev)
> > 
> > Can you take care of the following section mismatch.
> > WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference
> > from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init().
> 
> Sore, both fixed for v4.
> 
> > [...]
> >> +
> >> +		val = of_get_nand_ecc_mode(child);
> >> +		if (val >= 0)
> >> +			gpmc_nand_data->ecc_opt = val;
> > 
> > This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection
> > option between for BCH4 & BCH8 also.
> > Can you use the of_property_read_u32 (as done early) to pass the ecc selection
> > from dt file. This will help selection of BCH4 & BCH8 ecc options.
> 
> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and
> bring the enum in sync?

ecc_opt is for selecting different ecc layout and not for selecting ecc mode.

In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
	OMAP_ECC_HAMMING_CODE_HW_ROMCODE
	OMAP_ECC_BCH4_CODE_HW
	OMAP_ECC_BCH8_CODE_HW

So selection of ecc layout data should come from DT not ecc mode.

Thanks
Avinash

> 
> 
> Daniel
> 
> 
> 

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

[Murali Karicheri]

On 11/05/2012 08:29 AM, Philip, Avinash wrote:
> On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote:
>> On 05.11.2012 12:03, Philip, Avinash wrote:
>>> On Fri, Nov 02, 2012 at 20:55:56, 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>
>>> [...]
>>>> +
>>>> +		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>;
>>>> +
>>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm
>>> So the timings can be directly used to populate GPMC timings. Timings can found at
>>>
>>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;
>>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e
>>>
>>> [...]
>>>> +static int gpmc_probe_dt(struct platform_device *pdev)
>>> Can you take care of the following section mismatch.
>>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference
>>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init().
>> Sore, both fixed for v4.
>>
>>> [...]
>>>> +
>>>> +		val = of_get_nand_ecc_mode(child);
>>>> +		if (val >= 0)
>>>> +			gpmc_nand_data->ecc_opt = val;
>>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection
>>> option between for BCH4 & BCH8 also.
>>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection
>>> from dt file. This will help selection of BCH4 & BCH8 ecc options.
>> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and
>> bring the enum in sync?
> ecc_opt is for selecting different ecc layout and not for selecting ecc mode.
>
> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
> 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> 	OMAP_ECC_BCH4_CODE_HW
> 	OMAP_ECC_BCH8_CODE_HW
>
> So selection of ecc layout data should come from DT not ecc mode.
>
> Thanks
> Avinash
>
>>
>> Daniel
>>
>>
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Hi,

I am working on an AEMIF driver for DaVinci that is similar to this. Is 
this a memory or mfd device?

Murali

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

[Daniel Mack]

On 05.11.2012 14:29, Philip, Avinash wrote:
> On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote:
>> On 05.11.2012 12:03, Philip, Avinash wrote:
>>> On Fri, Nov 02, 2012 at 20:55:56, 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>
>>> [...]
>>>> +
>>>> +		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>;
>>>> +
>>>
>>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm
>>> So the timings can be directly used to populate GPMC timings. Timings can found at
>>>
>>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;
>>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e
>>>
>>> [...]
>>>> +static int gpmc_probe_dt(struct platform_device *pdev)
>>>
>>> Can you take care of the following section mismatch.
>>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference
>>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init().
>>
>> Sore, both fixed for v4.
>>
>>> [...]
>>>> +
>>>> +		val = of_get_nand_ecc_mode(child);
>>>> +		if (val >= 0)
>>>> +			gpmc_nand_data->ecc_opt = val;
>>>
>>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection
>>> option between for BCH4 & BCH8 also.
>>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection
>>> from dt file. This will help selection of BCH4 & BCH8 ecc options.
>>
>> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and
>> bring the enum in sync?
> 
> ecc_opt is for selecting different ecc layout and not for selecting ecc mode.
> 
> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
> 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> 	OMAP_ECC_BCH4_CODE_HW
> 	OMAP_ECC_BCH8_CODE_HW
> 
> So selection of ecc layout data should come from DT not ecc mode.

Ok, I see. I would still like to set them by string rather than magic
numbers that map to enum entries. Valid values would be "none", "hw",
"hw-romcode", "bch4" and "bch8". Are you ok with that?


Thanks,
Daniel

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

[Philip, Avinash]

On Wed, Nov 07, 2012 at 15:18:37, Daniel Mack wrote:
> On 05.11.2012 14:29, Philip, Avinash wrote:
> > On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote:
> >> On 05.11.2012 12:03, Philip, Avinash wrote:
> >>> On Fri, Nov 02, 2012 at 20:55:56, 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>
> >>> [...]
> >>>> +
> >>>> +		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>;
> >>>> +
> >>>
> >>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm
> >>> So the timings can be directly used to populate GPMC timings. Timings can found at
> >>>
> >>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;
> >>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e
> >>>
> >>> [...]
> >>>> +static int gpmc_probe_dt(struct platform_device *pdev)
> >>>
> >>> Can you take care of the following section mismatch.
> >>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference
> >>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init().
> >>
> >> Sore, both fixed for v4.
> >>
> >>> [...]
> >>>> +
> >>>> +		val = of_get_nand_ecc_mode(child);
> >>>> +		if (val >= 0)
> >>>> +			gpmc_nand_data->ecc_opt = val;
> >>>
> >>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection
> >>> option between for BCH4 & BCH8 also.
> >>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection
> >>> from dt file. This will help selection of BCH4 & BCH8 ecc options.
> >>
> >> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and
> >> bring the enum in sync?
> > 
> > ecc_opt is for selecting different ecc layout and not for selecting ecc mode.
> > 
> > In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
> > 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> > 	OMAP_ECC_BCH4_CODE_HW
> > 	OMAP_ECC_BCH8_CODE_HW
> > 
> > So selection of ecc layout data should come from DT not ecc mode.
> 
> Ok, I see. I would still like to set them by string rather than magic
> numbers that map to enum entries. Valid values would be "none", "hw",
> "hw-romcode", "bch4" and "bch8". Are you ok with that?

Ok, that's nice. Better use ecc_opt instead of ecc_mode.

Thanks
Avinash

> 
> 
> Thanks,
> Daniel
> 
> 

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

[Daniel Mack]

On 07.11.2012 16:37, Philip, Avinash wrote:
> On Wed, Nov 07, 2012 at 15:18:37, Daniel Mack wrote:
>> On 05.11.2012 14:29, Philip, Avinash wrote:
>>> On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote:
>>>> On 05.11.2012 12:03, Philip, Avinash wrote:
>>>>> On Fri, Nov 02, 2012 at 20:55:56, 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>
>>>>> [...]
>>>>>> +
>>>>>> +		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>;
>>>>>> +
>>>>>
>>>>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm
>>>>> So the timings can be directly used to populate GPMC timings. Timings can found at
>>>>>
>>>>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;
>>>>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e
>>>>>
>>>>> [...]
>>>>>> +static int gpmc_probe_dt(struct platform_device *pdev)
>>>>>
>>>>> Can you take care of the following section mismatch.
>>>>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference
>>>>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init().
>>>>
>>>> Sore, both fixed for v4.
>>>>
>>>>> [...]
>>>>>> +
>>>>>> +		val = of_get_nand_ecc_mode(child);
>>>>>> +		if (val >= 0)
>>>>>> +			gpmc_nand_data->ecc_opt = val;
>>>>>
>>>>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection
>>>>> option between for BCH4 & BCH8 also.
>>>>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection
>>>>> from dt file. This will help selection of BCH4 & BCH8 ecc options.
>>>>
>>>> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and
>>>> bring the enum in sync?
>>>
>>> ecc_opt is for selecting different ecc layout and not for selecting ecc mode.
>>>
>>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
>>> 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE
>>> 	OMAP_ECC_BCH4_CODE_HW
>>> 	OMAP_ECC_BCH8_CODE_HW
>>>
>>> So selection of ecc layout data should come from DT not ecc mode.
>>
>> Ok, I see. I would still like to set them by string rather than magic
>> numbers that map to enum entries. Valid values would be "none", "hw",
>> "hw-romcode", "bch4" and "bch8". Are you ok with that?
> 
> Ok, that's nice. Better use ecc_opt instead of ecc_mode.

I did some more extensive tests that include reading the same nand pages
from both U-Boot and the kernel with BCH8 ECC, and it turns out that
->is_elm_used needs to be set in the pdata in order to make this work.

So the question is whether we actually want to have a DT property for
that or just always enable that bit in case a hardware supported ecc
mode is selected. Any opinion on this?

That's the last topic before I'm clear to send off v4.


Thanks,
Daniel

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

[Daniel Mack]

On 11.11.2012 02:56, Daniel Mack wrote:
> On 07.11.2012 16:37, Philip, Avinash wrote:
>> On Wed, Nov 07, 2012 at 15:18:37, Daniel Mack wrote:
>>> On 05.11.2012 14:29, Philip, Avinash wrote:
>>>> On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote:
>>>>> On 05.11.2012 12:03, Philip, Avinash wrote:
>>>>>> On Fri, Nov 02, 2012 at 20:55:56, 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>
>>>>>> [...]
>>>>>>> +
>>>>>>> +		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>;
>>>>>>> +
>>>>>>
>>>>>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm
>>>>>> So the timings can be directly used to populate GPMC timings. Timings can found at
>>>>>>
>>>>>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;
>>>>>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e
>>>>>>
>>>>>> [...]
>>>>>>> +static int gpmc_probe_dt(struct platform_device *pdev)
>>>>>>
>>>>>> Can you take care of the following section mismatch.
>>>>>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference
>>>>>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init().
>>>>>
>>>>> Sore, both fixed for v4.
>>>>>
>>>>>> [...]
>>>>>>> +
>>>>>>> +		val = of_get_nand_ecc_mode(child);
>>>>>>> +		if (val >= 0)
>>>>>>> +			gpmc_nand_data->ecc_opt = val;
>>>>>>
>>>>>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection
>>>>>> option between for BCH4 & BCH8 also.
>>>>>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection
>>>>>> from dt file. This will help selection of BCH4 & BCH8 ecc options.
>>>>>
>>>>> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and
>>>>> bring the enum in sync?
>>>>
>>>> ecc_opt is for selecting different ecc layout and not for selecting ecc mode.
>>>>
>>>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
>>>> 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE
>>>> 	OMAP_ECC_BCH4_CODE_HW
>>>> 	OMAP_ECC_BCH8_CODE_HW
>>>>
>>>> So selection of ecc layout data should come from DT not ecc mode.
>>>
>>> Ok, I see. I would still like to set them by string rather than magic
>>> numbers that map to enum entries. Valid values would be "none", "hw",
>>> "hw-romcode", "bch4" and "bch8". Are you ok with that?
>>
>> Ok, that's nice. Better use ecc_opt instead of ecc_mode.
> 
> I did some more extensive tests that include reading the same nand pages
> from both U-Boot and the kernel with BCH8 ECC, and it turns out that
> ->is_elm_used needs to be set in the pdata in order to make this work.
> 
> So the question is whether we actually want to have a DT property for
> that or just always enable that bit in case a hardware supported ecc
> mode is selected. Any opinion on this?
> 
> That's the last topic before I'm clear to send off v4.

Any opionion on this, anyone?


Thanks,
Daniel

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

[Philip, Avinash]

On Sun, Nov 11, 2012 at 00:26:32, Daniel Mack wrote:
> On 07.11.2012 16:37, Philip, Avinash wrote:
> > On Wed, Nov 07, 2012 at 15:18:37, Daniel Mack wrote:
> >> On 05.11.2012 14:29, Philip, Avinash wrote:
> >>> On Mon, Nov 05, 2012 at 18:28:22, Daniel Mack wrote:
> >>>> On 05.11.2012 12:03, Philip, Avinash wrote:
> >>>>> On Fri, Nov 02, 2012 at 20:55:56, 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>
> >>>>> [...]
> >>>>>> +
> >>>>>> +		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>;
> >>>>>> +
> >>>>>
> >>>>> Can you take the timings (for example) from arago tree. The timings is tested in am335x-evm
> >>>>> So the timings can be directly used to populate GPMC timings. Timings can found at
> >>>>>
> >>>>> http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;
> >>>>> h=66bfbd2c5b35dc81edce0c24843c476161ab5978;hp=370630359cb8db711cf0941cd2a242e28ccfb61e
> >>>>>
> >>>>> [...]
> >>>>>> +static int gpmc_probe_dt(struct platform_device *pdev)
> >>>>>
> >>>>> Can you take care of the following section mismatch.
> >>>>> WARNING: vmlinux.o(.text+0x1e2d0): Section mismatch in reference
> >>>>> from the function gpmc_probe_dt() to the function .init.text:gpmc_nand_init().
> >>>>
> >>>> Sore, both fixed for v4.
> >>>>
> >>>>> [...]
> >>>>>> +
> >>>>>> +		val = of_get_nand_ecc_mode(child);
> >>>>>> +		if (val >= 0)
> >>>>>> +			gpmc_nand_data->ecc_opt = val;
> >>>>>
> >>>>> This will fail for BCH. Index of "soft_bch" is 5 & also don't have selection
> >>>>> option between for BCH4 & BCH8 also.
> >>>>> Can you use the of_property_read_u32 (as done early) to pass the ecc selection
> >>>>> from dt file. This will help selection of BCH4 & BCH8 ecc options.
> >>>>
> >>>> Hmm. Shouldn't we rather teach of_get_nand_ecc_mode() that two modes and
> >>>> bring the enum in sync?
> >>>
> >>> ecc_opt is for selecting different ecc layout and not for selecting ecc mode.
> >>>
> >>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
> >>> 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> >>> 	OMAP_ECC_BCH4_CODE_HW
> >>> 	OMAP_ECC_BCH8_CODE_HW
> >>>
> >>> So selection of ecc layout data should come from DT not ecc mode.
> >>
> >> Ok, I see. I would still like to set them by string rather than magic
> >> numbers that map to enum entries. Valid values would be "none", "hw",
> >> "hw-romcode", "bch4" and "bch8". Are you ok with that?
> > 
> > Ok, that's nice. Better use ecc_opt instead of ecc_mode.
> 
> I did some more extensive tests that include reading the same nand pages
> from both U-Boot and the kernel with BCH8 ECC, and it turns out that
> ->is_elm_used needs to be set in the pdata in order to make this work.
> 
> So the question is whether we actually want to have a DT property for
> that or just always enable that bit in case a hardware supported ecc
> mode is selected. Any opinion on this?

Yes, is_elm_used data should come from DT. There may be hardware which uses
BCH8 ecc scheme even without ELM hardware support with software error correction
support. So is_elm_used data should come from DT property.

Thanks
Avinash
 
 
> 
> That's the last topic before I'm clear to send off v4.
> 
> 
> Thanks,
> Daniel
> 
> 

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

[Peter Korsgaard]

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

Hi,

 >>>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
 >>>> OMAP_ECC_HAMMING_CODE_HW_ROMCODE
 >>>> OMAP_ECC_BCH4_CODE_HW
 >>>> OMAP_ECC_BCH8_CODE_HW
 >>>> 
 >>>> So selection of ecc layout data should come from DT not ecc mode.
 >>> 
 >>> Ok, I see. I would still like to set them by string rather than magic
 >>> numbers that map to enum entries. Valid values would be "none", "hw",
 >>> "hw-romcode", "bch4" and "bch8". Are you ok with that?
 >> 
 >> Ok, that's nice. Better use ecc_opt instead of ecc_mode.

 Daniel> I did some more extensive tests that include reading the same
 Daniel> nand pages from both U-Boot and the kernel with BCH8 ECC, and
 Daniel> it turns out that -> is_elm_used needs to be set in the pdata
 Daniel> in order to make this work.

So what you're saying is that the choice of ELM or not is not just an
optimization, it really changes the ECC layout? Perhaps it should be
treated as a seperate layout (E.G. bch8-elm) then?

-- 
Bye, Peter Korsgaard

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

[Daniel Mack]

On 20.11.2012 16:59, Peter Korsgaard wrote:
>>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes:
> 
> Hi,
> 
>  >>>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
>  >>>> OMAP_ECC_HAMMING_CODE_HW_ROMCODE
>  >>>> OMAP_ECC_BCH4_CODE_HW
>  >>>> OMAP_ECC_BCH8_CODE_HW
>  >>>> 
>  >>>> So selection of ecc layout data should come from DT not ecc mode.
>  >>> 
>  >>> Ok, I see. I would still like to set them by string rather than magic
>  >>> numbers that map to enum entries. Valid values would be "none", "hw",
>  >>> "hw-romcode", "bch4" and "bch8". Are you ok with that?
>  >> 
>  >> Ok, that's nice. Better use ecc_opt instead of ecc_mode.
> 
>  Daniel> I did some more extensive tests that include reading the same
>  Daniel> nand pages from both U-Boot and the kernel with BCH8 ECC, and
>  Daniel> it turns out that -> is_elm_used needs to be set in the pdata
>  Daniel> in order to make this work.
> 
> So what you're saying is that the choice of ELM or not is not just an
> optimization, it really changes the ECC layout? Perhaps it should be
> treated as a seperate layout (E.G. bch8-elm) then?

That is what I experienced, yes. The kernel was unable to parse NAND
pages that were written from U-Boot with bch8 hardware mode when the elm
module was not active. Maybe someone from TI can explain that? Giving it
a dedicated name would also solve the problem with the extra DT property.

I'll wait until this is decided before I resend a new set that also
fixes the issue with the errornousely forgotten Documentation file.


Thanks,
Daniel

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

[Philip, Avinash]

On Wed, Nov 21, 2012 at 15:45:23, Daniel Mack wrote:
> On 20.11.2012 16:59, Peter Korsgaard wrote:
> >>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes:
> > 
> > Hi,
> > 
> >  >>>> In omap2 driver NAND_ECC_HW ecc mode supports 3 ecc layout
> >  >>>> OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> >  >>>> OMAP_ECC_BCH4_CODE_HW
> >  >>>> OMAP_ECC_BCH8_CODE_HW
> >  >>>> 
> >  >>>> So selection of ecc layout data should come from DT not ecc mode.
> >  >>> 
> >  >>> Ok, I see. I would still like to set them by string rather than magic
> >  >>> numbers that map to enum entries. Valid values would be "none", "hw",
> >  >>> "hw-romcode", "bch4" and "bch8". Are you ok with that?
> >  >> 
> >  >> Ok, that's nice. Better use ecc_opt instead of ecc_mode.
> > 
> >  Daniel> I did some more extensive tests that include reading the same
> >  Daniel> nand pages from both U-Boot and the kernel with BCH8 ECC, and
> >  Daniel> it turns out that -> is_elm_used needs to be set in the pdata
> >  Daniel> in order to make this work.
> > 
> > So what you're saying is that the choice of ELM or not is not just an
> > optimization, it really changes the ECC layout? Perhaps it should be
> > treated as a seperate layout (E.G. bch8-elm) then?

Peter,

In patch [1], mtd: nand: omap2: Support for hardware BCH

ecc_layout made compatible with Rom Boot Loader ECC layout for am335x.

This action is based on is_elm_used flag.

Requirement of this flag is to identify the whether the platform
ELM module & based on this configure ELM module if present.

But ideally BCH8 ecc lay out didn't have a dependency on ELM, it
can work with software BCH ecc. RBL compatibility is missing
in software BCH because of addition of constant polynomial to
ecc vector. If we remove the dependency on erased page handling
by ecc vector with constant polynomial, software BCH can do the
job of RBL compatibility.

Ivan,
Do you have any suggestions?
Discussion for RBL compatibility found at [2].

It is good that software BCH also support RBL compatibility by suppressing
constant polynomial modification. Then ecc layout can be selected from
DT entry and error correction can be chosen between software/hardware
depending on the availability of ELM hardware.
Currently RBL BCH8 compatibility depends on the availability of ELM
hardware. Later once software BCH start supporting RBL compatibility,
we can remove  the check.

> 
> That is what I experienced, yes. The kernel was unable to parse NAND
> pages that were written from U-Boot with bch8 hardware mode when the elm
> module was not active. Maybe someone from TI can explain that? Giving it
> a dedicated name would also solve the problem with the extra DT property.

Daniel,

Currently BCH8 is supported with software ecc error correction in mainline.
The layout for BCH8 ECC layout is
0-1 -> BAD block markers
2-11-> oob free area
12-63-> BCH8 ECC.

RBL ECC layout is
0-1 -> BAD block markers
2-57-> BCH8 ecc layout
58-63-> OOB free area

As u-boot also maintain RBL ecc layout, write from U-boot
and read from Linux requires compatibility with RBL ecc layout.
The same is achieved in the patch [1], with by setting is_elm_used
to true.

1. https://lkml.org/lkml/2012/10/31/87
2. https://lkml.org/lkml/2012/10/11/20

Thanks
Avinash
> 
> I'll wait until this is decided before I resend a new set that also
> fixes the issue with the errornousely forgotten Documentation file.
> 
> 
> Thanks,
> Daniel
> 
> 

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

[Daniel Mack]

Hi Avinash,
Hi Peter,

On 23.11.2012 11:43, Philip, Avinash wrote:
> On Wed, Nov 21, 2012 at 15:45:23, Daniel Mack wrote:
>> On 20.11.2012 16:59, Peter Korsgaard wrote:
>>>>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes:

> Peter,
> 
> In patch [1], mtd: nand: omap2: Support for hardware BCH
> 
> ecc_layout made compatible with Rom Boot Loader ECC layout for am335x.
> 
> This action is based on is_elm_used flag.
> 
> Requirement of this flag is to identify the whether the platform
> ELM module & based on this configure ELM module if present.
> 
> But ideally BCH8 ecc lay out didn't have a dependency on ELM, it
> can work with software BCH ecc. RBL compatibility is missing
> in software BCH because of addition of constant polynomial to
> ecc vector. If we remove the dependency on erased page handling
> by ecc vector with constant polynomial, software BCH can do the
> job of RBL compatibility.
> 
> Ivan,
> Do you have any suggestions?
> Discussion for RBL compatibility found at [2].
> 
> It is good that software BCH also support RBL compatibility by suppressing
> constant polynomial modification. Then ecc layout can be selected from
> DT entry and error correction can be chosen between software/hardware
> depending on the availability of ELM hardware.
> Currently RBL BCH8 compatibility depends on the availability of ELM
> hardware. Later once software BCH start supporting RBL compatibility,
> we can remove  the check.
> 
>>
>> That is what I experienced, yes. The kernel was unable to parse NAND
>> pages that were written from U-Boot with bch8 hardware mode when the elm
>> module was not active. Maybe someone from TI can explain that? Giving it
>> a dedicated name would also solve the problem with the extra DT property.
> 
> Daniel,
> 
> Currently BCH8 is supported with software ecc error correction in mainline.
> The layout for BCH8 ECC layout is
> 0-1 -> BAD block markers
> 2-11-> oob free area
> 12-63-> BCH8 ECC.
> 
> RBL ECC layout is
> 0-1 -> BAD block markers
> 2-57-> BCH8 ecc layout
> 58-63-> OOB free area
> 
> As u-boot also maintain RBL ecc layout, write from U-boot
> and read from Linux requires compatibility with RBL ecc layout.
> The same is achieved in the patch [1], with by setting is_elm_used
> to true.
> 
> 1. https://lkml.org/lkml/2012/10/31/87
> 2. https://lkml.org/lkml/2012/10/11/20

So, after reading this, I'm still uncertain what's your preferred way of
handling the bindings here. Are you saying we should stick with the
is_elm_used flag, and subsequently care for BCH8 software mode
compatibility?

I would like to submit a new version soon, so it can be queued up for
the next merge window, and that decision is the last blocker currently
for sending out a new series.


Many thanks,
Daniel

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

[Philip, Avinash]

On Tue, Nov 27, 2012 at 19:30:54, Daniel Mack wrote:
> Hi Avinash,
> Hi Peter,
> 
> On 23.11.2012 11:43, Philip, Avinash wrote:
> > On Wed, Nov 21, 2012 at 15:45:23, Daniel Mack wrote:
> >> On 20.11.2012 16:59, Peter Korsgaard wrote:
> >>>>>>>> "Daniel" == Daniel Mack <zonque@gmail.com> writes:
> 
> > Peter,
> > 
> > In patch [1], mtd: nand: omap2: Support for hardware BCH
> > 
> > ecc_layout made compatible with Rom Boot Loader ECC layout for am335x.
> > 
> > This action is based on is_elm_used flag.
> > 
> > Requirement of this flag is to identify the whether the platform
> > ELM module & based on this configure ELM module if present.
> > 
> > But ideally BCH8 ecc lay out didn't have a dependency on ELM, it
> > can work with software BCH ecc. RBL compatibility is missing
> > in software BCH because of addition of constant polynomial to
> > ecc vector. If we remove the dependency on erased page handling
> > by ecc vector with constant polynomial, software BCH can do the
> > job of RBL compatibility.
> > 
> > Ivan,
> > Do you have any suggestions?
> > Discussion for RBL compatibility found at [2].
> > 
> > It is good that software BCH also support RBL compatibility by suppressing
> > constant polynomial modification. Then ecc layout can be selected from
> > DT entry and error correction can be chosen between software/hardware
> > depending on the availability of ELM hardware.
> > Currently RBL BCH8 compatibility depends on the availability of ELM
> > hardware. Later once software BCH start supporting RBL compatibility,
> > we can remove  the check.
> > 
> >>
> >> That is what I experienced, yes. The kernel was unable to parse NAND
> >> pages that were written from U-Boot with bch8 hardware mode when the elm
> >> module was not active. Maybe someone from TI can explain that? Giving it
> >> a dedicated name would also solve the problem with the extra DT property.
> > 
> > Daniel,
> > 
> > Currently BCH8 is supported with software ecc error correction in mainline.
> > The layout for BCH8 ECC layout is
> > 0-1 -> BAD block markers
> > 2-11-> oob free area
> > 12-63-> BCH8 ECC.
> > 
> > RBL ECC layout is
> > 0-1 -> BAD block markers
> > 2-57-> BCH8 ecc layout
> > 58-63-> OOB free area
> > 
> > As u-boot also maintain RBL ecc layout, write from U-boot
> > and read from Linux requires compatibility with RBL ecc layout.
> > The same is achieved in the patch [1], with by setting is_elm_used
> > to true.
> > 
> > 1. https://lkml.org/lkml/2012/10/31/87
> > 2. https://lkml.org/lkml/2012/10/11/20
> 
> So, after reading this, I'm still uncertain what's your preferred way of
> handling the bindings here. Are you saying we should stick with the
> is_elm_used flag, and subsequently care for BCH8 software mode
> compatibility?

Ideally RBL compatibility didn't depend on the availability of ELM module. So
from am335x perspective, RBL compatibility is achieved with ECC error correction
with ELM module. I would submit one more revision of BCH8 ELM error correction
support which would check for bch8-am335xrbl-compatible.
Note: RBL ecc layout can vary from SOC to SOC

Daniel,

So can you start supporting "bch8-am335xrbl-compatible" and remove usage of
is_elm_used in dt. This should come in ecc_opt field.

In omap2 NAND driver, AM335x RBL compatibility is achieved depending on ecc_layout
and runtime detection of elm module. So options related to can be
1. bch8-am335xrbl-compatible is enabled and run time detection of
   Of elm module passed, RBL compatibility achieved.
2. bch8-am335xrbl-compatible is enabled and run time detection of
   of elm module failed, RBL compatibility sacrificed but continue with
   software BCH8 error correction. Sacrificing RBL compatibility
   because of constant polynomial addition and usage of 14 byte instead of 13 byte.

Ivan,
Do you have any plan of removing addition of constant polynomial to ecc data
and go for extra byte checking to find erased pages?
This way we can start support BCH8 with RBL compatible and kernel
Didn't put any restriction of the ecc layout.

Thanks
Avinash

> 
> I would like to submit a new version soon, so it can be queued up for
> the next merge window, and that decision is the last blocker currently
> for sending out a new series.
> 
> 
> Many thanks,
> Daniel
> 
> 

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

[Ivan Djelic]

On Wed, Nov 28, 2012 at 05:01:30AM +0000, Philip, Avinash wrote:
(...)
> 
> Daniel,
> 
> So can you start supporting "bch8-am335xrbl-compatible" and remove usage of
> is_elm_used in dt. This should come in ecc_opt field.
> 
> In omap2 NAND driver, AM335x RBL compatibility is achieved depending on ecc_layout
> and runtime detection of elm module. So options related to can be
> 1. bch8-am335xrbl-compatible is enabled and run time detection of
>    Of elm module passed, RBL compatibility achieved.
> 2. bch8-am335xrbl-compatible is enabled and run time detection of
>    of elm module failed, RBL compatibility sacrificed but continue with
>    software BCH8 error correction. Sacrificing RBL compatibility
>    because of constant polynomial addition and usage of 14 byte instead of 13 byte.
> 
> Ivan,
> Do you have any plan of removing addition of constant polynomial to ecc data
> and go for extra byte checking to find erased pages?
> This way we can start support BCH8 with RBL compatible and kernel
> Didn't put any restriction of the ecc layout.

Hello Avinash,

Sorry about the response delay.
First a short reminder of pros and cons of the "constant polynomial addition"
(let's just call it "CPA") feature:

pros:
- it elegantly solves all problems related to reading an erased page (no clumsy hack needed)
- better performance: when a bitflip appears on an erased page (often this is a "sticky" bitflip),
  there is no need to perform a full scan+cleanup of the page each time it is read

cons:
- the ecc vector is not compatible with RBL

RBL compatibility is not necessary in my case, because I'm using the OMAP MLC ROM boot mode.
Rather than completely removing the CPA feature, I'd like to keep it as an option; it could
even be used with the ELM module.

I'm OK to submit a patch in this direction, but first I'd like to wait for the dust to settle
on arch/arm/mach-omap2 and mtd/nand/omap2.c with Afzal patches and everything; things have become
a bit complicated to follow recently :-)
Also, I think it would be nice to move BCH code out of omap2.c into a separate file.
What do you think ?

BR,
--
Ivan

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

[Philip, Avinash]

On Sun, Dec 02, 2012 at 03:20:14, Ivan Djelic wrote:
> On Wed, Nov 28, 2012 at 05:01:30AM +0000, Philip, Avinash wrote:
> (...)
> > 
> > Daniel,
> > 
> > So can you start supporting "bch8-am335xrbl-compatible" and remove usage of
> > is_elm_used in dt. This should come in ecc_opt field.
> > 
> > In omap2 NAND driver, AM335x RBL compatibility is achieved depending on ecc_layout
> > and runtime detection of elm module. So options related to can be
> > 1. bch8-am335xrbl-compatible is enabled and run time detection of
> >    Of elm module passed, RBL compatibility achieved.
> > 2. bch8-am335xrbl-compatible is enabled and run time detection of
> >    of elm module failed, RBL compatibility sacrificed but continue with
> >    software BCH8 error correction. Sacrificing RBL compatibility
> >    because of constant polynomial addition and usage of 14 byte instead of 13 byte.
> > 
> > Ivan,
> > Do you have any plan of removing addition of constant polynomial to ecc data
> > and go for extra byte checking to find erased pages?
> > This way we can start support BCH8 with RBL compatible and kernel
> > Didn't put any restriction of the ecc layout.
> 
> Hello Avinash,
> 
> Sorry about the response delay.
> First a short reminder of pros and cons of the "constant polynomial addition"
> (let's just call it "CPA") feature:
> 
> pros:
> - it elegantly solves all problems related to reading an erased page (no clumsy hack needed)
> - better performance: when a bitflip appears on an erased page (often this is a "sticky" bitflip),
>   there is no need to perform a full scan+cleanup of the page each time it is read

No need for scan + cleanup on each read, as the chances of encountering bit flips in erased page
is less. Also to find bit flips in erased page, compare ecc vector for read erased page against
a standard ecc vector. Presence of bit flips can find by checking the compare results. In case of
mismatch, should go for correction of bit flips in erased page with full scan.

So with this approach, we can nullify the effect of CPA for erased page bit flip handling.

> 
> cons:
> - the ecc vector is not compatible with RBL
> 
> RBL compatibility is not necessary in my case, because I'm using the OMAP MLC ROM boot mode.
> Rather than completely removing the CPA feature, I'd like to keep it as an option; it could
> even be used with the ELM module.

I agree RBL compatibility depends on the user. But with RBL compatibility, there is no sacrifice
of any existing feature. Is it right?

Also nand driver get simplified with removal of CPA, so that both HW & SW error correction
can go for same ecc calculation.

> 
> I'm OK to submit a patch in this direction, but first I'd like to wait for the dust to settle
> on arch/arm/mach-omap2 and mtd/nand/omap2.c with Afzal patches and everything; things have become
> a bit complicated to follow recently :-)

Afzal's changes are in & are settled now.

> Also, I think it would be nice to move BCH code out of omap2.c into a separate file.
> What do you think ?

With BCH code you mean, omap3_correct_data_bch()?

I think this can be part of omap2 driver it self as it is ecc correctable method for 
nand driver.

Thanks
Avinash

> 
> BR,
> --
> Ivan
> 

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

[Ivan Djelic]

On Wed, Dec 05, 2012 at 05:15:52AM +0000, Philip, Avinash wrote:
(...)
> > First a short reminder of pros and cons of the "constant polynomial addition"
> > (let's just call it "CPA") feature:
> > 
> > pros:
> > - it elegantly solves all problems related to reading an erased page (no clumsy hack needed)
> > - better performance: when a bitflip appears on an erased page (often this is a "sticky" bitflip),
> >   there is no need to perform a full scan+cleanup of the page each time it is read
> 
> No need for scan + cleanup on each read, as the chances of encountering bit flips in erased page
> is less. Also to find bit flips in erased page, compare ecc vector for read erased page against
> a standard ecc vector. Presence of bit flips can find by checking the compare results. In case of
> mismatch, should go for correction of bit flips in erased page with full scan.

Hi Avinash,

I explicitly mentioned the condition "when a bitflip appears on an erased page", in which case you
_do_ have to do a full scan upon each read, until you erase the block; and then, the bitflip may still be there
(this is what I called a "sticky" bitflip).
My experience with recent devices (4-bit/8-bit) is that erased pages with a single bitflip are not uncommon.
For those pages, there is undeniably a performance penalty compared to CPA.
Benchmarks would be needed to quantify the overall performance impact, but I suspect it is small.

> 
> So with this approach, we can nullify the effect of CPA for erased page bit flip handling.

Well, not completely. I would happily drop CPA is that were the case.
 
> > 
> > cons:
> > - the ecc vector is not compatible with RBL
> > 
> > RBL compatibility is not necessary in my case, because I'm using the OMAP MLC ROM boot mode.
> > Rather than completely removing the CPA feature, I'd like to keep it as an option; it could
> > even be used with the ELM module.
> 
> I agree RBL compatibility depends on the user. But with RBL compatibility, there is no sacrifice
> of any existing feature. Is it right?
> 
> Also nand driver get simplified with removal of CPA, so that both HW & SW error correction
> can go for same ecc calculation.

Indeed that would be a simplification.

> > 
> > I'm OK to submit a patch in this direction, but first I'd like to wait for the dust to settle
> > on arch/arm/mach-omap2 and mtd/nand/omap2.c with Afzal patches and everything; things have become
> > a bit complicated to follow recently :-)
> 
> Afzal's changes are in & are settled now.

What about this set: http://lists.infradead.org/pipermail/linux-mtd/2012-October/044337.html
I cannot see it in l2-mtd-2.6 ? Or did I miss something ?

Thanks,
--
Ivan

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

[Philip, Avinash]

On Thu, Dec 06, 2012 at 15:45:41, Ivan Djelic wrote:
> On Wed, Dec 05, 2012 at 05:15:52AM +0000, Philip, Avinash wrote:
> (...)
> > > First a short reminder of pros and cons of the "constant polynomial addition"
> > > (let's just call it "CPA") feature:
> > > 
> > > pros:
> > > - it elegantly solves all problems related to reading an erased page (no clumsy hack needed)
> > > - better performance: when a bitflip appears on an erased page (often this is a "sticky" bitflip),
> > >   there is no need to perform a full scan+cleanup of the page each time it is read
> > 
> > No need for scan + cleanup on each read, as the chances of encountering bit flips in erased page
> > is less. Also to find bit flips in erased page, compare ecc vector for read erased page against
> > a standard ecc vector. Presence of bit flips can find by checking the compare results. In case of
> > mismatch, should go for correction of bit flips in erased page with full scan.
> 
> Hi Avinash,
> 
> I explicitly mentioned the condition "when a bitflip appears on an erased page", in which case you
> _do_ have to do a full scan upon each read, until you erase the block; and then, the bitflip may still
> be there (this is what I called a "sticky" bitflip).

Bit flips in erased page require full scan.

> My experience with recent devices (4-bit/8-bit) is that erased pages with a 
> single bitflip are not uncommon.

What about erased page without bit flips? If we take the probability of bit flips 
and non-bit flips in erased page, erased pages with bit flips will be very less.

So probability of scanning erased page is less.

> For those pages, there is undeniably a performance penalty compared to CPA.

Pages with bit flips would have better performance with CPA as no explicit scan present.
Pages without bit flips, would have better performance if we compare against standard ecc
vector than with present implementation of software BCH.

The same can be achieved with software BCH also.


> Benchmarks would be needed to quantify the overall performance impact, but I suspect it is small.

So overall performance would be better with checking with standard ecc vector for finding erased page
and handling bit flip in erased page with full scan due to probability that bit flips in erased page 
is very less than pages without bit flips.
But with performance improvement for bit flipped erased page with CPA, we have to sacrifice RBL compatibility.
So a common layout across all components would be missing.

> 
> > 
> > So with this approach, we can nullify the effect of CPA for erased page bit flip handling.
> 
> Well, not completely. I would happily drop CPA is that were the case.

So the choices can be 

1. Performance drop in case of bit flip in erased page with full scan of page if bit flip in
   erased page. But can achieve RBL compatibility, simplification of ecc calculation.
2. Sacrifice RBL compatibility with CPA, but still performance can be improved for 
   erased pages without bit flips.

But initial discussion in this direction told RBL compatibility would be a better option
https://lkml.org/lkml/2012/10/11/240


>  
> > > 
> > > cons:
> > > - the ecc vector is not compatible with RBL
> > > 
> > > RBL compatibility is not necessary in my case, because I'm using the OMAP MLC ROM boot mode.
> > > Rather than completely removing the CPA feature, I'd like to keep it as an option; it could
> > > even be used with the ELM module.
> > 
> > I agree RBL compatibility depends on the user. But with RBL compatibility, there is no sacrifice
> > of any existing feature. Is it right?
> > 
> > Also nand driver get simplified with removal of CPA, so that both HW & SW error correction
> > can go for same ecc calculation.
> 
> Indeed that would be a simplification.
> 
> > > 
> > > I'm OK to submit a patch in this direction, but first I'd like to wait for the dust to settle
> > > on arch/arm/mach-omap2 and mtd/nand/omap2.c with Afzal patches and everything; things have become
> > > a bit complicated to follow recently :-)
> > 
> > Afzal's changes are in & are settled now.
> 
> What about this set: http://lists.infradead.org/pipermail/linux-mtd/2012-October/044337.html
> I cannot see it in l2-mtd-2.6 ? Or did I miss something ?

Yes, Afzal's changes are present in linux-next. I think it will get in next merge window.

http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=history;f=drivers/mtd/nand/omap2.c;h=0002d5e94f0d0e3b84f36d2ccb505c95a30b4cdb;hb=cfc4410f5d3de0f68139ddffe065b9889a41d3c0

These changes not present in l2-mtd-2.6

Thanks
Avinash

> 
> Thanks,
> --
> Ivan
> 

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

[Jon Hunter]

On 11/02/2012 10:25 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

Thanks. But I am still not completely happy with this. You may wish to
wait until we have resolved all the current comments before sending out
another version.

Cheers
Jon