[PATCH v6 0/4] mtd:nand:omap2: clean-up of supported ECC schemes

[PATCH v6 0/4] mtd:nand:omap2: clean-up of supported ECC schemes

[Pekon Gupta]

*Changes v5 -> v6*
[PATCH 1/4]: 
	- updated DT binding for gpmc-nand based on 'Olof Johansson's feedbacks
	http://lists.infradead.org/pipermail/linux-mtd/2013-August/048394.html
	- detection of ELM device via ti,elm-id DT node, moved to gpmc.c driver
[PATCH 2/4]
	- removed: support for following obselete ECC schemes
	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming ECC)
	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit Hamming ECC scheme)
	- updated: using omap_oobinfo as chip->ecc.layout for all ecc-schemes
	- clean: error messages
[PATCH 3/4] cleaned to include changes for OMAP_ECC_BCH8_CODE_HW only
[PATCH 4/4] updated to include DT property changes


*Changes v4 -> v5*
- Rebased to linux-next 
IMPORTANT: Need to revert commit fb1585b, [PATCH 2/4] part of previous version
	http://lists.infradead.org/pipermail/linux-mtd/2013-July/047441.html

- Swapped PATCH-1 & PATCH-2 to maintain bisectibility & compilation dependency
	http://lists.infradead.org/pipermail/linux-mtd/2013-July/047461.html

- PATCH-2: re-ordered call to is_elm_present() for later updates ELM driver
	- dropped changes in include/linux/platform_data/elm.h (not needed)
- PATCH-3: re-ordered call to is_elm_present() for later updates ELM driver
- Re-formated patch description (replaced tabs with white-spaces)


*Changes v3 -> v4*
(Resent with CC: devicetree-discuss@lists.ozlabs.org)
- [Patch 1/3] removed MTD_NAND_OMAP_BCH8 & MTD_NAND_OMAP_BCH4 from nand/Kconfig
	ECC scheme selectable via nand DT (nand-ecc-opt).
- [*] rebased for l2-mtd.git


*Changes v2 -> v3*
(Resent with Author Name fixed)
- PATCH-1: re-arranged code to remove redundancy, added NAND_BUSWIDTH_AUTO
- PATCH-2: updated nand-ecc-opt DT mapping and Documentation
- PATCH-3: code-cleaning + changes to match PATCH-1
- PATCH-4 <DROPPED> update DT attribute for ti,nand-ecc-opt 
	- received feedback to keep DT mapping independent of linuxism
- PATCH-4:<NEW> : ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt
	- independent patch for AM335x-evm.dts update based on PATCH-2


*Changes v1 -> v2*
	added 	[PATCH 3/4] and [PATCH 4/4]


*Patches in this series*
[PATCH 1/4]->[PATCH v5 2/4]: clean-up and optimization for supported ECC schemes.
[PATCH 2/4]->[PATCH v5 1/4]: add separate DT options each supported ECC scheme.
[PATCH 3/4]: update BCH4 ECC implementation (using ELM or using lib/bch.h)
[PATCH 4/4]: ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt

After this patch series, omap2-nand driver will supports following ECC schemes:
+---------------------------------------+---------------+---------------+
| ECC scheme                            |ECC calculation|Error detection|
+---------------------------------------+---------------+---------------+
|OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH4_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.h)|
|OMAP_ECC_BCH4_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.h)|
|OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
+---------------------------------------+---------------+---------------+
- Selection of OMAP_ECC_BCHx_CODE_HW_DETECTION_SW requires,
	Kconfig: CONFIG_MTD_NAND_ECC_BCH: enables S/W based BCH ECC algorithm.

- Selection of OMAP_ECC_BCHx_CODE_HW requires,
	Kconfig: CONFIG_MTD_NAND_OMAP_BCH: enables ELM H/W module.

Pekon Gupta (4):
  ARM: OMAP2+: cleaned-up DT support of various ECC schemes
  mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in
    device_probe
  mtd:nand:omap2: updated support for BCH4 ECC scheme
  ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt

 .../devicetree/bindings/mtd/gpmc-nand.txt          |  14 +-
 arch/arm/boot/dts/am335x-evm.dts                   |   2 +-
 arch/arm/mach-omap2/board-flash.c                  |   2 +-
 arch/arm/mach-omap2/gpmc.c                         |  47 +-
 drivers/mtd/nand/Kconfig                           |  30 +-
 drivers/mtd/nand/omap2.c                           | 526 ++++++++++-----------
 include/linux/platform_data/mtd-nand-omap2.h       |  18 +-
 7 files changed, 309 insertions(+), 330 deletions(-)

-- 
1.8.1

[PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Pekon Gupta]

OMAP NAND driver support multiple ECC scheme, which can used in following
different flavours, depending on in-build Hardware engines supported by SoC.

+---------------------------------------+---------------+---------------+
| ECC scheme                            |ECC calculation|Error detection|
+---------------------------------------+---------------+---------------+
|OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W            |
|(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
|(requires CONFIG_MTD_NAND_OMAP_BCH &&  |               |               |
| ti,elm-id in DT)                      |               |               |
+---------------------------------------+---------------+---------------+
To optimize footprint of omap2-nand driver, selection of some ECC schemes
also require enabling following Kconfigs, in addition to setting appropriate
DT bindings
- Kconfig:CONFIG_MTD_NAND_ECC_BCH        enables S/W based BCH ECC algorithm
- Kconfig:CONFIG_MTD_NAND_OMAP_BCH       enables H/W based BCH ECC algorithm

This patch
- updates DT binding for selection of ecc-scheme
- updates DT binding for detection of ELM h/w engine
- removes following obselete ECC schemes
	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming ECC)
	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit Hamming ECC scheme)
- updates DT binding documentation for mtd/gpmc-nand

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 .../devicetree/bindings/mtd/gpmc-nand.txt          | 14 +++----
 arch/arm/mach-omap2/board-flash.c                  |  2 +-
 arch/arm/mach-omap2/gpmc.c                         | 47 +++++++++++++++-------
 include/linux/platform_data/mtd-nand-omap2.h       | 23 +++++++----
 4 files changed, 56 insertions(+), 30 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index df338cb..958e5d5 100644
--- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -21,11 +21,8 @@ Optional properties:
 				is wired that way. If not specified, a bus
 				width of 8 is assumed.
 
- - ti,nand-ecc-opt:		A string setting the ECC layout to use. One of:
-
-		"sw"		Software method (default)
-		"hw"		Hardware method
-		"hw-romcode"	gpmc hamming mode method & romcode layout
+ - ti,nand-ecc-scheme:		A string setting the ECC layout to use. One of:
+		"ham1"		1-bit Hamming ecc code
 		"bch4"		4-bit BCH ecc code
 		"bch8"		8-bit BCH ecc code
 
@@ -36,8 +33,11 @@ Optional properties:
 		"prefetch-dma"		Prefetch enabled sDMA mode
 		"prefetch-irq"		Prefetch enabled irq mode
 
- - elm_id:	Specifies elm device node. This is required to support BCH
- 		error correction using ELM module.
+ - ti,elm-id:	Specifies pHandle of the ELM devicetree node.
+		ELM is an on-chip hardware engine on TI SoC which is used for
+		locating ECC errors for BCHx algorithms. SoC devices which have
+		ELM hardware engines should specify this device node in .dtsi
+		Using ELM for ECC error correction frees some CPU cycles.
 
 For inline partiton table parsing (optional):
 
diff --git a/arch/arm/mach-omap2/board-flash.c b/arch/arm/mach-omap2/board-flash.c
index fc20a61..ac82512 100644
--- a/arch/arm/mach-omap2/board-flash.c
+++ b/arch/arm/mach-omap2/board-flash.c
@@ -142,7 +142,7 @@ __init board_nand_init(struct mtd_partition *nand_parts, u8 nr_parts, u8 cs,
 	board_nand_data.nr_parts	= nr_parts;
 	board_nand_data.devsize		= nand_type;
 
-	board_nand_data.ecc_opt = OMAP_ECC_HAMMING_CODE_DEFAULT;
+	board_nand_data.ecc_opt = OMAP_ECC_BCH8_CODE_HW;
 	gpmc_nand_init(&board_nand_data, gpmc_t);
 }
 #endif /* CONFIG_MTD_NAND_OMAP2 || CONFIG_MTD_NAND_OMAP2_MODULE */
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 9f4795a..6409884 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1340,15 +1340,6 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
 }
 
 #ifdef CONFIG_MTD_NAND
-
-static const char * const nand_ecc_opts[] = {
-	[OMAP_ECC_HAMMING_CODE_DEFAULT]		= "sw",
-	[OMAP_ECC_HAMMING_CODE_HW]		= "hw",
-	[OMAP_ECC_HAMMING_CODE_HW_ROMCODE]	= "hw-romcode",
-	[OMAP_ECC_BCH4_CODE_HW]			= "bch4",
-	[OMAP_ECC_BCH8_CODE_HW]			= "bch8",
-};
-
 static const char * const nand_xfer_types[] = {
 	[NAND_OMAP_PREFETCH_POLLED]		= "prefetch-polled",
 	[NAND_OMAP_POLLED]			= "polled",
@@ -1363,6 +1354,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
 	const char *s;
 	struct gpmc_timings gpmc_t;
 	struct omap_nand_platform_data *gpmc_nand_data;
+	const __be32 *parp;
+	int lenp;
 
 	if (of_property_read_u32(child, "reg", &val) < 0) {
 		dev_err(&pdev->dev, "%s has no 'reg' property\n",
@@ -1378,12 +1371,36 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
 	gpmc_nand_data->cs = val;
 	gpmc_nand_data->of_node = child;
 
-	if (!of_property_read_string(child, "ti,nand-ecc-opt", &s))
-		for (val = 0; val < ARRAY_SIZE(nand_ecc_opts); val++)
-			if (!strcasecmp(s, nand_ecc_opts[val])) {
-				gpmc_nand_data->ecc_opt = val;
-				break;
-			}
+	/* Detect availability of ELM module */
+	parp = of_get_property(child, "ti,elm-id", &lenp);
+	if ((parp == NULL) && (lenp != (sizeof(void *) * 2))) {
+		pr_warn("%s: ti,elm-id property not found\n", __func__);
+		gpmc_nand_data->elm_of_node = NULL;
+	} else {
+		gpmc_nand_data->elm_of_node =
+				of_find_node_by_phandle(be32_to_cpup(parp));
+	}
+	/* select NAND ecc-scheme */
+	if (of_property_read_string(child, "ti,nand-ecc-scheme", &s)) {
+		pr_err("%s: valid ti,nand-ecc-scheme not found\n", __func__);
+		return -ENODEV;
+	}
+	if (!strcmp(s, "ham1"))
+		gpmc_nand_data->ecc_opt = OMAP_ECC_HAMMING_CODE_HW;
+	else if (!strcmp(s, "bch4"))
+		if (gpmc_nand_data->elm_of_node)
+			gpmc_nand_data->ecc_opt = OMAP_ECC_BCH4_CODE_HW;
+		else
+			gpmc_nand_data->ecc_opt =
+				OMAP_ECC_BCH4_CODE_HW_DETECTION_SW;
+	else if (!strcmp(s, "bch8"))
+		if (gpmc_nand_data->elm_of_node)
+			gpmc_nand_data->ecc_opt = OMAP_ECC_BCH8_CODE_HW;
+		else
+			gpmc_nand_data->ecc_opt =
+				OMAP_ECC_BCH8_CODE_HW_DETECTION_SW;
+	else
+		pr_err("%s: ti,ecc-scheme: invalid property value\n", __func__);
 
 	if (!of_property_read_string(child, "ti,nand-xfer-type", &s))
 		for (val = 0; val < ARRAY_SIZE(nand_xfer_types); val++)
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index 6bf9ef4..b4c2c5a 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -23,13 +23,21 @@ enum nand_io {
 };
 
 enum omap_ecc {
-		/* 1-bit ecc: stored at end of spare area */
-	OMAP_ECC_HAMMING_CODE_DEFAULT = 0, /* Default, s/w method */
-	OMAP_ECC_HAMMING_CODE_HW, /* gpmc to detect the error */
-		/* 1-bit ecc: stored at beginning of spare area as romcode */
-	OMAP_ECC_HAMMING_CODE_HW_ROMCODE, /* gpmc method & romcode layout */
-	OMAP_ECC_BCH4_CODE_HW, /* 4-bit BCH ecc code */
-	OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */
+	/* 1-bit  ECC calculation by Software, Error detection by Software */
+	OMAP_ECC_HAMMING_CODE_DEFAULT = 0,
+	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
+	OMAP_ECC_HAMMING_CODE_HW,
+	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
+	/* ECC layout compatible to legacy ROMCODE. */
+	OMAP_ECC_HAMMING_CODE_HW_ROMCODE,
+	/* 4-bit  ECC calculation by GPMC, Error detection by Software */
+	OMAP_ECC_BCH4_CODE_HW_DETECTION_SW,
+	/* 4-bit  ECC calculation by GPMC, Error detection by ELM */
+	OMAP_ECC_BCH4_CODE_HW,
+	/* 8-bit  ECC calculation by GPMC, Error detection by Software */
+	OMAP_ECC_BCH8_CODE_HW_DETECTION_SW,
+	/* 8-bit  ECC calculation by GPMC, Error detection by ELM */
+	OMAP_ECC_BCH8_CODE_HW,
 };
 
 struct gpmc_nand_regs {
@@ -63,5 +71,6 @@ struct omap_nand_platform_data {
 
 	/* for passing the partitions */
 	struct device_node	*of_node;
+	struct device_node	*elm_of_node;
 };
 #endif
-- 
1.8.1

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

> 
> This patch
> - updates DT binding for selection of ecc-scheme
> - updates DT binding for detection of ELM h/w engine
> - removes following obselete ECC schemes
> 	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming
> ECC)
> 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit
> Hamming ECC scheme)
> - updates DT binding documentation for mtd/gpmc-nand
> 
> Signed-off-by: Pekon Gupta <pekon-l0cyMroinI0@public.gmane.org>
> ---

Dear Olof and other DT Maintainers,

This patch series has missed multiple merge windows, and
much of the other development work on mtd/nand/omap
driver is gated due to this.
So, request you to please review this patch set and Ack it
if all your mentioned concerns are resolved.


With regards, pekon

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

+ akpm

On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
> 
> > 
> > This patch
> > - updates DT binding for selection of ecc-scheme
> > - updates DT binding for detection of ELM h/w engine
> > - removes following obselete ECC schemes
> > 	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming
> > ECC)
> > 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit
> > Hamming ECC scheme)
> > - updates DT binding documentation for mtd/gpmc-nand
> > 
> > Signed-off-by: Pekon Gupta <pekon-l0cyMroinI0@public.gmane.org>
> > ---
> 
> Dear Olof and other DT Maintainers,
> 
> This patch series has missed multiple merge windows, and
> much of the other development work on mtd/nand/omap
> driver is gated due to this.
> So, request you to please review this patch set and Ack it
> if all your mentioned concerns are resolved.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote:
> + akpm
> 
> On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
> > 
> > > 
> > > This patch
> > > - updates DT binding for selection of ecc-scheme
> > > - updates DT binding for detection of ELM h/w engine
> > > - removes following obselete ECC schemes
> > > 	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming
> > > ECC)
> > > 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit
> > > Hamming ECC scheme)
> > > - updates DT binding documentation for mtd/gpmc-nand
> > > 
> > > Signed-off-by: Pekon Gupta <pekon@ti.com>
> > > ---
> > 
> > Dear Olof and other DT Maintainers,
> > 
> > This patch series has missed multiple merge windows, and
> > much of the other development work on mtd/nand/omap
> > driver is gated due to this.
> > So, request you to please review this patch set and Ack it
> > if all your mentioned concerns are resolved.

I've been waiting on Olof's response for this one. It seems like you
addressed his comments well enough for me, although (in line with his
comments) you are admittedly still breaking the DT ABI for these
devices. IMO, that's OK given the low quality of the original bindings.

I will make my own last pass at this series and push it to l2-mtd.git if
it looks OK.

Brian

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

Hi Pekon,

On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote:
> + akpm
> 
> On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
> > 
> > > 
> > > This patch
> > > - updates DT binding for selection of ecc-scheme
> > > - updates DT binding for detection of ELM h/w engine
> > > - removes following obselete ECC schemes
> > > 	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming
> > > ECC)
> > > 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit
> > > Hamming ECC scheme)
> > > - updates DT binding documentation for mtd/gpmc-nand
> > > 
> > > Signed-off-by: Pekon Gupta <pekon@ti.com>
> > > ---
> > 
> > Dear Olof and other DT Maintainers,
> > 
> > This patch series has missed multiple merge windows, and
> > much of the other development work on mtd/nand/omap
> > driver is gated due to this.

Also, to be fair here: you only started CC'ing the appropriate DT
people/list around v4, after which you got a (somewhat) prompt and
thorough review of the DT bindings. Then several months passed before
you addressed the reviews. So the "multiple merge windows" is not
entirely to be blamed on others ;)

Brian

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

Hi Brian,

> 
> Hi Pekon,
> 
> On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote:
> > + akpm
> >
> > On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
[snip]
> > >
> > > Dear Olof and other DT Maintainers,
> > >
> > > This patch series has missed multiple merge windows, and
> > > much of the other development work on mtd/nand/omap
> > > driver is gated due to this.
> 
> Also, to be fair here: you only started CC'ing the appropriate DT
> people/list around v4, after which you got a (somewhat) prompt and
> thorough review of the DT bindings. Then several months passed before
> you addressed the reviews. So the "multiple merge windows" is not
> entirely to be blamed on others ;)
> 
> Brian

Few points I would like to clarify here, *without* pointing anyone..

(1) It was Olof's comments which directed me to cc: devicetree-discuss list.
So wrong list was always cc-ed till v4.
Refer http://permalink.gmane.org/gmane.linux.ports.arm.kernel/249662

(2) The devicetree list has been updated in MAINTAINER file towards
end of July (22/July/2013) in following commit. Whereas the patch v4
was submitted on 2/July/2013. So I wasn't aware of this new DT maillist.
commit d0fb18c5c0caf2ed0eecf3d0145450ae708ed75a
Commit:     Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
CommitDate: 2013-07-22

(3) When a maintainer gives a NAK, I expect him to at-least give
directions on what to change in the patch. There were no comments
given, neither new patch reviewed by the DT maintainer even after
sending multiple request directly.
http://lists.infradead.org/pipermail/linux-mtd/2013-July/047441.html
http://lists.infradead.org/pipermail/linux-mtd/2013-July/047629.html

Its only when Artem and yourself pitched in by sending a mail to
new DT mail-list that the DT maintainer reviewed and provided the
comments for fixes.
But by that time 3.11-rc6 had already gone past, and so I knew this
Series cannot go in, so I wanted to wait some more, to see if there
were any more comments on this. And frankly I was too much
frustrated by then.

Just to speak my opinion.
We all understand that maintainers are heavily loaded by tons of
emails, And reviewing each patch instantly is not possible.

But this load is now passing to developers like me as frustration,
because most of energy is being spent in re-submitting patches
again and again, without any proper direction or conclusion.
And then there is no time and energy left for good work, where
we can contribute to optimizing frame-works for performance or
adding new features.
So I see many good developers distracting away from mainline.

Thus, there should be a mechanism, where such load can be
distributed, and there are less emails. And developers don't
have to re-submit patch multiple times, by collecting most reviews
at once.  This way developer's like me can spend more time in doing
other constructive things.

Please consider this as constructive feedback, without targeting
any individual or team.


with regards, pekon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

On Wed, Sep 25, 2013 at 07:24:26PM +0000, Gupta, Pekon wrote:
> > On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote:
> > > + akpm
> > >
> > > On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
> [snip]
> > > >
> > > > Dear Olof and other DT Maintainers,
> > > >
> > > > This patch series has missed multiple merge windows, and
> > > > much of the other development work on mtd/nand/omap
> > > > driver is gated due to this.
> > 
> > Also, to be fair here: you only started CC'ing the appropriate DT
> > people/list around v4, after which you got a (somewhat) prompt and
> > thorough review of the DT bindings. Then several months passed before
> > you addressed the reviews. So the "multiple merge windows" is not
> > entirely to be blamed on others ;)
> > 
> > Brian
> 
> Few points I would like to clarify here, *without* pointing anyone..
> 
> (1) It was Olof's comments which directed me to cc: devicetree-discuss list.
> So wrong list was always cc-ed till v4.
> Refer http://permalink.gmane.org/gmane.linux.ports.arm.kernel/249662
> 
> (2) The devicetree list has been updated in MAINTAINER file towards
> end of July (22/July/2013) in following commit. Whereas the patch v4
> was submitted on 2/July/2013. So I wasn't aware of this new DT maillist.
> commit d0fb18c5c0caf2ed0eecf3d0145450ae708ed75a
> Commit:     Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> CommitDate: 2013-07-22

Did you not notice the 'bounce' emails once Grant made the (IMO unwise)
decision to shut down the DT mailing list instead of just redirecting?

> (3) When a maintainer gives a NAK, I expect him to at-least give
> directions on what to change in the patch. There were no comments
> given, neither new patch reviewed by the DT maintainer even after
> sending multiple request directly.
> http://lists.infradead.org/pipermail/linux-mtd/2013-July/047441.html
> http://lists.infradead.org/pipermail/linux-mtd/2013-July/047629.html

Those are links to your own emails, not to any NAKs. Any NAKs I saw
listed reasons. I NAK'd based on the ABI breakage and lack of review by
the DT maintainership; Arnd NAK'd based on similar reasons; Olof later
(once you got the right list) gave constructive criticism on how to
remove the Linux-isms and other software implementation details from the
DT binding.

> Its only when Artem and yourself pitched in by sending a mail to
> new DT mail-list that the DT maintainer reviewed and provided the
> comments for fixes.
> But by that time 3.11-rc6 had already gone past, and so I knew this
> Series cannot go in, so I wanted to wait some more, to see if there
> were any more comments on this. And frankly I was too much
> frustrated by then.
> 
> Just to speak my opinion.
> We all understand that maintainers are heavily loaded by tons of
> emails, And reviewing each patch instantly is not possible.
> 
> But this load is now passing to developers like me as frustration,
> because most of energy is being spent in re-submitting patches
> again and again, without any proper direction or conclusion.
> And then there is no time and energy left for good work, where
> we can contribute to optimizing frame-works for performance or
> adding new features.
> So I see many good developers distracting away from mainline.

I sincerely hope that this isn't (or at least doesn't continue to be)
the norm.

> Thus, there should be a mechanism, where such load can be
> distributed, and there are less emails. And developers don't
> have to re-submit patch multiple times, by collecting most reviews
> at once.  This way developer's like me can spend more time in doing
> other constructive things.

I understand your concerns. I have been frustrated with slow responses
on MTD stuff as well, which is why I'm stepping in to review and commit
more. But I see that your patch hit a unique combination of events that
helped slow things down even more than usual.

MTD maintainership has been slowing down for a while, and in the
midst of your patch series, I began stepping in to take care of some of
that load. I didn't quite have a good picture of what patches were
pending without comments at that point, so patch series like yours were
in limbo with no one to review.

Additionally, Grant stepped down from DT maintainership and
correspondingly shut down and transitioned the DT mailing list. This
directly affected your patch series.

Lastly, it is extra difficult to deal with patch sets like this that
cross subsystems and require reviews from multiple constituencies. And
that is even more difficult when you aren't even emailing the correct
people (for whatever reason).

I believe that many of the factors that slowed down your particular
series have been ameliorated.

Time has allowed developers to become more aware of the change in DT
maintainership and mailing list, and I can more promptly redirect
developers who are still CC'ing the wrong people/lists.

I am able to spend a little more time on MTD stuff, to help push
development along a little faster.

Olof has given good advice on your DT binding and has (slowly) been
responding to other requests for DT review that make it to his list. I
see that he hasn't followed up on your changes (this v6), so pinging him
(as you did) is probably the correct approach. But please do recognize
that the DT list is very high volume, so it's hard to get good timely
responses there.

Anyway, at this point I think your patch series should be nearly
complete. I made a few comments on your patches, and I'd imagine you
only should need one more revision (v7) before I can accept it to the
l2-mtd.git tree.

> Please consider this as constructive feedback, without targeting
> any individual or team.

Thank you for your thoughtful response.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Olof Johansson]

On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
<computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Olof has given good advice on your DT binding and has (slowly) been
> responding to other requests for DT review that make it to his list. I
> see that he hasn't followed up on your changes (this v6), so pinging him
> (as you did) is probably the correct approach. But please do recognize
> that the DT list is very high volume, so it's hard to get good timely
> responses there.

I am not a DT mainainer, but sometimes when I see a binding that
appears to be wrong I speak up. In this case, the binding was one of
those.

That doesn't mean you should necessarily rely on me for the rest of
the review, there are several dedicated maintainers right now that
should be able to spread the load across them, and it is their ack you
should seek on the binding, not mine.

That being said: this latest version looks good to me. See how much
simpler the binding got when you stopped trying to describe driver
behavior and focused on describing hardware? That's the way we want it
to look like.


So, I have no more objections to it, and I hope you can get a quick
review from a DT maintainer on the rest of the binding.


-Olof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote:
> On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> 
> > Olof has given good advice on your DT binding and has (slowly) been
> > responding to other requests for DT review that make it to his list. I
> > see that he hasn't followed up on your changes (this v6), so pinging him
> > (as you did) is probably the correct approach. But please do recognize
> > that the DT list is very high volume, so it's hard to get good timely
> > responses there.
> 
> I am not a DT mainainer, but sometimes when I see a binding that
> appears to be wrong I speak up. In this case, the binding was one of
> those.

Whoops, my bad. I was deceived by the responses I've seen from you on
other issues (thanks, BTW). In that case, I haven't seen any response
from a proper DT binding maintainer :(
 
> So, I have no more objections to it, and I hope you can get a quick
> review from a DT maintainer on the rest of the binding.

At this point, I'm comfortable going ahead without their ack, since they
obviously don't care/don't have the manpower to review.

Brian

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Olof Johansson]

On Wed, Sep 25, 2013 at 2:29 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote:
>> On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
>> <computersforpeace@gmail.com> wrote:
>>
>> > Olof has given good advice on your DT binding and has (slowly) been
>> > responding to other requests for DT review that make it to his list. I
>> > see that he hasn't followed up on your changes (this v6), so pinging him
>> > (as you did) is probably the correct approach. But please do recognize
>> > that the DT list is very high volume, so it's hard to get good timely
>> > responses there.
>>
>> I am not a DT mainainer, but sometimes when I see a binding that
>> appears to be wrong I speak up. In this case, the binding was one of
>> those.
>
> Whoops, my bad. I was deceived by the responses I've seen from you on
> other issues (thanks, BTW). In that case, I haven't seen any response
> from a proper DT binding maintainer :(
>
>> So, I have no more objections to it, and I hope you can get a quick
>> review from a DT maintainer on the rest of the binding.
>
> At this point, I'm comfortable going ahead without their ack, since they
> obviously don't care/don't have the manpower to review.

No, that is not how we handle device tree bindings. They need to be
reviewed, since we are moving over to a model where they will be
considered ABI and can't be changed after the fact. We have a long
backlog of mostly-unreviewed old bindings that we're going to do a
pass through and then lock down, but it would be good to not add to
that backlog with newer bindings.

In other words, there's a strong desire for actual acks on bindings
from those maintainers these days.


-Olof

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

On Wed, Sep 25, 2013 at 2:32 PM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> On Wed, Sep 25, 2013 at 2:29 PM, Brian Norris
> <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote:
>>> On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
>>> <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>> > Olof has given good advice on your DT binding and has (slowly) been
>>> > responding to other requests for DT review that make it to his list. I
>>> > see that he hasn't followed up on your changes (this v6), so pinging him
>>> > (as you did) is probably the correct approach. But please do recognize
>>> > that the DT list is very high volume, so it's hard to get good timely
>>> > responses there.
>>>
>>> I am not a DT mainainer, but sometimes when I see a binding that
>>> appears to be wrong I speak up. In this case, the binding was one of
>>> those.
>>
>> Whoops, my bad. I was deceived by the responses I've seen from you on
>> other issues (thanks, BTW). In that case, I haven't seen any response
>> from a proper DT binding maintainer :(
>>
>>> So, I have no more objections to it, and I hope you can get a quick
>>> review from a DT maintainer on the rest of the binding.
>>
>> At this point, I'm comfortable going ahead without their ack, since they
>> obviously don't care/don't have the manpower to review.
>
> No, that is not how we handle device tree bindings. They need to be
> reviewed, since we are moving over to a model where they will be
> considered ABI and can't be changed after the fact. We have a long
> backlog of mostly-unreviewed old bindings that we're going to do a
> pass through and then lock down, but it would be good to not add to
> that backlog with newer bindings.
>
> In other words, there's a strong desire for actual acks on bindings
> from those maintainers these days.

This only works if we get a response. I'll repeat this fact: I have
seen absolutely zero response from any DT maintainer regarding this
binding, and they've been CC'd in some capacity since July:

Old revision from July (cross-posted, including the old DT list):
http://thread.gmane.org/gmane.linux.ports.arm.omap/100484/

New list:
http://www.mail-archive.com/linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg95238.html

All official DT binding maintainers are CC'd here: you can't say you
want more review of bindings, yet fail to review them across 3
versions and almost 3 months. Ball's in your court.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Mark Rutland]

On Wed, Sep 25, 2013 at 10:29:03PM +0100, Brian Norris wrote:
> On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote:
> > On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
> > <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > > Olof has given good advice on your DT binding and has (slowly) been
> > > responding to other requests for DT review that make it to his list. I
> > > see that he hasn't followed up on your changes (this v6), so pinging him
> > > (as you did) is probably the correct approach. But please do recognize
> > > that the DT list is very high volume, so it's hard to get good timely
> > > responses there.
> > 
> > I am not a DT mainainer, but sometimes when I see a binding that
> > appears to be wrong I speak up. In this case, the binding was one of
> > those.
> 
> Whoops, my bad. I was deceived by the responses I've seen from you on
> other issues (thanks, BTW). In that case, I haven't seen any response
> from a proper DT binding maintainer :(

Hmm... this is the first email in this thread I've received, and I don't
have older postings either. It looks like I must have cocked up
subscribing to the devicetree list, but as I was CC'd directly on many
patches I hadn't noticed.

As far as I can see I wasn't CC'd directly on any version of this
series, which would have helped to highlight the series as needing
review.

Apologies for that. I've attempted to correct the problem. Hopefully
I've got this right and mails are not being silently dropped somewhere.

Pekon, could you please re-send this version of the patches?

Cheers,
Mark.

>  
> > So, I have no more objections to it, and I hope you can get a quick
> > review from a DT maintainer on the rest of the binding.
> 
> At this point, I'm comfortable going ahead without their ack, since they
> obviously don't care/don't have the manpower to review.
> 
> Brian
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

Hi Mark,

> 
> Pekon, could you please re-send this version of the patches?
> 

As already there are feedbacks on the patches, so re-sending the
Patch series might clutter someone else's mailbox.
Will it be possible for you to fetch the patches from MTD archives?
else I would send you the patches separately..

I'm attaching the URL from MTD archives for each patch separately,
and you can follow that thread to see existing comments.

[PATCH v6 0/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048613.html

[PATCH v6 1/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048611.html

[PATCH v6 2/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048612.html

[PATCH v6 3/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048615.html

[PATCH v6 3/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048614.html


with regards, pekon

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

> 
> Anyway, at this point I think your patch series should be nearly
> complete. I made a few comments on your patches, and I'd imagine you
> only should need one more revision (v7) before I can accept it to the
> l2-mtd.git tree.
> 
Yes surely I will send next version (v7), but it might take few days.
As some more feedbacks on [PATCH 1] are pending for final conclusion
and this needs to be properly re-tested.

And thanks much to you and Olof for the feedbacks.
I agree some of Olof's feedbacks on DT bindings gave me new view to
look at things, And further simplified the NAND driver code.

with regards, pekon

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

On Thu, Sep 12, 2013 at 05:20:16PM +0530, Pekon Gupta wrote:
> OMAP NAND driver support multiple ECC scheme, which can used in following
> different flavours, depending on in-build Hardware engines supported by SoC.
> 
> +---------------------------------------+---------------+---------------+
> | ECC scheme                            |ECC calculation|Error detection|
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W            |
> |(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> |(requires CONFIG_MTD_NAND_OMAP_BCH &&  |               |               |
> | ti,elm-id in DT)                      |               |               |
> +---------------------------------------+---------------+---------------+
> To optimize footprint of omap2-nand driver, selection of some ECC schemes
> also require enabling following Kconfigs, in addition to setting appropriate
> DT bindings
> - Kconfig:CONFIG_MTD_NAND_ECC_BCH        enables S/W based BCH ECC algorithm
> - Kconfig:CONFIG_MTD_NAND_OMAP_BCH       enables H/W based BCH ECC algorithm
> 
> This patch
> - updates DT binding for selection of ecc-scheme
> - updates DT binding for detection of ELM h/w engine
> - removes following obselete ECC schemes
> 	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming ECC)
> 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit Hamming ECC scheme)
> - updates DT binding documentation for mtd/gpmc-nand
> 
> Signed-off-by: Pekon Gupta <pekon-l0cyMroinI0@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/gpmc-nand.txt          | 14 +++----
>  arch/arm/mach-omap2/board-flash.c                  |  2 +-
>  arch/arm/mach-omap2/gpmc.c                         | 47 +++++++++++++++-------
>  include/linux/platform_data/mtd-nand-omap2.h       | 23 +++++++----
>  4 files changed, 56 insertions(+), 30 deletions(-)
> 
...
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 9f4795a..6409884 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
...
> @@ -1378,12 +1371,36 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>  	gpmc_nand_data->cs = val;
>  	gpmc_nand_data->of_node = child;
>  
> -	if (!of_property_read_string(child, "ti,nand-ecc-opt", &s))
> -		for (val = 0; val < ARRAY_SIZE(nand_ecc_opts); val++)
> -			if (!strcasecmp(s, nand_ecc_opts[val])) {
> -				gpmc_nand_data->ecc_opt = val;
> -				break;
> -			}
> +	/* Detect availability of ELM module */
> +	parp = of_get_property(child, "ti,elm-id", &lenp);
> +	if ((parp == NULL) && (lenp != (sizeof(void *) * 2))) {

I think && should be ||

> +		pr_warn("%s: ti,elm-id property not found\n", __func__);
> +		gpmc_nand_data->elm_of_node = NULL;
> +	} else {
> +		gpmc_nand_data->elm_of_node =
> +				of_find_node_by_phandle(be32_to_cpup(parp));
> +	}
> +	/* select NAND ecc-scheme */
> +	if (of_property_read_string(child, "ti,nand-ecc-scheme", &s)) {
> +		pr_err("%s: valid ti,nand-ecc-scheme not found\n", __func__);
> +		return -ENODEV;
> +	}
> +	if (!strcmp(s, "ham1"))
> +		gpmc_nand_data->ecc_opt = OMAP_ECC_HAMMING_CODE_HW;
> +	else if (!strcmp(s, "bch4"))
> +		if (gpmc_nand_data->elm_of_node)
> +			gpmc_nand_data->ecc_opt = OMAP_ECC_BCH4_CODE_HW;
> +		else
> +			gpmc_nand_data->ecc_opt =
> +				OMAP_ECC_BCH4_CODE_HW_DETECTION_SW;
> +	else if (!strcmp(s, "bch8"))
> +		if (gpmc_nand_data->elm_of_node)
> +			gpmc_nand_data->ecc_opt = OMAP_ECC_BCH8_CODE_HW;
> +		else
> +			gpmc_nand_data->ecc_opt =
> +				OMAP_ECC_BCH8_CODE_HW_DETECTION_SW;
> +	else
> +		pr_err("%s: ti,ecc-scheme: invalid property value\n", __func__);
>  
>  	if (!of_property_read_string(child, "ti,nand-xfer-type", &s))
>  		for (val = 0; val < ARRAY_SIZE(nand_xfer_types); val++)

Otherwise, I think this patch is OK.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Rob Herring]

On Thu, Sep 12, 2013 at 6:50 AM, Pekon Gupta <pekon@ti.com> wrote:
> OMAP NAND driver support multiple ECC scheme, which can used in following
> different flavours, depending on in-build Hardware engines supported by SoC.
>
> +---------------------------------------+---------------+---------------+
> | ECC scheme                            |ECC calculation|Error detection|
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W            |
> |(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> |(requires CONFIG_MTD_NAND_OMAP_BCH &&  |               |               |
> | ti,elm-id in DT)                      |               |               |
> +---------------------------------------+---------------+---------------+
> To optimize footprint of omap2-nand driver, selection of some ECC schemes
> also require enabling following Kconfigs, in addition to setting appropriate
> DT bindings
> - Kconfig:CONFIG_MTD_NAND_ECC_BCH        enables S/W based BCH ECC algorithm
> - Kconfig:CONFIG_MTD_NAND_OMAP_BCH       enables H/W based BCH ECC algorithm
>
> This patch
> - updates DT binding for selection of ecc-scheme
> - updates DT binding for detection of ELM h/w engine
> - removes following obselete ECC schemes
>         OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming ECC)
>         OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit Hamming ECC scheme)
> - updates DT binding documentation for mtd/gpmc-nand
>
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  .../devicetree/bindings/mtd/gpmc-nand.txt          | 14 +++----
>  arch/arm/mach-omap2/board-flash.c                  |  2 +-
>  arch/arm/mach-omap2/gpmc.c                         | 47 +++++++++++++++-------
>  include/linux/platform_data/mtd-nand-omap2.h       | 23 +++++++----
>  4 files changed, 56 insertions(+), 30 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> index df338cb..958e5d5 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> @@ -21,11 +21,8 @@ Optional properties:
>                                 is wired that way. If not specified, a bus
>                                 width of 8 is assumed.
>
> - - ti,nand-ecc-opt:            A string setting the ECC layout to use. One of:
> -
> -               "sw"            Software method (default)
> -               "hw"            Hardware method
> -               "hw-romcode"    gpmc hamming mode method & romcode layout
> + - ti,nand-ecc-scheme:         A string setting the ECC layout to use. One of:
> +               "ham1"          1-bit Hamming ecc code

As has been pointed out, this breaks compatibility which should be
avoided unless you know the state of use of this binding. I fail to
see the advantage of using scheme over opt. You could simply add ham1
here and maintain compatibility. Instead of removing sw, hw, etc. you
can simply deprecate them or decide that the kernel doesn't support
those options.

However, since you are willing to break compatibility, then you should
the generic NAND binding and use nand-ecc-mode adding the values you
need.

Documentation/devicetree/bindings/mtd/nand.txt:
* MTD generic binding

- nand-ecc-mode : String, operation mode of the NAND ecc mode.
  Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
  "soft_bch".
- nand-bus-width : 8 or 16 bus width if not present 8
- nand-on-flash-bbt: boolean to enable on flash bbt option if not present false


>                 "bch4"          4-bit BCH ecc code
>                 "bch8"          8-bit BCH ecc code
>
> @@ -36,8 +33,11 @@ Optional properties:
>                 "prefetch-dma"          Prefetch enabled sDMA mode
>                 "prefetch-irq"          Prefetch enabled irq mode
>
> - - elm_id:     Specifies elm device node. This is required to support BCH
> -               error correction using ELM module.
> + - ti,elm-id:  Specifies pHandle of the ELM devicetree node.
> +               ELM is an on-chip hardware engine on TI SoC which is used for
> +               locating ECC errors for BCHx algorithms. SoC devices which have
> +               ELM hardware engines should specify this device node in .dtsi
> +               Using ELM for ECC error correction frees some CPU cycles.

While yes, this is better name and documentation, I don't know that
breaking compatibility is justified.

Rob

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

Hi,

> >
> > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > index df338cb..958e5d5 100644
> > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > @@ -21,11 +21,8 @@ Optional properties:
> >                                 is wired that way. If not specified, a bus
> >                                 width of 8 is assumed.
> >
> > - - ti,nand-ecc-opt:            A string setting the ECC layout to use. One of:
> > -
> > -               "sw"            Software method (default)
> > -               "hw"            Hardware method
> > -               "hw-romcode"    gpmc hamming mode method & romcode layout
> > + - ti,nand-ecc-scheme:         A string setting the ECC layout to use. One of:
> > +               "ham1"          1-bit Hamming ecc code
> 
> As has been pointed out, this breaks compatibility which should be
> avoided unless you know the state of use of this binding. I fail to
> see the advantage of using scheme over opt. You could simply add ham1
> here and maintain compatibility. Instead of removing sw, hw, etc. you
> can simply deprecate them or decide that the kernel doesn't support
> those options.
> 
Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier 
comments from Olof. So either way is fine with me. Should I revert
it back to 'ti,nand-ecc-opt' ?

Also, "sw", "hw_romcode" have been deprecated, they are no longer
supported in driver. So shouldn't they be removed from the documentation ?

> However, since you are willing to break compatibility, then you should
> the generic NAND binding and use nand-ecc-mode adding the values you
> need.
> 
> Documentation/devicetree/bindings/mtd/nand.txt:
> * MTD generic binding
> 
> - nand-ecc-mode : String, operation mode of the NAND ecc mode.
>   Supported values are: "none", "soft", "hw", "hw_syndrome",
> "hw_oob_first",
>   "soft_bch".

Yes I can use generic 'nand-ecc-mode', But the binding values like
"soft", "hw", "soft_bch" are too generic to describe the hardware.
- BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16
  so "soft_bch" is ambiguous.
- Similarly "soft" and "hw" do not determine the algorithm used
   like Hamming or BCH.

(a) Should I update the generic binding values (if no one else is using) ? like 
	sw -> ham1_sw
	hw -> ham1_hw
	soft_bch-> soft_bch4, soft_bch8
OR 
(b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ?
      keeping the old ones intact. And how should I handle un-supported
     values, (Is pr_err during kernel probe enough ?).

[...]

> > - - elm_id:     Specifies elm device node. This is required to support BCH
> > -               error correction using ELM module.
> > + - ti,elm-id:  Specifies pHandle of the ELM devicetree node.
> > +               ELM is an on-chip hardware engine on TI SoC which is used for
> > +               locating ECC errors for BCHx algorithms. SoC devices which have
> > +               ELM hardware engines should specify this device node in .dtsi
> > +               Using ELM for ECC error correction frees some CPU cycles.
> 
> While yes, this is better name and documentation, I don't know that
> breaking compatibility is justified.
> 
As this is TI specific binding, so manufacturer's name should have been
included.  But as this was missed while adding this binding, so this should
be fixed now. (Olof also recommended the same).

AFAIK, For TI's NAND OMAP driver, currently there are not many
end-users are using these bindings from mainline DT kernel.
So this patch series mainly aims to cleanup NAND driver so that migrate
to latest DT based kernel from board-file one is easy.
So changes should be acceptable from end-user's point, its only matter
of which path to take. 
Request you to please help me finalize this before I resend next patch
series addressing other comments from Brian.

Thank You..
with regards, pekon

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

> > From: Rob Herring [mailto:robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> > > From: Pekon Gupta [mailto:pekon-l0cyMroinI0@public.gmane.org]
> > >
> > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > index df338cb..958e5d5 100644
> > > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > @@ -21,11 +21,8 @@ Optional properties:
> > >                                 is wired that way. If not specified, a bus
> > >                                 width of 8 is assumed.
> > >
> > > - - ti,nand-ecc-opt:            A string setting the ECC layout to use. One of:
> > > -
> > > -               "sw"            Software method (default)
> > > -               "hw"            Hardware method
> > > -               "hw-romcode"    gpmc hamming mode method & romcode
> layout
> > > + - ti,nand-ecc-scheme:         A string setting the ECC layout to use. One of:
> > > +               "ham1"          1-bit Hamming ecc code
> >
> > As has been pointed out, this breaks compatibility which should be
> > avoided unless you know the state of use of this binding. I fail to
> > see the advantage of using scheme over opt. You could simply add ham1
> > here and maintain compatibility. Instead of removing sw, hw, etc. you
> > can simply deprecate them or decide that the kernel doesn't support
> > those options.
> >
> Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier
> comments from Olof. So either way is fine with me. Should I revert
> it back to 'ti,nand-ecc-opt' ?
> 
> Also, "sw", "hw_romcode" have been deprecated, they are no longer
> supported in driver. So shouldn't they be removed from the documentation
> ?
> 
> > However, since you are willing to break compatibility, then you should
> > the generic NAND binding and use nand-ecc-mode adding the values you
> > need.
> >
> > Documentation/devicetree/bindings/mtd/nand.txt:
> > * MTD generic binding
> >
> > - nand-ecc-mode : String, operation mode of the NAND ecc mode.
> >   Supported values are: "none", "soft", "hw", "hw_syndrome",
> > "hw_oob_first",
> >   "soft_bch".
> 
> Yes I can use generic 'nand-ecc-mode', But the binding values like
> "soft", "hw", "soft_bch" are too generic to describe the hardware.
> - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16
>   so "soft_bch" is ambiguous.
> - Similarly "soft" and "hw" do not determine the algorithm used
>    like Hamming or BCH.
> 
> (a) Should I update the generic binding values (if no one else is using) ? like
> 	sw -> ham1_sw
> 	hw -> ham1_hw
> 	soft_bch-> soft_bch4, soft_bch8
> OR
> (b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ?
>       keeping the old ones intact. And how should I handle un-supported
>      values, (Is pr_err during kernel probe enough ?).
> 
> [...]
> 
> > > - - elm_id:     Specifies elm device node. This is required to support BCH
> > > -               error correction using ELM module.
> > > + - ti,elm-id:  Specifies pHandle of the ELM devicetree node.
> > > +               ELM is an on-chip hardware engine on TI SoC which is used for
> > > +               locating ECC errors for BCHx algorithms. SoC devices which have
> > > +               ELM hardware engines should specify this device node in .dtsi
> > > +               Using ELM for ECC error correction frees some CPU cycles.
> >
> > While yes, this is better name and documentation, I don't know that
> > breaking compatibility is justified.
> >
> As this is TI specific binding, so manufacturer's name should have been
> included.  But as this was missed while adding this binding, so this should
> be fixed now. (Olof also recommended the same).
> 
> AFAIK, For TI's NAND OMAP driver, currently there are not many
> end-users are using these bindings from mainline DT kernel.
> So this patch series mainly aims to cleanup NAND driver so that migrate
> to latest DT based kernel from board-file one is easy.
> So changes should be acceptable from end-user's point, its only matter
> of which path to take.
> Request you to please help me finalize this before I resend next patch
> series addressing other comments from Brian.
> 

+ Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
+ Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
+ Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
+ Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

CC other DT maintainers, who were missed in this branch of mail-chain.

with regards, pekon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Mark Rutland]

On Thu, Sep 26, 2013 at 11:54:39AM +0100, Gupta, Pekon wrote:
> 
> > > From: Rob Herring [mailto:robherring2@gmail.com]
> > > > From: Pekon Gupta [mailto:pekon@ti.com]
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > > index df338cb..958e5d5 100644
> > > > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > > @@ -21,11 +21,8 @@ Optional properties:
> > > >                                 is wired that way. If not specified, a bus
> > > >                                 width of 8 is assumed.
> > > >
> > > > - - ti,nand-ecc-opt:            A string setting the ECC layout to use. One of:
> > > > -
> > > > -               "sw"            Software method (default)
> > > > -               "hw"            Hardware method
> > > > -               "hw-romcode"    gpmc hamming mode method & romcode
> > layout
> > > > + - ti,nand-ecc-scheme:         A string setting the ECC layout to use. One of:
> > > > +               "ham1"          1-bit Hamming ecc code
> > >
> > > As has been pointed out, this breaks compatibility which should be
> > > avoided unless you know the state of use of this binding. I fail to
> > > see the advantage of using scheme over opt. You could simply add ham1
> > > here and maintain compatibility. Instead of removing sw, hw, etc. you
> > > can simply deprecate them or decide that the kernel doesn't support
> > > those options.
> > >
> > Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier
> > comments from Olof. So either way is fine with me. Should I revert
> > it back to 'ti,nand-ecc-opt' ?

I think that if the binding is already in use then we shouldn't break
it, unless you're _definitely_ the only user.

> > 
> > Also, "sw", "hw_romcode" have been deprecated, they are no longer
> > supported in driver. So shouldn't they be removed from the documentation
> > ?

Deprecated properties should be marked as deprecated, but continue to be
documented. That at least prevents the names being repurposed in an
incompatible way, and explains to anyone who looks at the document that
the proeprty is deprecated rather than simply undocumented.

> > 
> > > However, since you are willing to break compatibility, then you should
> > > the generic NAND binding and use nand-ecc-mode adding the values you
> > > need.
> > >
> > > Documentation/devicetree/bindings/mtd/nand.txt:
> > > * MTD generic binding
> > >
> > > - nand-ecc-mode : String, operation mode of the NAND ecc mode.
> > >   Supported values are: "none", "soft", "hw", "hw_syndrome",
> > > "hw_oob_first",
> > >   "soft_bch".
> > 
> > Yes I can use generic 'nand-ecc-mode', But the binding values like
> > "soft", "hw", "soft_bch" are too generic to describe the hardware.
> > - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16
> >   so "soft_bch" is ambiguous.

It does indeed seem like the generic properties are not generic enough,
at least from my quick look other them. However, I am not familiar with
NAND, so I may have misunderstood.

> > - Similarly "soft" and "hw" do not determine the algorithm used
> >    like Hamming or BCH.
> > 
> > (a) Should I update the generic binding values (if no one else is using) ? like
> > 	sw -> ham1_sw
> > 	hw -> ham1_hw
> > 	soft_bch-> soft_bch4, soft_bch8

What do the current property names actually correspond to logically? If
we did this and there are existing users, the old DTs need to continue
functioning.

> > OR
> > (b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ?
> >       keeping the old ones intact. And how should I handle un-supported
> >      values, (Is pr_err during kernel probe enough ?).

I think something like this, but with the names from (a) would be
preferrable.

> > 
> > [...]
> > 
> > > > - - elm_id:     Specifies elm device node. This is required to support BCH
> > > > -               error correction using ELM module.
> > > > + - ti,elm-id:  Specifies pHandle of the ELM devicetree node.
> > > > +               ELM is an on-chip hardware engine on TI SoC which is used for
> > > > +               locating ECC errors for BCHx algorithms. SoC devices which have
> > > > +               ELM hardware engines should specify this device node in .dtsi
> > > > +               Using ELM for ECC error correction frees some CPU cycles.
> > >
> > > While yes, this is better name and documentation, I don't know that
> > > breaking compatibility is justified.
> > >
> > As this is TI specific binding, so manufacturer's name should have been
> > included.  But as this was missed while adding this binding, so this should
> > be fixed now. (Olof also recommended the same).

We could update the binding to prefer ti,elm-id, and deprecate elm_id
while maintaining support in the kernel.

Thanks,
Mark.

> > 
> > AFAIK, For TI's NAND OMAP driver, currently there are not many
> > end-users are using these bindings from mainline DT kernel.
> > So this patch series mainly aims to cleanup NAND driver so that migrate
> > to latest DT based kernel from board-file one is easy.
> > So changes should be acceptable from end-user's point, its only matter
> > of which path to take.
> > Request you to please help me finalize this before I resend next patch
> > series addressing other comments from Brian.
> > 
> 
> + Mark Rutland <mark.rutland@arm.com>
> + Pawel Moll <pawel.moll@arm.com>
> + Ian Campbell <ijc+devicetree@hellion.org.uk>
> + Stephen Warren <swarren@wwwdotorg.org>
> 
> CC other DT maintainers, who were missed in this branch of mail-chain.
> 
> with regards, pekon
> 

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

Hi All,

So, based on feedbacks from everyone, I could come to following
conclusions. Please confirm, if those are acceptable ?

> From: Mark Rutland [mailto:mark.rutland@arm.com]
> > 
> On Thu, Sep 26, 2013 at 11:54:39AM +0100, Gupta, Pekon wrote:
> >
> > > > From: Rob Herring [mailto:robherring2@gmail.com]
> > > > > From: Pekon Gupta [mailto:pekon@ti.com]
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > > b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > > > index df338cb..958e5d5 100644
> > > > > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > > > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > > > @@ -21,11 +21,8 @@ Optional properties:
> > > > >                                 is wired that way. If not specified, a bus
> > > > >                                 width of 8 is assumed.
> > > > >
> > > > > - - ti,nand-ecc-opt:            A string setting the ECC layout to use. One of:
> > > > > -
> > > > > -               "sw"            Software method (default)
> > > > > -               "hw"            Hardware method
> > > > > -               "hw-romcode"    gpmc hamming mode method & romcode
> > > layout
> > > > > + - ti,nand-ecc-scheme:         A string setting the ECC layout to use.
> One of:
> > > > > +               "ham1"          1-bit Hamming ecc code
> > > >
> > > > As has been pointed out, this breaks compatibility which should be
> > > > avoided unless you know the state of use of this binding. I fail to
> > > > see the advantage of using scheme over opt. You could simply add
> ham1
> > > > here and maintain compatibility. Instead of removing sw, hw, etc. you
> > > > can simply deprecate them or decide that the kernel doesn't support
> > > > those options.
> > > >
> > > Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier
> > > comments from Olof. So either way is fine with me. Should I revert
> > > it back to 'ti,nand-ecc-opt' ?
> 
> I think that if the binding is already in use then we shouldn't break
> it, unless you're _definitely_ the only user.
> 
Agreed, would revert back to 'ti,nand-ecc-scheme'


> > >
> > > Also, "sw", "hw_romcode" have been deprecated, they are no longer
> > > supported in driver. So shouldn't they be removed from the
> documentation
> > > ?
> 
> Deprecated properties should be marked as deprecated, but continue to be
> documented. That at least prevents the names being repurposed in an
> incompatible way, and explains to anyone who looks at the document that
> the proeprty is deprecated rather than simply undocumented.
>
Agreed.
- keep existing values in documentation (sw, hw, hw_romcode) but mark
  them as deprecated.
- add new values (ham1, bch4, bch8,..)

 
> > >
> > > > However, since you are willing to break compatibility, then you should
> > > > the generic NAND binding and use nand-ecc-mode adding the values
> you
> > > > need.
> > > >
> > > > Documentation/devicetree/bindings/mtd/nand.txt:
> > > > * MTD generic binding
> > > >
> > > > - nand-ecc-mode : String, operation mode of the NAND ecc mode.
> > > >   Supported values are: "none", "soft", "hw", "hw_syndrome",
> > > > "hw_oob_first",
> > > >   "soft_bch".
> > >
> > > Yes I can use generic 'nand-ecc-mode', But the binding values like
> > > "soft", "hw", "soft_bch" are too generic to describe the hardware.
> > > - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16
> > >   so "soft_bch" is ambiguous.
> 
> It does indeed seem like the generic properties are not generic enough,
> at least from my quick look other them. However, I am not familiar with
> NAND, so I may have misunderstood.
> 
Would not use generic 'nand-ecc-mode' property, instead continue
with 'ti,nand-ecc-opt'.


> > > - Similarly "soft" and "hw" do not determine the algorithm used
> > >    like Hamming or BCH.
> > >
> > > (a) Should I update the generic binding values (if no one else is using) ?
> like
> > > 	sw -> ham1_sw
> > > 	hw -> ham1_hw
> > > 	soft_bch-> soft_bch4, soft_bch8
> 
> What do the current property names actually correspond to logically? If
> we did this and there are existing users, the old DTs need to continue
> functioning.
> 
> > > OR
> > > (b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ?
> > >       keeping the old ones intact. And how should I handle un-supported
> > >      values, (Is pr_err during kernel probe enough ?).
> 
> I think something like this, but with the names from (a) would be
> preferrable.
> 
As Brian pointed, implementation of ecc-scheme can be very different
from vendor to vendor, and even SoC to SoC within same vendor,
Thus its difficult to generalize as common DT binding for everyone.
- Even if we try to do, there would be too many values, out of which
  only selectable would be applicable for a given SoC.
- And some platforms might need extra DT information to get driver
  working, because h/w was designed that way. So it would be mess.
So, its better not to have a generic ecc-scheme binding, instead let every
vendor define it specifically.

So for TI OMAP NAND driver, I'm continuing to use 'ti,nand-ecc-opt'
as described above. Is this acceptable ?


> > >
> > > [...]
> > >
> > > > > - - elm_id:     Specifies elm device node. This is required to support
> BCH
> > > > > -               error correction using ELM module.
> > > > > + - ti,elm-id:  Specifies pHandle of the ELM devicetree node.
> > > > > +               ELM is an on-chip hardware engine on TI SoC which is used
> for
> > > > > +               locating ECC errors for BCHx algorithms. SoC devices which
> have
> > > > > +               ELM hardware engines should specify this device node in
> .dtsi
> > > > > +               Using ELM for ECC error correction frees some CPU cycles.
> > > >
> > > > While yes, this is better name and documentation, I don't know that
> > > > breaking compatibility is justified.
> > > >
> > > As this is TI specific binding, so manufacturer's name should have been
> > > included.  But as this was missed while adding this binding, so this should
> > > be fixed now. (Olof also recommended the same).
> 
> We could update the binding to prefer ti,elm-id, and deprecate elm_id
> while maintaining support in the kernel.
> 
Agreed. 
- would keep elm_id but mark it deprecated in Documentation
- add new ti,elm-id

> > >
> > > AFAIK, For TI's NAND OMAP driver, currently there are not many
> > > end-users are using these bindings from mainline DT kernel.
> > > So this patch series mainly aims to cleanup NAND driver so that migrate
> > > to latest DT based kernel from board-file one is easy.
> > > So changes should be acceptable from end-user's point, its only matter
> > > of which path to take.
> > > Request you to please help me finalize this before I resend next patch
> > > series addressing other comments from Brian.
> > >

with regards, pekon

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

[I see Mark made some of the same comments while I was typing this
email]

On Thu, Sep 26, 2013 at 06:08:42AM +0000, Gupta, Pekon wrote:
> > From: Rob Herring [mailto:robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> > > From: Pekon Gupta [mailto:pekon-l0cyMroinI0@public.gmane.org]
> > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > index df338cb..958e5d5 100644
> > > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > > @@ -21,11 +21,8 @@ Optional properties:
> > >                                 is wired that way. If not specified, a bus
> > >                                 width of 8 is assumed.
> > >
> > > - - ti,nand-ecc-opt:            A string setting the ECC layout to use. One of:
> > > -
> > > -               "sw"            Software method (default)
> > > -               "hw"            Hardware method
> > > -               "hw-romcode"    gpmc hamming mode method & romcode layout
> > > + - ti,nand-ecc-scheme:         A string setting the ECC layout to use. One of:
> > > +               "ham1"          1-bit Hamming ecc code
> > 
> > As has been pointed out, this breaks compatibility which should be
> > avoided unless you know the state of use of this binding. I fail to
> > see the advantage of using scheme over opt. You could simply add ham1
> > here and maintain compatibility. Instead of removing sw, hw, etc. you
> > can simply deprecate them or decide that the kernel doesn't support
> > those options.
> > 
> Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier 
> comments from Olof. So either way is fine with me. Should I revert
> it back to 'ti,nand-ecc-opt' ?
> 
> Also, "sw", "hw_romcode" have been deprecated, they are no longer
> supported in driver. So shouldn't they be removed from the documentation ?

I think leaving them in the documentation but marking them as
"deprecated" makes sense.

> > However, since you are willing to break compatibility, then you should
> > the generic NAND binding and use nand-ecc-mode adding the values you
> > need.
> > 
> > Documentation/devicetree/bindings/mtd/nand.txt:
> > * MTD generic binding
> > 
> > - nand-ecc-mode : String, operation mode of the NAND ecc mode.
> >   Supported values are: "none", "soft", "hw", "hw_syndrome",
> > "hw_oob_first",
> >   "soft_bch".
> 
> Yes I can use generic 'nand-ecc-mode', But the binding values like
> "soft", "hw", "soft_bch" are too generic to describe the hardware.
> - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16
>   so "soft_bch" is ambiguous.
> - Similarly "soft" and "hw" do not determine the algorithm used
>    like Hamming or BCH.
> 
> (a) Should I update the generic binding values (if no one else is using) ? like 
> 	sw -> ham1_sw
> 	hw -> ham1_hw
> 	soft_bch-> soft_bch4, soft_bch8
> OR 
> (b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ?
>       keeping the old ones intact. And how should I handle un-supported
>      values, (Is pr_err during kernel probe enough ?).

The existing nand-ecc-mode binding is a bit peculiar and hard to
maintain generically for all NAND, IMO. ECC is often very specific to
each driver/controller. What's to say that a given software BCH4 library
(such as the one found in Linux) will match a given controller's HW BCH4
layout and encoding format? Perhaps Pekon's OMAP NAND driver can
straighten things out such that HW and SW ECC are interchangeable for a
given BCH mode, but we can't guarantee all drivers/controllers make this
possible.

So, what's the advantage of using this generic binding (nand-ecc-mode)
for all NAND hardware instead of defining distinct hardware-specific
bindings for different sets of hardware? The former seems like we will
clutter up the nand-ecc-mode namespace such that any one set of
hardware/software will only support a small subset of the potential
options. And it doesn't seem to win us a lot.

What's more, this single binding doesn't seem flexible enough for the
hardware I deal with. I have a NAND controller whose ECC HW can be
programmed to one of dozens of different ECC modes, parameterized by
strength (i.e., BCH-x, where x varies) and ECC step/sector size (i.e.,
each ECC block covers 512B or 1024B of data).

So I'm not convinced that extending this nand-ecc-mode property is
correct at all. But if we do want to, perhaps we'd need to introduce
additional orthogonal properties to specify strength and step size,
rather than listing all combinations as separate values for
nand-ecc-mode.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 2/4] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe

[Pekon Gupta]

OMAP NAND driver support multiple ECC scheme, which can used in following
different flavours, depending on in-build Hardware engines supported by SoC.

+---------------------------------------+---------------+---------------+
| ECC scheme                            |ECC calculation|Error detection|
+---------------------------------------+---------------+---------------+
|OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W            |
|(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
|(requires CONFIG_MTD_NAND_OMAP_BCH &&  |               |               |
| ti,elm-id in DT)                      |               |               |
+---------------------------------------+---------------+---------------+
To optimize footprint of omap2-nand driver, selection of some ECC schemes
also require enabling following Kconfigs, in addition to setting appropriate
DT bindings
- Kconfig:CONFIG_MTD_NAND_ECC_BCH        enables S/W based BCH ECC algorithm
- Kconfig:CONFIG_MTD_NAND_OMAP_BCH       enables H/W based BCH ECC algorithm

This patch
- removes OMAP_ECC_HAMMING_CODE_DEFAULT and OMAP_ECC_HAMMING_CODE_HW_ROMCODE
- separates the configurations for other ECC schemes
- fixes dependency issues based on Kconfig options
- updates ELM device detection in is_elm_present()
- cleans redundant code

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap2.c                     | 530 +++++++++++++--------------
 include/linux/platform_data/mtd-nand-omap2.h |   7 +-
 2 files changed, 260 insertions(+), 277 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 4ecf0e5..420078f 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -25,8 +25,10 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
+#ifdef CONFIG_MTD_NAND_ECC_BCH
 #include <linux/bch.h>
+#endif
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
 #include <linux/platform_data/elm.h>
 #endif
 
@@ -141,6 +143,9 @@
 #define BCH_ECC_SIZE0		0x0	/* ecc_size0 = 0, no oob protection */
 #define BCH_ECC_SIZE1		0x20	/* ecc_size1 = 32 */
 
+#define BADBLOCK_MARKER_LENGTH		0x2
+#define OMAP_ECC_BCH8_POLYNOMIAL	0x201b
+
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
 static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
 	0xac, 0x6b, 0xff, 0x99, 0x7b};
@@ -182,14 +187,11 @@ struct omap_nand_info {
 	u_char				*buf;
 	int					buf_len;
 	struct gpmc_nand_regs		reg;
-
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
-	struct bch_control             *bch;
-	struct nand_ecclayout           ecclayout;
+	/* fields specific for BCHx_HW ECC scheme */
+	struct bch_control              *bch;
 	bool				is_elm_used;
 	struct device			*elm_dev;
 	struct device_node		*of_node;
-#endif
 };
 
 /**
@@ -1058,8 +1060,6 @@ static int omap_dev_ready(struct mtd_info *mtd)
 	}
 }
 
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
-
 /**
  * omap3_enable_hwecc_bch - Program OMAP3 GPMC to perform BCH ECC correction
  * @mtd: MTD device structure
@@ -1141,6 +1141,7 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
 	writel(ECCCLEAR | ECC1, info->reg.gpmc_ecc_control);
 }
 
+#ifdef CONFIG_MTD_NAND_ECC_BCH
 /**
  * omap3_calculate_ecc_bch4 - Generate 7 bytes of ECC bytes
  * @mtd: MTD device structure
@@ -1227,6 +1228,62 @@ static int omap3_calculate_ecc_bch8(struct mtd_info *mtd, const u_char *dat,
 }
 
 /**
+ * omap3_correct_data_bch - Decode received data and correct errors
+ * @mtd: MTD device structure
+ * @data: page data
+ * @read_ecc: ecc read from nand flash
+ * @calc_ecc: ecc read from HW ECC registers
+ */
+static int omap3_correct_data_bch(struct mtd_info *mtd, u_char *data,
+				  u_char *read_ecc, u_char *calc_ecc)
+{
+	int i, count;
+	/* cannot correct more than 8 errors */
+	unsigned int errloc[8];
+	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
+						   mtd);
+
+	count = decode_bch(info->bch, NULL, 512, read_ecc, calc_ecc, NULL,
+			   errloc);
+	if (count > 0) {
+		/* correct errors */
+		for (i = 0; i < count; i++) {
+			/* correct data only, not ecc bytes */
+			if (errloc[i] < 8*512)
+				data[errloc[i]/8] ^= 1 << (errloc[i] & 7);
+			pr_debug("corrected bitflip %u\n", errloc[i]);
+		}
+	} else if (count < 0) {
+		pr_err("ecc unrecoverable error\n");
+	}
+	return count;
+}
+
+/**
+ * omap3_free_bch - Release BCH ecc resources
+ * @mtd: MTD device structure
+ */
+static void omap3_free_bch(struct mtd_info *mtd)
+{
+	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
+						   mtd);
+	if (info->bch) {
+		free_bch(info->bch);
+		info->bch = NULL;
+	}
+}
+
+#else
+
+static void omap3_free_bch(struct mtd_info *mtd)
+{
+}
+
+#endif /* CONFIG_MTD_NAND_ECC_BCH */
+
+
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+/**
  * omap3_calculate_ecc_bch - Generate bytes of ECC bytes
  * @mtd:	MTD device structure
  * @dat:	The pointer to data on which ecc is computed
@@ -1519,38 +1576,6 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
 }
 
 /**
- * omap3_correct_data_bch - Decode received data and correct errors
- * @mtd: MTD device structure
- * @data: page data
- * @read_ecc: ecc read from nand flash
- * @calc_ecc: ecc read from HW ECC registers
- */
-static int omap3_correct_data_bch(struct mtd_info *mtd, u_char *data,
-				  u_char *read_ecc, u_char *calc_ecc)
-{
-	int i, count;
-	/* cannot correct more than 8 errors */
-	unsigned int errloc[8];
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-
-	count = decode_bch(info->bch, NULL, 512, read_ecc, calc_ecc, NULL,
-			   errloc);
-	if (count > 0) {
-		/* correct errors */
-		for (i = 0; i < count; i++) {
-			/* correct data only, not ecc bytes */
-			if (errloc[i] < 8*512)
-				data[errloc[i]/8] ^= 1 << (errloc[i] & 7);
-			pr_debug("corrected bitflip %u\n", errloc[i]);
-		}
-	} else if (count < 0) {
-		pr_err("ecc unrecoverable error\n");
-	}
-	return count;
-}
-
-/**
  * omap_write_page_bch - BCH ecc based write page function for entire page
  * @mtd:		mtd info structure
  * @chip:		nand chip info structure
@@ -1637,186 +1662,32 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
 }
 
 /**
- * omap3_free_bch - Release BCH ecc resources
- * @mtd: MTD device structure
- */
-static void omap3_free_bch(struct mtd_info *mtd)
-{
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-	if (info->bch) {
-		free_bch(info->bch);
-		info->bch = NULL;
-	}
-}
-
-/**
- * omap3_init_bch - Initialize BCH ECC
- * @mtd: MTD device structure
- * @ecc_opt: OMAP ECC mode (OMAP_ECC_BCH4_CODE_HW or OMAP_ECC_BCH8_CODE_HW)
- */
-static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
-{
-	int max_errors;
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-#ifdef CONFIG_MTD_NAND_OMAP_BCH8
-	const int hw_errors = BCH8_MAX_ERROR;
-#else
-	const int hw_errors = BCH4_MAX_ERROR;
-#endif
-	enum bch_ecc bch_type;
-	const __be32 *parp;
-	int lenp;
-	struct device_node *elm_node;
-
-	info->bch = NULL;
-
-	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ?
-		BCH8_MAX_ERROR : BCH4_MAX_ERROR;
-	if (max_errors != hw_errors) {
-		pr_err("cannot configure %d-bit BCH ecc, only %d-bit supported",
-		       max_errors, hw_errors);
-		goto fail;
-	}
-
-	info->nand.ecc.size = 512;
-	info->nand.ecc.hwctl = omap3_enable_hwecc_bch;
-	info->nand.ecc.mode = NAND_ECC_HW;
-	info->nand.ecc.strength = max_errors;
-
-	if (hw_errors == BCH8_MAX_ERROR)
-		bch_type = BCH8_ECC;
-	else
-		bch_type = BCH4_ECC;
-
-	/* Detect availability of ELM module */
-	parp = of_get_property(info->of_node, "elm_id", &lenp);
-	if ((parp == NULL) && (lenp != (sizeof(void *) * 2))) {
-		pr_err("Missing elm_id property, fall back to Software BCH\n");
-		info->is_elm_used = false;
-	} else {
-		struct platform_device *pdev;
-
-		elm_node = of_find_node_by_phandle(be32_to_cpup(parp));
-		pdev = of_find_device_by_node(elm_node);
-		info->elm_dev = &pdev->dev;
-
-		if (elm_config(info->elm_dev, bch_type) == 0)
-			info->is_elm_used = true;
-	}
-
-	if (info->is_elm_used && (mtd->writesize <= 4096)) {
-
-		if (hw_errors == BCH8_MAX_ERROR)
-			info->nand.ecc.bytes = BCH8_SIZE;
-		else
-			info->nand.ecc.bytes = BCH4_SIZE;
-
-		info->nand.ecc.correct = omap_elm_correct_data;
-		info->nand.ecc.calculate = omap3_calculate_ecc_bch;
-		info->nand.ecc.read_page = omap_read_page_bch;
-		info->nand.ecc.write_page = omap_write_page_bch;
-	} else {
-		/*
-		 * software bch library is only used to detect and
-		 * locate errors
-		 */
-		info->bch = init_bch(13, max_errors,
-				0x201b /* hw polynomial */);
-		if (!info->bch)
-			goto fail;
-
-		info->nand.ecc.correct = omap3_correct_data_bch;
-
-		/*
-		 * The number of corrected errors in an ecc block that will
-		 * trigger block scrubbing defaults to the ecc strength (4 or 8)
-		 * Set mtd->bitflip_threshold here to define a custom threshold.
-		 */
-
-		if (max_errors == 8) {
-			info->nand.ecc.bytes = 13;
-			info->nand.ecc.calculate = omap3_calculate_ecc_bch8;
-		} else {
-			info->nand.ecc.bytes = 7;
-			info->nand.ecc.calculate = omap3_calculate_ecc_bch4;
-		}
-	}
-
-	pr_info("enabling NAND BCH ecc with %d-bit correction\n", max_errors);
-	return 0;
-fail:
-	omap3_free_bch(mtd);
-	return -1;
-}
-
-/**
- * omap3_init_bch_tail - Build an oob layout for BCH ECC correction.
- * @mtd: MTD device structure
+ * is_elm_present - checks for presence of ELM module by scanning DT nodes
+ * @omap_nand_info: NAND device structure containing platform data
+ * @bch_type: 0x0=BCH4, 0x1=BCH8, 0x2=BCH16
  */
-static int omap3_init_bch_tail(struct mtd_info *mtd)
+static int is_elm_present(struct omap_nand_info *info,
+			struct device_node *elm_node, enum bch_ecc bch_type)
 {
-	int i, steps, offset;
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-	struct nand_ecclayout *layout = &info->ecclayout;
-
-	/* build oob layout */
-	steps = mtd->writesize/info->nand.ecc.size;
-	layout->eccbytes = steps*info->nand.ecc.bytes;
-
-	/* do not bother creating special oob layouts for small page devices */
-	if (mtd->oobsize < 64) {
-		pr_err("BCH ecc is not supported on small page devices\n");
-		goto fail;
+	struct platform_device *pdev;
+	info->is_elm_used = false;
+	/* check whether elm-id is passed via DT */
+	if (!elm_node) {
+		pr_err("nand: error: ELM DT node not found\n");
+		return -ENODEV;
 	}
-
-	/* reserve 2 bytes for bad block marker */
-	if (layout->eccbytes+2 > mtd->oobsize) {
-		pr_err("no oob layout available for oobsize %d eccbytes %u\n",
-		       mtd->oobsize, layout->eccbytes);
-		goto fail;
+	pdev = of_find_device_by_node(elm_node);
+	/* check whether ELM device is registered */
+	if (!pdev) {
+		pr_err("nand: error: ELM device not found\n");
+		return -ENODEV;
 	}
-
-	/* ECC layout compatible with RBL for BCH8 */
-	if (info->is_elm_used && (info->nand.ecc.bytes == BCH8_SIZE))
-		offset = 2;
-	else
-		offset = mtd->oobsize - layout->eccbytes;
-
-	/* put ecc bytes at oob tail */
-	for (i = 0; i < layout->eccbytes; i++)
-		layout->eccpos[i] = offset + i;
-
-	if (info->is_elm_used && (info->nand.ecc.bytes == BCH8_SIZE))
-		layout->oobfree[0].offset = 2 + layout->eccbytes * steps;
-	else
-		layout->oobfree[0].offset = 2;
-
-	layout->oobfree[0].length = mtd->oobsize-2-layout->eccbytes;
-	info->nand.ecc.layout = layout;
-
-	if (!(info->nand.options & NAND_BUSWIDTH_16))
-		info->nand.badblock_pattern = &bb_descrip_flashbased;
+	/* ELM module available, now configure it */
+	info->elm_dev = &pdev->dev;
+	if (elm_config(info->elm_dev, bch_type))
+		return -ENODEV;
+	info->is_elm_used = true;
 	return 0;
-fail:
-	omap3_free_bch(mtd);
-	return -1;
-}
-
-#else
-static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
-{
-	pr_err("CONFIG_MTD_NAND_OMAP_BCH is not enabled\n");
-	return -1;
-}
-static int omap3_init_bch_tail(struct mtd_info *mtd)
-{
-	return -1;
-}
-static void omap3_free_bch(struct mtd_info *mtd)
-{
 }
 #endif /* CONFIG_MTD_NAND_OMAP_BCH */
 
@@ -1824,10 +1695,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 {
 	struct omap_nand_info		*info;
 	struct omap_nand_platform_data	*pdata;
+	struct mtd_info			*mtd;
+	struct nand_chip		*chip;
+	struct nand_ecclayout		*ecclayout;
 	int				err;
-	int				i, offset;
-	dma_cap_mask_t mask;
-	unsigned sig;
+	int				i;
+	dma_cap_mask_t			mask;
+	unsigned			sig;
 	struct resource			*res;
 	struct mtd_part_parser_data	ppdata = {};
 
@@ -1846,20 +1720,20 @@ static int omap_nand_probe(struct platform_device *pdev)
 	spin_lock_init(&info->controller.lock);
 	init_waitqueue_head(&info->controller.wq);
 
-	info->pdev = pdev;
+	mtd			= &info->mtd;
+	mtd->name		= dev_name(&pdev->dev);
+	mtd->owner		= THIS_MODULE;
+	mtd->priv		= &info->nand;
+	chip			= mtd->priv;
 
+	info->pdev		= pdev;
 	info->gpmc_cs		= pdata->cs;
 	info->reg		= pdata->reg;
+	info->bch		= NULL;
 
-	info->mtd.priv		= &info->nand;
-	info->mtd.name		= dev_name(&pdev->dev);
-	info->mtd.owner		= THIS_MODULE;
-
-	info->nand.options	= pdata->devsize;
+	info->nand.options	= NAND_BUSWIDTH_AUTO;
 	info->nand.options	|= NAND_SKIP_BBTSCAN;
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
 	info->of_node		= pdata->of_node;
-#endif
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (res == NULL) {
@@ -1903,6 +1777,30 @@ static int omap_nand_probe(struct platform_device *pdev)
 		info->nand.chip_delay = 50;
 	}
 
+	/* scan NAND device conncted to controller */
+	if (nand_scan_ident(mtd, 1, NULL)) {
+		err = -ENXIO;
+		goto out_release_mem_region;
+	}
+	pr_info("%s: detected %s NAND flash\n", DRIVER_NAME,
+			(info->nand.options & NAND_BUSWIDTH_16) ? "x16" : "x8");
+	if ((info->nand.options & NAND_BUSWIDTH_16) !=
+			(pdata->devsize & NAND_BUSWIDTH_16)) {
+		pr_err("%s: but incorrectly configured as %s", DRIVER_NAME,
+			(pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : "x8");
+		err = -EINVAL;
+		goto out_release_mem_region;
+	}
+
+	/* check for small page devices */
+	if ((mtd->oobsize < 64) &&
+		(pdata->ecc_opt != OMAP_ECC_HAMMING_CODE_HW)) {
+		pr_err("small page devices are not supported\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+	}
+
+	/* populate read & write API based on xfer_type selected */
 	switch (pdata->xfer_type) {
 	case NAND_OMAP_PREFETCH_POLLED:
 		info->nand.read_buf   = omap_read_buf_pref;
@@ -1992,64 +1890,152 @@ static int omap_nand_probe(struct platform_device *pdev)
 		goto out_release_mem_region;
 	}
 
-	/* select the ecc type */
-	if (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_DEFAULT)
-		info->nand.ecc.mode = NAND_ECC_SOFT;
-	else if ((pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW) ||
-		(pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW_ROMCODE)) {
+	/* populate MTD interface based on ECC scheme */
+	chip->ecclayout		= &omap_oobinfo;
+	chip->ecc.layout	= &omap_oobinfo;
+	ecclayout		= &omap_oobinfo;
+	switch (pdata->ecc_opt) {
+	case OMAP_ECC_HAMMING_CODE_HW:
+		pr_info("nand: using OMAP_ECC_HAMMING_CODE_HW\n");
+		info->nand.ecc.mode             = NAND_ECC_HW;
 		info->nand.ecc.bytes            = 3;
 		info->nand.ecc.size             = 512;
 		info->nand.ecc.strength         = 1;
 		info->nand.ecc.calculate        = omap_calculate_ecc;
 		info->nand.ecc.hwctl            = omap_enable_hwecc;
 		info->nand.ecc.correct          = omap_correct_data;
-		info->nand.ecc.mode             = NAND_ECC_HW;
-	} else if ((pdata->ecc_opt == OMAP_ECC_BCH4_CODE_HW) ||
-		   (pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW)) {
-		err = omap3_init_bch(&info->mtd, pdata->ecc_opt);
-		if (err) {
+		/* define custom ECC layout */
+		ecclayout->eccbytes		= info->nand.ecc.bytes *
+							(mtd->writesize /
+							info->nand.ecc.size);
+		if (info->nand.options & NAND_BUSWIDTH_16)
+			ecclayout->eccpos[0]	= BADBLOCK_MARKER_LENGTH;
+		else
+			ecclayout->eccpos[0]	= 1;
+		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
+							ecclayout->eccbytes;
+		goto custom_ecc_layout;
+		break;
+	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
+#ifdef CONFIG_MTD_NAND_ECC_BCH
+		pr_info("nand: using OMAP_ECC_BCH4_CODE_HW_DETECTION_SW\n");
+		info->nand.ecc.mode		= NAND_ECC_HW;
+		info->nand.ecc.size		= 512;
+		info->nand.ecc.bytes		= 7;
+		info->nand.ecc.strength		= 4;
+		info->nand.ecc.hwctl		= omap3_enable_hwecc_bch;
+		info->nand.ecc.correct		= omap3_correct_data_bch;
+		info->nand.ecc.calculate	= omap3_calculate_ecc_bch4;
+		/* define custom ECC layout */
+		ecclayout->eccbytes		= info->nand.ecc.bytes *
+						(mtd->writesize /
+						info->nand.ecc.size);
+		ecclayout->eccpos[0]		= info->mtd.oobsize -
+						ecclayout->eccbytes;
+		ecclayout->oobfree->offset	= BADBLOCK_MARKER_LENGTH;
+		/* software bch library is used for locating errors */
+		info->bch = init_bch(info->nand.ecc.bytes,
+					info->nand.ecc.strength,
+					OMAP_ECC_BCH8_POLYNOMIAL);
+		if (!info->bch) {
+			pr_err("nand: error: unable to use s/w BCH library\n");
 			err = -EINVAL;
 			goto out_release_mem_region;
 		}
-	}
-
-	/* 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 custom_ecc_layout;
+#else
+		pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+#endif
+		break;
+	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
+#ifdef CONFIG_MTD_NAND_ECC_BCH
+		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
+		info->nand.ecc.mode		= NAND_ECC_HW;
+		info->nand.ecc.size		= 512;
+		info->nand.ecc.bytes		= 13;
+		info->nand.ecc.strength		= 8;
+		info->nand.ecc.hwctl		= omap3_enable_hwecc_bch;
+		info->nand.ecc.correct		= omap3_correct_data_bch;
+		info->nand.ecc.calculate	= omap3_calculate_ecc_bch8;
+		/* define custom ECC layout */
+		ecclayout->eccbytes		= info->nand.ecc.bytes *
+							(mtd->writesize /
+							info->nand.ecc.size);
+		ecclayout->eccpos[0]		= info->mtd.oobsize -
+							ecclayout->eccbytes;
+		ecclayout->oobfree->offset	= BADBLOCK_MARKER_LENGTH;
+		/* software bch library is used for locating errors */
+		info->bch = init_bch(info->nand.ecc.bytes,
+					info->nand.ecc.strength,
+					OMAP_ECC_BCH8_POLYNOMIAL);
+		if (!info->bch) {
+			pr_err("nand: error: unable to use s/w BCH library\n");
+			err = -EINVAL;
 			goto out_release_mem_region;
 		}
-	}
-
-	/* rom code layout */
-	if (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) {
-
-		if (info->nand.options & NAND_BUSWIDTH_16)
-			offset = 2;
-		else {
-			offset = 1;
-			info->nand.badblock_pattern = &bb_descrip_flashbased;
-		}
-		omap_oobinfo.eccbytes = 3 * (info->mtd.oobsize/16);
-		for (i = 0; i < omap_oobinfo.eccbytes; i++)
-			omap_oobinfo.eccpos[i] = i+offset;
-
-		omap_oobinfo.oobfree->offset = offset + omap_oobinfo.eccbytes;
-		omap_oobinfo.oobfree->length = info->mtd.oobsize -
-					(offset + omap_oobinfo.eccbytes);
-
-		info->nand.ecc.layout = &omap_oobinfo;
-	} else if ((pdata->ecc_opt == OMAP_ECC_BCH4_CODE_HW) ||
-		   (pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW)) {
-		/* build OOB layout for BCH ECC correction */
-		err = omap3_init_bch_tail(&info->mtd);
-		if (err) {
-			err = -EINVAL;
+		goto custom_ecc_layout;
+#else
+		pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+#endif
+		break;
+	case OMAP_ECC_BCH8_CODE_HW:
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme\n");
+		info->nand.ecc.mode		= NAND_ECC_HW;
+		info->nand.ecc.size		= 512;
+		/* 14th bit is kept reserved for ROM-code compatibility */
+		info->nand.ecc.bytes		= 13 + 1;
+		info->nand.ecc.strength		= 8;
+		info->nand.ecc.hwctl		= omap3_enable_hwecc_bch;
+		info->nand.ecc.correct		= omap_elm_correct_data;
+		info->nand.ecc.calculate	= omap3_calculate_ecc_bch;
+		info->nand.ecc.read_page	= omap_read_page_bch;
+		info->nand.ecc.write_page	= omap_write_page_bch;
+		/* This ECC scheme requires ELM H/W block */
+		if (is_elm_present(info, pdata->elm_of_node, BCH8_ECC) < 0) {
+			pr_err("nand: error: could not initialize ELM\n");
+			err = -ENODEV;
 			goto out_release_mem_region;
 		}
+		/* define custom ECC layout */
+		ecclayout->eccbytes		= info->nand.ecc.bytes *
+							(mtd->writesize /
+							info->nand.ecc.size);
+		ecclayout->eccpos[0]		= BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
+							ecclayout->eccbytes;
+		goto custom_ecc_layout;
+#else
+		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+#endif
+		break;
+	default:
+		pr_err("nand: error: invalid or unsupported ECC scheme\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+	}
+
+custom_ecc_layout:
+	/* populate remaining info for custom ecc layout */
+	pr_info("%s: using custom ecc layout\n", DRIVER_NAME);
+	ecclayout->oobfree->length = mtd->oobsize - BADBLOCK_MARKER_LENGTH
+						- ecclayout->eccbytes;
+	if (!(info->nand.options & NAND_BUSWIDTH_16))
+		info->nand.badblock_pattern = &bb_descrip_flashbased;
+	for (i = 1; i < ecclayout->eccbytes; i++)
+		ecclayout->eccpos[i] = ecclayout->eccpos[0] + i;
+	/* check if NAND OOBSIZE meets ECC scheme requirement */
+	if (mtd->oobsize < (ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH)) {
+		pr_err("not enough OOB bytes required = %d, available=%d\n",
+			ecclayout->eccbytes, mtd->oobsize);
+		err = -EINVAL;
+		goto out_release_mem_region;
 	}
 
 	/* second phase scan */
@@ -2074,7 +2060,9 @@ out_release_mem_region:
 	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:
+	omap3_free_bch(&info->mtd);
 	kfree(info);
 
 	return err;
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index b4c2c5a..9648caa 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -23,13 +23,8 @@ enum nand_io {
 };
 
 enum omap_ecc {
-	/* 1-bit  ECC calculation by Software, Error detection by Software */
-	OMAP_ECC_HAMMING_CODE_DEFAULT = 0,
 	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
-	OMAP_ECC_HAMMING_CODE_HW,
-	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
-	/* ECC layout compatible to legacy ROMCODE. */
-	OMAP_ECC_HAMMING_CODE_HW_ROMCODE,
+	OMAP_ECC_HAMMING_CODE_HW = 0,
 	/* 4-bit  ECC calculation by GPMC, Error detection by Software */
 	OMAP_ECC_BCH4_CODE_HW_DETECTION_SW,
 	/* 4-bit  ECC calculation by GPMC, Error detection by ELM */
-- 
1.8.1

Re: [PATCH v6 2/4] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe

[Brian Norris]

Hi Pekon,

On Thu, Sep 12, 2013 at 05:20:17PM +0530, Pekon Gupta wrote:
> OMAP NAND driver support multiple ECC scheme, which can used in following
> different flavours, depending on in-build Hardware engines supported by SoC.
> 
> +---------------------------------------+---------------+---------------+
> | ECC scheme                            |ECC calculation|Error detection|
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W            |
> |(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> |(requires CONFIG_MTD_NAND_OMAP_BCH &&  |               |               |
> | ti,elm-id in DT)                      |               |               |
> +---------------------------------------+---------------+---------------+
> To optimize footprint of omap2-nand driver, selection of some ECC schemes
> also require enabling following Kconfigs, in addition to setting appropriate
> DT bindings
> - Kconfig:CONFIG_MTD_NAND_ECC_BCH        enables S/W based BCH ECC algorithm
> - Kconfig:CONFIG_MTD_NAND_OMAP_BCH       enables H/W based BCH ECC algorithm
> 
> This patch

... does too many unrelated things in a single patch. I am not
comfortable taking large amounts of refactoring mixed in with Kconfig
and #ifdef changes. Can you please separate the steps you list below
into multiple patches and describe each one? I think you are doing many
trivial things, but it's difficult to separate the noise out from the
substantial changes.

Or, you can wait even longer until I have more time to kill :)

> - removes OMAP_ECC_HAMMING_CODE_DEFAULT and OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> - separates the configurations for other ECC schemes
> - fixes dependency issues based on Kconfig options
> - updates ELM device detection in is_elm_present()
> - cleans redundant code

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/4] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe

[Brian Norris]

BTW, I'll elaborate on a few things that are hidden in the noise here.

On Thu, Sep 12, 2013 at 05:20:17PM +0530, Pekon Gupta wrote:
> OMAP NAND driver support multiple ECC scheme, which can used in following
> different flavours, depending on in-build Hardware engines supported by SoC.
...
> This patch
> - removes OMAP_ECC_HAMMING_CODE_DEFAULT and OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> - separates the configurations for other ECC schemes
> - fixes dependency issues based on Kconfig options
> - updates ELM device detection in is_elm_present()
> - cleans redundant code
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/nand/omap2.c                     | 530 +++++++++++++--------------
>  include/linux/platform_data/mtd-nand-omap2.h |   7 +-
>  2 files changed, 260 insertions(+), 277 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 4ecf0e5..420078f 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
...
> @@ -1846,20 +1720,20 @@ static int omap_nand_probe(struct platform_device *pdev)
>  	spin_lock_init(&info->controller.lock);
>  	init_waitqueue_head(&info->controller.wq);
>  
> -	info->pdev = pdev;
> +	mtd			= &info->mtd;
> +	mtd->name		= dev_name(&pdev->dev);
> +	mtd->owner		= THIS_MODULE;
> +	mtd->priv		= &info->nand;
> +	chip			= mtd->priv;
>  
> +	info->pdev		= pdev;
>  	info->gpmc_cs		= pdata->cs;
>  	info->reg		= pdata->reg;
> +	info->bch		= NULL;
>  
> -	info->mtd.priv		= &info->nand;
> -	info->mtd.name		= dev_name(&pdev->dev);
> -	info->mtd.owner		= THIS_MODULE;
> -
> -	info->nand.options	= pdata->devsize;
> +	info->nand.options	= NAND_BUSWIDTH_AUTO;

You're changing the buswidth detection significantly. You don't even
mention that in the commit description. This should be a separate patch.

>  	info->nand.options	|= NAND_SKIP_BBTSCAN;
> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
>  	info->of_node		= pdata->of_node;
> -#endif
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (res == NULL) {
> @@ -1903,6 +1777,30 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		info->nand.chip_delay = 50;
>  	}
>  
> +	/* scan NAND device conncted to controller */
> +	if (nand_scan_ident(mtd, 1, NULL)) {
> +		err = -ENXIO;
> +		goto out_release_mem_region;
> +	}
> +	pr_info("%s: detected %s NAND flash\n", DRIVER_NAME,
> +			(info->nand.options & NAND_BUSWIDTH_16) ? "x16" : "x8");

Do you really need to print this unconditionally? nand_base already has
some device info printed out (not the buswidth), so if it's really
needed, it should go there. But we really don't need to print out all
this IMO.

Perhaps move the print into the 'if' block, so that you only alert the
user about the buswidth if it doesn't match.

> +	if ((info->nand.options & NAND_BUSWIDTH_16) !=
> +			(pdata->devsize & NAND_BUSWIDTH_16)) {
> +		pr_err("%s: but incorrectly configured as %s", DRIVER_NAME,
> +			(pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : "x8");
> +		err = -EINVAL;
> +		goto out_release_mem_region;
> +	}
> +
> +	/* check for small page devices */
> +	if ((mtd->oobsize < 64) &&
> +		(pdata->ecc_opt != OMAP_ECC_HAMMING_CODE_HW)) {
> +		pr_err("small page devices are not supported\n");
> +		err = -EINVAL;
> +		goto out_release_mem_region;
> +	}
> +
> +	/* populate read & write API based on xfer_type selected */
>  	switch (pdata->xfer_type) {
>  	case NAND_OMAP_PREFETCH_POLLED:
>  		info->nand.read_buf   = omap_read_buf_pref;
> @@ -1992,64 +1890,152 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		goto out_release_mem_region;
>  	}
>  
> -	/* select the ecc type */
> -	if (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_DEFAULT)
> -		info->nand.ecc.mode = NAND_ECC_SOFT;
> -	else if ((pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW) ||
> -		(pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW_ROMCODE)) {
> +	/* populate MTD interface based on ECC scheme */
> +	chip->ecclayout		= &omap_oobinfo;

This field was recently removed in l2-mtd.git. It was redundant with
chip->ecc.layout.

> +	chip->ecc.layout	= &omap_oobinfo;
> +	ecclayout		= &omap_oobinfo;
> +	switch (pdata->ecc_opt) {
> +	case OMAP_ECC_HAMMING_CODE_HW:
> +		pr_info("nand: using OMAP_ECC_HAMMING_CODE_HW\n");
> +		info->nand.ecc.mode             = NAND_ECC_HW;
>  		info->nand.ecc.bytes            = 3;
>  		info->nand.ecc.size             = 512;
>  		info->nand.ecc.strength         = 1;
>  		info->nand.ecc.calculate        = omap_calculate_ecc;
>  		info->nand.ecc.hwctl            = omap_enable_hwecc;
>  		info->nand.ecc.correct          = omap_correct_data;
...

So please, separate pure refactoring from significant changes.

Brian

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v6 3/4] mtd:nand:omap2: updated support for BCH4 ECC scheme

[Pekon Gupta]

This patch adds following two flavours of BCH4 ECC scheme in omap2-nand driver
- OMAP_ECC_BCH4_CODE_HW_DETECTION_SW
	- uses GPMC H/W engine for calculating ECC.
	- uses software library (lib/bch.h & nand_bch.h) for error correction.

- OMAP_ECC_BCH4_CODE_HW
	- uses GPMC H/W engine for calculating ECC.
	- uses ELM H/W engine for error correction.

With this patch omap2-nand driver supports following ECC schemes:
+---------------------------------------+---------------+---------------+
| ECC scheme                            |ECC calculation|Error detection|
+---------------------------------------+---------------+---------------+
|OMAP_ECC_HAMMING_CODE_DEFAULT          |S/W            |S/W            |
|OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
|OMAP_ECC_HAMMING_CODE_HW_ROMCODE       |H/W (GPMC)     |S/W            |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH4_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.h)|
|OMAP_ECC_BCH4_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.h)|
|OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
+---------------------------------------+---------------+---------------+
Important:
- Selection of OMAP_ECC_BCHx_CODE_HW_DETECTION_SW requires,
	Kconfig: CONFIG_MTD_NAND_ECC_BCH: enables S/W based BCH ECC algorithm.

- Selection of OMAP_ECC_BCHx_CODE_HW requires,
	Kconfig: CONFIG_MTD_NAND_OMAP_BCH: enables ELM H/W module.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/Kconfig |  30 ++---------
 drivers/mtd/nand/omap2.c | 136 ++++++++++++++++++++++-------------------------
 2 files changed, 68 insertions(+), 98 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index d885298..5836039 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -96,35 +96,13 @@ config MTD_NAND_OMAP2
 
 config MTD_NAND_OMAP_BCH
 	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
-	tristate "Enable support for hardware BCH error correction"
+	tristate "Support hardware based BCH error correction"
 	default n
 	select BCH
-	select BCH_CONST_PARAMS
 	help
-	 Support for hardware BCH error correction.
-
-choice
-	prompt "BCH error correction capability"
-	depends on MTD_NAND_OMAP_BCH
-
-config MTD_NAND_OMAP_BCH8
-	bool "8 bits / 512 bytes (recommended)"
-	help
-	 Support correcting up to 8 bitflips per 512-byte block.
-	 This will use 13 bytes of spare area per 512 bytes of page data.
-	 This is the recommended mode, as 4-bit mode does not work
-	 on some OMAP3 revisions, due to a hardware bug.
-
-config MTD_NAND_OMAP_BCH4
-	bool "4 bits / 512 bytes"
-	help
-	 Support correcting up to 4 bitflips per 512-byte block.
-	 This will use 7 bytes of spare area per 512 bytes of page data.
-	 Note that this mode does not work on some OMAP3 revisions, due to a
-	 hardware bug. Please check your OMAP datasheet before selecting this
-	 mode.
-
-endchoice
+	  Some devices have built-in ELM hardware engine, which can be used to
+	  locate and correct errors when using BCH ECC scheme. This enables the
+	  driver support for same.
 
 if MTD_NAND_OMAP_BCH
 config BCH_CONST_M
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 420078f..6f4596c 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -27,6 +27,7 @@
 
 #ifdef CONFIG_MTD_NAND_ECC_BCH
 #include <linux/bch.h>
+#include <linux/mtd/nand_bch.h>
 #endif
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
 #include <linux/platform_data/elm.h>
@@ -144,7 +145,6 @@
 #define BCH_ECC_SIZE1		0x20	/* ecc_size1 = 32 */
 
 #define BADBLOCK_MARKER_LENGTH		0x2
-#define OMAP_ECC_BCH8_POLYNOMIAL	0x201b
 
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
 static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
@@ -185,10 +185,9 @@ struct omap_nand_info {
 		OMAP_NAND_IO_WRITE,	/* write */
 	} iomode;
 	u_char				*buf;
-	int					buf_len;
+	int				buf_len;
 	struct gpmc_nand_regs		reg;
 	/* fields specific for BCHx_HW ECC scheme */
-	struct bch_control              *bch;
 	bool				is_elm_used;
 	struct device			*elm_dev;
 	struct device_node		*of_node;
@@ -1227,58 +1226,6 @@ static int omap3_calculate_ecc_bch8(struct mtd_info *mtd, const u_char *dat,
 	return 0;
 }
 
-/**
- * omap3_correct_data_bch - Decode received data and correct errors
- * @mtd: MTD device structure
- * @data: page data
- * @read_ecc: ecc read from nand flash
- * @calc_ecc: ecc read from HW ECC registers
- */
-static int omap3_correct_data_bch(struct mtd_info *mtd, u_char *data,
-				  u_char *read_ecc, u_char *calc_ecc)
-{
-	int i, count;
-	/* cannot correct more than 8 errors */
-	unsigned int errloc[8];
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-
-	count = decode_bch(info->bch, NULL, 512, read_ecc, calc_ecc, NULL,
-			   errloc);
-	if (count > 0) {
-		/* correct errors */
-		for (i = 0; i < count; i++) {
-			/* correct data only, not ecc bytes */
-			if (errloc[i] < 8*512)
-				data[errloc[i]/8] ^= 1 << (errloc[i] & 7);
-			pr_debug("corrected bitflip %u\n", errloc[i]);
-		}
-	} else if (count < 0) {
-		pr_err("ecc unrecoverable error\n");
-	}
-	return count;
-}
-
-/**
- * omap3_free_bch - Release BCH ecc resources
- * @mtd: MTD device structure
- */
-static void omap3_free_bch(struct mtd_info *mtd)
-{
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-	if (info->bch) {
-		free_bch(info->bch);
-		info->bch = NULL;
-	}
-}
-
-#else
-
-static void omap3_free_bch(struct mtd_info *mtd)
-{
-}
-
 #endif /* CONFIG_MTD_NAND_ECC_BCH */
 
 
@@ -1725,16 +1672,17 @@ static int omap_nand_probe(struct platform_device *pdev)
 	mtd->owner		= THIS_MODULE;
 	mtd->priv		= &info->nand;
 	chip			= mtd->priv;
+	chip->ecc.priv		= NULL;
 
 	info->pdev		= pdev;
 	info->gpmc_cs		= pdata->cs;
 	info->reg		= pdata->reg;
-	info->bch		= NULL;
 
 	info->nand.options	= NAND_BUSWIDTH_AUTO;
 	info->nand.options	|= NAND_SKIP_BBTSCAN;
 	info->of_node		= pdata->of_node;
 
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (res == NULL) {
 		err = -EINVAL;
@@ -1924,20 +1872,21 @@ static int omap_nand_probe(struct platform_device *pdev)
 		info->nand.ecc.bytes		= 7;
 		info->nand.ecc.strength		= 4;
 		info->nand.ecc.hwctl		= omap3_enable_hwecc_bch;
-		info->nand.ecc.correct		= omap3_correct_data_bch;
+		info->nand.ecc.correct		= nand_bch_correct_data;
 		info->nand.ecc.calculate	= omap3_calculate_ecc_bch4;
 		/* define custom ECC layout */
 		ecclayout->eccbytes		= info->nand.ecc.bytes *
-						(mtd->writesize /
-						info->nand.ecc.size);
+							(mtd->writesize /
+							info->nand.ecc.size);
 		ecclayout->eccpos[0]		= info->mtd.oobsize -
-						ecclayout->eccbytes;
+							ecclayout->eccbytes;
 		ecclayout->oobfree->offset	= BADBLOCK_MARKER_LENGTH;
 		/* software bch library is used for locating errors */
-		info->bch = init_bch(info->nand.ecc.bytes,
-					info->nand.ecc.strength,
-					OMAP_ECC_BCH8_POLYNOMIAL);
-		if (!info->bch) {
+		info->nand.ecc.priv		= nand_bch_init(mtd,
+						info->nand.ecc.size,
+						info->nand.ecc.bytes,
+						&info->nand.ecc.layout);
+		if (!info->nand.ecc.priv) {
 			pr_err("nand: error: unable to use s/w BCH library\n");
 			err = -EINVAL;
 			goto out_release_mem_region;
@@ -1949,6 +1898,39 @@ static int omap_nand_probe(struct platform_device *pdev)
 		goto out_release_mem_region;
 #endif
 		break;
+	case OMAP_ECC_BCH4_CODE_HW:
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+		pr_info("nand: using OMAP_ECC_BCH4_CODE_HW ECC scheme\n");
+		info->nand.ecc.mode		= NAND_ECC_HW;
+		info->nand.ecc.size		= 512;
+		/* 14th bit is kept reserved for ROM-code compatibility */
+		info->nand.ecc.bytes		= 7 + 1;
+		info->nand.ecc.strength		= 4;
+		info->nand.ecc.hwctl		= omap3_enable_hwecc_bch;
+		info->nand.ecc.correct		= omap_elm_correct_data;
+		info->nand.ecc.calculate	= omap3_calculate_ecc_bch;
+		info->nand.ecc.read_page	= omap_read_page_bch;
+		info->nand.ecc.write_page	= omap_write_page_bch;
+		/* This ECC scheme requires ELM H/W block */
+		if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) {
+			pr_err("nand: error: could not initialize ELM\n");
+			err = -ENODEV;
+			goto out_release_mem_region;
+		}
+		/* define custom ECC layout */
+		ecclayout->eccbytes		= info->nand.ecc.bytes *
+							(mtd->writesize /
+							info->nand.ecc.size);
+		ecclayout->eccpos[0]		= BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
+							ecclayout->eccbytes;
+		goto custom_ecc_layout;
+#else
+		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+#endif
+		break;
 	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
 #ifdef CONFIG_MTD_NAND_ECC_BCH
 		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
@@ -1957,7 +1939,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 		info->nand.ecc.bytes		= 13;
 		info->nand.ecc.strength		= 8;
 		info->nand.ecc.hwctl		= omap3_enable_hwecc_bch;
-		info->nand.ecc.correct		= omap3_correct_data_bch;
+		info->nand.ecc.correct		= nand_bch_correct_data;
 		info->nand.ecc.calculate	= omap3_calculate_ecc_bch8;
 		/* define custom ECC layout */
 		ecclayout->eccbytes		= info->nand.ecc.bytes *
@@ -1967,10 +1949,11 @@ static int omap_nand_probe(struct platform_device *pdev)
 							ecclayout->eccbytes;
 		ecclayout->oobfree->offset	= BADBLOCK_MARKER_LENGTH;
 		/* software bch library is used for locating errors */
-		info->bch = init_bch(info->nand.ecc.bytes,
-					info->nand.ecc.strength,
-					OMAP_ECC_BCH8_POLYNOMIAL);
-		if (!info->bch) {
+		info->nand.ecc.priv		= nand_bch_init(mtd,
+						info->nand.ecc.size,
+						info->nand.ecc.bytes,
+						&info->nand.ecc.layout);
+		if (!info->nand.ecc.priv) {
 			pr_err("nand: error: unable to use s/w BCH library\n");
 			err = -EINVAL;
 			goto out_release_mem_region;
@@ -2062,7 +2045,12 @@ out_release_mem_region:
 	release_mem_region(info->phys_base, info->mem_size);
 
 out_free_info:
-	omap3_free_bch(&info->mtd);
+#ifdef CONFIG_MTD_NAND_ECC_BCH
+	if (info->nand.ecc.priv) {
+		nand_bch_free(info->nand.ecc.priv);
+		info->nand.ecc.priv = NULL;
+	}
+#endif
 	kfree(info);
 
 	return err;
@@ -2073,8 +2061,12 @@ static int omap_nand_remove(struct platform_device *pdev)
 	struct mtd_info *mtd = platform_get_drvdata(pdev);
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 							mtd);
-	omap3_free_bch(&info->mtd);
-
+#ifdef CONFIG_MTD_NAND_ECC_BCH
+	if (info->nand.ecc.priv) {
+		nand_bch_free(info->nand.ecc.priv);
+		info->nand.ecc.priv = NULL;
+	}
+#endif
 	if (info->dma)
 		dma_release_channel(info->dma);
 
-- 
1.8.1

Re: [PATCH v6 3/4] mtd:nand:omap2: updated support for BCH4 ECC scheme

[Brian Norris]

On Thu, Sep 12, 2013 at 05:20:18PM +0530, Pekon Gupta wrote:
> This patch adds following two flavours of BCH4 ECC scheme in omap2-nand driver
> - OMAP_ECC_BCH4_CODE_HW_DETECTION_SW
> 	- uses GPMC H/W engine for calculating ECC.
> 	- uses software library (lib/bch.h & nand_bch.h) for error correction.
> 
> - OMAP_ECC_BCH4_CODE_HW
> 	- uses GPMC H/W engine for calculating ECC.
> 	- uses ELM H/W engine for error correction.
> 
> With this patch omap2-nand driver supports following ECC schemes:
> +---------------------------------------+---------------+---------------+
> | ECC scheme                            |ECC calculation|Error detection|
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_HAMMING_CODE_DEFAULT          |S/W            |S/W            |
> |OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
> |OMAP_ECC_HAMMING_CODE_HW_ROMCODE       |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH4_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.h)|
> |OMAP_ECC_BCH4_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.h)|
> |OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> +---------------------------------------+---------------+---------------+
> Important:
> - Selection of OMAP_ECC_BCHx_CODE_HW_DETECTION_SW requires,
> 	Kconfig: CONFIG_MTD_NAND_ECC_BCH: enables S/W based BCH ECC algorithm.
> 
> - Selection of OMAP_ECC_BCHx_CODE_HW requires,
> 	Kconfig: CONFIG_MTD_NAND_OMAP_BCH: enables ELM H/W module.
> 
> Signed-off-by: Pekon Gupta <pekon-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/mtd/nand/Kconfig |  30 ++---------
>  drivers/mtd/nand/omap2.c | 136 ++++++++++++++++++++++-------------------------
>  2 files changed, 68 insertions(+), 98 deletions(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index d885298..5836039 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -96,35 +96,13 @@ config MTD_NAND_OMAP2
>  
>  config MTD_NAND_OMAP_BCH
>  	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
> -	tristate "Enable support for hardware BCH error correction"
> +	tristate "Support hardware based BCH error correction"
>  	default n
>  	select BCH
> -	select BCH_CONST_PARAMS
>  	help
> -	 Support for hardware BCH error correction.
> -
> -choice
> -	prompt "BCH error correction capability"
> -	depends on MTD_NAND_OMAP_BCH
> -
> -config MTD_NAND_OMAP_BCH8
> -	bool "8 bits / 512 bytes (recommended)"
> -	help
> -	 Support correcting up to 8 bitflips per 512-byte block.
> -	 This will use 13 bytes of spare area per 512 bytes of page data.
> -	 This is the recommended mode, as 4-bit mode does not work
> -	 on some OMAP3 revisions, due to a hardware bug.
> -
> -config MTD_NAND_OMAP_BCH4
> -	bool "4 bits / 512 bytes"
> -	help
> -	 Support correcting up to 4 bitflips per 512-byte block.
> -	 This will use 7 bytes of spare area per 512 bytes of page data.
> -	 Note that this mode does not work on some OMAP3 revisions, due to a
> -	 hardware bug. Please check your OMAP datasheet before selecting this
> -	 mode.
> -
> -endchoice
> +	  Some devices have built-in ELM hardware engine, which can be used to
> +	  locate and correct errors when using BCH ECC scheme. This enables the
> +	  driver support for same.
>  
>  if MTD_NAND_OMAP_BCH
>  config BCH_CONST_M
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 420078f..6f4596c 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -27,6 +27,7 @@
>  
>  #ifdef CONFIG_MTD_NAND_ECC_BCH
>  #include <linux/bch.h>
> +#include <linux/mtd/nand_bch.h>
>  #endif
>  #ifdef CONFIG_MTD_NAND_OMAP_BCH
>  #include <linux/platform_data/elm.h>
> @@ -144,7 +145,6 @@
>  #define BCH_ECC_SIZE1		0x20	/* ecc_size1 = 32 */
>  
>  #define BADBLOCK_MARKER_LENGTH		0x2
> -#define OMAP_ECC_BCH8_POLYNOMIAL	0x201b
>  
>  #ifdef CONFIG_MTD_NAND_OMAP_BCH
>  static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
> @@ -185,10 +185,9 @@ struct omap_nand_info {
>  		OMAP_NAND_IO_WRITE,	/* write */
>  	} iomode;
>  	u_char				*buf;
> -	int					buf_len;
> +	int				buf_len;

Please do not make unrelated, arbitrary changes to whitespace as part of
a patch that solves other problems.

>  	struct gpmc_nand_regs		reg;
>  	/* fields specific for BCHx_HW ECC scheme */
> -	struct bch_control              *bch;
>  	bool				is_elm_used;
>  	struct device			*elm_dev;
>  	struct device_node		*of_node;

...

> @@ -1725,16 +1672,17 @@ static int omap_nand_probe(struct platform_device *pdev)
>  	mtd->owner		= THIS_MODULE;
>  	mtd->priv		= &info->nand;
>  	chip			= mtd->priv;
> +	chip->ecc.priv		= NULL;
>  
>  	info->pdev		= pdev;
>  	info->gpmc_cs		= pdata->cs;
>  	info->reg		= pdata->reg;
> -	info->bch		= NULL;
>  
>  	info->nand.options	= NAND_BUSWIDTH_AUTO;
>  	info->nand.options	|= NAND_SKIP_BBTSCAN;
>  	info->of_node		= pdata->of_node;
>  
> +

Extraneous whitespace.

>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (res == NULL) {
>  		err = -EINVAL;
> @@ -1924,20 +1872,21 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		info->nand.ecc.bytes		= 7;
>  		info->nand.ecc.strength		= 4;
>  		info->nand.ecc.hwctl		= omap3_enable_hwecc_bch;
> -		info->nand.ecc.correct		= omap3_correct_data_bch;
> +		info->nand.ecc.correct		= nand_bch_correct_data;
>  		info->nand.ecc.calculate	= omap3_calculate_ecc_bch4;
>  		/* define custom ECC layout */
>  		ecclayout->eccbytes		= info->nand.ecc.bytes *
> -						(mtd->writesize /
> -						info->nand.ecc.size);
> +							(mtd->writesize /
> +							info->nand.ecc.size);

Again, extraneous changes to the whitespace.

>  		ecclayout->eccpos[0]		= info->mtd.oobsize -
> -						ecclayout->eccbytes;
> +							ecclayout->eccbytes;

Ditto.

>  		ecclayout->oobfree->offset	= BADBLOCK_MARKER_LENGTH;
>  		/* software bch library is used for locating errors */
> -		info->bch = init_bch(info->nand.ecc.bytes,
> -					info->nand.ecc.strength,
> -					OMAP_ECC_BCH8_POLYNOMIAL);
> -		if (!info->bch) {
> +		info->nand.ecc.priv		= nand_bch_init(mtd,
> +						info->nand.ecc.size,
> +						info->nand.ecc.bytes,
> +						&info->nand.ecc.layout);
> +		if (!info->nand.ecc.priv) {
>  			pr_err("nand: error: unable to use s/w BCH library\n");
>  			err = -EINVAL;
>  			goto out_release_mem_region;
> @@ -1949,6 +1898,39 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		goto out_release_mem_region;
>  #endif
>  		break;
> +	case OMAP_ECC_BCH4_CODE_HW:
> +#ifdef CONFIG_MTD_NAND_OMAP_BCH
> +		pr_info("nand: using OMAP_ECC_BCH4_CODE_HW ECC scheme\n");
> +		info->nand.ecc.mode		= NAND_ECC_HW;
> +		info->nand.ecc.size		= 512;
> +		/* 14th bit is kept reserved for ROM-code compatibility */
> +		info->nand.ecc.bytes		= 7 + 1;
> +		info->nand.ecc.strength		= 4;
> +		info->nand.ecc.hwctl		= omap3_enable_hwecc_bch;
> +		info->nand.ecc.correct		= omap_elm_correct_data;
> +		info->nand.ecc.calculate	= omap3_calculate_ecc_bch;
> +		info->nand.ecc.read_page	= omap_read_page_bch;
> +		info->nand.ecc.write_page	= omap_write_page_bch;
> +		/* This ECC scheme requires ELM H/W block */
> +		if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) {
> +			pr_err("nand: error: could not initialize ELM\n");
> +			err = -ENODEV;
> +			goto out_release_mem_region;
> +		}
> +		/* define custom ECC layout */
> +		ecclayout->eccbytes		= info->nand.ecc.bytes *
> +							(mtd->writesize /
> +							info->nand.ecc.size);
> +		ecclayout->eccpos[0]		= BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
> +							ecclayout->eccbytes;
> +		goto custom_ecc_layout;

You don't need these 'goto' statements, when they are followed by a
'break' which takes you to the same place. And generally, we reserve
goto for use only with error paths, not the common path.

I see you did the same thing in patch 2. Please remove them there too.

> +#else
> +		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
> +		err = -EINVAL;
> +		goto out_release_mem_region;
> +#endif
> +		break;
>  	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
>  #ifdef CONFIG_MTD_NAND_ECC_BCH
>  		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
> @@ -1957,7 +1939,7 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		info->nand.ecc.bytes		= 13;
>  		info->nand.ecc.strength		= 8;
>  		info->nand.ecc.hwctl		= omap3_enable_hwecc_bch;
> -		info->nand.ecc.correct		= omap3_correct_data_bch;
> +		info->nand.ecc.correct		= nand_bch_correct_data;
>  		info->nand.ecc.calculate	= omap3_calculate_ecc_bch8;
>  		/* define custom ECC layout */
>  		ecclayout->eccbytes		= info->nand.ecc.bytes *
> @@ -1967,10 +1949,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>  							ecclayout->eccbytes;
>  		ecclayout->oobfree->offset	= BADBLOCK_MARKER_LENGTH;
>  		/* software bch library is used for locating errors */
> -		info->bch = init_bch(info->nand.ecc.bytes,
> -					info->nand.ecc.strength,
> -					OMAP_ECC_BCH8_POLYNOMIAL);
> -		if (!info->bch) {
> +		info->nand.ecc.priv		= nand_bch_init(mtd,
> +						info->nand.ecc.size,
> +						info->nand.ecc.bytes,
> +						&info->nand.ecc.layout);
> +		if (!info->nand.ecc.priv) {
>  			pr_err("nand: error: unable to use s/w BCH library\n");
>  			err = -EINVAL;
>  			goto out_release_mem_region;
> @@ -2062,7 +2045,12 @@ out_release_mem_region:
>  	release_mem_region(info->phys_base, info->mem_size);
>  
>  out_free_info:
> -	omap3_free_bch(&info->mtd);

Didn't you just add this line in the previous patch? Your patch series
is confusing me...

> +#ifdef CONFIG_MTD_NAND_ECC_BCH
> +	if (info->nand.ecc.priv) {
> +		nand_bch_free(info->nand.ecc.priv);
> +		info->nand.ecc.priv = NULL;
> +	}
> +#endif

I don't think you need this #ifdef, do you? nand_bch_free is stubbed out
for the !CONFIG_MTD_NAND_ECC_BCH case.

>  	kfree(info);
>  
>  	return err;
> @@ -2073,8 +2061,12 @@ static int omap_nand_remove(struct platform_device *pdev)
>  	struct mtd_info *mtd = platform_get_drvdata(pdev);
>  	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>  							mtd);
> -	omap3_free_bch(&info->mtd);
> -
> +#ifdef CONFIG_MTD_NAND_ECC_BCH
> +	if (info->nand.ecc.priv) {
> +		nand_bch_free(info->nand.ecc.priv);
> +		info->nand.ecc.priv = NULL;
> +	}
> +#endif

Ditto.

>  	if (info->dma)
>  		dma_release_channel(info->dma);
>  

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 4/4] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt

[Pekon Gupta]

DT property values for OMAP based gpmc-nand have been updated
to match changes in commit:
	6faf096 ARM: OMAP2+: cleaned-up DT support of various ECC schemes
Refer: Documentation/devicetree/bindings/mtd/gpmc-nand.txt

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 arch/arm/boot/dts/am335x-evm.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index e8ec875..e28a2ac 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -268,7 +268,7 @@
 			nand@0,0 {
 				reg = <0 0 0>; /* CS0, offset 0 */
 				nand-bus-width = <8>;
-				ti,nand-ecc-opt = "bch8";
+				ti,nand-ecc-scheme = "bch8";
 				gpmc,device-nand = "true";
 				gpmc,device-width = <1>;
 				gpmc,sync-clk-ps = <0>;
-- 
1.8.1

Re: [PATCH v6 4/4] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt

[Brian Norris]

On Thu, Sep 12, 2013 at 05:20:19PM +0530, Pekon Gupta wrote:
> DT property values for OMAP based gpmc-nand have been updated
> to match changes in commit:
> 	6faf096 ARM: OMAP2+: cleaned-up DT support of various ECC schemes

Whose commit ID is this? Your patch is not merged yet, so don't use a
commit ID.

> Refer: Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  arch/arm/boot/dts/am335x-evm.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index e8ec875..e28a2ac 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -268,7 +268,7 @@
>  			nand@0,0 {
>  				reg = <0 0 0>; /* CS0, offset 0 */
>  				nand-bus-width = <8>;
> -				ti,nand-ecc-opt = "bch8";
> +				ti,nand-ecc-scheme = "bch8";
>  				gpmc,device-nand = "true";
>  				gpmc,device-width = <1>;
>  				gpmc,sync-clk-ps = <0>;

This change should probably go in patch 1 anyway, when you break the
ABI.

Brian

RE: [PATCH v6 4/4] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt

[Gupta, Pekon]

> From: Brian Norris [mailto:computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> 
> On Thu, Sep 12, 2013 at 05:20:19PM +0530, Pekon Gupta wrote:
> > DT property values for OMAP based gpmc-nand have been updated
> > to match changes in commit:
> > 	6faf096 ARM: OMAP2+: cleaned-up DT support of various ECC
> schemes
> 
> Whose commit ID is this? Your patch is not merged yet, so don't use a
> commit ID.
> 
> > Refer: Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> >
> > Signed-off-by: Pekon Gupta <pekon-l0cyMroinI0@public.gmane.org>
> > ---
> >  arch/arm/boot/dts/am335x-evm.dts | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/am335x-evm.dts
> b/arch/arm/boot/dts/am335x-evm.dts
> > index e8ec875..e28a2ac 100644
> > --- a/arch/arm/boot/dts/am335x-evm.dts
> > +++ b/arch/arm/boot/dts/am335x-evm.dts
> > @@ -268,7 +268,7 @@
> >  			nand@0,0 {
> >  				reg = <0 0 0>; /* CS0, offset 0 */
> >  				nand-bus-width = <8>;
> > -				ti,nand-ecc-opt = "bch8";
> > +				ti,nand-ecc-scheme = "bch8";
> >  				gpmc,device-nand = "true";
> >  				gpmc,device-width = <1>;
> >  				gpmc,sync-clk-ps = <0>;
> 
> This change should probably go in patch 1 anyway, when you break the
> ABI.
> 
As this patch series touches three different trees.. 
(1) arch/arm/boot/dts/..  maintained by Benoit Cousson (benoit.cousson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org)
(2) arch/arm/mach-omap2/.. maintained by Tony Lindgren (tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org)
(3) driver/mtd/nand/.. maintained by Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

It was observed that there were conflicts usually during final merge,
Hence I separated out the patches.
However, if all are in sync and the series gets committed in above trees
simultaneously, then this conflicts can be avoided.
So, if Tony & Benoit are ok then I'll merge this [PATCH 4/4] in [PATCH 1/4].

with regards, pekon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/4] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt

[Javier Martinez Canillas]

On Thu, Sep 26, 2013 at 1:28 PM, Gupta, Pekon <pekon@ti.com> wrote:
>> From: Brian Norris [mailto:computersforpeace@gmail.com]
>>
>> On Thu, Sep 12, 2013 at 05:20:19PM +0530, Pekon Gupta wrote:
>> > DT property values for OMAP based gpmc-nand have been updated
>> > to match changes in commit:
>> >     6faf096 ARM: OMAP2+: cleaned-up DT support of various ECC
>> schemes
>>
>> Whose commit ID is this? Your patch is not merged yet, so don't use a
>> commit ID.
>>
>> > Refer: Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>> >
>> > Signed-off-by: Pekon Gupta <pekon@ti.com>
>> > ---
>> >  arch/arm/boot/dts/am335x-evm.dts | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm/boot/dts/am335x-evm.dts
>> b/arch/arm/boot/dts/am335x-evm.dts
>> > index e8ec875..e28a2ac 100644
>> > --- a/arch/arm/boot/dts/am335x-evm.dts
>> > +++ b/arch/arm/boot/dts/am335x-evm.dts
>> > @@ -268,7 +268,7 @@
>> >                     nand@0,0 {
>> >                             reg = <0 0 0>; /* CS0, offset 0 */
>> >                             nand-bus-width = <8>;
>> > -                           ti,nand-ecc-opt = "bch8";
>> > +                           ti,nand-ecc-scheme = "bch8";
>> >                             gpmc,device-nand = "true";
>> >                             gpmc,device-width = <1>;
>> >                             gpmc,sync-clk-ps = <0>;
>>
>> This change should probably go in patch 1 anyway, when you break the
>> ABI.
>>
> As this patch series touches three different trees..
> (1) arch/arm/boot/dts/..  maintained by Benoit Cousson (benoit.cousson@linaro.org)

Hi Pekon,

Please note that Benoit's current email address is bcousson@baylibre.com.

I had cc'ed him now with the correct address.

> (2) arch/arm/mach-omap2/.. maintained by Tony Lindgren (tony@atomide.com)
> (3) driver/mtd/nand/.. maintained by Brian Norris <computersforpeace@gmail.com>
>
> It was observed that there were conflicts usually during final merge,
> Hence I separated out the patches.
> However, if all are in sync and the series gets committed in above trees
> simultaneously, then this conflicts can be avoided.
> So, if Tony & Benoit are ok then I'll merge this [PATCH 4/4] in [PATCH 1/4].
>
> with regards, pekon
> --

Best regards,
Javier

RE: [PATCH v6 0/4] mtd:nand:omap2: clean-up of supported ECC schemes

[Gupta, Pekon]

> 
> *Changes v5 -> v6*
> [PATCH 1/4]:
> 	- updated DT binding for gpmc-nand based on 'Olof Johansson's
> feedbacks
> 	http://lists.infradead.org/pipermail/linux-mtd/2013-
> August/048394.html
> 	- detection of ELM device via ti,elm-id DT node, moved to gpmc.c
> driver
> [PATCH 2/4]
> 	- removed: support for following obselete ECC schemes
> 	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming
> ECC)
> 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit
> Hamming ECC scheme)
> 	- updated: using omap_oobinfo as chip->ecc.layout for all ecc-
> schemes
> 	- clean: error messages
> [PATCH 3/4] cleaned to include changes for OMAP_ECC_BCH8_CODE_HW
> only
> [PATCH 4/4] updated to include DT property changes
> 
> 
> *Changes v4 -> v5*
> - Rebased to linux-next
> IMPORTANT: Need to revert commit fb1585b, [PATCH 2/4] part of previous
> version
> 	http://lists.infradead.org/pipermail/linux-mtd/2013-July/047441.html
> 
> - Swapped PATCH-1 & PATCH-2 to maintain bisectibility & compilation
> dependency
> 	http://lists.infradead.org/pipermail/linux-mtd/2013-July/047461.html
> 
> - PATCH-2: re-ordered call to is_elm_present() for later updates ELM driver
> 	- dropped changes in include/linux/platform_data/elm.h (not
> needed)
> - PATCH-3: re-ordered call to is_elm_present() for later updates ELM driver
> - Re-formated patch description (replaced tabs with white-spaces)
> 
> 
> *Changes v3 -> v4*
> (Resent with CC: devicetree-discuss@lists.ozlabs.org)
> - [Patch 1/3] removed MTD_NAND_OMAP_BCH8 &
> MTD_NAND_OMAP_BCH4 from nand/Kconfig
> 	ECC scheme selectable via nand DT (nand-ecc-opt).
> - [*] rebased for l2-mtd.git
> 
> 
> *Changes v2 -> v3*
> (Resent with Author Name fixed)
> - PATCH-1: re-arranged code to remove redundancy, added
> NAND_BUSWIDTH_AUTO
> - PATCH-2: updated nand-ecc-opt DT mapping and Documentation
> - PATCH-3: code-cleaning + changes to match PATCH-1
> - PATCH-4 <DROPPED> update DT attribute for ti,nand-ecc-opt
> 	- received feedback to keep DT mapping independent of linuxism
> - PATCH-4:<NEW> : ARM: dts: AM33xx: updated default ECC scheme in nand-
> ecc-opt
> 	- independent patch for AM335x-evm.dts update based on PATCH-2
> 
> 
> *Changes v1 -> v2*
> 	added 	[PATCH 3/4] and [PATCH 4/4]
> 
> 
> *Patches in this series*
> [PATCH 1/4]->[PATCH v5 2/4]: clean-up and optimization for supported ECC
> schemes.
> [PATCH 2/4]->[PATCH v5 1/4]: add separate DT options each supported ECC
> scheme.
> [PATCH 3/4]: update BCH4 ECC implementation (using ELM or using lib/bch.h)
> [PATCH 4/4]: ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-
> opt
> 
> After this patch series, omap2-nand driver will supports following ECC
> schemes:
> +---------------------------------------+---------------+---------------+
> | ECC scheme                            |ECC calculation|Error detection|
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH4_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W
> (lib/bch.h)|
> |OMAP_ECC_BCH4_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W
> (lib/bch.h)|
> |OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> +---------------------------------------+---------------+---------------+
> - Selection of OMAP_ECC_BCHx_CODE_HW_DETECTION_SW requires,
> 	Kconfig: CONFIG_MTD_NAND_ECC_BCH: enables S/W based BCH
> ECC algorithm.
> 
> - Selection of OMAP_ECC_BCHx_CODE_HW requires,
> 	Kconfig: CONFIG_MTD_NAND_OMAP_BCH: enables ELM H/W
> module.
> 
> Pekon Gupta (4):
>   ARM: OMAP2+: cleaned-up DT support of various ECC schemes
>   mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in
>     device_probe
>   mtd:nand:omap2: updated support for BCH4 ECC scheme
>   ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt
> 
>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  14 +-
>  arch/arm/boot/dts/am335x-evm.dts                   |   2 +-
>  arch/arm/mach-omap2/board-flash.c                  |   2 +-
>  arch/arm/mach-omap2/gpmc.c                         |  47 +-
>  drivers/mtd/nand/Kconfig                           |  30 +-
>  drivers/mtd/nand/omap2.c                           | 526 ++++++++++-----------
>  include/linux/platform_data/mtd-nand-omap2.h       |  18 +-
>  7 files changed, 309 insertions(+), 330 deletions(-)
> 
> --
> 1.8.1
+ Rob Herring <robherring2@gmail.com>
+ Mark Rutland <mark.rutland@arm.com>
+ Pawel Moll <pawel.moll@arm.com>
+ Ian Campbell <ijc+devicetree@hellion.org.uk>
+ Stephen Warren <swarren@wwwdotorg.org>

CC other DT maintainers, who were missed earlier in mail-chain.

with regards, pekon

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/