* [PATCH v8 1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes
2013-10-11 13:36 [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Pekon Gupta
@ 2013-10-11 13:36 ` Pekon Gupta
2013-10-11 15:55 ` Felipe Balbi
2013-10-11 13:36 ` [PATCH v8 2/6] ARM: OMAP2+: cleaned-up DT support of various ECC schemes Pekon Gupta
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Pekon Gupta @ 2013-10-11 13:36 UTC (permalink / raw)
To: mark.rutland, robherring2, olof, computersforpeace, dedekind1
Cc: devicetree, Pawel.Moll, arnd, swarren, tony, ijc+devicetree,
avinashphilipk, balbi, linux-mtd, Pekon Gupta, bcousson,
linux-omap, dwmw2
OMAP NAND driver currently supports multiple flavours of 1-bit Hamming
ecc-scheme, like:
- OMAP_ECC_HAMMING_CODE_DEFAULT
1-bit hamming ecc code using software library
- OMAP_ECC_HAMMING_CODE_HW
1-bit hamming ecc-code using GPMC h/w engine
- OMAP_ECC_HAMMING_CODE_HW_ROMCODE
1-bit hamming ecc-code using GPMC h/w engin with ecc-layout compatible
to ROM code.
This patch combines above multiple ecc-schemes into single implementation:
- OMAP_ECC_HAM1_CODE_HW
1-bit hamming ecc-code using GPMC h/w engine with ROM-code compatible
ecc-layout.
Signed-off-by: Pekon Gupta <pekon@ti.com>
---
Documentation/devicetree/bindings/mtd/gpmc-nand.txt | 8 ++++----
arch/arm/mach-omap2/board-flash.c | 2 +-
arch/arm/mach-omap2/gpmc.c | 4 +---
drivers/mtd/nand/omap2.c | 9 +++------
include/linux/platform_data/mtd-nand-omap2.h | 6 +-----
5 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index df338cb..25ee232 100644
--- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -22,10 +22,10 @@ Optional properties:
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
+ "sw" <deprecated> use "ham1" instead
+ "hw" <deprecated> use "ham1" instead
+ "hw-romcode" <deprecated> use "ham1" instead
+ "ham1" 1-bit Hamming ecc code
"bch4" 4-bit BCH ecc code
"bch8" 8-bit BCH ecc code
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 579697a..c9fb353 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1342,9 +1342,7 @@ 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_HAM1_CODE_HW] = "ham1",
[OMAP_ECC_BCH4_CODE_HW] = "bch4",
[OMAP_ECC_BCH8_CODE_HW] = "bch8",
};
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 4ecf0e5..8d521aa 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1993,10 +1993,7 @@ static int omap_nand_probe(struct platform_device *pdev)
}
/* 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)) {
+ if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
info->nand.ecc.bytes = 3;
info->nand.ecc.size = 512;
info->nand.ecc.strength = 1;
@@ -2025,7 +2022,7 @@ static int omap_nand_probe(struct platform_device *pdev)
}
/* rom code layout */
- if (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) {
+ if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
if (info->nand.options & NAND_BUSWIDTH_16)
offset = 2;
@@ -2033,7 +2030,7 @@ static int omap_nand_probe(struct platform_device *pdev)
offset = 1;
info->nand.badblock_pattern = &bb_descrip_flashbased;
}
- omap_oobinfo.eccbytes = 3 * (info->mtd.oobsize/16);
+ omap_oobinfo.eccbytes = 3 * (info->mtd.writesize / 512);
for (i = 0; i < omap_oobinfo.eccbytes; i++)
omap_oobinfo.eccpos[i] = i+offset;
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index 6bf9ef4..cb5a54a 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -23,11 +23,7 @@ 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_HAM1_CODE_HW = 0, /* 1-bit Hamming ecc code */
OMAP_ECC_BCH4_CODE_HW, /* 4-bit BCH ecc code */
OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */
};
--
1.8.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes
2013-10-11 13:36 ` [PATCH v8 1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes Pekon Gupta
@ 2013-10-11 15:55 ` Felipe Balbi
2013-10-11 18:33 ` Tony Lindgren
0 siblings, 1 reply; 27+ messages in thread
From: Felipe Balbi @ 2013-10-11 15:55 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, computersforpeace, dedekind1,
Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
Andrew Morton
[-- Attachment #1: Type: text/plain, Size: 5465 bytes --]
On Fri, Oct 11, 2013 at 07:06:38PM +0530, Pekon Gupta wrote:
> OMAP NAND driver currently supports multiple flavours of 1-bit Hamming
> ecc-scheme, like:
> - OMAP_ECC_HAMMING_CODE_DEFAULT
> 1-bit hamming ecc code using software library
> - OMAP_ECC_HAMMING_CODE_HW
> 1-bit hamming ecc-code using GPMC h/w engine
> - OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> 1-bit hamming ecc-code using GPMC h/w engin with ecc-layout compatible
> to ROM code.
>
> This patch combines above multiple ecc-schemes into single implementation:
> - OMAP_ECC_HAM1_CODE_HW
> 1-bit hamming ecc-code using GPMC h/w engine with ROM-code compatible
> ecc-layout.
>
> Signed-off-by: Pekon Gupta <pekon@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
> ---
> Documentation/devicetree/bindings/mtd/gpmc-nand.txt | 8 ++++----
> arch/arm/mach-omap2/board-flash.c | 2 +-
> arch/arm/mach-omap2/gpmc.c | 4 +---
> drivers/mtd/nand/omap2.c | 9 +++------
> include/linux/platform_data/mtd-nand-omap2.h | 6 +-----
> 5 files changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> index df338cb..25ee232 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> @@ -22,10 +22,10 @@ Optional properties:
> 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
> + "sw" <deprecated> use "ham1" instead
> + "hw" <deprecated> use "ham1" instead
> + "hw-romcode" <deprecated> use "ham1" instead
> + "ham1" 1-bit Hamming ecc code
> "bch4" 4-bit BCH ecc code
> "bch8" 8-bit BCH ecc code
>
> 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 579697a..c9fb353 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1342,9 +1342,7 @@ 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_HAM1_CODE_HW] = "ham1",
> [OMAP_ECC_BCH4_CODE_HW] = "bch4",
> [OMAP_ECC_BCH8_CODE_HW] = "bch8",
> };
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 4ecf0e5..8d521aa 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1993,10 +1993,7 @@ static int omap_nand_probe(struct platform_device *pdev)
> }
>
> /* 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)) {
> + if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
> info->nand.ecc.bytes = 3;
> info->nand.ecc.size = 512;
> info->nand.ecc.strength = 1;
> @@ -2025,7 +2022,7 @@ static int omap_nand_probe(struct platform_device *pdev)
> }
>
> /* rom code layout */
> - if (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) {
> + if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
>
> if (info->nand.options & NAND_BUSWIDTH_16)
> offset = 2;
> @@ -2033,7 +2030,7 @@ static int omap_nand_probe(struct platform_device *pdev)
> offset = 1;
> info->nand.badblock_pattern = &bb_descrip_flashbased;
> }
> - omap_oobinfo.eccbytes = 3 * (info->mtd.oobsize/16);
> + omap_oobinfo.eccbytes = 3 * (info->mtd.writesize / 512);
> for (i = 0; i < omap_oobinfo.eccbytes; i++)
> omap_oobinfo.eccpos[i] = i+offset;
>
> diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
> index 6bf9ef4..cb5a54a 100644
> --- a/include/linux/platform_data/mtd-nand-omap2.h
> +++ b/include/linux/platform_data/mtd-nand-omap2.h
> @@ -23,11 +23,7 @@ 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_HAM1_CODE_HW = 0, /* 1-bit Hamming ecc code */
> OMAP_ECC_BCH4_CODE_HW, /* 4-bit BCH ecc code */
> OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */
> };
> --
> 1.8.1
>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes
2013-10-11 15:55 ` Felipe Balbi
@ 2013-10-11 18:33 ` Tony Lindgren
0 siblings, 0 replies; 27+ messages in thread
From: Tony Lindgren @ 2013-10-11 18:33 UTC (permalink / raw)
To: Felipe Balbi
Cc: Pekon Gupta, mark.rutland, robherring2, olof, computersforpeace,
dedekind1, Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd,
bcousson, avinashphilipk, linux-mtd, linux-omap, devicetree,
Andrew Morton
* Felipe Balbi <balbi@ti.com> [131011 09:04]:
> On Fri, Oct 11, 2013 at 07:06:38PM +0530, Pekon Gupta wrote:
> > OMAP NAND driver currently supports multiple flavours of 1-bit Hamming
> > ecc-scheme, like:
> > - OMAP_ECC_HAMMING_CODE_DEFAULT
> > 1-bit hamming ecc code using software library
> > - OMAP_ECC_HAMMING_CODE_HW
> > 1-bit hamming ecc-code using GPMC h/w engine
> > - OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> > 1-bit hamming ecc-code using GPMC h/w engin with ecc-layout compatible
> > to ROM code.
> >
> > This patch combines above multiple ecc-schemes into single implementation:
> > - OMAP_ECC_HAM1_CODE_HW
> > 1-bit hamming ecc-code using GPMC h/w engine with ROM-code compatible
> > ecc-layout.
> >
> > Signed-off-by: Pekon Gupta <pekon@ti.com>
>
> Reviewed-by: Felipe Balbi <balbi@ti.com>
This one looks safe to queue via the MTD tree for the arch/arm/*omap* parts:
Acked-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 2/6] ARM: OMAP2+: cleaned-up DT support of various ECC schemes
2013-10-11 13:36 [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Pekon Gupta
2013-10-11 13:36 ` [PATCH v8 1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes Pekon Gupta
@ 2013-10-11 13:36 ` Pekon Gupta
2013-10-11 15:55 ` Felipe Balbi
2013-10-11 13:36 ` [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe Pekon Gupta
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Pekon Gupta @ 2013-10-11 13:36 UTC (permalink / raw)
To: mark.rutland, robherring2, olof, computersforpeace, dedekind1
Cc: Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
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_HAM1_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 error detection done in software
- Kconfig:CONFIG_MTD_NAND_OMAP_BCH error detection done by h/w engine
DT binding updates in this patch are:
- ti,elm-id: replaces elm_id
- ti,nand-ecc-opts: supported values ham1, bch4, and bch8
selection of h/w or s/w implementation depends on ti,elm-id
Signed-off-by: Pekon Gupta <pekon@ti.com>
---
.../devicetree/bindings/mtd/gpmc-nand.txt | 8 +++-
arch/arm/mach-omap2/gpmc.c | 45 ++++++++++++++++------
include/linux/platform_data/mtd-nand-omap2.h | 14 +++++--
3 files changed, 50 insertions(+), 17 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index 25ee232..5e1f31b 100644
--- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -36,8 +36,12 @@ 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.
+ - elm_id: <deprecated> use "ti,elm-id" instead
+ - 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/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index c9fb353..9473c9f 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1341,12 +1341,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_HAM1_CODE_HW] = "ham1",
- [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",
@@ -1376,12 +1370,39 @@ 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 */
+ gpmc_nand_data->elm_of_node = of_parse_phandle(child, "ti,elm-id", 0);
+ if (gpmc_nand_data->elm_of_node == NULL)
+ gpmc_nand_data->elm_of_node =
+ of_parse_phandle(child, "elm_id", 0);
+ if (gpmc_nand_data->elm_of_node == NULL)
+ pr_warn("%s: ti,elm-id property not found\n", __func__);
+
+ /* select NAND ecc-opt */
+ if (of_property_read_string(child, "ti,nand-ecc-opt", &s)) {
+ pr_err("%s: ti,nand-ecc-opt not found\n", __func__);
+ return -ENODEV;
+ }
+ if (!strcmp(s, "ham1") || !strcmp(s, "sw") ||
+ !strcmp(s, "hw") || !strcmp(s, "hw-romcode"))
+ gpmc_nand_data->ecc_opt =
+ OMAP_ECC_HAM1_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,nand-ecc-opt invalid 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 cb5a54a..4da5bfa 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -23,9 +23,16 @@ enum nand_io {
};
enum omap_ecc {
- OMAP_ECC_HAM1_CODE_HW = 0, /* 1-bit Hamming ecc code */
- 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 GPMC, Error detection by Software */
+ OMAP_ECC_HAM1_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 */
+ 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 {
@@ -59,5 +66,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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 2/6] ARM: OMAP2+: cleaned-up DT support of various ECC schemes
2013-10-11 13:36 ` [PATCH v8 2/6] ARM: OMAP2+: cleaned-up DT support of various ECC schemes Pekon Gupta
@ 2013-10-11 15:55 ` Felipe Balbi
2013-10-11 18:32 ` Tony Lindgren
0 siblings, 1 reply; 27+ messages in thread
From: Felipe Balbi @ 2013-10-11 15:55 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, computersforpeace, dedekind1,
Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
Andrew Morton
[-- Attachment #1: Type: text/plain, Size: 6802 bytes --]
On Fri, Oct 11, 2013 at 07:06:39PM +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_HAM1_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 error detection done in software
> - Kconfig:CONFIG_MTD_NAND_OMAP_BCH error detection done by h/w engine
>
> DT binding updates in this patch are:
> - ti,elm-id: replaces elm_id
> - ti,nand-ecc-opts: supported values ham1, bch4, and bch8
> selection of h/w or s/w implementation depends on ti,elm-id
>
> Signed-off-by: Pekon Gupta <pekon@ti.com>
since this maintains backwards compatibility, I think it should be
alright to apply
Reviewed-by: Felipe Balbi <balbi@ti.com>
> ---
> .../devicetree/bindings/mtd/gpmc-nand.txt | 8 +++-
> arch/arm/mach-omap2/gpmc.c | 45 ++++++++++++++++------
> include/linux/platform_data/mtd-nand-omap2.h | 14 +++++--
> 3 files changed, 50 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> index 25ee232..5e1f31b 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> @@ -36,8 +36,12 @@ 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.
> + - elm_id: <deprecated> use "ti,elm-id" instead
> + - 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/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index c9fb353..9473c9f 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1341,12 +1341,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_HAM1_CODE_HW] = "ham1",
> - [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",
> @@ -1376,12 +1370,39 @@ 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 */
> + gpmc_nand_data->elm_of_node = of_parse_phandle(child, "ti,elm-id", 0);
> + if (gpmc_nand_data->elm_of_node == NULL)
> + gpmc_nand_data->elm_of_node =
> + of_parse_phandle(child, "elm_id", 0);
> + if (gpmc_nand_data->elm_of_node == NULL)
> + pr_warn("%s: ti,elm-id property not found\n", __func__);
> +
> + /* select NAND ecc-opt */
> + if (of_property_read_string(child, "ti,nand-ecc-opt", &s)) {
> + pr_err("%s: ti,nand-ecc-opt not found\n", __func__);
> + return -ENODEV;
> + }
> + if (!strcmp(s, "ham1") || !strcmp(s, "sw") ||
> + !strcmp(s, "hw") || !strcmp(s, "hw-romcode"))
> + gpmc_nand_data->ecc_opt =
> + OMAP_ECC_HAM1_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,nand-ecc-opt invalid 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 cb5a54a..4da5bfa 100644
> --- a/include/linux/platform_data/mtd-nand-omap2.h
> +++ b/include/linux/platform_data/mtd-nand-omap2.h
> @@ -23,9 +23,16 @@ enum nand_io {
> };
>
> enum omap_ecc {
> - OMAP_ECC_HAM1_CODE_HW = 0, /* 1-bit Hamming ecc code */
> - 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 GPMC, Error detection by Software */
> + OMAP_ECC_HAM1_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 */
> + 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 {
> @@ -59,5 +66,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
>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 2/6] ARM: OMAP2+: cleaned-up DT support of various ECC schemes
2013-10-11 15:55 ` Felipe Balbi
@ 2013-10-11 18:32 ` Tony Lindgren
0 siblings, 0 replies; 27+ messages in thread
From: Tony Lindgren @ 2013-10-11 18:32 UTC (permalink / raw)
To: Felipe Balbi
Cc: Pekon Gupta, mark.rutland, robherring2, olof, computersforpeace,
dedekind1, Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd,
bcousson, avinashphilipk, linux-mtd, linux-omap, devicetree,
Andrew Morton
* Felipe Balbi <balbi@ti.com> [131011 09:03]:
> On Fri, Oct 11, 2013 at 07:06:39PM +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_HAM1_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 error detection done in software
> > - Kconfig:CONFIG_MTD_NAND_OMAP_BCH error detection done by h/w engine
> >
> > DT binding updates in this patch are:
> > - ti,elm-id: replaces elm_id
> > - ti,nand-ecc-opts: supported values ham1, bch4, and bch8
> > selection of h/w or s/w implementation depends on ti,elm-id
> >
> > Signed-off-by: Pekon Gupta <pekon@ti.com>
>
> since this maintains backwards compatibility, I think it should be
> alright to apply
>
> Reviewed-by: Felipe Balbi <balbi@ti.com>
This patch looks safe to queue via the MTD tree for the arch/arm/*omap*
parts in this patch:
Acked-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe
2013-10-11 13:36 [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Pekon Gupta
2013-10-11 13:36 ` [PATCH v8 1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes Pekon Gupta
2013-10-11 13:36 ` [PATCH v8 2/6] ARM: OMAP2+: cleaned-up DT support of various ECC schemes Pekon Gupta
@ 2013-10-11 13:36 ` Pekon Gupta
2013-10-11 15:55 ` Felipe Balbi
2013-10-11 19:28 ` Brian Norris
2013-10-11 13:36 ` [PATCH v8 4/6] mtd:nand:omap2: updated support for BCH4 ECC scheme Pekon Gupta
` (3 subsequent siblings)
6 siblings, 2 replies; 27+ messages in thread
From: Pekon Gupta @ 2013-10-11 13:36 UTC (permalink / raw)
To: mark.rutland, robherring2, olof, computersforpeace, dedekind1
Cc: Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
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_HAM1_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
- separates the configurations code for each 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 | 450 +++++++++++++++++++++++------------------------
1 file changed, 220 insertions(+), 230 deletions(-)
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 8d521aa..fb96251 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 2
+#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,7 @@ static int omap_dev_ready(struct mtd_info *mtd)
}
}
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
-
+#if defined(CONFIG_MTD_NAND_ECC_BCH) || defined(CONFIG_MTD_NAND_OMAP_BCH)
/**
* omap3_enable_hwecc_bch - Program OMAP3 GPMC to perform BCH ECC correction
* @mtd: MTD device structure
@@ -1140,7 +1141,9 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
/* Clear ecc and enable bits */
writel(ECCCLEAR | ECC1, info->reg.gpmc_ecc_control);
}
+#endif
+#ifdef CONFIG_MTD_NAND_ECC_BCH
/**
* omap3_calculate_ecc_bch4 - Generate 7 bytes of ECC bytes
* @mtd: MTD device structure
@@ -1225,7 +1228,9 @@ static int omap3_calculate_ecc_bch8(struct mtd_info *mtd, const u_char *dat,
return 0;
}
+#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
@@ -1517,7 +1522,9 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
return stat;
}
+#endif /* CONFIG_MTD_NAND_OMAP_BCH */
+#ifdef CONFIG_MTD_NAND_ECC_BCH
/**
* omap3_correct_data_bch - Decode received data and correct errors
* @mtd: MTD device structure
@@ -1549,7 +1556,9 @@ static int omap3_correct_data_bch(struct mtd_info *mtd, u_char *data,
}
return count;
}
+#endif /* CONFIG_MTD_NAND_ECC_BCH */
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
/**
* omap_write_page_bch - BCH ecc based write page function for entire page
* @mtd: mtd info structure
@@ -1637,197 +1646,68 @@ 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
+ * 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 void omap3_free_bch(struct mtd_info *mtd)
+static int is_elm_present(struct omap_nand_info *info,
+ struct device_node *elm_node, enum bch_ecc bch_type)
{
- 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;
+ 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;
}
-
- 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;
- }
+ 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;
}
-
- pr_info("enabling NAND BCH ecc with %d-bit correction\n", max_errors);
+ /* 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;
}
+#endif /* CONFIG_MTD_NAND_ECC_BCH */
+#ifdef CONFIG_MTD_NAND_ECC_BCH
/**
- * omap3_init_bch_tail - Build an oob layout for BCH ECC correction.
+ * omap3_free_bch - Release BCH ecc resources
* @mtd: MTD device structure
*/
-static int omap3_init_bch_tail(struct mtd_info *mtd)
+static void omap3_free_bch(struct mtd_info *mtd)
{
- 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;
- }
-
- /* 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;
+ if (info->bch) {
+ free_bch(info->bch);
+ info->bch = NULL;
}
-
- /* 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;
- 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 */
+#endif /* CONFIG_MTD_NAND_ECC_BCH */
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 *nand_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 +1726,18 @@ 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;
+ nand_chip = &info->nand;
+ 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_SKIP_BBTSCAN;
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
+ nand_chip->options |= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
info->of_node = pdata->of_node;
-#endif
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (res == NULL) {
@@ -1903,6 +1781,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,
+ (nand_chip->options & NAND_BUSWIDTH_16) ? "x16" : "x8");
+ if ((nand_chip->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_HAM1_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,61 +1894,148 @@ static int omap_nand_probe(struct platform_device *pdev)
goto out_release_mem_region;
}
- /* select the ecc type */
- if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_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) {
+ /* populate MTD interface based on ECC scheme */
+ nand_chip->ecc.layout = &omap_oobinfo;
+ ecclayout = &omap_oobinfo;
+ switch (pdata->ecc_opt) {
+ case OMAP_ECC_HAM1_CODE_HW:
+ pr_info("nand: using OMAP_ECC_HAM1_CODE_HW\n");
+ nand_chip->ecc.mode = NAND_ECC_HW;
+ nand_chip->ecc.bytes = 3;
+ nand_chip->ecc.size = 512;
+ nand_chip->ecc.strength = 1;
+ nand_chip->ecc.calculate = omap_calculate_ecc;
+ nand_chip->ecc.hwctl = omap_enable_hwecc;
+ nand_chip->ecc.correct = omap_correct_data;
+ /* define custom ECC layout */
+ ecclayout->eccbytes = nand_chip->ecc.bytes *
+ (mtd->writesize /
+ nand_chip->ecc.size);
+ if (nand_chip->options & NAND_BUSWIDTH_16)
+ ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH;
+ else
+ ecclayout->eccpos[0] = 1;
+ ecclayout->oobfree->offset = ecclayout->eccpos[0] +
+ ecclayout->eccbytes;
+ 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");
+ nand_chip->ecc.mode = NAND_ECC_HW;
+ nand_chip->ecc.size = 512;
+ nand_chip->ecc.bytes = 7;
+ nand_chip->ecc.strength = 4;
+ nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
+ nand_chip->ecc.correct = omap3_correct_data_bch;
+ nand_chip->ecc.calculate = omap3_calculate_ecc_bch4;
+ /* define custom ECC layout */
+ ecclayout->eccbytes = nand_chip->ecc.bytes *
+ (mtd->writesize /
+ nand_chip->ecc.size);
+ ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH;
+ ecclayout->oobfree->offset = ecclayout->eccpos[0] +
+ ecclayout->eccbytes;
+ /* software bch library is used for locating errors */
+ info->bch = init_bch(nand_chip->ecc.bytes,
+ nand_chip->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;
}
- }
+ break;
+#else
+ pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
+ err = -EINVAL;
+ goto out_release_mem_region;
+#endif
- /* 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;
+ 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");
+ nand_chip->ecc.mode = NAND_ECC_HW;
+ nand_chip->ecc.size = 512;
+ nand_chip->ecc.bytes = 13;
+ nand_chip->ecc.strength = 8;
+ nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
+ nand_chip->ecc.correct = omap3_correct_data_bch;
+ nand_chip->ecc.calculate = omap3_calculate_ecc_bch8;
+ /* define custom ECC layout */
+ ecclayout->eccbytes = nand_chip->ecc.bytes *
+ (mtd->writesize /
+ nand_chip->ecc.size);
+ ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH;
+ ecclayout->oobfree->offset = ecclayout->eccpos[0] +
+ ecclayout->eccbytes;
+ /* software bch library is used for locating errors */
+ info->bch = init_bch(nand_chip->ecc.bytes,
+ nand_chip->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_HAM1_CODE_HW) {
+ break;
+#else
+ pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
+ err = -EINVAL;
+ goto out_release_mem_region;
+#endif
- 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.writesize / 512);
- 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;
+ case OMAP_ECC_BCH8_CODE_HW:
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+ pr_info("nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme\n");
+ nand_chip->ecc.mode = NAND_ECC_HW;
+ nand_chip->ecc.size = 512;
+ /* 14th bit is kept reserved for ROM-code compatibility */
+ nand_chip->ecc.bytes = 13 + 1;
+ nand_chip->ecc.strength = 8;
+ nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
+ nand_chip->ecc.correct = omap_elm_correct_data;
+ nand_chip->ecc.calculate = omap3_calculate_ecc_bch;
+ nand_chip->ecc.read_page = omap_read_page_bch;
+ nand_chip->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");
goto out_release_mem_region;
}
+ /* define custom ECC layout */
+ ecclayout->eccbytes = nand_chip->ecc.bytes *
+ (mtd->writesize /
+ nand_chip->ecc.size);
+ ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH;
+ ecclayout->oobfree->offset = ecclayout->eccpos[0] +
+ ecclayout->eccbytes;
+ break;
+#else
+ pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
+ err = -EINVAL;
+ goto out_release_mem_region;
+#endif
+
+ default:
+ pr_err("nand: error: invalid or unsupported ECC scheme\n");
+ err = -EINVAL;
+ goto out_release_mem_region;
+ }
+
+ /* 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 (!(nand_chip->options & NAND_BUSWIDTH_16))
+ nand_chip->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 */
@@ -2072,6 +2061,7 @@ out_release_mem_region:
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;
--
1.8.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe
2013-10-11 13:36 ` [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe Pekon Gupta
@ 2013-10-11 15:55 ` Felipe Balbi
2013-10-11 19:28 ` Brian Norris
1 sibling, 0 replies; 27+ messages in thread
From: Felipe Balbi @ 2013-10-11 15:55 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, computersforpeace, dedekind1,
Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
Andrew Morton
[-- Attachment #1: Type: text/plain, Size: 22330 bytes --]
On Fri, Oct 11, 2013 at 07:06:40PM +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_HAM1_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
> - separates the configurations code for each 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>
Reviewed-by: Felipe Balbi <balbi@ti.com>
> ---
> drivers/mtd/nand/omap2.c | 450 +++++++++++++++++++++++------------------------
> 1 file changed, 220 insertions(+), 230 deletions(-)
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 8d521aa..fb96251 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 2
> +#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,7 @@ static int omap_dev_ready(struct mtd_info *mtd)
> }
> }
>
> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
> -
> +#if defined(CONFIG_MTD_NAND_ECC_BCH) || defined(CONFIG_MTD_NAND_OMAP_BCH)
> /**
> * omap3_enable_hwecc_bch - Program OMAP3 GPMC to perform BCH ECC correction
> * @mtd: MTD device structure
> @@ -1140,7 +1141,9 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
> /* Clear ecc and enable bits */
> writel(ECCCLEAR | ECC1, info->reg.gpmc_ecc_control);
> }
> +#endif
>
> +#ifdef CONFIG_MTD_NAND_ECC_BCH
> /**
> * omap3_calculate_ecc_bch4 - Generate 7 bytes of ECC bytes
> * @mtd: MTD device structure
> @@ -1225,7 +1228,9 @@ static int omap3_calculate_ecc_bch8(struct mtd_info *mtd, const u_char *dat,
>
> return 0;
> }
> +#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
> @@ -1517,7 +1522,9 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
>
> return stat;
> }
> +#endif /* CONFIG_MTD_NAND_OMAP_BCH */
>
> +#ifdef CONFIG_MTD_NAND_ECC_BCH
> /**
> * omap3_correct_data_bch - Decode received data and correct errors
> * @mtd: MTD device structure
> @@ -1549,7 +1556,9 @@ static int omap3_correct_data_bch(struct mtd_info *mtd, u_char *data,
> }
> return count;
> }
> +#endif /* CONFIG_MTD_NAND_ECC_BCH */
>
> +#ifdef CONFIG_MTD_NAND_OMAP_BCH
> /**
> * omap_write_page_bch - BCH ecc based write page function for entire page
> * @mtd: mtd info structure
> @@ -1637,197 +1646,68 @@ 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
> + * 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 void omap3_free_bch(struct mtd_info *mtd)
> +static int is_elm_present(struct omap_nand_info *info,
> + struct device_node *elm_node, enum bch_ecc bch_type)
> {
> - 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;
> + 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;
> }
> -
> - 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;
> - }
> + 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;
> }
> -
> - pr_info("enabling NAND BCH ecc with %d-bit correction\n", max_errors);
> + /* 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;
> }
> +#endif /* CONFIG_MTD_NAND_ECC_BCH */
>
> +#ifdef CONFIG_MTD_NAND_ECC_BCH
> /**
> - * omap3_init_bch_tail - Build an oob layout for BCH ECC correction.
> + * omap3_free_bch - Release BCH ecc resources
> * @mtd: MTD device structure
> */
> -static int omap3_init_bch_tail(struct mtd_info *mtd)
> +static void omap3_free_bch(struct mtd_info *mtd)
> {
> - 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;
> - }
> -
> - /* 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;
> + if (info->bch) {
> + free_bch(info->bch);
> + info->bch = NULL;
> }
> -
> - /* 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;
> - 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 */
> +#endif /* CONFIG_MTD_NAND_ECC_BCH */
>
> 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 *nand_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 +1726,18 @@ 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;
> + nand_chip = &info->nand;
> + 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_SKIP_BBTSCAN;
> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
> + nand_chip->options |= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
> info->of_node = pdata->of_node;
> -#endif
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (res == NULL) {
> @@ -1903,6 +1781,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,
> + (nand_chip->options & NAND_BUSWIDTH_16) ? "x16" : "x8");
> + if ((nand_chip->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_HAM1_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,61 +1894,148 @@ static int omap_nand_probe(struct platform_device *pdev)
> goto out_release_mem_region;
> }
>
> - /* select the ecc type */
> - if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_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) {
> + /* populate MTD interface based on ECC scheme */
> + nand_chip->ecc.layout = &omap_oobinfo;
> + ecclayout = &omap_oobinfo;
> + switch (pdata->ecc_opt) {
> + case OMAP_ECC_HAM1_CODE_HW:
> + pr_info("nand: using OMAP_ECC_HAM1_CODE_HW\n");
> + nand_chip->ecc.mode = NAND_ECC_HW;
> + nand_chip->ecc.bytes = 3;
> + nand_chip->ecc.size = 512;
> + nand_chip->ecc.strength = 1;
> + nand_chip->ecc.calculate = omap_calculate_ecc;
> + nand_chip->ecc.hwctl = omap_enable_hwecc;
> + nand_chip->ecc.correct = omap_correct_data;
> + /* define custom ECC layout */
> + ecclayout->eccbytes = nand_chip->ecc.bytes *
> + (mtd->writesize /
> + nand_chip->ecc.size);
> + if (nand_chip->options & NAND_BUSWIDTH_16)
> + ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH;
> + else
> + ecclayout->eccpos[0] = 1;
> + ecclayout->oobfree->offset = ecclayout->eccpos[0] +
> + ecclayout->eccbytes;
> + 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");
> + nand_chip->ecc.mode = NAND_ECC_HW;
> + nand_chip->ecc.size = 512;
> + nand_chip->ecc.bytes = 7;
> + nand_chip->ecc.strength = 4;
> + nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
> + nand_chip->ecc.correct = omap3_correct_data_bch;
> + nand_chip->ecc.calculate = omap3_calculate_ecc_bch4;
> + /* define custom ECC layout */
> + ecclayout->eccbytes = nand_chip->ecc.bytes *
> + (mtd->writesize /
> + nand_chip->ecc.size);
> + ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH;
> + ecclayout->oobfree->offset = ecclayout->eccpos[0] +
> + ecclayout->eccbytes;
> + /* software bch library is used for locating errors */
> + info->bch = init_bch(nand_chip->ecc.bytes,
> + nand_chip->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;
> }
> - }
> + break;
> +#else
> + pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
> + err = -EINVAL;
> + goto out_release_mem_region;
> +#endif
>
> - /* 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;
> + 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");
> + nand_chip->ecc.mode = NAND_ECC_HW;
> + nand_chip->ecc.size = 512;
> + nand_chip->ecc.bytes = 13;
> + nand_chip->ecc.strength = 8;
> + nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
> + nand_chip->ecc.correct = omap3_correct_data_bch;
> + nand_chip->ecc.calculate = omap3_calculate_ecc_bch8;
> + /* define custom ECC layout */
> + ecclayout->eccbytes = nand_chip->ecc.bytes *
> + (mtd->writesize /
> + nand_chip->ecc.size);
> + ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH;
> + ecclayout->oobfree->offset = ecclayout->eccpos[0] +
> + ecclayout->eccbytes;
> + /* software bch library is used for locating errors */
> + info->bch = init_bch(nand_chip->ecc.bytes,
> + nand_chip->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_HAM1_CODE_HW) {
> + break;
> +#else
> + pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
> + err = -EINVAL;
> + goto out_release_mem_region;
> +#endif
>
> - 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.writesize / 512);
> - 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;
> + case OMAP_ECC_BCH8_CODE_HW:
> +#ifdef CONFIG_MTD_NAND_OMAP_BCH
> + pr_info("nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme\n");
> + nand_chip->ecc.mode = NAND_ECC_HW;
> + nand_chip->ecc.size = 512;
> + /* 14th bit is kept reserved for ROM-code compatibility */
> + nand_chip->ecc.bytes = 13 + 1;
> + nand_chip->ecc.strength = 8;
> + nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
> + nand_chip->ecc.correct = omap_elm_correct_data;
> + nand_chip->ecc.calculate = omap3_calculate_ecc_bch;
> + nand_chip->ecc.read_page = omap_read_page_bch;
> + nand_chip->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");
> goto out_release_mem_region;
> }
> + /* define custom ECC layout */
> + ecclayout->eccbytes = nand_chip->ecc.bytes *
> + (mtd->writesize /
> + nand_chip->ecc.size);
> + ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH;
> + ecclayout->oobfree->offset = ecclayout->eccpos[0] +
> + ecclayout->eccbytes;
> + break;
> +#else
> + pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
> + err = -EINVAL;
> + goto out_release_mem_region;
> +#endif
> +
> + default:
> + pr_err("nand: error: invalid or unsupported ECC scheme\n");
> + err = -EINVAL;
> + goto out_release_mem_region;
> + }
> +
> + /* 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 (!(nand_chip->options & NAND_BUSWIDTH_16))
> + nand_chip->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 */
> @@ -2072,6 +2061,7 @@ out_release_mem_region:
> 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;
> --
> 1.8.1
>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe
2013-10-11 13:36 ` [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe Pekon Gupta
2013-10-11 15:55 ` Felipe Balbi
@ 2013-10-11 19:28 ` Brian Norris
2013-10-12 23:58 ` Gupta, Pekon
1 sibling, 1 reply; 27+ messages in thread
From: Brian Norris @ 2013-10-11 19:28 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, dedekind1, Pawel.Moll,
ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree
On Fri, Oct 11, 2013 at 07:06:40PM +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_HAM1_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
> - separates the configurations code for each ECC schemes
> - fixes dependency issues based on Kconfig options
> - updates ELM device detection in is_elm_present()
> - cleans redundant code
I'll repeat my previous comment on this patch, since it seems entirely
ignored:
<quote>
[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.
</quote>
Considering your frustration, it is certainly in your best interest to
make your patches more easily reviewable and to address my comments when
I make them. I cannot sign off on your patches in the current state, and
you have failed to properly address my comments on the nearly identical
code from weeks ago.
Please, put more effort into splitting your patches into reviewable
chunks and into writing *good* descriptions -- right now, 90% of your
commit message consists of a repeated ECC table, which does not actually
describe your changes. The remaining description is a jumble of multiple
unrelated thoughts, reflecting the similarly confusing patches. A
similar criticism applies to your other patches, whose descriptions do
not adequately reflect their contents.
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
> drivers/mtd/nand/omap2.c | 450 +++++++++++++++++++++++------------------------
> 1 file changed, 220 insertions(+), 230 deletions(-)
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 8d521aa..fb96251 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
>
Why do you even need the #ifdef's for the #include's? It is not harmful
to include headers for stuff that is only conditionally used. In fact,
this creates a problem later when you try to use nand_bch_free(), and
you have to surround it with extra #ifdef's.
In short: these #ifdef's just breed more #ifdef's and cause the code to
become harder to read and less maintainable.
(I commented about the #ifdef's around nand_bch_free() in v6, and you
did not address the comment.)
> @@ -1846,20 +1726,18 @@ 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;
> + nand_chip = &info->nand;
> + 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_SKIP_BBTSCAN;
> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
> + nand_chip->options |= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
I recommended (in v6) you split the NAND_BUSWIDTH_AUTO change to a new
patch and to describe it in the commit. You did neither.
> info->of_node = pdata->of_node;
> -#endif
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (res == NULL) {
> @@ -1903,6 +1781,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,
> + (nand_chip->options & NAND_BUSWIDTH_16) ? "x16" : "x8");
I recommended you kill this in v6, and you did not address my comments.
> + if ((nand_chip->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;
...
Brian
^ permalink raw reply [flat|nested] 27+ messages in thread* RE: [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe
2013-10-11 19:28 ` Brian Norris
@ 2013-10-12 23:58 ` Gupta, Pekon
2013-10-13 1:40 ` Brian Norris
0 siblings, 1 reply; 27+ messages in thread
From: Gupta, Pekon @ 2013-10-12 23:58 UTC (permalink / raw)
To: Brian Norris
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, arnd@arndb.de,
Pawel.Moll@arm.com, ijc+devicetree@hellion.org.uk,
tony@atomide.com, dedekind1@gmail.com, avinashphilipk@gmail.com,
Balbi, Felipe, robherring2@gmail.com, bcousson@baylibre.com,
swarren@wwwdotorg.org, olof@lixom.net,
linux-mtd@lists.infradead.org, linux-omap@vger.kernel.org,
dwmw2@infradead.org
Hi Brian,
Thanks for such detailed review, please see some replies below..
> From: Brian Norris [mailto:computersforpeace@gmail.com]
> > On Fri, Oct 11, 2013 at 07:06:40PM +0530, Pekon Gupta wrote:
[...]
> Why do you even need the #ifdef's for the #include's? It is not harmful
> to include headers for stuff that is only conditionally used. In fact,
> this creates a problem later when you try to use nand_bch_free(), and
> you have to surround it with extra #ifdef's.
>
> In short: these #ifdef's just breed more #ifdef's and cause the code to
> become harder to read and less maintainable.
>
There are 2 problems here:
(1)
I have removed #ifdef across headers in next version. But I cannot remove
#ifdef across some callbacks for cc.hwctl(), ecc.calculate(), ecc.correct(),
because then compilation would throw warnings for un-used functions.
(2)
OMAP driver uses generic lib/bch.c which is compiled only when
CONFIG_MTD_NAND_ECC_BCH is enabled. So to avoid compilation
issues, I had to put #ifdefs on all wrapper functions which use lib/bch.c
or nand_bch.c
I myself have tried in previous versions to avoid #ifdefs, but I ended
up in one or the other problem like (1) | (2) above.
And this is where randconfig test failed earlier when Arnd Bergmann
commented, so some #ifdef would hv to be carried till be deprecate
some legacy ecc-schemes.
However, with patch split many redundant #ifdefs are now removed.
> (I commented about the #ifdef's around nand_bch_free() in v6, and you
> did not address the comment.)
>
Done now.. My bad again, I somehow mis-interpreted nand_bch.c earlier.
> > @@ -1846,20 +1726,18 @@ static int omap_nand_probe(struct
> > - info->nand.options |= NAND_SKIP_BBTSCAN;
> > -#ifdef CONFIG_MTD_NAND_OMAP_BCH
> > + nand_chip->options |= NAND_SKIP_BBTSCAN |
> NAND_BUSWIDTH_AUTO;
>
> I recommended (in v6) you split the NAND_BUSWIDTH_AUTO change to a
> new
> patch and to describe it in the commit. You did neither.
>
Sorry missed this purposely because I could not separate out the changes.
But now I have re-worked this in next revision as a separate patch.
> > + /* 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,
> > + (nand_chip->options & NAND_BUSWIDTH_16) ?
> "x16" : "x8");
>
> I recommended you kill this in v6, and you did not address my comments.
>
Sorry, this was my bad.
with regards, pekon
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe
2013-10-12 23:58 ` Gupta, Pekon
@ 2013-10-13 1:40 ` Brian Norris
0 siblings, 0 replies; 27+ messages in thread
From: Brian Norris @ 2013-10-13 1:40 UTC (permalink / raw)
To: Gupta, Pekon
Cc: mark.rutland@arm.com, robherring2@gmail.com, olof@lixom.net,
dedekind1@gmail.com, Pawel.Moll@arm.com,
ijc+devicetree@hellion.org.uk, swarren@wwwdotorg.org,
dwmw2@infradead.org, arnd@arndb.de, tony@atomide.com,
bcousson@baylibre.com, avinashphilipk@gmail.com, Balbi, Felipe,
linux-mtd@lists.infradead.org, linux-omap@vger.kernel.org,
devicetree@vger.kernel.org
Hi Pekon,
On 10/12/2013 04:58 PM, Gupta, Pekon wrote:
>> From: Brian Norris [mailto:computersforpeace@gmail.com]
>>> On Fri, Oct 11, 2013 at 07:06:40PM +0530, Pekon Gupta wrote:
> [...]
>> Why do you even need the #ifdef's for the #include's? It is not harmful
>> to include headers for stuff that is only conditionally used. In fact,
>> this creates a problem later when you try to use nand_bch_free(), and
>> you have to surround it with extra #ifdef's.
>>
>> In short: these #ifdef's just breed more #ifdef's and cause the code to
>> become harder to read and less maintainable.
First off, I should clarify this: the above comment was speaking *only*
about surrounding the #include's with #ifdef's. Such #ifdef's only make
everything else harder.
Now, I recognize that there are still many cases where you may need
#ifdef's in the body of the code. I was only trying to hit the easiest ones.
> There are 2 problems here:
> (1)
> I have removed #ifdef across headers in next version. But I cannot remove
> #ifdef across some callbacks for cc.hwctl(), ecc.calculate(), ecc.correct(),
> because then compilation would throw warnings for un-used functions.
The __maybe_unused attribute can be given to functions that will compile
but are unused, and the compiler will just remove unused ones from the
binary without warning. That is probably (weakly) preferable to
#ifdef's, if you can manage it. But I understand some #ifdef's are
necessary.
> (2)
> OMAP driver uses generic lib/bch.c which is compiled only when
> CONFIG_MTD_NAND_ECC_BCH is enabled. So to avoid compilation
> issues, I had to put #ifdefs on all wrapper functions which use lib/bch.c
> or nand_bch.c
nand_bch.c (and its corresponding nand_bch.h) has stub implementations
so that extra #ifdef's often aren't needed. But include/linux/bch.h does
not, so you are correct.
> I myself have tried in previous versions to avoid #ifdefs, but I ended
> up in one or the other problem like (1) | (2) above.
> And this is where randconfig test failed earlier when Arnd Bergmann
> commented, so some #ifdef would hv to be carried till be deprecate
> some legacy ecc-schemes.
> However, with patch split many redundant #ifdefs are now removed.
Great, thanks for the effort.
Regards,
Brian
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 4/6] mtd:nand:omap2: updated support for BCH4 ECC scheme
2013-10-11 13:36 [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Pekon Gupta
` (2 preceding siblings ...)
2013-10-11 13:36 ` [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe Pekon Gupta
@ 2013-10-11 13:36 ` Pekon Gupta
2013-10-11 15:55 ` Felipe Balbi
2013-10-11 20:28 ` Brian Norris
2013-10-11 13:36 ` [PATCH v8 5/6] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt Pekon Gupta
` (2 subsequent siblings)
6 siblings, 2 replies; 27+ messages in thread
From: Pekon Gupta @ 2013-10-11 13:36 UTC (permalink / raw)
To: mark.rutland, robherring2, olof, computersforpeace, dedekind1
Cc: Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
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_HAM1_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) |
+---------------------------------------+---------------+---------------+
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 | 134 +++++++++++++++++++++--------------------------
2 files changed, 63 insertions(+), 101 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 fb96251..a783dae 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 2
-#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,
@@ -188,7 +188,6 @@ struct omap_nand_info {
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;
@@ -1522,43 +1521,7 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
return stat;
}
-#endif /* CONFIG_MTD_NAND_OMAP_BCH */
-#ifdef CONFIG_MTD_NAND_ECC_BCH
-/**
- * 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;
-}
-#endif /* CONFIG_MTD_NAND_ECC_BCH */
-
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
/**
* omap_write_page_bch - BCH ecc based write page function for entire page
* @mtd: mtd info structure
@@ -1675,28 +1638,6 @@ static int is_elm_present(struct omap_nand_info *info,
}
#endif /* CONFIG_MTD_NAND_ECC_BCH */
-#ifdef CONFIG_MTD_NAND_ECC_BCH
-/**
- * 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 */
-
static int omap_nand_probe(struct platform_device *pdev)
{
struct omap_nand_info *info;
@@ -1731,10 +1672,10 @@ static int omap_nand_probe(struct platform_device *pdev)
mtd->owner = THIS_MODULE;
mtd->priv = &info->nand;
nand_chip = &info->nand;
+ nand_chip->ecc.priv = NULL;
info->pdev = pdev;
info->gpmc_cs = pdata->cs;
info->reg = pdata->reg;
- info->bch = NULL;
nand_chip->options |= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
info->of_node = pdata->of_node;
@@ -1927,7 +1868,7 @@ static int omap_nand_probe(struct platform_device *pdev)
nand_chip->ecc.bytes = 7;
nand_chip->ecc.strength = 4;
nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
- nand_chip->ecc.correct = omap3_correct_data_bch;
+ info->nand.ecc.correct = nand_bch_correct_data;
nand_chip->ecc.calculate = omap3_calculate_ecc_bch4;
/* define custom ECC layout */
ecclayout->eccbytes = nand_chip->ecc.bytes *
@@ -1937,10 +1878,11 @@ static int omap_nand_probe(struct platform_device *pdev)
ecclayout->oobfree->offset = ecclayout->eccpos[0] +
ecclayout->eccbytes;
/* software bch library is used for locating errors */
- info->bch = init_bch(nand_chip->ecc.bytes,
- nand_chip->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;
}
@@ -1951,6 +1893,39 @@ static int omap_nand_probe(struct platform_device *pdev)
goto out_release_mem_region;
#endif
+ 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 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;
+ break;
+#else
+ pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
+ err = -EINVAL;
+ goto out_release_mem_region;
+#endif
+
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");
@@ -1959,7 +1934,7 @@ static int omap_nand_probe(struct platform_device *pdev)
nand_chip->ecc.bytes = 13;
nand_chip->ecc.strength = 8;
nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
- nand_chip->ecc.correct = omap3_correct_data_bch;
+ info->nand.ecc.correct = nand_bch_correct_data;
nand_chip->ecc.calculate = omap3_calculate_ecc_bch8;
/* define custom ECC layout */
ecclayout->eccbytes = nand_chip->ecc.bytes *
@@ -1969,10 +1944,11 @@ static int omap_nand_probe(struct platform_device *pdev)
ecclayout->oobfree->offset = ecclayout->eccpos[0] +
ecclayout->eccbytes;
/* software bch library is used for locating errors */
- info->bch = init_bch(nand_chip->ecc.bytes,
- nand_chip->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;
@@ -2023,7 +1999,6 @@ static int omap_nand_probe(struct platform_device *pdev)
}
/* 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 (!(nand_chip->options & NAND_BUSWIDTH_16))
@@ -2061,7 +2036,12 @@ out_release_mem_region:
free_irq(info->gpmc_irq_fifo, info);
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;
@@ -2072,8 +2052,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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 4/6] mtd:nand:omap2: updated support for BCH4 ECC scheme
2013-10-11 13:36 ` [PATCH v8 4/6] mtd:nand:omap2: updated support for BCH4 ECC scheme Pekon Gupta
@ 2013-10-11 15:55 ` Felipe Balbi
2013-10-11 20:28 ` Brian Norris
1 sibling, 0 replies; 27+ messages in thread
From: Felipe Balbi @ 2013-10-11 15:55 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, computersforpeace, dedekind1,
Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
Andrew Morton
[-- Attachment #1: Type: text/plain, Size: 12255 bytes --]
On Fri, Oct 11, 2013 at 07:06:41PM +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_HAM1_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) |
> +---------------------------------------+---------------+---------------+
> 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>
Reviewed-by: Felipe Balbi <balbi@ti.com>
> ---
> drivers/mtd/nand/Kconfig | 30 ++---------
> drivers/mtd/nand/omap2.c | 134 +++++++++++++++++++++--------------------------
> 2 files changed, 63 insertions(+), 101 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 fb96251..a783dae 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 2
> -#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,
> @@ -188,7 +188,6 @@ struct omap_nand_info {
> 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;
> @@ -1522,43 +1521,7 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
>
> return stat;
> }
> -#endif /* CONFIG_MTD_NAND_OMAP_BCH */
>
> -#ifdef CONFIG_MTD_NAND_ECC_BCH
> -/**
> - * 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;
> -}
> -#endif /* CONFIG_MTD_NAND_ECC_BCH */
> -
> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
> /**
> * omap_write_page_bch - BCH ecc based write page function for entire page
> * @mtd: mtd info structure
> @@ -1675,28 +1638,6 @@ static int is_elm_present(struct omap_nand_info *info,
> }
> #endif /* CONFIG_MTD_NAND_ECC_BCH */
>
> -#ifdef CONFIG_MTD_NAND_ECC_BCH
> -/**
> - * 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 */
> -
> static int omap_nand_probe(struct platform_device *pdev)
> {
> struct omap_nand_info *info;
> @@ -1731,10 +1672,10 @@ static int omap_nand_probe(struct platform_device *pdev)
> mtd->owner = THIS_MODULE;
> mtd->priv = &info->nand;
> nand_chip = &info->nand;
> + nand_chip->ecc.priv = NULL;
> info->pdev = pdev;
> info->gpmc_cs = pdata->cs;
> info->reg = pdata->reg;
> - info->bch = NULL;
>
> nand_chip->options |= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
> info->of_node = pdata->of_node;
> @@ -1927,7 +1868,7 @@ static int omap_nand_probe(struct platform_device *pdev)
> nand_chip->ecc.bytes = 7;
> nand_chip->ecc.strength = 4;
> nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
> - nand_chip->ecc.correct = omap3_correct_data_bch;
> + info->nand.ecc.correct = nand_bch_correct_data;
> nand_chip->ecc.calculate = omap3_calculate_ecc_bch4;
> /* define custom ECC layout */
> ecclayout->eccbytes = nand_chip->ecc.bytes *
> @@ -1937,10 +1878,11 @@ static int omap_nand_probe(struct platform_device *pdev)
> ecclayout->oobfree->offset = ecclayout->eccpos[0] +
> ecclayout->eccbytes;
> /* software bch library is used for locating errors */
> - info->bch = init_bch(nand_chip->ecc.bytes,
> - nand_chip->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;
> }
> @@ -1951,6 +1893,39 @@ static int omap_nand_probe(struct platform_device *pdev)
> goto out_release_mem_region;
> #endif
>
> + 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 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;
> + break;
> +#else
> + pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
> + err = -EINVAL;
> + goto out_release_mem_region;
> +#endif
> +
> 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");
> @@ -1959,7 +1934,7 @@ static int omap_nand_probe(struct platform_device *pdev)
> nand_chip->ecc.bytes = 13;
> nand_chip->ecc.strength = 8;
> nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
> - nand_chip->ecc.correct = omap3_correct_data_bch;
> + info->nand.ecc.correct = nand_bch_correct_data;
> nand_chip->ecc.calculate = omap3_calculate_ecc_bch8;
> /* define custom ECC layout */
> ecclayout->eccbytes = nand_chip->ecc.bytes *
> @@ -1969,10 +1944,11 @@ static int omap_nand_probe(struct platform_device *pdev)
> ecclayout->oobfree->offset = ecclayout->eccpos[0] +
> ecclayout->eccbytes;
> /* software bch library is used for locating errors */
> - info->bch = init_bch(nand_chip->ecc.bytes,
> - nand_chip->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;
> @@ -2023,7 +1999,6 @@ static int omap_nand_probe(struct platform_device *pdev)
> }
>
> /* 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 (!(nand_chip->options & NAND_BUSWIDTH_16))
> @@ -2061,7 +2036,12 @@ out_release_mem_region:
> free_irq(info->gpmc_irq_fifo, info);
> 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;
> @@ -2072,8 +2052,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
>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 4/6] mtd:nand:omap2: updated support for BCH4 ECC scheme
2013-10-11 13:36 ` [PATCH v8 4/6] mtd:nand:omap2: updated support for BCH4 ECC scheme Pekon Gupta
2013-10-11 15:55 ` Felipe Balbi
@ 2013-10-11 20:28 ` Brian Norris
1 sibling, 0 replies; 27+ messages in thread
From: Brian Norris @ 2013-10-11 20:28 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, dedekind1, Pawel.Moll,
ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree
On Fri, Oct 11, 2013 at 07:06:41PM +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_HAM1_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) |
> +---------------------------------------+---------------+---------------+
> 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 | 134 +++++++++++++++++++++--------------------------
> 2 files changed, 63 insertions(+), 101 deletions(-)
>
...
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index fb96251..a783dae 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1927,7 +1868,7 @@ static int omap_nand_probe(struct platform_device *pdev)
> nand_chip->ecc.bytes = 7;
> nand_chip->ecc.strength = 4;
> nand_chip->ecc.hwctl = omap3_enable_hwecc_bch;
> - nand_chip->ecc.correct = omap3_correct_data_bch;
> + info->nand.ecc.correct = nand_bch_correct_data;
Your patch description doesn't talk about this (and the deletion of
omap3_correct_data_bch() above). Is the NAND BCH library a drop-in
replacement for omap3_correct_data_bch()? If so, TELL me about it in the
commit description. If not, this is an incompatible change and should at
least be documented so that people can understand the change. These
questions are being asked by the DT guys, so include it in your
descriptions.
Also, you're very inconsistent on using 'nand_chip' vs. 'info->nand'.
You added 'nand_chip' amid the noise of patch 3, so if you have it, use
it consistently throughout probe(). Or remove it and don't use it at
all. (This would be an independent patch from patch 3 and 4, in case
you're wondering, since it causes a lot of diff noise.)
> nand_chip->ecc.calculate = omap3_calculate_ecc_bch4;
> /* define custom ECC layout */
> ecclayout->eccbytes = nand_chip->ecc.bytes *
> @@ -2061,7 +2036,12 @@ out_release_mem_region:
> free_irq(info->gpmc_irq_fifo, info);
> 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
As noted previously, the #ifdef should not be necessary.
> kfree(info);
>
> return err;
> @@ -2072,8 +2052,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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 5/6] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt
2013-10-11 13:36 [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Pekon Gupta
` (3 preceding siblings ...)
2013-10-11 13:36 ` [PATCH v8 4/6] mtd:nand:omap2: updated support for BCH4 ECC scheme Pekon Gupta
@ 2013-10-11 13:36 ` Pekon Gupta
2013-10-11 15:56 ` Felipe Balbi
2013-10-11 13:36 ` [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls Pekon Gupta
2013-10-11 21:09 ` [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Brian Norris
6 siblings, 1 reply; 27+ messages in thread
From: Pekon Gupta @ 2013-10-11 13:36 UTC (permalink / raw)
To: mark.rutland, robherring2, olof, computersforpeace, dedekind1
Cc: devicetree, Pawel.Moll, arnd, swarren, tony, ijc+devicetree,
avinashphilipk, balbi, linux-mtd, Pekon Gupta, bcousson,
linux-omap, dwmw2
Updated DTS to replace deprecated binding with newer values
Refer: Documentation/devicetree/bindings/mtd/gpmc-nand.txt
Signed-off-by: Pekon Gupta <pekon@ti.com>
---
arch/arm/boot/dts/am335x-evm.dts | 3 +--
arch/arm/boot/dts/omap3430-sdp.dts | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index e8ec875..1aee6ac 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -269,7 +269,6 @@
reg = <0 0 0>; /* CS0, offset 0 */
nand-bus-width = <8>;
ti,nand-ecc-opt = "bch8";
- gpmc,device-nand = "true";
gpmc,device-width = <1>;
gpmc,sync-clk-ps = <0>;
gpmc,cs-on-ns = <0>;
@@ -296,7 +295,7 @@
#address-cells = <1>;
#size-cells = <1>;
- elm_id = <&elm>;
+ ti,elm-id = <&elm>;
/* MTD partition table */
partition@0 {
diff --git a/arch/arm/boot/dts/omap3430-sdp.dts b/arch/arm/boot/dts/omap3430-sdp.dts
index e2249bc..501f863 100644
--- a/arch/arm/boot/dts/omap3430-sdp.dts
+++ b/arch/arm/boot/dts/omap3430-sdp.dts
@@ -105,7 +105,7 @@
reg = <1 0 0x08000000>;
nand-bus-width = <8>;
- ti,nand-ecc-opt = "sw";
+ ti,nand-ecc-opt = "ham1";
gpmc,cs-on-ns = <0>;
gpmc,cs-rd-off-ns = <36>;
gpmc,cs-wr-off-ns = <36>;
--
1.8.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 5/6] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt
2013-10-11 13:36 ` [PATCH v8 5/6] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt Pekon Gupta
@ 2013-10-11 15:56 ` Felipe Balbi
0 siblings, 0 replies; 27+ messages in thread
From: Felipe Balbi @ 2013-10-11 15:56 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, computersforpeace, dedekind1,
Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
Andrew Morton
[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]
On Fri, Oct 11, 2013 at 07:06:42PM +0530, Pekon Gupta wrote:
> Updated DTS to replace deprecated binding with newer values
> Refer: Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>
> Signed-off-by: Pekon Gupta <pekon@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
> ---
> arch/arm/boot/dts/am335x-evm.dts | 3 +--
> arch/arm/boot/dts/omap3430-sdp.dts | 2 +-
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index e8ec875..1aee6ac 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -269,7 +269,6 @@
> reg = <0 0 0>; /* CS0, offset 0 */
> nand-bus-width = <8>;
> ti,nand-ecc-opt = "bch8";
> - gpmc,device-nand = "true";
> gpmc,device-width = <1>;
> gpmc,sync-clk-ps = <0>;
> gpmc,cs-on-ns = <0>;
> @@ -296,7 +295,7 @@
>
> #address-cells = <1>;
> #size-cells = <1>;
> - elm_id = <&elm>;
> + ti,elm-id = <&elm>;
>
> /* MTD partition table */
> partition@0 {
> diff --git a/arch/arm/boot/dts/omap3430-sdp.dts b/arch/arm/boot/dts/omap3430-sdp.dts
> index e2249bc..501f863 100644
> --- a/arch/arm/boot/dts/omap3430-sdp.dts
> +++ b/arch/arm/boot/dts/omap3430-sdp.dts
> @@ -105,7 +105,7 @@
> reg = <1 0 0x08000000>;
> nand-bus-width = <8>;
>
> - ti,nand-ecc-opt = "sw";
> + ti,nand-ecc-opt = "ham1";
> gpmc,cs-on-ns = <0>;
> gpmc,cs-rd-off-ns = <36>;
> gpmc,cs-wr-off-ns = <36>;
> --
> 1.8.1
>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls
2013-10-11 13:36 [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Pekon Gupta
` (4 preceding siblings ...)
2013-10-11 13:36 ` [PATCH v8 5/6] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt Pekon Gupta
@ 2013-10-11 13:36 ` Pekon Gupta
2013-10-11 15:56 ` Felipe Balbi
2013-10-11 18:15 ` Brian Norris
2013-10-11 21:09 ` [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Brian Norris
6 siblings, 2 replies; 27+ messages in thread
From: Pekon Gupta @ 2013-10-11 13:36 UTC (permalink / raw)
To: mark.rutland, robherring2, olof, computersforpeace, dedekind1
Cc: Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
Pekon Gupta
"Managed Device Resource" or devm_xx calls takes care of automatic freeing
of the resource in case of:
- failure during driver probe
- failure during resource allocation
- detaching or unloading of driver module (rmmod)
Reference: Documentation/driver-model/devres.txt
Though OMAP NAND driver handles freeing of resource allocation in most of
the cases, but using devm_xx provides more clean and effortless approach
to handle all such cases.
Signed-off-by: Pekon Gupta <pekon@ti.com>
---
drivers/mtd/nand/omap2.c | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index a783dae..0f2b0d1 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1658,7 +1658,8 @@ static int omap_nand_probe(struct platform_device *pdev)
return -ENODEV;
}
- info = kzalloc(sizeof(struct omap_nand_info), GFP_KERNEL);
+ info = devm_kzalloc(&pdev->dev, sizeof(struct omap_nand_info),
+ GFP_KERNEL);
if (!info)
return -ENOMEM;
@@ -1690,13 +1691,14 @@ static int omap_nand_probe(struct platform_device *pdev)
info->phys_base = res->start;
info->mem_size = resource_size(res);
- if (!request_mem_region(info->phys_base, info->mem_size,
- pdev->dev.driver->name)) {
+ if (!devm_request_mem_region(&pdev->dev, info->phys_base,
+ info->mem_size, pdev->dev.driver->name)) {
err = -EBUSY;
goto out_free_info;
}
- info->nand.IO_ADDR_R = ioremap(info->phys_base, info->mem_size);
+ info->nand.IO_ADDR_R = devm_ioremap(&pdev->dev, info->phys_base,
+ info->mem_size);
if (!info->nand.IO_ADDR_R) {
err = -ENOMEM;
goto out_release_mem_region;
@@ -1799,8 +1801,9 @@ static int omap_nand_probe(struct platform_device *pdev)
err = -ENODEV;
goto out_release_mem_region;
}
- err = request_irq(info->gpmc_irq_fifo, omap_nand_irq,
- IRQF_SHARED, "gpmc-nand-fifo", info);
+ err = devm_request_irq(&pdev->dev, info->gpmc_irq_fifo,
+ omap_nand_irq, IRQF_SHARED,
+ "gpmc-nand-fifo", info);
if (err) {
dev_err(&pdev->dev, "requesting irq(%d) error:%d",
info->gpmc_irq_fifo, err);
@@ -1814,8 +1817,9 @@ static int omap_nand_probe(struct platform_device *pdev)
err = -ENODEV;
goto out_release_mem_region;
}
- err = request_irq(info->gpmc_irq_count, omap_nand_irq,
- IRQF_SHARED, "gpmc-nand-count", info);
+ err = devm_request_irq(&pdev->dev, info->gpmc_irq_count,
+ omap_nand_irq, IRQF_SHARED,
+ "gpmc-nand-count", info);
if (err) {
dev_err(&pdev->dev, "requesting irq(%d) error:%d",
info->gpmc_irq_count, err);
@@ -2031,10 +2035,10 @@ out_release_mem_region:
if (info->dma)
dma_release_channel(info->dma);
if (info->gpmc_irq_count > 0)
- free_irq(info->gpmc_irq_count, info);
+ devm_free_irq(&pdev->dev, info->gpmc_irq_count, info);
if (info->gpmc_irq_fifo > 0)
- free_irq(info->gpmc_irq_fifo, info);
- release_mem_region(info->phys_base, info->mem_size);
+ devm_free_irq(&pdev->dev, info->gpmc_irq_fifo, info);
+ devm_release_mem_region(&pdev->dev, info->phys_base, info->mem_size);
out_free_info:
#ifdef CONFIG_MTD_NAND_ECC_BCH
if (info->nand.ecc.priv) {
@@ -2042,7 +2046,7 @@ out_free_info:
info->nand.ecc.priv = NULL;
}
#endif
- kfree(info);
+ devm_kfree(&pdev->dev, info);
return err;
}
@@ -2062,15 +2066,15 @@ static int omap_nand_remove(struct platform_device *pdev)
dma_release_channel(info->dma);
if (info->gpmc_irq_count > 0)
- free_irq(info->gpmc_irq_count, info);
+ devm_free_irq(&pdev->dev, info->gpmc_irq_count, info);
if (info->gpmc_irq_fifo > 0)
- free_irq(info->gpmc_irq_fifo, info);
+ devm_free_irq(&pdev->dev, info->gpmc_irq_fifo, info);
/* Release NAND device, its internal structures and partitions */
nand_release(&info->mtd);
- iounmap(info->nand.IO_ADDR_R);
- release_mem_region(info->phys_base, info->mem_size);
- kfree(info);
+ devm_iounmap(&pdev->dev, info->nand.IO_ADDR_R);
+ devm_release_mem_region(&pdev->dev, info->phys_base, info->mem_size);
+ devm_kfree(&pdev->dev, info);
return 0;
}
--
1.8.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls
2013-10-11 13:36 ` [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls Pekon Gupta
@ 2013-10-11 15:56 ` Felipe Balbi
2013-10-11 18:15 ` Brian Norris
1 sibling, 0 replies; 27+ messages in thread
From: Felipe Balbi @ 2013-10-11 15:56 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, computersforpeace, dedekind1,
Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree,
Andrew Morton
[-- Attachment #1: Type: text/plain, Size: 4571 bytes --]
On Fri, Oct 11, 2013 at 07:06:43PM +0530, Pekon Gupta wrote:
> "Managed Device Resource" or devm_xx calls takes care of automatic freeing
> of the resource in case of:
> - failure during driver probe
> - failure during resource allocation
> - detaching or unloading of driver module (rmmod)
> Reference: Documentation/driver-model/devres.txt
>
> Though OMAP NAND driver handles freeing of resource allocation in most of
> the cases, but using devm_xx provides more clean and effortless approach
> to handle all such cases.
>
> Signed-off-by: Pekon Gupta <pekon@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
> ---
> drivers/mtd/nand/omap2.c | 38 +++++++++++++++++++++-----------------
> 1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index a783dae..0f2b0d1 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1658,7 +1658,8 @@ static int omap_nand_probe(struct platform_device *pdev)
> return -ENODEV;
> }
>
> - info = kzalloc(sizeof(struct omap_nand_info), GFP_KERNEL);
> + info = devm_kzalloc(&pdev->dev, sizeof(struct omap_nand_info),
> + GFP_KERNEL);
> if (!info)
> return -ENOMEM;
>
> @@ -1690,13 +1691,14 @@ static int omap_nand_probe(struct platform_device *pdev)
> info->phys_base = res->start;
> info->mem_size = resource_size(res);
>
> - if (!request_mem_region(info->phys_base, info->mem_size,
> - pdev->dev.driver->name)) {
> + if (!devm_request_mem_region(&pdev->dev, info->phys_base,
> + info->mem_size, pdev->dev.driver->name)) {
> err = -EBUSY;
> goto out_free_info;
> }
>
> - info->nand.IO_ADDR_R = ioremap(info->phys_base, info->mem_size);
> + info->nand.IO_ADDR_R = devm_ioremap(&pdev->dev, info->phys_base,
> + info->mem_size);
> if (!info->nand.IO_ADDR_R) {
> err = -ENOMEM;
> goto out_release_mem_region;
> @@ -1799,8 +1801,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> err = -ENODEV;
> goto out_release_mem_region;
> }
> - err = request_irq(info->gpmc_irq_fifo, omap_nand_irq,
> - IRQF_SHARED, "gpmc-nand-fifo", info);
> + err = devm_request_irq(&pdev->dev, info->gpmc_irq_fifo,
> + omap_nand_irq, IRQF_SHARED,
> + "gpmc-nand-fifo", info);
> if (err) {
> dev_err(&pdev->dev, "requesting irq(%d) error:%d",
> info->gpmc_irq_fifo, err);
> @@ -1814,8 +1817,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> err = -ENODEV;
> goto out_release_mem_region;
> }
> - err = request_irq(info->gpmc_irq_count, omap_nand_irq,
> - IRQF_SHARED, "gpmc-nand-count", info);
> + err = devm_request_irq(&pdev->dev, info->gpmc_irq_count,
> + omap_nand_irq, IRQF_SHARED,
> + "gpmc-nand-count", info);
> if (err) {
> dev_err(&pdev->dev, "requesting irq(%d) error:%d",
> info->gpmc_irq_count, err);
> @@ -2031,10 +2035,10 @@ out_release_mem_region:
> if (info->dma)
> dma_release_channel(info->dma);
> if (info->gpmc_irq_count > 0)
> - free_irq(info->gpmc_irq_count, info);
> + devm_free_irq(&pdev->dev, info->gpmc_irq_count, info);
> if (info->gpmc_irq_fifo > 0)
> - free_irq(info->gpmc_irq_fifo, info);
> - release_mem_region(info->phys_base, info->mem_size);
> + devm_free_irq(&pdev->dev, info->gpmc_irq_fifo, info);
> + devm_release_mem_region(&pdev->dev, info->phys_base, info->mem_size);
> out_free_info:
> #ifdef CONFIG_MTD_NAND_ECC_BCH
> if (info->nand.ecc.priv) {
> @@ -2042,7 +2046,7 @@ out_free_info:
> info->nand.ecc.priv = NULL;
> }
> #endif
> - kfree(info);
> + devm_kfree(&pdev->dev, info);
>
> return err;
> }
> @@ -2062,15 +2066,15 @@ static int omap_nand_remove(struct platform_device *pdev)
> dma_release_channel(info->dma);
>
> if (info->gpmc_irq_count > 0)
> - free_irq(info->gpmc_irq_count, info);
> + devm_free_irq(&pdev->dev, info->gpmc_irq_count, info);
> if (info->gpmc_irq_fifo > 0)
> - free_irq(info->gpmc_irq_fifo, info);
> + devm_free_irq(&pdev->dev, info->gpmc_irq_fifo, info);
>
> /* Release NAND device, its internal structures and partitions */
> nand_release(&info->mtd);
> - iounmap(info->nand.IO_ADDR_R);
> - release_mem_region(info->phys_base, info->mem_size);
> - kfree(info);
> + devm_iounmap(&pdev->dev, info->nand.IO_ADDR_R);
> + devm_release_mem_region(&pdev->dev, info->phys_base, info->mem_size);
> + devm_kfree(&pdev->dev, info);
> return 0;
> }
>
> --
> 1.8.1
>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls
2013-10-11 13:36 ` [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls Pekon Gupta
2013-10-11 15:56 ` Felipe Balbi
@ 2013-10-11 18:15 ` Brian Norris
2013-10-11 18:28 ` Tony Lindgren
1 sibling, 1 reply; 27+ messages in thread
From: Brian Norris @ 2013-10-11 18:15 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, dedekind1, Pawel.Moll,
ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree
Hi Pekon,
On Fri, Oct 11, 2013 at 07:06:43PM +0530, Pekon Gupta wrote:
> "Managed Device Resource" or devm_xx calls takes care of automatic freeing
> of the resource in case of:
> - failure during driver probe
> - failure during resource allocation
> - detaching or unloading of driver module (rmmod)
> Reference: Documentation/driver-model/devres.txt
>
> Though OMAP NAND driver handles freeing of resource allocation in most of
> the cases, but using devm_xx provides more clean and effortless approach
> to handle all such cases.
Judging by your patch, I think you missed the point of the devm_*
managed functions. They are useful because you don't need to do any of
the cleanup (kfree(), iounmap(), etc.) yourself. I'll note the changes
that are necessary below, but seeing as this is an add-on to your patch
series, I may merge the rest of series without this, and if so, you can
just resubmit this patch separately.
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
> drivers/mtd/nand/omap2.c | 38 +++++++++++++++++++++-----------------
> 1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index a783dae..0f2b0d1 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1658,7 +1658,8 @@ static int omap_nand_probe(struct platform_device *pdev)
> return -ENODEV;
> }
>
> - info = kzalloc(sizeof(struct omap_nand_info), GFP_KERNEL);
> + info = devm_kzalloc(&pdev->dev, sizeof(struct omap_nand_info),
> + GFP_KERNEL);
> if (!info)
> return -ENOMEM;
>
> @@ -1690,13 +1691,14 @@ static int omap_nand_probe(struct platform_device *pdev)
> info->phys_base = res->start;
> info->mem_size = resource_size(res);
>
> - if (!request_mem_region(info->phys_base, info->mem_size,
> - pdev->dev.driver->name)) {
> + if (!devm_request_mem_region(&pdev->dev, info->phys_base,
> + info->mem_size, pdev->dev.driver->name)) {
> err = -EBUSY;
> goto out_free_info;
> }
>
> - info->nand.IO_ADDR_R = ioremap(info->phys_base, info->mem_size);
> + info->nand.IO_ADDR_R = devm_ioremap(&pdev->dev, info->phys_base,
> + info->mem_size);
> if (!info->nand.IO_ADDR_R) {
> err = -ENOMEM;
> goto out_release_mem_region;
> @@ -1799,8 +1801,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> err = -ENODEV;
> goto out_release_mem_region;
> }
> - err = request_irq(info->gpmc_irq_fifo, omap_nand_irq,
> - IRQF_SHARED, "gpmc-nand-fifo", info);
> + err = devm_request_irq(&pdev->dev, info->gpmc_irq_fifo,
> + omap_nand_irq, IRQF_SHARED,
> + "gpmc-nand-fifo", info);
> if (err) {
> dev_err(&pdev->dev, "requesting irq(%d) error:%d",
> info->gpmc_irq_fifo, err);
> @@ -1814,8 +1817,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> err = -ENODEV;
> goto out_release_mem_region;
> }
> - err = request_irq(info->gpmc_irq_count, omap_nand_irq,
> - IRQF_SHARED, "gpmc-nand-count", info);
> + err = devm_request_irq(&pdev->dev, info->gpmc_irq_count,
> + omap_nand_irq, IRQF_SHARED,
> + "gpmc-nand-count", info);
> if (err) {
> dev_err(&pdev->dev, "requesting irq(%d) error:%d",
> info->gpmc_irq_count, err);
> @@ -2031,10 +2035,10 @@ out_release_mem_region:
> if (info->dma)
> dma_release_channel(info->dma);
> if (info->gpmc_irq_count > 0)
> - free_irq(info->gpmc_irq_count, info);
> + devm_free_irq(&pdev->dev, info->gpmc_irq_count, info);
Just drop the 'free'.
> if (info->gpmc_irq_fifo > 0)
> - free_irq(info->gpmc_irq_fifo, info);
> - release_mem_region(info->phys_base, info->mem_size);
> + devm_free_irq(&pdev->dev, info->gpmc_irq_fifo, info);
Drop the 'free'.
> + devm_release_mem_region(&pdev->dev, info->phys_base, info->mem_size);
Drop this line.
> out_free_info:
> #ifdef CONFIG_MTD_NAND_ECC_BCH
> if (info->nand.ecc.priv) {
> @@ -2042,7 +2046,7 @@ out_free_info:
> info->nand.ecc.priv = NULL;
> }
> #endif
> - kfree(info);
> + devm_kfree(&pdev->dev, info);
This line is also not needed.
So in the end, the 'gotos' and error path of your probe function will be
much simpler. You will only need to manage the info->dma DMA channel
(i.e., dma_release_channel()).
>
> return err;
> }
> @@ -2062,15 +2066,15 @@ static int omap_nand_remove(struct platform_device *pdev)
> dma_release_channel(info->dma);
>
> if (info->gpmc_irq_count > 0)
> - free_irq(info->gpmc_irq_count, info);
> + devm_free_irq(&pdev->dev, info->gpmc_irq_count, info);
This 'free' is also not needed.
> if (info->gpmc_irq_fifo > 0)
> - free_irq(info->gpmc_irq_fifo, info);
> + devm_free_irq(&pdev->dev, info->gpmc_irq_fifo, info);
Ditto.
>
> /* Release NAND device, its internal structures and partitions */
> nand_release(&info->mtd);
> - iounmap(info->nand.IO_ADDR_R);
> - release_mem_region(info->phys_base, info->mem_size);
> - kfree(info);
> + devm_iounmap(&pdev->dev, info->nand.IO_ADDR_R);
> + devm_release_mem_region(&pdev->dev, info->phys_base, info->mem_size);
> + devm_kfree(&pdev->dev, info);
Ditto for all 3.
Your 'remove' function will also be much shorter.
> return 0;
> }
>
Thanks,
Brian
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls
2013-10-11 18:15 ` Brian Norris
@ 2013-10-11 18:28 ` Tony Lindgren
2013-10-11 19:27 ` Brian Norris
0 siblings, 1 reply; 27+ messages in thread
From: Tony Lindgren @ 2013-10-11 18:28 UTC (permalink / raw)
To: Brian Norris
Cc: Pekon Gupta, mark.rutland, robherring2, olof, dedekind1,
Pawel.Moll, ijc+devicetree, swarren, dwmw2, arnd, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree
* Brian Norris <computersforpeace@gmail.com> [131011 11:23]:
> Hi Pekon,
>
> On Fri, Oct 11, 2013 at 07:06:43PM +0530, Pekon Gupta wrote:
> > "Managed Device Resource" or devm_xx calls takes care of automatic freeing
> > of the resource in case of:
> > - failure during driver probe
> > - failure during resource allocation
> > - detaching or unloading of driver module (rmmod)
> > Reference: Documentation/driver-model/devres.txt
> >
> > Though OMAP NAND driver handles freeing of resource allocation in most of
> > the cases, but using devm_xx provides more clean and effortless approach
> > to handle all such cases.
>
> Judging by your patch, I think you missed the point of the devm_*
> managed functions. They are useful because you don't need to do any of
> the cleanup (kfree(), iounmap(), etc.) yourself. I'll note the changes
> that are necessary below, but seeing as this is an add-on to your patch
> series, I may merge the rest of series without this, and if so, you can
> just resubmit this patch separately.
FYI, the .dts changes should be queued separately by Benoit to avoid
pointless merge conflicts. The arch/arm/mach-omap2/gpmc.c changes I
need to look, hopefully I can ack those for you today so you can take
the code related changes into the MTD tree.
Regards,
Tony
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls
2013-10-11 18:28 ` Tony Lindgren
@ 2013-10-11 19:27 ` Brian Norris
2013-10-11 21:46 ` Tony Lindgren
0 siblings, 1 reply; 27+ messages in thread
From: Brian Norris @ 2013-10-11 19:27 UTC (permalink / raw)
To: Tony Lindgren
Cc: Pekon Gupta, Mark Rutland, robherring2, olof@lixom.net,
Artem Bityutskiy, Pawel Moll, Ian Campbell, Stephen Warren,
David Woodhouse, Arnd Bergmann, bcousson, Avinash Philip,
Balbi, Felipe, linux-mtd@lists.infradead.org,
linux-omap@vger.kernel.org, devicetree@vger.kernel.org
On Fri, Oct 11, 2013 at 11:28 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Brian Norris <computersforpeace@gmail.com> [131011 11:23]:
>> Hi Pekon,
>>
>> On Fri, Oct 11, 2013 at 07:06:43PM +0530, Pekon Gupta wrote:
>> > "Managed Device Resource" or devm_xx calls takes care of automatic freeing
>> > of the resource in case of:
>> > - failure during driver probe
>> > - failure during resource allocation
>> > - detaching or unloading of driver module (rmmod)
>> > Reference: Documentation/driver-model/devres.txt
>> >
>> > Though OMAP NAND driver handles freeing of resource allocation in most of
>> > the cases, but using devm_xx provides more clean and effortless approach
>> > to handle all such cases.
>>
>> Judging by your patch, I think you missed the point of the devm_*
>> managed functions. They are useful because you don't need to do any of
>> the cleanup (kfree(), iounmap(), etc.) yourself. I'll note the changes
>> that are necessary below, but seeing as this is an add-on to your patch
>> series, I may merge the rest of series without this, and if so, you can
>> just resubmit this patch separately.
>
> FYI, the .dts changes should be queued separately by Benoit to avoid
> pointless merge conflicts. The arch/arm/mach-omap2/gpmc.c changes I
> need to look, hopefully I can ack those for you today so you can take
> the code related changes into the MTD tree.
Why are you replying to this patch, instead of the DTS?
Also, I don't think all of this code is ready. There are several
comments from weeks ago that Pekon hasn't addressed. It's possible the
DT binding changes can go in, but not some of the later patches, yet.
Brian
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls
2013-10-11 19:27 ` Brian Norris
@ 2013-10-11 21:46 ` Tony Lindgren
2013-10-11 22:14 ` Brian Norris
0 siblings, 1 reply; 27+ messages in thread
From: Tony Lindgren @ 2013-10-11 21:46 UTC (permalink / raw)
To: Brian Norris
Cc: Pekon Gupta, Mark Rutland, robherring2, olof@lixom.net,
Artem Bityutskiy, Pawel Moll, Ian Campbell, Stephen Warren,
David Woodhouse, Arnd Bergmann, bcousson, Avinash Philip,
Balbi, Felipe, linux-mtd@lists.infradead.org,
linux-omap@vger.kernel.org, devicetree@vger.kernel.org
* Brian Norris <computersforpeace@gmail.com> [131011 12:35]:
> On Fri, Oct 11, 2013 at 11:28 AM, Tony Lindgren <tony@atomide.com> wrote:
> >
> > FYI, the .dts changes should be queued separately by Benoit to avoid
> > pointless merge conflicts. The arch/arm/mach-omap2/gpmc.c changes I
> > need to look, hopefully I can ack those for you today so you can take
> > the code related changes into the MTD tree.
>
> Why are you replying to this patch, instead of the DTS?
Because you said you might merrge some parts of the series and I've
seen many merge cycles of pointless merge conflicts with driver
maintainers merging .dts files along with the driver changes? :)
Pekon, can please post the .dts changes entirely separately from
the driver changes in the future?
> Also, I don't think all of this code is ready. There are several
> comments from weeks ago that Pekon hasn't addressed. It's possible the
> DT binding changes can go in, but not some of the later patches, yet.
Yes that's up to you. I have not looked at the MTD parts and I
appreciate the fact that you are reviewing that stuff. I've acked
the arch/arm/*omap* parts so hopefully I'm out of the loop now.
Regards,
Tony
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls
2013-10-11 21:46 ` Tony Lindgren
@ 2013-10-11 22:14 ` Brian Norris
0 siblings, 0 replies; 27+ messages in thread
From: Brian Norris @ 2013-10-11 22:14 UTC (permalink / raw)
To: Tony Lindgren
Cc: Pekon Gupta, Mark Rutland, Rob Herring, olof@lixom.net,
Artem Bityutskiy, Pawel Moll, Ian Campbell, Stephen Warren,
David Woodhouse, Arnd Bergmann, Benoit Cousson, Avinash Philip,
Balbi, Felipe, linux-mtd@lists.infradead.org,
linux-omap@vger.kernel.org, devicetree@vger.kernel.org
On Fri, Oct 11, 2013 at 2:46 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Brian Norris <computersforpeace@gmail.com> [131011 12:35]:
>> On Fri, Oct 11, 2013 at 11:28 AM, Tony Lindgren <tony@atomide.com> wrote:
>> >
>> > FYI, the .dts changes should be queued separately by Benoit to avoid
>> > pointless merge conflicts. The arch/arm/mach-omap2/gpmc.c changes I
>> > need to look, hopefully I can ack those for you today so you can take
>> > the code related changes into the MTD tree.
>>
>> Why are you replying to this patch, instead of the DTS?
>
> Because you said you might merrge some parts of the series and I've
> seen many merge cycles of pointless merge conflicts with driver
> maintainers merging .dts files along with the driver changes? :)
Sure, thanks for pointing this out. I may have fallen into that myself.
> Pekon, can please post the .dts changes entirely separately from
> the driver changes in the future?
>
>> Also, I don't think all of this code is ready. There are several
>> comments from weeks ago that Pekon hasn't addressed. It's possible the
>> DT binding changes can go in, but not some of the later patches, yet.
>
> Yes that's up to you. I have not looked at the MTD parts and I
> appreciate the fact that you are reviewing that stuff. I've acked
> the arch/arm/*omap* parts so hopefully I'm out of the loop now.
Yes, I think so. The MTD stuff is the only remaining weak point I see.
Brian
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes
2013-10-11 13:36 [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Pekon Gupta
` (5 preceding siblings ...)
2013-10-11 13:36 ` [PATCH v8 6/6] mtd: nand: omap: updated devm_xx for all resource allocation and free calls Pekon Gupta
@ 2013-10-11 21:09 ` Brian Norris
2013-10-12 22:26 ` Gupta, Pekon
6 siblings, 1 reply; 27+ messages in thread
From: Brian Norris @ 2013-10-11 21:09 UTC (permalink / raw)
To: Pekon Gupta
Cc: mark.rutland, robherring2, olof, dedekind1, Pawel.Moll,
ijc+devicetree, swarren, dwmw2, arnd, tony, bcousson,
avinashphilipk, balbi, linux-mtd, linux-omap, devicetree
Hi Pekon,
I will try to summarize the standing of your patch series.
Patches 1 and 2 look good and have addressed all of the DT maintainers'
comments, AFAICT. They are ready to go in, except that the following
patches are not ready; they should probably go in together.
You ignored most of my comments to patch 3, and it is insufficiently
documented. Please take a look at my comments, both here (in v8) and in
v6. It still should be split into more patches.
Patch 4 does too much without describing it. Why are you dropping the
old omap3_correct_data_bch() code? Is this just refactoring? If so, you
should say so. And this also suggests that you have two logical changes
going on that should be separated into different patches; one to
refactor the open-coded NAND/BCH library to replace it with the
nand_bch.{c,h} support library and one for the new ECC modes.
Patch 5 is good but should wait until the other DT parts are acceptable
and merged into MTD.
Patch 6 needs rewriting to use devm_* functions properly, but it was
never integral to this patch series. You can improve it and resend with
this series or just send it separately afterward.
On Fri, Oct 11, 2013 at 07:06:37PM +0530, Pekon Gupta wrote:
> *Changes v7 -> v8*
> [PATCH 1/6] <no updates>
> [PATCH 2/6]
> - updated DT parsing of "ti,nand-ecc-opts" so that its "ham1" remains
> compatible to "sw","hw","hw-romcode"
> - updated DT parsing of "ti,elm-id" to retain compatibility to "elm_id"
> - using of_parse_phandle() to get ELM device pointer from DT
> [PATCH 3..6/6] <commit log updates>
>
>
> *Changes v6 -> v7*
> [PATCH 1/6] <NEW> split from [PATCH v6 2/4] as per feedbacks of Brain Norris
> [PATCH 2/6] incorporated feedbacks from DT maintainers
> [PATCH 3/6] patch cleaning and incorporated feedbacks from Brain Norris
You did not incorporate most of my feedback here. Also, my name is
'Brian' not 'Brain', although I'm flattered ;)
> [PATCH 4/6] rebasing changes and cleanup
> [PATCH 5/6] updated omap3430-sdp.dts
> [PATCH 6/6] <NEW> updated for devm_xx
...
> After this patch series, omap2-nand driver will supports following ECC schemes:
> +---------------------------------------+---------------+---------------+
> | ECC scheme |ECC calculation|Error detection|
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_HAM1_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) |
> +---------------------------------------+---------------+---------------+
>
> 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 error detection done in software
> - Kconfig:CONFIG_MTD_NAND_OMAP_BCH error detection done by h/w engine
>
>
> Pekon Gupta (6):
> mtd: nand: omap: combine different flavours of 1-bit hamming ecc
> schemes
> 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
> mtd: nand: omap: updated devm_xx for all resource allocation and free
> calls
>
> .../devicetree/bindings/mtd/gpmc-nand.txt | 16 +-
> arch/arm/boot/dts/am335x-evm.dts | 3 +-
> arch/arm/boot/dts/omap3430-sdp.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 | 569 ++++++++++-----------
> include/linux/platform_data/mtd-nand-omap2.h | 18 +-
> 8 files changed, 333 insertions(+), 354 deletions(-)
Brian
^ permalink raw reply [flat|nested] 27+ messages in thread* RE: [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes
2013-10-11 21:09 ` [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes Brian Norris
@ 2013-10-12 22:26 ` Gupta, Pekon
2013-10-13 1:40 ` Brian Norris
0 siblings, 1 reply; 27+ messages in thread
From: Gupta, Pekon @ 2013-10-12 22:26 UTC (permalink / raw)
To: Brian Norris
Cc: mark.rutland@arm.com, robherring2@gmail.com, olof@lixom.net,
dedekind1@gmail.com, Pawel.Moll@arm.com,
ijc+devicetree@hellion.org.uk, swarren@wwwdotorg.org,
dwmw2@infradead.org, arnd@arndb.de, tony@atomide.com,
bcousson@baylibre.com, avinashphilipk@gmail.com, Balbi, Felipe,
linux-mtd@lists.infradead.org, linux-omap@vger.kernel.org,
devicetree@vger.kernel.org
Hi Brain,
>
> Hi Pekon,
>
> I will try to summarize the standing of your patch series.
>
> Patches 1 and 2 look good and have addressed all of the DT maintainers'
> comments, AFAICT. They are ready to go in, except that the following
> patches are not ready; they should probably go in together.
>
> You ignored most of my comments to patch 3, and it is insufficiently
> documented. Please take a look at my comments, both here (in v8) and in
> v6. It still should be split into more patches.
>
I tried splitting the earlier [PATCH 3/6], therefore a new patch for merging
various Hamming ECC schemes was spawned. But, I could not clean more
because I would have broken the NAND functionality in between the
patches.
My apologies, I missed some of you other comments, so I'm preparing
next version by splitting [PATCH 3/6] into more sub-patches for ease of
review.
Most #ifdef were put to suppress warning of un-used functions during
compile time. So those cannot be removed, otherwise this patch would
give compile warnings under randconfig.
> Patch 4 does too much without describing it. Why are you dropping the
> old omap3_correct_data_bch() code? Is this just refactoring? If so, you
> should say so. And this also suggests that you have two logical changes
> going on that should be separated into different patches; one to
> refactor the open-coded NAND/BCH library to replace it with the
> nand_bch.{c,h} support library and one for the new ECC modes.
>
Agreed, I update commit log to be more explicit that here nand_bch.c
actually encapsulates lib/bch.c.
Here also I cannot remove #ifdef across nand_bch_free() because
it frees some bch control data, which would not be defined for all
ecc-schemes (it is specific to xx_SW ecc schemes). Hence removing
#ifdef here would give null-pointer exception.
> Patch 5 is good but should wait until the other DT parts are acceptable
> and merged into MTD.
>
> Patch 6 needs rewriting to use devm_* functions properly, but it was
> never integral to this patch series. You can improve it and resend with
> this series or just send it separately afterward.
>
Yes I understood this, but I think it would be still good to explicitly
free the resources in case of "module_remove()", because that would
free all resources instantaneously without waiting for devm_ to
iterate thru the list when it wakes-up (I guess).
I'm not knowledgeable of exact implementation of devm_ and how
often does it iterates the list and frees the resources for corresponding
un-registered devices. So trusting you I'll include your feedbacks here.
with regards, pekon
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes
2013-10-12 22:26 ` Gupta, Pekon
@ 2013-10-13 1:40 ` Brian Norris
0 siblings, 0 replies; 27+ messages in thread
From: Brian Norris @ 2013-10-13 1:40 UTC (permalink / raw)
To: Gupta, Pekon
Cc: mark.rutland@arm.com, robherring2@gmail.com, olof@lixom.net,
dedekind1@gmail.com, Pawel.Moll@arm.com,
ijc+devicetree@hellion.org.uk, swarren@wwwdotorg.org,
dwmw2@infradead.org, arnd@arndb.de, tony@atomide.com,
bcousson@baylibre.com, avinashphilipk@gmail.com, Balbi, Felipe,
linux-mtd@lists.infradead.org, linux-omap@vger.kernel.org,
devicetree@vger.kernel.org
Hi Pekon,
On 10/12/2013 03:26 PM, Gupta, Pekon wrote:
>> I will try to summarize the standing of your patch series.
>>
>> Patches 1 and 2 look good and have addressed all of the DT maintainers'
>> comments, AFAICT. They are ready to go in, except that the following
>> patches are not ready; they should probably go in together.
>>
>> You ignored most of my comments to patch 3, and it is insufficiently
>> documented. Please take a look at my comments, both here (in v8) and in
>> v6. It still should be split into more patches.
>>
> I tried splitting the earlier [PATCH 3/6], therefore a new patch for merging
> various Hamming ECC schemes was spawned. But, I could not clean more
> because I would have broken the NAND functionality in between the
> patches.
From what I understand, your comment "cleans redundant code" in patch 3
is entirely its own logical change; you're replacing the SW ECC in
omap2.c with the library wrapper in lib/bch.c, right? So this should be
described as such (as "replacing XXX with YYY") and can easily be its
own patch?
Also, you add an 'mtd' and a 'nand_chip' (can you just call this 'chip'
or maybe 'nand', like in other drivers?) variable to the probe function,
and then partially convert the function over to inconsistently using mtd
vs. info->mtd and nand_chip vs. info->nand. If you want to add these
variables, just make it a separate patch.
I also mentioned your changes to the buswidth code, which seem to have
snuck in again. This should be a separate patch.
> My apologies, I missed some of you other comments, so I'm preparing
> next version by splitting [PATCH 3/6] into more sub-patches for ease of
> review.
Great, thanks.
> Most #ifdef were put to suppress warning of un-used functions during
> compile time. So those cannot be removed, otherwise this patch would
> give compile warnings under randconfig.
Could be marked __maybe_unused instead? #ifdef's are also OK, if
necessary. There were just a few places I pointed out that they was
certainly not needed.
>> Patch 4 does too much without describing it. Why are you dropping the
>> old omap3_correct_data_bch() code? Is this just refactoring? If so, you
>> should say so. And this also suggests that you have two logical changes
>> going on that should be separated into different patches; one to
>> refactor the open-coded NAND/BCH library to replace it with the
>> nand_bch.{c,h} support library and one for the new ECC modes.
>>
> Agreed, I update commit log to be more explicit that here nand_bch.c
> actually encapsulates lib/bch.c.
> Here also I cannot remove #ifdef across nand_bch_free() because
> it frees some bch control data, which would not be defined for all
> ecc-schemes (it is specific to xx_SW ecc schemes). Hence removing
> #ifdef here would give null-pointer exception.
Note that nand_bch_free() (and many others in nand_bch.h) are given
empty inline implementation for CONFIG_MTD_NAND_ECC_BCH=n. But if you
are unsure, then you can leave the #ifdef's to be conservative.
>> Patch 5 is good but should wait until the other DT parts are acceptable
>> and merged into MTD.
>>
>> Patch 6 needs rewriting to use devm_* functions properly, but it was
>> never integral to this patch series. You can improve it and resend with
>> this series or just send it separately afterward.
>>
> Yes I understood this, but I think it would be still good to explicitly
> free the resources in case of "module_remove()", because that would
> free all resources instantaneously without waiting for devm_ to
> iterate thru the list when it wakes-up (I guess).
devm_* is part of the driver core, so it does the cleanup immediately on
a non-zero return from the probe() callback, or after exit from the
remove() callback. It does not have to "wake up". Its only overhead is
the small amount of memory used for the linked-list management.
Again, this is an optional piece of work, so don't worry about it too
much. But you get 0 benefit (just a slight increase in memory usage) if
you just convert all the kfree() calls to devm_kfree().
> I'm not knowledgeable of exact implementation of devm_ and how
> often does it iterates the list and frees the resources for corresponding
> un-registered devices. So trusting you I'll include your feedbacks here.
I haven't closely reviewed 100% of its implementation, but it is fairly
straightforward and commonly used in many drivers. You should be fine
using it as documented and recommended by me.
Thanks,
Brian
^ permalink raw reply [flat|nested] 27+ messages in thread