[PATCH v2 0/5] Assorted OMAP2 NAND clean-ups

[PATCH v2 0/5] Assorted OMAP2 NAND clean-ups

[Ezequiel Garcia]

Hello everyone,

As it was discussed recently in the mailing list, the omap2-nand driver currently
has an issue preventing proper ONFI detection of 16-bit devices (other drivers
may suffer from this same issue).

In an attempt to address such issue, patch 2/5 "mtd: nand: omap2: Fix device
detection path" uses NAND_BUSWIDTH_AUTO (actually discarding any DT property)
and leaves the bus width detection the NAND core code.

I've based the patch in previously sent work by Pekon Gupta, did some work and
added my name on it. I hope it's fine to handle the authorship like this.
Otherwise, feel free to tell.

This has been tested in a Beaglebone black (AM335x) board with a 16-bit Micron
NAND device, both ONFI and array-based detections work fine:

[    1.253200] omap-gpmc 50000000.gpmc: GPMC revision 6.0
[    1.260638] NAND device: Manufacturer ID: 0x2c, Chip ID: 0xcc (Micron MT29F4G16ABADAH4)
[    1.269110] NAND device: 512MiB, SLC, page size: 2048, OOB size: 64
[    1.275721] Missing elm_id property, fall back to Software BCH
[    1.283546] enabling NAND BCH ecc with 8-bit correction
[    1.290219] 8 ofpart partitions found on MTD device omap2-nand.0
[    1.296591] Creating 8 MTD partitions on "omap2-nand.0":
[    1.302192] 0x000000000000-0x000000020000 : "SPL1"
[    1.309884] 0x000000020000-0x000000040000 : "SPL2"
[    1.316332] 0x000000040000-0x000000060000 : "SPL3"
[    1.322599] 0x000000060000-0x000000080000 : "SPL4"
[    1.328877] 0x000000080000-0x000000260000 : "U-boot"
[    1.336763] 0x000000260000-0x000000280000 : "environment"
[    1.343575] 0x000000280000-0x000000780000 : "Kernel"
[    1.354074] 0x000000780000-0x000010000000 : "File-System"

Also, a quick run of nandtest ends successfully:

$ nandtest /dev/mtd7
ECC corrections: 0
ECC failures   : 0
Bad blocks     : 2
BBT blocks     : 0
Bad block at 0x00000000
Bad block at 0x00520000
[...]
Finished pass 1 successfully

The first patch 1/5 is included just for reference of the DT GPMC configuration
currently in use, together with required pinmuxing, in case anyone feels like
testing. It's not intended to be merged, since it doesn't belong to the Beaglebone
board itself, but instead it's an attached daughter board.

The other patches are just minor cleanups I came across while looking at the code.
As usual, the patchset applies on top of latest l2-mtd's master branch.

Pekon, Brian: Do you think this solution might work for 8-bit and 16-bit devices?

I've been going through the GPMC code again, and it looks like some cleaning is needed
there two, but that's a different story!

Feel free to comment or provide any feedback!

Changes from v1:

  * As noticed by Belisko Marek, re-added the out_free_info label
    in PATCH 4/5 to prevent build failures. The result is the same
    and the label is removed in the last patch, together with the last
    users of it.

Ezequiel Garcia (3):
  mtd: nand: omap2: Fix OMAP_BCH option dependency
  mtd: nand: omap2: Use devm_kzalloc
  mtd: nand: omap2: Use devm_ioremap_resource

Pekon Gupta (2):
  ARM: am335x-bone: Add support for 16-bit NAND cape
  mtd: nand: omap2: Fix device detection path

 arch/arm/boot/dts/am335x-bone-common.dtsi | 28 ++++++++++++
 arch/arm/boot/dts/am335x-bone.dts         | 75 +++++++++++++++++++++++++++++++
 arch/arm/boot/dts/am335x-boneblack.dts    | 69 ++++++++++++++++++++++++++++
 drivers/mtd/nand/Kconfig                  |  2 +-
 drivers/mtd/nand/omap2.c                  | 52 +++++----------------
 5 files changed, 185 insertions(+), 41 deletions(-)

-- 
1.8.1.5

[PATCH v2 1/5] ARM: am335x-bone: Add support for 16-bit NAND cape

[Ezequiel Garcia]

From: Pekon Gupta <pekon@ti.com>

Add GPMC timing configurations and NAND partitions for the Beaglebone
"NAND cape". Further information and datasheets of this cape can be found at:
- http://beagleboardtoys.info/index.php?title=BeagleBone_Memory_Expansion
- http://beagleboardtoys.info/index.php?title=BeagleBone_4Gb_16-Bit_NAND_Module

Signed-off-by: Pekon Gupta <pekon@ti.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
This patch is not really intended for inclusion. I'm just adding it
to the series for reference and in case anyone wants to test.

In particular, this patch includes two different GPMC configurations:
one at am335x-bone-common.dtsi, and another one at am335x-boneblack.dts,
overriding the first one. The former is Pekon's, while the latter is mine,
to support the flash device on my setup.

 arch/arm/boot/dts/am335x-bone-common.dtsi | 28 ++++++++++++
 arch/arm/boot/dts/am335x-bone.dts         | 75 +++++++++++++++++++++++++++++++
 arch/arm/boot/dts/am335x-boneblack.dts    | 69 ++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+)

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi b/arch/arm/boot/dts/am335x-bone-common.dtsi
index 2f66ded..73dcc58 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -107,6 +107,34 @@
 				0x14c (PIN_INPUT_PULLDOWN | MUX_MODE7)
 			>;
 		};
+
+		nandflash_pins_s0: nandflash_pins_s0 {
+			pinctrl-single,pins = <
+				0x0 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad0.gpmc_ad0 */
+				0x4 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad1.gpmc_ad1 */
+				0x8 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad2.gpmc_ad2 */
+				0xc (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad3.gpmc_ad3 */
+				0x10 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad4.gpmc_ad4 */
+				0x14 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad5.gpmc_ad5 */
+				0x18 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad6.gpmc_ad6 */
+				0x1c (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad7.gpmc_ad7 */
+				0x20 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad8.gpmc_ad8 */
+				0x24 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad9.gpmc_ad9 */
+				0x28 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad10.gpmc_ad10 */
+				0x2c (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad11.gpmc_ad11 */
+				0x30 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad12.gpmc_ad12 */
+				0x34 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad13.gpmc_ad13 */
+				0x38 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad14.gpmc_ad14 */
+				0x3c (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_ad15.gpmc_ad15 */
+				0x70 (PIN_INPUT_PULLUP | MUX_MODE0)	/* gpmc_wait0.gpmc_wait0 */
+				0x74 (PIN_INPUT_PULLUP | MUX_MODE7)	/* gpmc_wpn.gpio0_30 */
+				0x7c (PIN_OUTPUT | MUX_MODE0)		/* gpmc_csn0.gpmc_csn0  */
+				0x90 (PIN_OUTPUT | MUX_MODE0)		/* gpmc_advn_ale.gpmc_advn_ale */
+				0x94 (PIN_OUTPUT | MUX_MODE0)		/* gpmc_oen_ren.gpmc_oen_ren */
+				0x98 (PIN_OUTPUT | MUX_MODE0)		/* gpmc_wen.gpmc_wen */
+				0x9c (PIN_OUTPUT | MUX_MODE0)		/* gpmc_be0n_cle.gpmc_be0n_cle */
+			>;
+		};
 	};
 
 	ocp {
diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts
index 7993c48..d9172c7 100644
--- a/arch/arm/boot/dts/am335x-bone.dts
+++ b/arch/arm/boot/dts/am335x-bone.dts
@@ -9,3 +9,78 @@
 
 #include "am33xx.dtsi"
 #include "am335x-bone-common.dtsi"
+
+&elm {
+	status = "okay";
+};
+
+&gpmc {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&nandflash_pins_s0>;
+	ranges = <0 0 0x08000000 0x10000000>;	/* CS0: NAND */
+	nand@0,0 {
+		reg = <0 0 0>; /* CS0, offset 0 */
+		nand-bus-width = <16>;
+		ti,nand-ecc-opt = "bch8";
+		gpmc,device-width = <2>;
+		gpmc,sync-clk-ps = <0>;
+		gpmc,cs-on-ns = <0>;
+		gpmc,cs-rd-off-ns = <740>;
+		gpmc,cs-wr-off-ns = <740>;
+		gpmc,adv-on-ns = <0>;
+		gpmc,adv-rd-off-ns = <740>;
+		gpmc,adv-wr-off-ns = <740>;
+		gpmc,we-on-ns = <90>;
+		gpmc,we-off-ns = <600>;
+		gpmc,oe-on-ns = <150>;
+		gpmc,oe-off-ns = <650>;
+		gpmc,access-ns = <560>;
+		gpmc,rd-cycle-ns = <740>;
+		gpmc,wr-cycle-ns = <740>;
+		gpmc,wait-on-read = "true";
+		gpmc,wait-on-write = "true";
+		gpmc,bus-turnaround-ns = <0>;
+		gpmc,cycle2cycle-delay-ns = <0>;
+		gpmc,clk-activation-ns = <0>;
+		gpmc,wait-monitoring-ns = <0>;
+		gpmc,wr-access-ns = <40>;
+		gpmc,wr-data-mux-bus-ns = <0>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ti,elm-id = <&elm>;
+		/* MTD partition table */
+		partition@0 {
+			label = "SPL1";
+			reg = <0x00000000 0x000020000>;
+		};
+		partition@1 {
+			label = "SPL2";
+			reg = <0x00020000 0x00020000>;
+		};
+		partition@2 {
+			label = "SPL3";
+			reg = <0x00040000 0x00020000>;
+		};
+		partition@3 {
+			label = "SPL4";
+			reg = <0x00060000 0x00020000>;
+		};
+		partition@4 {
+			label = "U-boot";
+			reg = <0x00080000 0x001e0000>;
+		};
+		partition@5 {
+			label = "environment";
+			reg = <0x00260000 0x00020000>;
+		};
+		partition@6 {
+			label = "Kernel";
+			reg = <0x00280000 0x00500000>;
+		};
+		partition@7 {
+			label = "File-System";
+			reg = <0x00780000 0x0F880000>;
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
index 197cadf..e30f58a 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -15,3 +15,72 @@
 	regulator-max-microvolt = <1800000>;
 	regulator-always-on;
 };
+
+&elm {
+	status = "okay";
+};
+
+&gpmc {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&nandflash_pins_s0>;
+	ranges = <0 0 0x08000000 0x10000000>;	/* CS0: NAND */
+	nand@0,0 {
+		reg = <0 0 0>; /* CS0, offset 0 */
+		nand-bus-width = <16>;
+		ti,nand-ecc-opt = "bch8";
+
+		gpmc,device-width = <2>;
+		gpmc,sync-clk-ps = <0>;
+		gpmc,cs-on-ns = <0>;
+		gpmc,cs-rd-off-ns = <44>;
+		gpmc,cs-wr-off-ns = <44>;
+		gpmc,adv-on-ns = <6>;
+		gpmc,adv-rd-off-ns = <34>;
+		gpmc,adv-wr-off-ns = <44>;
+		gpmc,we-off-ns = <40>;
+		gpmc,oe-off-ns = <54>;
+		gpmc,access-ns = <64>;
+		gpmc,rd-cycle-ns = <82>;
+		gpmc,wr-cycle-ns = <82>;
+		gpmc,wr-access-ns = <40>;
+		gpmc,wr-data-mux-bus-ns = <0>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ti,elm-id = <&elm>;
+		/* MTD partition table */
+		partition@0 {
+			label = "SPL1";
+			reg = <0x00000000 0x000020000>;
+		};
+		partition@1 {
+			label = "SPL2";
+			reg = <0x00020000 0x00020000>;
+		};
+		partition@2 {
+			label = "SPL3";
+			reg = <0x00040000 0x00020000>;
+		};
+		partition@3 {
+			label = "SPL4";
+			reg = <0x00060000 0x00020000>;
+		};
+		partition@4 {
+			label = "U-boot";
+			reg = <0x00080000 0x001e0000>;
+		};
+		partition@5 {
+			label = "environment";
+			reg = <0x00260000 0x00020000>;
+		};
+		partition@6 {
+			label = "Kernel";
+			reg = <0x00280000 0x00500000>;
+		};
+		partition@7 {
+			label = "File-System";
+			reg = <0x00780000 0x0F880000>;
+		};
+	};
+};
-- 
1.8.1.5

[PATCH v2 2/5] mtd: nand: omap2: Fix device detection path

[Ezequiel Garcia]

From: Pekon Gupta <pekon@ti.com>

Because the device bus can be 8-bit or 16-bit width, yet ONFI detection
cannot work in 16-bit mode, we need to set the NAND_BUSWIDTH_AUTO option
which allows proper initialization configuration.

Once the bus width is detected, nand_scan_ident() updates the nand_chip struct
'option' field to use the appropriate read/write functions and configure
the ECC engine.

Signed-off-by: Pekon Gupta <pekon@ti.com>
[rebased and clean-up a bit pekon's original work]
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/omap2.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 4ecf0e5..e01a936 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1855,8 +1855,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	info->mtd.name		= dev_name(&pdev->dev);
 	info->mtd.owner		= THIS_MODULE;
 
-	info->nand.options	= pdata->devsize;
-	info->nand.options	|= NAND_SKIP_BBTSCAN;
+	info->nand.options	|= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
 	info->of_node		= pdata->of_node;
 #endif
@@ -1903,6 +1902,10 @@ static int omap_nand_probe(struct platform_device *pdev)
 		info->nand.chip_delay = 50;
 	}
 
+	err = nand_scan_ident(&info->mtd, 1, NULL);
+	if (err < 0)
+		goto out_release_mem_region;
+
 	switch (pdata->xfer_type) {
 	case NAND_OMAP_PREFETCH_POLLED:
 		info->nand.read_buf   = omap_read_buf_pref;
@@ -2013,17 +2016,6 @@ static int omap_nand_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* DIP switches on some boards change between 8 and 16 bit
-	 * bus widths for flash.  Try the other width if the first try fails.
-	 */
-	if (nand_scan_ident(&info->mtd, 1, NULL)) {
-		info->nand.options ^= NAND_BUSWIDTH_16;
-		if (nand_scan_ident(&info->mtd, 1, NULL)) {
-			err = -ENXIO;
-			goto out_release_mem_region;
-		}
-	}
-
 	/* rom code layout */
 	if (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) {
 
-- 
1.8.1.5

[PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option dependency

[Ezequiel Garcia]

This option does not need to depend in MTD_NAND, for it's enclosed
under it. Also, it's wrong to make it depend in ARCH_OMAP3 only
since the controller is used in a wider range of SoCs.

Instead, just leave the dependency on the OMAP2 driver option.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index d885298..8187466 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -95,7 +95,7 @@ config MTD_NAND_OMAP2
 	  platforms.
 
 config MTD_NAND_OMAP_BCH
-	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
+	depends on MTD_NAND_OMAP2
 	tristate "Enable support for hardware BCH error correction"
 	default n
 	select BCH
-- 
1.8.1.5

RE: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option dependency

[Gupta, Pekon]

> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> Subject: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option
> dependency
> 
> This option does not need to depend in MTD_NAND, for it's enclosed
> under it. Also, it's wrong to make it depend in ARCH_OMAP3 only
> since the controller is used in a wider range of SoCs.
> 
> Instead, just leave the dependency on the OMAP2 driver option.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/nand/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index d885298..8187466 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -95,7 +95,7 @@ config MTD_NAND_OMAP2
>  	  platforms.
> 
>  config MTD_NAND_OMAP_BCH
> -	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
> +	depends on MTD_NAND_OMAP2
>  	tristate "Enable support for hardware BCH error correction"
>  	default n
>  	select BCH
> --
> 1.8.1.5

Acked-by: Pekon Gupta <pekon@ti.com>
Reported-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

with regards, pekon

Re: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option dependency

[Ezequiel Garcia]

On Fri, Oct 25, 2013 at 11:26:06AM +0000, Gupta, Pekon wrote:
> > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > Subject: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option
> > dependency
> > 
> > This option does not need to depend in MTD_NAND, for it's enclosed
> > under it. Also, it's wrong to make it depend in ARCH_OMAP3 only
> > since the controller is used in a wider range of SoCs.
> > 
> > Instead, just leave the dependency on the OMAP2 driver option.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  drivers/mtd/nand/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > index d885298..8187466 100644
> > --- a/drivers/mtd/nand/Kconfig
> > +++ b/drivers/mtd/nand/Kconfig
> > @@ -95,7 +95,7 @@ config MTD_NAND_OMAP2
> >  	  platforms.
> > 
> >  config MTD_NAND_OMAP_BCH
> > -	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
> > +	depends on MTD_NAND_OMAP2
> >  	tristate "Enable support for hardware BCH error correction"
> >  	default n
> >  	select BCH
> > --
> > 1.8.1.5
> 
> Acked-by: Pekon Gupta <pekon@ti.com>

Thanks!

> Reported-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 

FWIW, If I'm signing-off this commit, it's implicit that I'm the
reporter.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option dependency

[Ezequiel Garcia]

On Fri, Oct 25, 2013 at 11:26:06AM +0000, Gupta, Pekon wrote:
> > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > Subject: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option
> > dependency
> > 
> > This option does not need to depend in MTD_NAND, for it's enclosed
> > under it. Also, it's wrong to make it depend in ARCH_OMAP3 only
> > since the controller is used in a wider range of SoCs.
> > 
> > Instead, just leave the dependency on the OMAP2 driver option.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  drivers/mtd/nand/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > index d885298..8187466 100644
> > --- a/drivers/mtd/nand/Kconfig
> > +++ b/drivers/mtd/nand/Kconfig
> > @@ -95,7 +95,7 @@ config MTD_NAND_OMAP2
> >  	  platforms.
> > 
> >  config MTD_NAND_OMAP_BCH
> > -	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
> > +	depends on MTD_NAND_OMAP2
> >  	tristate "Enable support for hardware BCH error correction"
> >  	default n
> >  	select BCH
> > --
> > 1.8.1.5
> 
> Acked-by: Pekon Gupta <pekon@ti.com>

Brian,

I almost forgot about this one, and I just came across the issue
while configuring my board.

I believe this one is simple enough to be pulled in for v3.14
(it could be in v3.13, but it's already too late).

Or maybe you're not taking stuff for v3.14 yet...
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option dependency

[Brian Norris]

On Tue, Nov 12, 2013 at 05:45:56PM -0300, Ezequiel Garcia wrote:
> On Fri, Oct 25, 2013 at 11:26:06AM +0000, Gupta, Pekon wrote:
> > > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > > Subject: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option
> > > dependency
> > > 
> > > This option does not need to depend in MTD_NAND, for it's enclosed
> > > under it. Also, it's wrong to make it depend in ARCH_OMAP3 only
> > > since the controller is used in a wider range of SoCs.
> > > 
> > > Instead, just leave the dependency on the OMAP2 driver option.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > >  drivers/mtd/nand/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > > index d885298..8187466 100644
> > > --- a/drivers/mtd/nand/Kconfig
> > > +++ b/drivers/mtd/nand/Kconfig
> > > @@ -95,7 +95,7 @@ config MTD_NAND_OMAP2
> > >  	  platforms.
> > > 
> > >  config MTD_NAND_OMAP_BCH
> > > -	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
> > > +	depends on MTD_NAND_OMAP2
> > >  	tristate "Enable support for hardware BCH error correction"
> > >  	default n
> > >  	select BCH
> > > --
> > > 1.8.1.5
> > 
> > Acked-by: Pekon Gupta <pekon@ti.com>
> 
> Brian,
> 
> I almost forgot about this one, and I just came across the issue
> while configuring my board.
> 
> I believe this one is simple enough to be pulled in for v3.14
> (it could be in v3.13, but it's already too late).
> 
> Or maybe you're not taking stuff for v3.14 yet...

Thanks for the reminder. The rest of the series has been discussed
separately or is otherwise superceded by other patch series, right?

Applied this one patch to l2-mtd.git, branch 'next'. Thanks.

Brian

Re: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option dependency

[Ezequiel Garcia]

On Tue, Nov 12, 2013 at 02:56:47PM -0800, Brian Norris wrote:
> On Tue, Nov 12, 2013 at 05:45:56PM -0300, Ezequiel Garcia wrote:
> > On Fri, Oct 25, 2013 at 11:26:06AM +0000, Gupta, Pekon wrote:
> > > > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > > > Subject: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option
> > > > dependency
> > > > 
> > > > This option does not need to depend in MTD_NAND, for it's enclosed
> > > > under it. Also, it's wrong to make it depend in ARCH_OMAP3 only
> > > > since the controller is used in a wider range of SoCs.
> > > > 
> > > > Instead, just leave the dependency on the OMAP2 driver option.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > > ---
> > > >  drivers/mtd/nand/Kconfig | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > > > index d885298..8187466 100644
> > > > --- a/drivers/mtd/nand/Kconfig
> > > > +++ b/drivers/mtd/nand/Kconfig
> > > > @@ -95,7 +95,7 @@ config MTD_NAND_OMAP2
> > > >  	  platforms.
> > > > 
> > > >  config MTD_NAND_OMAP_BCH
> > > > -	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
> > > > +	depends on MTD_NAND_OMAP2
> > > >  	tristate "Enable support for hardware BCH error correction"
> > > >  	default n
> > > >  	select BCH
> > > > --
> > > > 1.8.1.5
> > > 
> > > Acked-by: Pekon Gupta <pekon@ti.com>
> > 
> > Brian,
> > 
> > I almost forgot about this one, and I just came across the issue
> > while configuring my board.
> > 
> > I believe this one is simple enough to be pulled in for v3.14
> > (it could be in v3.13, but it's already too late).
> > 
> > Or maybe you're not taking stuff for v3.14 yet...
> 
> Thanks for the reminder. The rest of the series has been discussed
> separately or is otherwise superceded by other patch series, right?
> 

The rest of the series can be divided in two groups:

1. Trivial stuff: I'll sent re-based patches on top of v3.13-rc1
or maybe Pekon will take care of it. It's not a big deal.

2. The ONFI detection stuff: it's not trivial and it's still under
discussion in separate threads. The last work was suggested by Pekon
in his last mail, and I guess he'll post a separate patch soon.

> Applied this one patch to l2-mtd.git, branch 'next'. Thanks.

Great, thanks.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/5] mtd: nand: omap2: Use devm_kzalloc

[Ezequiel Garcia]

This simplifies the error path and makes the code less error-prone.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/omap2.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index e01a936..d3155b2 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1837,7 +1837,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	info = kzalloc(sizeof(struct omap_nand_info), GFP_KERNEL);
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
@@ -2067,8 +2067,6 @@ out_release_mem_region:
 		free_irq(info->gpmc_irq_fifo, info);
 	release_mem_region(info->phys_base, info->mem_size);
 out_free_info:
-	kfree(info);
-
 	return err;
 }
 
@@ -2091,7 +2089,6 @@ static int omap_nand_remove(struct platform_device *pdev)
 	nand_release(&info->mtd);
 	iounmap(info->nand.IO_ADDR_R);
 	release_mem_region(info->phys_base, info->mem_size);
-	kfree(info);
 	return 0;
 }
 
-- 
1.8.1.5

RE: [PATCH v2 4/5] mtd: nand: omap2: Use devm_kzalloc

[Gupta, Pekon]

Hi,

> 
> This simplifies the error path and makes the code less error-prone.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/nand/omap2.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index e01a936..d3155b2 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1837,7 +1837,7 @@ static int omap_nand_probe(struct
> platform_device *pdev)
>  		return -ENODEV;
>  	}
> 
> -	info = kzalloc(sizeof(struct omap_nand_info), GFP_KERNEL);
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
> 
> @@ -2067,8 +2067,6 @@ out_release_mem_region:
>  		free_irq(info->gpmc_irq_fifo, info);
>  	release_mem_region(info->phys_base, info->mem_size);
>  out_free_info:
> -	kfree(info);
> -
>  	return err;
>  }
> 
> @@ -2091,7 +2089,6 @@ static int omap_nand_remove(struct
> platform_device *pdev)
>  	nand_release(&info->mtd);
>  	iounmap(info->nand.IO_ADDR_R);
>  	release_mem_region(info->phys_base, info->mem_size);
> -	kfree(info);
>  	return 0;
>  }
> 
> --
> 1.8.1.5

I think these changes are already done as part of following patch..
http://lists.infradead.org/pipermail/linux-mtd/2013-October/049418.html

Did your rebase on my patch-set ?


with regards, pekon

Re: [PATCH v2 4/5] mtd: nand: omap2: Use devm_kzalloc

[Ezequiel Garcia]

On Fri, Oct 25, 2013 at 10:25:02AM +0000, Gupta, Pekon wrote:
> Hi,
> 
> > 
> > This simplifies the error path and makes the code less error-prone.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  drivers/mtd/nand/omap2.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> > index e01a936..d3155b2 100644
> > --- a/drivers/mtd/nand/omap2.c
> > +++ b/drivers/mtd/nand/omap2.c
> > @@ -1837,7 +1837,7 @@ static int omap_nand_probe(struct
> > platform_device *pdev)
> >  		return -ENODEV;
> >  	}
> > 
> > -	info = kzalloc(sizeof(struct omap_nand_info), GFP_KERNEL);
> > +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> >  	if (!info)
> >  		return -ENOMEM;
> > 
> > @@ -2067,8 +2067,6 @@ out_release_mem_region:
> >  		free_irq(info->gpmc_irq_fifo, info);
> >  	release_mem_region(info->phys_base, info->mem_size);
> >  out_free_info:
> > -	kfree(info);
> > -
> >  	return err;
> >  }
> > 
> > @@ -2091,7 +2089,6 @@ static int omap_nand_remove(struct
> > platform_device *pdev)
> >  	nand_release(&info->mtd);
> >  	iounmap(info->nand.IO_ADDR_R);
> >  	release_mem_region(info->phys_base, info->mem_size);
> > -	kfree(info);
> >  	return 0;
> >  }
> > 
> > --
> > 1.8.1.5
> 
> I think these changes are already done as part of following patch..
> http://lists.infradead.org/pipermail/linux-mtd/2013-October/049418.html
> 
> Did your rebase on my patch-set ?
> 

Hm.. well the problem with that patch is that it's in the middle of an
unrelated series. As I already told you, I think you should have pushed
that as a one-patch fix. Have you seen that suggestion?

On the other side, you're fixing too many things in that single patch,
for my taste. Maybe I'm not the smarter developer, but going through
that patch is not easy to catch if there's no mistake done.

Usually if it's possible to split a patch (maintaining consistency) it makes
the reviewing process easier.
If you'd rather send this devm_xxx change yourself that's fine by me,
but *please* split the patch in two and write proper commit messages.

Anyway: this is just a silly change, the important one is the other
nand_scan_ident() fix. Could you help me review that?

I'm interested in knowing how will that work with 8-bit and 16-bit devices.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 4/5] mtd: nand: omap2: Use devm_kzalloc

[Gupta, Pekon]

> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> 
> Hm.. well the problem with that patch is that it's in the middle of an
> unrelated series. As I already told you, I think you should have pushed
> that as a one-patch fix. Have you seen that suggestion?
> 
Yes, I know.. actually the original patch series, when it started somewhere
April (or before) is very different from the version v11 now :-).
This devm_ update was added in middle of v6-v7 version change
(Most of the changes since first version of this patchset is captured in 
Cover-letter).


> On the other side, you're fixing too many things in that single patch,
> for my taste. Maybe I'm not the smarter developer, but going through
> that patch is not easy to catch if there's no mistake done.
> 
> Usually if it's possible to split a patch (maintaining consistency) it makes
> the reviewing process easier.
> If you'd rather send this devm_xxx change yourself that's fine by me,
> 
Ahh nothing like that.. Brian had already reviewed these couple of times
And it was only [Patch 04/10] which was last one remaining..
I just said it because this might show up in merge conflict .. or rejects..

> but *please* split the patch in two and write proper commit messages.
> 
> Anyway: this is just a silly change, the important one is the other
> nand_scan_ident() fix. Could you help me review that?
> 
> I'm interested in knowing how will that work with 8-bit and 16-bit devices.
> --
Yes, I'm just preparing the scenario where BUSWIDTH_AUTO would fail..
unless you do GPMC driver changes also.. same issue was found by
Matthieu CASTET (matthieu.castet@parrot.com)
(please see my other mail)


with regards, pekon

Re: [PATCH v2 4/5] mtd: nand: omap2: Use devm_kzalloc

[Ezequiel Garcia]

On Fri, Oct 25, 2013 at 11:09:14AM +0000, Gupta, Pekon wrote:
> > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > 
> > Hm.. well the problem with that patch is that it's in the middle of an
> > unrelated series. As I already told you, I think you should have pushed
> > that as a one-patch fix. Have you seen that suggestion?
> > 
> Yes, I know.. actually the original patch series, when it started somewhere
> April (or before) is very different from the version v11 now :-).
> This devm_ update was added in middle of v6-v7 version change
> (Most of the changes since first version of this patchset is captured in 
> Cover-letter).
> 
> 

Well, in order to *avoid* having a patchset flowing for 5 months and 11
revisions you coudl try to keep series small. You could have that single
fix merged if you send it alone. Not sure why you insist in *not* doing
that.

> > On the other side, you're fixing too many things in that single patch,
> > for my taste. Maybe I'm not the smarter developer, but going through
> > that patch is not easy to catch if there's no mistake done.
> > 
> > Usually if it's possible to split a patch (maintaining consistency) it makes
> > the reviewing process easier.
> > If you'd rather send this devm_xxx change yourself that's fine by me,
> > 
> Ahh nothing like that.. Brian had already reviewed these couple of times

Ah, good. In that case you should add "Reviewed-by" if Brian already
reviewed it. IMHO, the patch could be cleaner and the commit message
could be better.

> And it was only [Patch 04/10] which was last one remaining..

Yes, and because you added *another* patch to the series you keep
spinning patchset versions.

> I just said it because this might show up in merge conflict .. or rejects..
> 
> > but *please* split the patch in two and write proper commit messages.
> > 
> > Anyway: this is just a silly change, the important one is the other
> > nand_scan_ident() fix. Could you help me review that?
> > 
> > I'm interested in knowing how will that work with 8-bit and 16-bit devices.
> > --
> Yes, I'm just preparing the scenario where BUSWIDTH_AUTO would fail..
> unless you do GPMC driver changes also.. same issue was found by
> Matthieu CASTET (matthieu.castet@parrot.com)
> (please see my other mail)
> 

OK, let's try to focus in that patch alone, I'd like to move forward.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] mtd: nand: omap2: Use devm_ioremap_resource

[Ezequiel Garcia]

This simplifies the code and makes it less error-prone. In fact,
this commit fixes a missing iounmap() in the cleanup error path.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/omap2.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index d3155b2..2d896da 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1830,6 +1830,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	unsigned sig;
 	struct resource			*res;
 	struct mtd_part_parser_data	ppdata = {};
+	void __iomem			*base;
 
 	pdata = dev_get_platdata(&pdev->dev);
 	if (pdata == NULL) {
@@ -1861,29 +1862,15 @@ static int omap_nand_probe(struct platform_device *pdev)
 #endif
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res == NULL) {
-		err = -EINVAL;
-		dev_err(&pdev->dev, "error getting memory resource\n");
-		goto out_free_info;
-	}
-
-	info->phys_base = res->start;
-	info->mem_size = resource_size(res);
-
-	if (!request_mem_region(info->phys_base, info->mem_size,
-				pdev->dev.driver->name)) {
-		err = -EBUSY;
-		goto out_free_info;
-	}
-
-	info->nand.IO_ADDR_R = ioremap(info->phys_base, info->mem_size);
-	if (!info->nand.IO_ADDR_R) {
-		err = -ENOMEM;
-		goto out_release_mem_region;
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base)) {
+		err = PTR_ERR(base);
+		return err;
 	}
 
 	info->nand.controller = &info->controller;
 
+	info->nand.IO_ADDR_R = base;
 	info->nand.IO_ADDR_W = info->nand.IO_ADDR_R;
 	info->nand.cmd_ctrl  = omap_hwcontrol;
 
@@ -1904,7 +1891,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 
 	err = nand_scan_ident(&info->mtd, 1, NULL);
 	if (err < 0)
-		goto out_release_mem_region;
+		return err;
 
 	switch (pdata->xfer_type) {
 	case NAND_OMAP_PREFETCH_POLLED:
@@ -2065,8 +2052,6 @@ out_release_mem_region:
 		free_irq(info->gpmc_irq_count, info);
 	if (info->gpmc_irq_fifo > 0)
 		free_irq(info->gpmc_irq_fifo, info);
-	release_mem_region(info->phys_base, info->mem_size);
-out_free_info:
 	return err;
 }
 
@@ -2087,8 +2072,6 @@ static int omap_nand_remove(struct platform_device *pdev)
 
 	/* Release NAND device, its internal structures and partitions */
 	nand_release(&info->mtd);
-	iounmap(info->nand.IO_ADDR_R);
-	release_mem_region(info->phys_base, info->mem_size);
 	return 0;
 }
 
-- 
1.8.1.5

RE: [PATCH v2 0/5] Assorted OMAP2 NAND clean-ups

[Gupta, Pekon]

Hi,

> -----Original Message-----
> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
[...]

> Pekon, Brian: Do you think this solution might work for 8-bit and 16-bit
> devices?
> 
I think NAND_BUSWIDTH_AUTO (without GPMC changes) would fail in
following scenarios..

Case-1: configuring gpmc,device-width=1 from DT when using x16 device.
As your NAND driver is using NAND_BUSWIDTH_AUTO, it should
ignore this DT config, and based on ONFI params it should work as x16

Case-2: configuring gpmc,device-width=2 from DT when using x8 device.
As your NAND driver is using NAND_BUSWIDTH_AUTO, it should
ignore this DT config, and based on ONFI params it should work as x8

NAND device may get detected correctly, but try doing write and read
to NAND, and I think, it would fail for Case-2 at-least.. 
Can you please check ?


> I've been going through the GPMC code again, and it looks like some cleaning
> is needed
> there two, but that's a different story!
> 
Actually having NAND_BUSWIDTH_AUTO would require change in GPMC
driver also.. please refer my comments in.. 
http://lists.infradead.org/pipermail/linux-mtd/2013-October/049284.html

reason of failure is given above...


with regards, pekon

Re: [PATCH v2 0/5] Assorted OMAP2 NAND clean-ups

[Ezequiel Garcia]

Pekon,

On Fri, Oct 25, 2013 at 11:15:57AM +0000, Gupta, Pekon wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> [...]
> 
> > Pekon, Brian: Do you think this solution might work for 8-bit and 16-bit
> > devices?
> > 
> I think NAND_BUSWIDTH_AUTO (without GPMC changes) would fail in
> following scenarios..
> 
> Case-1: configuring gpmc,device-width=1 from DT when using x16 device.

... which is wrong. That's why we have a DT property to configure that.
The GPMC *must* be properly configured.

> As your NAND driver is using NAND_BUSWIDTH_AUTO, it should
> ignore this DT config, and based on ONFI params it should work as x16
> 

Hm.. I don't think so. The auto-stuff is just for the NAND driver, not
for the memory controller. I don't know much about hardware, but in my mind
I imagine them as different controllers.

> Case-2: configuring gpmc,device-width=2 from DT when using x8 device.

... which is also wrong.

Once again, you're mis-configuring the GPMC. We cannot expect the NAND
driver to work properly if the GPMC is not properly initialized, don't
you think?

> Actually having NAND_BUSWIDTH_AUTO would require change in GPMC
> driver also.. please refer my comments in.. 
> http://lists.infradead.org/pipermail/linux-mtd/2013-October/049284.html
> 

Well, I think the approach should be different and much simpler: GPMC
*must* be properly configured and then NAND can do ONFI detection
starting in 8-bit and then switching to 16-bit if needed.

This is what this patch is doing: it _fixes_ the NAND driver ONFI detection,
_provided_ the GPMC is well-prepared.

You seem to think that GPMC + NAND should be able to automagically detect
the device and work, but I don't think that's even physically possible, for
the reasons you have just exposed.

I think this fix is simple enough.

BTW: The GPMC code ignores the DT value in 'gpmc,device-width' and uses
'nand-bus-width' instead, but that's a different bug and a different fix :)
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/5] Assorted OMAP2 NAND clean-ups

[Ezequiel Garcia]

Hey Pekon,

On Fri, Oct 25, 2013 at 08:48:36AM -0300, Ezequiel Garcia wrote:
> Pekon,
> 
> On Fri, Oct 25, 2013 at 11:15:57AM +0000, Gupta, Pekon wrote:
> > Hi,
> > 
> > > -----Original Message-----
> > > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > [...]
> > 
> > > Pekon, Brian: Do you think this solution might work for 8-bit and 16-bit
> > > devices?
> > > 
> > I think NAND_BUSWIDTH_AUTO (without GPMC changes) would fail in
> > following scenarios..
> > 
> > Case-1: configuring gpmc,device-width=1 from DT when using x16 device.
> 
> ... which is wrong. That's why we have a DT property to configure that.
> The GPMC *must* be properly configured.
> 
> > As your NAND driver is using NAND_BUSWIDTH_AUTO, it should
> > ignore this DT config, and based on ONFI params it should work as x16
> > 
> 
> Hm.. I don't think so. The auto-stuff is just for the NAND driver, not
> for the memory controller. I don't know much about hardware, but in my mind
> I imagine them as different controllers.
> 
> > Case-2: configuring gpmc,device-width=2 from DT when using x8 device.
> 
> ... which is also wrong.
> 
> Once again, you're mis-configuring the GPMC. We cannot expect the NAND
> driver to work properly if the GPMC is not properly initialized, don't
> you think?
> 
> > Actually having NAND_BUSWIDTH_AUTO would require change in GPMC
> > driver also.. please refer my comments in.. 
> > http://lists.infradead.org/pipermail/linux-mtd/2013-October/049284.html
> > 
> 
> Well, I think the approach should be different and much simpler: GPMC
> *must* be properly configured and then NAND can do ONFI detection
> starting in 8-bit and then switching to 16-bit if needed.
> 
> This is what this patch is doing: it _fixes_ the NAND driver ONFI detection,
> _provided_ the GPMC is well-prepared.
> 
> You seem to think that GPMC + NAND should be able to automagically detect
> the device and work, but I don't think that's even physically possible, for
> the reasons you have just exposed.
> 

Sorry to insist: any comments about this?

If you have access to the AM335xEVM (which has an 8-bit NAND, as far as I know)
and also to the Beaglebone 16-bit NAND cape, then you can test this patchset
with all the different combinations: 8-bit vs. 16-bit and array-based vs.
ONFI-based detection.

If it works, then problem solved. If it doesn't, I'm sure we can find a
solution. The driver is currently buggy, so we need to address this sooner
or later.

In addition, the outcome of our discussion will probably be helpful for other
drivers/controllers, since this ONFI issue is likely to be common.

If you can do the testing, that would be great: keep in mind that the
GPMC must be correctly configured at all times. You can use my recent
GPMC patchset for that (which also need some testing).

Thanks in advance,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 0/5] Assorted OMAP2 NAND clean-ups

[Gupta, Pekon]

Hi Ezequiel Garcia,

Sorry I'm bit out of my place.. so not able to sync often with mails..
However plz see my replies below..

[...]

> >
> > > Pekon, Brian: Do you think this solution might work for 8-bit and 16-bit
> > > devices?
> > >
> > I think NAND_BUSWIDTH_AUTO (without GPMC changes) would fail in
> > following scenarios..
> >
> > Case-1: configuring gpmc,device-width=1 from DT when using x16 device.
> 
> ... which is wrong. That's why we have a DT property to configure that.
> The GPMC *must* be properly configured.
> 
No, I think the main intention of using NAND_BUSWIDTH_AUTO is that
your controller should not depend on DT or any other user-input to detect
NAND bus-width, (whether its gpmc,device-width or anything else).


> > As your NAND driver is using NAND_BUSWIDTH_AUTO, it should
> > ignore this DT config, and based on ONFI params it should work as x16
> >
> 
> Hm.. I don't think so. The auto-stuff is just for the NAND driver, not
> for the memory controller. I don't know much about hardware, but in my
> mind
> I imagine them as different controllers.
> 
TI's GPMC hardware engine can do much more than supporting only
NAND devices, therefore there is an independent GPMC driver, and
dependent NAND driver. 
So when a NAND device is connected all GPMC driver does is initializes
GPMC hardware based on static DT bindings, and pass the control to
NAND driver. But there should be a mechanism to override these
static DT configurations done in GPMC driver by NAND driver.
(it is this piece which is missing today).


> > Case-2: configuring gpmc,device-width=2 from DT when using x8 device.
> 
> ... which is also wrong.
> 
> Once again, you're mis-configuring the GPMC. We cannot expect the NAND
> driver to work properly if the GPMC is not properly initialized, don't
> you think?
> 
> > Actually having NAND_BUSWIDTH_AUTO would require change in GPMC
> > driver also.. please refer my comments in..
> > http://lists.infradead.org/pipermail/linux-mtd/2013-October/049284.html
> >
> 
> Well, I think the approach should be different and much simpler: GPMC
> *must* be properly configured and then NAND can do ONFI detection
> starting in 8-bit and then switching to 16-bit if needed.
> 
> This is what this patch is doing: it _fixes_ the NAND driver ONFI detection,
> _provided_ the GPMC is well-prepared.
> 
> You seem to think that GPMC + NAND should be able to automagically detect
> the device and work, but I don't think that's even physically possible, for
> the reasons you have just exposed.
> 
> I think this fix is simple enough.
> 
No, I still think there should be a way to tell the user that there is a
mis-match between bus-width configured in DT, and that detected
by ONFI probe. If it was straight forward, this would have been already
accepted earlier :-)

http://lists.infradead.org/pipermail/linux-mtd/2013-October/049154.html

(read the comments from Brian by following above patch thread)

Problem is if there is a mis-match in your bus-width due to incorrect
GPMC DT binding configurations, still NAND probe and everything will
pass correctly, because nand_scan_ident() uses read_byte() which
returns same for both x8 and x16 devices.
You will find issues when you actually do page reads and writes. And
this is where you would see corrupted half-page data. And user
would be clueless on why he is seeing such erratic behavior.

This is why I added checks for mismatch of DT binding right during
nand probe(), in above mentioned patch. I could just remove calling
nand_scan_ident() second-time and it becomes your patch, but then
it would be just validating the DT setting, not pure auto-detection.


> 
> BTW: The GPMC code ignores the DT value in 'gpmc,device-width' and uses
> 'nand-bus-width' instead, but that's a different bug and a different fix :)
> 
I think you mean the opposie.. GPMC driver uses gpmc,device-width..
Refer: $KERNEL/arch/arm/mach-omap2/gpmc.c
@@gpmc_read_settings_dt(...)
	of_property_read_u32(np, "gpmc,device-width", &p->device_width);

'nand-bus-width' is not used anywhere instead..


with regards, pekon

Re: [PATCH v2 0/5] Assorted OMAP2 NAND clean-ups

[Ezequiel Garcia]

Hey Pekon,

On Tue, Oct 29, 2013 at 08:14:34PM +0000, Gupta, Pekon wrote:
> 
> Sorry I'm bit out of my place.. so not able to sync often with mails..
> However plz see my replies below..
> 

Sure, no problem. Thanks for taking the time to discuss this!

[..]
> > 
> > You seem to think that GPMC + NAND should be able to automagically detect
> > the device and work, but I don't think that's even physically possible, for
> > the reasons you have just exposed.
> > 
> > I think this fix is simple enough.
> > 
> No, I still think there should be a way to tell the user that there is a
> mis-match between bus-width configured in DT, and that detected
> by ONFI probe. If it was straight forward, this would have been already
> accepted earlier :-)
> 
> http://lists.infradead.org/pipermail/linux-mtd/2013-October/049154.html
> 
> (read the comments from Brian by following above patch thread)
> 

Hmm... Well, I think your whole point of view is:

1. Mixing what's NAND controller and what's GPMC controller.
They are two different controllers, which means two different
drivers, and different parameters.

So from this perspective, trying to 'fix' GPMC configuration
from the NAND driver is a completely bad idea.

2. Trying to over-report the status to the user. The GPMC _must_
be properly configured before anything else. These NAND aren't plug
and play, so the 'user' knows the bus width anyway. If the user
has misconfigured the GPMC, then it's fine for things to not work.

Keep in mind the DT is not a configuration tool, but rather it's a
way to describe hardware. So, hardware parameters as bus width needs
to be properly described.

I've followed the above patch and, as far as I can read, Brian says: (quoting)

""
My opinion: the platform- and device-tree-provided buswidth is
unnecessary; it was previouisly only a suggestion which your driver
would readily discard, and it isn't really needed now. You can probably
get by with just NAND_BUSWIDTH_AUTO and (at most) a warning if the
auto-configured buswidth is different than the platform specified.

But the real point: you need to clearly communicate what you are
choosing in this patch. Either you want to

 (1) strictly follow the buswidth provided by the platform or

 (2) use the nand_base.c BUSWIDTH_AUTO automatic configuration.

Trying to mix both (as your patch currently does) just makes
everything worse.
""

Note that, in the above, Brian is talking about the NAND buswidth,
(not the GPMC). As he suggests, the proper thing to do is to simply
use BUSWIDTH_AUTO and discard any other parameter.

> > 
> > BTW: The GPMC code ignores the DT value in 'gpmc,device-width' and uses
> > 'nand-bus-width' instead, but that's a different bug and a different fix :)
> > 
> I think you mean the opposie.. GPMC driver uses gpmc,device-width..
> Refer: $KERNEL/arch/arm/mach-omap2/gpmc.c
> @@gpmc_read_settings_dt(...)
> 	of_property_read_u32(np, "gpmc,device-width", &p->device_width);
> 
> 'nand-bus-width' is not used anywhere instead..
> 

No, I meant just that: the current GPMC driver _ignores_ the DT value for
gpmc,device-width and overrides it with the 'nand-bus-width'.
See below for a stripped version of gpmc_nand_init():

int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
		   struct gpmc_timings *gpmc_t)
{
[...]
		if (gpmc_nand_data->of_node) {
			gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
		} else {
			/* .... */
		}

		s.device_nand = true;

		if (gpmc_nand_data->devsize == NAND_BUSWIDTH_16)
			s.device_width = GPMC_DEVWIDTH_16BIT;
		else
			s.device_width = GPMC_DEVWIDTH_8BIT;
}

The code first reads 'device_width' from gpmc_read_settings_dt(), and then
overrides it with the NAND buswidth value! Once again, the above repeats
the 'mixing parameter' scheme you are suggesting.

(I've already submitted a patchset fixing this. See here:
http://www.spinics.net/lists/arm-kernel/msg282594.html)

I think our current discussion can be summarized as this:

  * Should we fix the GPMC buswidth configuration, using the
    auto-detected NAND buswidth?

IMO, the answer is "NO!", as it results in fucked-up design.

But it's a _design_ choice, there's no technical reason why you can't
export some GPMC configuration function and call it from the driver.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html