Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties | linux-devicetree

(off-list ancestor, not in this archive)

----- Original Message -----
From: "Brian Norris" <computersforpeace@gmail.com>
Sent: Friday, January 23, 2015 2:30:26 AM

On Wed, Jan 14, 2015 at 05:41:49PM -0600, Aaron Sierra wrote:
From: Jordan Friendshuh <redacted>

Support the generic nand-ecc-mode, nand-ecc-strength, and
nand-ecc-step-size device-tree properties with the Freescale UPM NAND
driver.

This patch preserves the default software ECC mode while adding the
ability to use BCH ECC for larger NAND devices.

Signed-off-by: Jordan Friendshuh <redacted>
Signed-off-by: Aaron Sierra <redacted>
---
v2:
    * Now using ECC mode and strength helpers from of_mtd.h
    * ECC mode and strength checking is more robust
v3 (resent due to [PATCH 1/2] v2 update):
    * Require nand-ecc-step-size for soft_bch.
    * Simplify mode/strength/step parameter checking.

 .../devicetree/bindings/mtd/fsl-upm-nand.txt       | 32 +++++++++++
 drivers/mtd/nand/Kconfig                           |  1 +
 drivers/mtd/nand/fsl_upm.c                         | 66
 ++++++++++++++++++++--
I was thinking about this a bit more, and it seems like we could really
just factor this all into the core nand_base code with something like
the following patch. It could possibly use some smarter logic to rule
out certain combinations (but some of those are already caught in
nand_scan_tail() anyway). What do you think?
Brian,
If the NAND device tree property fetching were moved out of fsl_upm,
I think it should not be called within nand_scan(). I think that
it's imperative that each driver be able to access these properties
before handing off to nand_scan(), since there are hardware ECC
modes that only drivers will know how to error check.

Also, catching errors in nand_scan_tail() tends to result in BUG()s.

That said, this could be useful as a publicly exported function that
individual drivers are responsible for calling (maybe in of_mtd.c).

I have a couple of additional comments inline below.

diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
index 72755d7ec25d..fc4834946233 100644
--- a/drivers/mtd/nand/fsl_upm.c
+++ b/drivers/mtd/nand/fsl_upm.c
@@ -181,6 +181,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun,
 	flash_np = of_get_next_child(upm_np, NULL);
 	if (!flash_np)
 		return -ENODEV;
+	fun->chip.dn = flash_np;
 
 	fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start,
 				  flash_np->name);
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 3f24b587304f..b701c3f23da1 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -48,6 +48,7 @@
 #include <linux/leds.h>
 #include <linux/io.h>
 #include <linux/mtd/partitions.h>
+#include <linux/of_mtd.h>
 
 /* Define default oob placement schemes for large and small page devices */
 static struct nand_ecclayout nand_oob_8 = {
@@ -3780,6 +3781,33 @@ ident_done:
 	return type;
 }
 
+static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip,
+			struct device_node *dn)
Assuming that of_mtd.c is the best place to put this, the
prototype could be simplified a little:

int of_nand_chip_init(struct device_node *dn, struct nand_chip *chip)

+{
+	int ecc_mode, ecc_strength, ecc_step;
+
+	if (of_get_nand_bus_width(dn) == 16)
+		chip->options |= NAND_BUSWIDTH_16;
+
+	if (of_get_nand_on_flash_bbt(dn))
+		chip->bbt_options |= NAND_BBT_USE_FLASH;
+
+	ecc_mode = of_get_nand_ecc_mode(dn);
+	ecc_strength = of_get_nand_ecc_strength(dn);
+	ecc_step = of_get_nand_ecc_step_size(dn);
+
+	if (ecc_mode >= 0)
+		chip->ecc.mode = ecc_mode;
+
+	if (ecc_strength >= 0)
+		chip->ecc.strength = ecc_strength;
+
+	if (ecc_step > 0)
+		chip->ecc.size = ecc_step;
+
+	return 0;
+}
+
 /**
  * nand_scan_ident - [NAND Interface] Scan for the NAND device
  * @mtd: MTD device structure
@@ -3797,6 +3825,13 @@ int nand_scan_ident(struct mtd_info *mtd, int
maxchips,
 	int i, nand_maf_id, nand_dev_id;
 	struct nand_chip *chip = mtd->priv;
 	struct nand_flash_dev *type;
+	int ret;
+
+	if (chip->dn) {
+		ret = nand_dt_init(mtd, chip, chip->dn);
+		if (ret)
+			return ret;
+	}
 
 	/* Set the default functions */
 	nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 3d4ea7eb2b68..e0f40e12a2c8 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -26,6 +26,8 @@
 
 struct mtd_info;
 struct nand_flash_dev;
+struct device_node;
+
 /* Scan and identify a NAND device */
 extern int nand_scan(struct mtd_info *mtd, int max_chips);
 /*
@@ -542,6 +544,7 @@ struct nand_buffers {
  *			flash device
  * @IO_ADDR_W:		[BOARDSPECIFIC] address to write the 8 I/O lines of the
  *			flash device.
+ * @dn:			[BOARDSPECIFIC] device node describing this instance
  * @read_byte:		[REPLACEABLE] read one byte from the chip
  * @read_word:		[REPLACEABLE] read one word from the chip
  * @write_byte:		[REPLACEABLE] write a single byte to the chip on the
@@ -644,6 +647,8 @@ struct nand_chip {
 	void __iomem *IO_ADDR_R;
 	void __iomem *IO_ADDR_W;
 
+	struct device_node *dn;
+
This addition to struct nand_chip wouldn't be needed, then.

 	uint8_t (*read_byte)(struct mtd_info *mtd);
 	u16 (*read_word)(struct mtd_info *mtd);
 	void (*write_byte)(struct mtd_info *mtd, uint8_t byte);
You hinted at implementing stronger error checking. If we went
this route, would it make sense to only error check the software
ECC modes?

-Aaron

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help

Possibly related (same subject, not in this thread)