Patchwork [v7,2/6] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

login
register
mail settings
Submitter pekon gupta
Date Oct. 4, 2013, 7:49 p.m.
Message ID <1380916188-24206-3-git-send-email-pekon@ti.com>
Download mbox | patch
Permalink /patch/280723/
State New
Headers show

Comments

pekon gupta - Oct. 4, 2013, 7:49 p.m.
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                         | 47 ++++++++++++++++------
 include/linux/platform_data/mtd-nand-omap2.h       | 14 +++++--
 3 files changed, 52 insertions(+), 17 deletions(-)
Mark Rutland - Oct. 7, 2013, 11:35 a.m.
On Fri, Oct 04, 2013 at 08:49:44PM +0100, 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>
> ---
>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  8 +++-
>  arch/arm/mach-omap2/gpmc.c                         | 47 ++++++++++++++++------
>  include/linux/platform_data/mtd-nand-omap2.h       | 14 +++++--
>  3 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> index 25ee232..7785666 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.

s/pHandle/phandle/

> +		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 1c45b72..5a607fa 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",
> @@ -1361,6 +1355,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>  	const char *s;
>  	struct gpmc_timings gpmc_t;
>  	struct omap_nand_platform_data *gpmc_nand_data;
> +	const __be32 *phandle;

I don't think you need this. With of_parse_phandle you should never need
to see the raw phandle value.

> +	int lenp;
>  
>  	if (of_property_read_u32(child, "reg", &val) < 0) {
>  		dev_err(&pdev->dev, "%s has no 'reg' property\n",
> @@ -1376,12 +1372,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 */
> +	phandle = of_get_property(child, "ti,elm-id", &lenp);
> +	if ((phandle == NULL) || (lenp != sizeof(void *))) {
> +		pr_warn("%s: ti,elm-id property not found\n", __func__);
> +		gpmc_nand_data->elm_of_node = NULL;
> +	} else {
> +		gpmc_nand_data->elm_of_node =
> +				of_find_node_by_phandle(be32_to_cpup(phandle));
> +	}

Use of_parse_handle rather than open-coding it:

	gpmc_nand_data->elm_of_node = 
		of_parse_phandle(child, "ti,elm-id", 0);

Was "elm_id" ever handled previously? I don't see any code handling it
being remove or modified.

Thanks,
Mark.
pekon gupta - Oct. 7, 2013, 12:18 p.m.
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> > On Fri, Oct 04, 2013 at 08:49:44PM +0100, Pekon Gupta wrote:

 [snip]

> >
> > diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-
> omap2/gpmc.c
> > index 1c45b72..5a607fa 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",
> > @@ -1361,6 +1355,8 @@ static int gpmc_probe_nand_child(struct
> platform_device *pdev,
> >  	const char *s;
> >  	struct gpmc_timings gpmc_t;
> >  	struct omap_nand_platform_data *gpmc_nand_data;
> > +	const __be32 *phandle;
> 
> I don't think you need this. With of_parse_phandle you should never need
> to see the raw phandle value.
> 
> > +	int lenp;
> >
> >  	if (of_property_read_u32(child, "reg", &val) < 0) {
> >  		dev_err(&pdev->dev, "%s has no 'reg' property\n",
> > @@ -1376,12 +1372,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 */
> > +	phandle = of_get_property(child, "ti,elm-id", &lenp);
> > +	if ((phandle == NULL) || (lenp != sizeof(void *))) {
> > +		pr_warn("%s: ti,elm-id property not found\n", __func__);
> > +		gpmc_nand_data->elm_of_node = NULL;
> > +	} else {
> > +		gpmc_nand_data->elm_of_node =
> > +
> 	of_find_node_by_phandle(be32_to_cpup(phandle));
> > +	}
> 
> Use of_parse_handle rather than open-coding it:
> 
> 	gpmc_nand_data->elm_of_node =
> 		of_parse_phandle(child, "ti,elm-id", 0);
> 
> Was "elm_id" ever handled previously? I don't see any code handling it
> being remove or modified.
> 
Yes, this piece of code has been moved from omap2.c (nand-driver)
to gpmc.c (GPMC driver), so as to perform all DT related parsing
at single place.  
This change was needed as per feedbacks from Olof to simply
DT bindings so that they include only H/W related stuff.

Please refer omap2.c: omap3_init_bch() in [PATCH 3/6] of same series
where 'elm_id' DT parsing was original present.
-	/* 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;



with regards, pekon

Patch

diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index 25ee232..7785666 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 1c45b72..5a607fa 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",
@@ -1361,6 +1355,8 @@  static int gpmc_probe_nand_child(struct platform_device *pdev,
 	const char *s;
 	struct gpmc_timings gpmc_t;
 	struct omap_nand_platform_data *gpmc_nand_data;
+	const __be32 *phandle;
+	int lenp;
 
 	if (of_property_read_u32(child, "reg", &val) < 0) {
 		dev_err(&pdev->dev, "%s has no 'reg' property\n",
@@ -1376,12 +1372,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 */
+	phandle = of_get_property(child, "ti,elm-id", &lenp);
+	if ((phandle == NULL) || (lenp != sizeof(void *))) {
+		pr_warn("%s: ti,elm-id property not found\n", __func__);
+		gpmc_nand_data->elm_of_node = NULL;
+	} else {
+		gpmc_nand_data->elm_of_node =
+				of_find_node_by_phandle(be32_to_cpup(phandle));
+	}
+	/* 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"))
+		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