[PATCH] mtd: gpmi: add gpmi_devdata{} to simplify the code

[PATCH] mtd: gpmi: add gpmi_devdata{} to simplify the code

[Huang Shijie]

More and more chips use the GPMI controller, but these chips may use different
version of the IPs for GPMI and BCH. Different IPs have
 different features, such as the BCH's maximum ECC strength:

     imx23/imx28 -- the BCH's maximum ECC strength is 20
     imx6q       -- the BCH's maximum ECC strength is 40
     imx6sx      -- the BCH's maximum ECC strength is 62

This patch does the following things:

  [1] add a new data structure, gpmi_devdata{}, to store the information for
      each IP. Besides the IP version, we store the following information:
         <1> BCH's maximum ECC strength.
         <2> the maximum chain delay in ns used by the EDO mode.

      but we may add more information in future.

  [2] add the gpmi_devdata_imx{23|28|6q} to replace the gpmi_ids.

  [3] simplify the code by using the ECC strength from gpmi_devdata, such as
      gpmi_check_ecc() and legacy_set_geometry();

  [4] use the maximum chain delay to initialize the EDO mode,
      see gpmi_compute_edo_timing().

  [5] rewrite the macros, such GPMI_IS_MX{23|28|6Q}.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |    5 +--
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   53 +++++++++++++++++---------------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   28 ++++++++++------
 3 files changed, 46 insertions(+), 40 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index dd1df60..ec4db2a 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -861,7 +861,7 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
 	struct resources *r = &this->resources;
 	unsigned long rate = clk_get_rate(r->clock[0]);
 	int mode = this->timing_mode;
-	int dll_threshold = 16; /* in ns */
+	int dll_threshold = this->devdata->max_chain_delay;
 	unsigned long delay;
 	unsigned long clk_period;
 	int t_rea;
@@ -886,9 +886,6 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
 	/* [3] for GPMI_HW_GPMI_CTRL1 */
 	hw->wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
 
-	if (GPMI_IS_MX6Q(this))
-		dll_threshold = 12;
-
 	/*
 	 * Enlarge 10 times for the numerator and denominator in {3}.
 	 * This make us to get more accurate result.
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index bb77f75..e88d64e 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -53,6 +53,24 @@ static struct nand_ecclayout gpmi_hw_ecclayout = {
 	.oobfree = { {.offset = 0, .length = 0} }
 };
 
+static const struct gpmi_devdata gpmi_devdata_imx23 = {
+	.type = IS_MX23,
+	.bch_max_ecc_strength = 20,
+	.max_chain_delay = 16,
+};
+
+static const struct gpmi_devdata gpmi_devdata_imx28 = {
+	.type = IS_MX28,
+	.bch_max_ecc_strength = 20,
+	.max_chain_delay = 16,
+};
+
+static const struct gpmi_devdata gpmi_devdata_imx6q = {
+	.type = IS_MX6Q,
+	.bch_max_ecc_strength = 40,
+	.max_chain_delay = 12,
+};
+
 static irqreturn_t bch_irq(int irq, void *cookie)
 {
 	struct gpmi_nand_data *this = cookie;
@@ -102,14 +120,8 @@ static inline bool gpmi_check_ecc(struct gpmi_nand_data *this)
 		/* The mx23/mx28 only support the GF13. */
 		if (geo->gf_len == 14)
 			return false;
-
-		if (geo->ecc_strength > MXS_ECC_STRENGTH_MAX)
-			return false;
-	} else if (GPMI_IS_MX6Q(this)) {
-		if (geo->ecc_strength > MX6_ECC_STRENGTH_MAX)
-			return false;
 	}
-	return true;
+	return geo->ecc_strength <= this->devdata->bch_max_ecc_strength;
 }
 
 /*
@@ -270,8 +282,7 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
 			"We can not support this nand chip."
 			" Its required ecc strength(%d) is beyond our"
 			" capability(%d).\n", geo->ecc_strength,
-			(GPMI_IS_MX6Q(this) ? MX6_ECC_STRENGTH_MAX
-					: MXS_ECC_STRENGTH_MAX));
+			this->devdata->bch_max_ecc_strength);
 		return -EINVAL;
 	}
 
@@ -1740,23 +1751,16 @@ err_out:
 	return ret;
 }
 
-static const struct platform_device_id gpmi_ids[] = {
-	{ .name = "imx23-gpmi-nand", .driver_data = IS_MX23, },
-	{ .name = "imx28-gpmi-nand", .driver_data = IS_MX28, },
-	{ .name = "imx6q-gpmi-nand", .driver_data = IS_MX6Q, },
-	{}
-};
-
 static const struct of_device_id gpmi_nand_id_table[] = {
 	{
 		.compatible = "fsl,imx23-gpmi-nand",
-		.data = (void *)&gpmi_ids[IS_MX23],
+		.data = (void *)&gpmi_devdata_imx23,
 	}, {
 		.compatible = "fsl,imx28-gpmi-nand",
-		.data = (void *)&gpmi_ids[IS_MX28],
+		.data = (void *)&gpmi_devdata_imx28,
 	}, {
 		.compatible = "fsl,imx6q-gpmi-nand",
-		.data = (void *)&gpmi_ids[IS_MX6Q],
+		.data = (void *)&gpmi_devdata_imx6q,
 	}, {}
 };
 MODULE_DEVICE_TABLE(of, gpmi_nand_id_table);
@@ -1767,18 +1771,18 @@ static int gpmi_nand_probe(struct platform_device *pdev)
 	const struct of_device_id *of_id;
 	int ret;
 
+	this = devm_kzalloc(&pdev->dev, sizeof(*this), GFP_KERNEL);
+	if (!this)
+		return -ENOMEM;
+
 	of_id = of_match_device(gpmi_nand_id_table, &pdev->dev);
 	if (of_id) {
-		pdev->id_entry = of_id->data;
+		this->devdata = of_id->data;
 	} else {
 		dev_err(&pdev->dev, "Failed to find the right device id.\n");
 		return -ENODEV;
 	}
 
-	this = devm_kzalloc(&pdev->dev, sizeof(*this), GFP_KERNEL);
-	if (!this)
-		return -ENOMEM;
-
 	platform_set_drvdata(pdev, this);
 	this->pdev  = pdev;
 	this->dev   = &pdev->dev;
@@ -1823,7 +1827,6 @@ static struct platform_driver gpmi_nand_driver = {
 	},
 	.probe   = gpmi_nand_probe,
 	.remove  = gpmi_nand_remove,
-	.id_table = gpmi_ids,
 };
 module_platform_driver(gpmi_nand_driver);
 
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index 4c801fa..7904e83 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -119,11 +119,24 @@ struct nand_timing {
 	int8_t  tRHOH_in_ns;
 };
 
+enum gpmi_type {
+	IS_MX23,
+	IS_MX28,
+	IS_MX6Q
+};
+
+struct gpmi_devdata {
+	enum gpmi_type type;
+	int bch_max_ecc_strength;
+	int max_chain_delay; /* See the async EDO mode */
+};
+
 struct gpmi_nand_data {
 	/* flags */
 #define GPMI_ASYNC_EDO_ENABLED	(1 << 0)
 #define GPMI_TIMING_INIT_OK	(1 << 1)
 	int			flags;
+	const struct gpmi_devdata *devdata;
 
 	/* System Interface */
 	struct device		*dev;
@@ -281,15 +294,8 @@ extern int gpmi_read_page(struct gpmi_nand_data *,
 #define STATUS_ERASED		0xff
 #define STATUS_UNCORRECTABLE	0xfe
 
-/* BCH's bit correction capability. */
-#define MXS_ECC_STRENGTH_MAX	20	/* mx23 and mx28 */
-#define MX6_ECC_STRENGTH_MAX	40
-
-/* Use the platform_id to distinguish different Archs. */
-#define IS_MX23			0x0
-#define IS_MX28			0x1
-#define IS_MX6Q			0x2
-#define GPMI_IS_MX23(x)		((x)->pdev->id_entry->driver_data == IS_MX23)
-#define GPMI_IS_MX28(x)		((x)->pdev->id_entry->driver_data == IS_MX28)
-#define GPMI_IS_MX6Q(x)		((x)->pdev->id_entry->driver_data == IS_MX6Q)
+/* Use the devdata to distinguish different Archs. */
+#define GPMI_IS_MX23(x)		((x)->devdata->type == IS_MX23)
+#define GPMI_IS_MX28(x)		((x)->devdata->type == IS_MX28)
+#define GPMI_IS_MX6Q(x)		((x)->devdata->type == IS_MX6Q)
 #endif
-- 
1.7.3.2

RE: [PATCH] mtd: gpmi: add gpmi_devdata{} to simplify the code

[Gupta, Pekon]

>From: Huang Shijie
>
>More and more chips use the GPMI controller, but these chips may use different
>version of the IPs for GPMI and BCH. Different IPs have
> different features, such as the BCH's maximum ECC strength:
>
>     imx23/imx28 -- the BCH's maximum ECC strength is 20
>     imx6q       -- the BCH's maximum ECC strength is 40
>     imx6sx      -- the BCH's maximum ECC strength is 62
>
>This patch does the following things:
>
>  [1] add a new data structure, gpmi_devdata{}, to store the information for
>      each IP.
Please see if you can use existing DT binding and framework for all these.


> Besides the IP version, we store the following information:
>         <1> BCH's maximum ECC strength.
>         <2> the maximum chain delay in ns used by the EDO mode.
>
>      but we may add more information in future.
>
Ezequiel has got approval for 2 new generic MTD bindings (refer l2-mtd.git)
	commit 8dd49165ef5f46b5ad9ba296c559ccff315f9421
	mtd: nand: Add a devicetree binding for ECC strength and ECC step size
So, I think <1> can be replaced by "nand-ecc-strength".


>
>  [2] add the gpmi_devdata_imx{23|28|6q} to replace the gpmi_ids.
>
Can you use "compatible" string in DT for this ?


>  [3] simplify the code by using the ECC strength from gpmi_devdata, such as
>      gpmi_check_ecc() and legacy_set_geometry();
>
>  [4] use the maximum chain delay to initialize the EDO mode,
>      see gpmi_compute_edo_timing().
>
>  [5] rewrite the macros, such GPMI_IS_MX{23|28|6Q}.
>
>Signed-off-by: Huang Shijie <b32955@freescale.com>
>---

with regards, pekon

Re: [PATCH] mtd: gpmi: add gpmi_devdata{} to simplify the code

[Huang Shijie]

于 2014年03月21日 18:29, Gupta, Pekon 写道:
> Ezequiel has got approval for 2 new generic MTD bindings (refer l2-mtd.git)
> 	commit 8dd49165ef5f46b5ad9ba296c559ccff315f9421
> 	mtd: nand: Add a devicetree binding for ECC strength and ECC step size
> So, I think<1>  can be replaced by "nand-ecc-strength".
>
>
"nand-ecc-strength" is add the ECC strength for the NAND, but my patch 
is add
the maximum ECC strength the BCH controller can support.

they are different things.
>> >
>> >    [2] add the gpmi_devdata_imx{23|28|6q} to replace the gpmi_ids.
>> >
> Can you use "compatible" string in DT for this ?
>
>
sorry, could you please give me an example?

I do not know what's your meaning. :(

thanks
Huang Shijie

RE: [PATCH] mtd: gpmi: add gpmi_devdata{} to simplify the code

[Gupta, Pekon]

>From: Huang Shijie [mailto:b32955@freescale.com]
>于 2014年03月21日 18:29, Gupta, Pekon 写道:
>> Ezequiel has got approval for 2 new generic MTD bindings (refer l2-mtd.git)
>> 	commit 8dd49165ef5f46b5ad9ba296c559ccff315f9421
>> 	mtd: nand: Add a devicetree binding for ECC strength and ECC step size
>> So, I think<1>  can be replaced by "nand-ecc-strength".
>>
>>
>"nand-ecc-strength" is add the ECC strength for the NAND, but my patch
>is add
>the maximum ECC strength the BCH controller can support.
>
>they are different things.
>
Ok then it's also tied to your IP / SoC revision right ?

>>> >
>>> >    [2] add the gpmi_devdata_imx{23|28|6q} to replace the gpmi_ids.
>>> >
>> Can you use "compatible" string in DT for this ?
>>
>>
>sorry, could you please give me an example?
>
>I do not know what's your meaning. :(
>

Can you use something like this, instead of populating static for each chip.

if (of_device_is_compatible(of_node, "imx23") || of_device_is_compatible(child, "imx28")) {
	bch_max_ecc_strength = 20;
	max_chain_delay = 16;
} elseif (of_device_is_compatible(of_node, "imx6q") {
	bch_max_ecc_strength = 40;
	max_chain_delay = 12;
} elseif (of_device_is_compatible(of_node, "imx6sx") {
	bch_max_ecc_strength = 40;
	max_chain_delay = 0; /* whatever is here */
} else {
	bch_max_ecc_strength = 1; /* whatever is default */
	max_chain_delay = 0; /* whatever is default */
}


with regards, pekon

Re: [PATCH] mtd: gpmi: add gpmi_devdata{} to simplify the code

[Ezequiel Garcia]

On Mar 21, Gupta, Pekon wrote:
> >From: Huang Shijie [mailto:b32955@freescale.com]
[..[
> >> Can you use "compatible" string in DT for this ?
> >>
> >>
> >sorry, could you please give me an example?
> >
> >I do not know what's your meaning. :(
> >
> 
> Can you use something like this, instead of populating static for each chip.
> 
> if (of_device_is_compatible(of_node, "imx23") || of_device_is_compatible(child, "imx28")) {
> 	bch_max_ecc_strength = 20;
> 	max_chain_delay = 16;
> } elseif (of_device_is_compatible(of_node, "imx6q") {
> 	bch_max_ecc_strength = 40;
> 	max_chain_delay = 12;
> } elseif (of_device_is_compatible(of_node, "imx6sx") {
> 	bch_max_ecc_strength = 40;
> 	max_chain_delay = 0; /* whatever is here */
> } else {
> 	bch_max_ecc_strength = 1; /* whatever is default */
> 	max_chain_delay = 0; /* whatever is default */
> }
> 

IMHO, this is much more expensive, harder to maintain and less readable.

Using the compatible string to match a compatible data, and putting IP-specific
data in there (just like this patch is doing), seems like a much nicer approach.
FWIW, we are using the same trick on a few mvebu drivers.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH] mtd: gpmi: add gpmi_devdata{} to simplify the code

[Huang Shijie]

On Fri, Mar 21, 2014 at 11:15:21AM +0000, Gupta, Pekon wrote:
> >From: Huang Shijie [mailto:b32955@freescale.com]
> >于 2014年03月21日 18:29, Gupta, Pekon 写道:
> >> Ezequiel has got approval for 2 new generic MTD bindings (refer l2-mtd.git)
> >> 	commit 8dd49165ef5f46b5ad9ba296c559ccff315f9421
> >> 	mtd: nand: Add a devicetree binding for ECC strength and ECC step size
> >> So, I think<1>  can be replaced by "nand-ecc-strength".
> >>
> >>
> >"nand-ecc-strength" is add the ECC strength for the NAND, but my patch
> >is add
> >the maximum ECC strength the BCH controller can support.
> >
> >they are different things.
> >
> Ok then it's also tied to your IP / SoC revision right ?

yes.
> 
> >>> >
> >>> >    [2] add the gpmi_devdata_imx{23|28|6q} to replace the gpmi_ids.
> >>> >
> >> Can you use "compatible" string in DT for this ?
> >>
> >>
> >sorry, could you please give me an example?
> >
> >I do not know what's your meaning. :(
> >
> 
> Can you use something like this, instead of populating static for each chip.
> 
> if (of_device_is_compatible(of_node, "imx23") || of_device_is_compatible(child, "imx28")) {
> 	bch_max_ecc_strength = 20;
> 	max_chain_delay = 16;
> } elseif (of_device_is_compatible(of_node, "imx6q") {
> 	bch_max_ecc_strength = 40;
> 	max_chain_delay = 12;
> } elseif (of_device_is_compatible(of_node, "imx6sx") {
> 	bch_max_ecc_strength = 40;
> 	max_chain_delay = 0; /* whatever is here */
> } else {
> 	bch_max_ecc_strength = 1; /* whatever is default */
> 	max_chain_delay = 0; /* whatever is default */
> }
> 

thanks Pekon. But as Ezequiel said, i also think that this patch is more readable.

Thanks again
Huang Shijie

Re: [PATCH] mtd: gpmi: add gpmi_devdata{} to simplify the code

[Brian Norris]

On Fri, Mar 21, 2014 at 06:19:39PM +0800, Huang Shijie wrote:
> More and more chips use the GPMI controller, but these chips may use different
> version of the IPs for GPMI and BCH. Different IPs have
>  different features, such as the BCH's maximum ECC strength:
> 
>      imx23/imx28 -- the BCH's maximum ECC strength is 20
>      imx6q       -- the BCH's maximum ECC strength is 40
>      imx6sx      -- the BCH's maximum ECC strength is 62
> 
> This patch does the following things:
> 
>   [1] add a new data structure, gpmi_devdata{}, to store the information for
>       each IP. Besides the IP version, we store the following information:
>          <1> BCH's maximum ECC strength.
>          <2> the maximum chain delay in ns used by the EDO mode.
> 
>       but we may add more information in future.
> 
>   [2] add the gpmi_devdata_imx{23|28|6q} to replace the gpmi_ids.
> 
>   [3] simplify the code by using the ECC strength from gpmi_devdata, such as
>       gpmi_check_ecc() and legacy_set_geometry();
> 
>   [4] use the maximum chain delay to initialize the EDO mode,
>       see gpmi_compute_edo_timing().
> 
>   [5] rewrite the macros, such GPMI_IS_MX{23|28|6Q}.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>

Pushed to l2-mtd.git. Thanks!

Brian