[PATCHv4 0/5] mtd: gpmi: make blockmark swapping optional

[PATCHv4 0/5] mtd: gpmi: make blockmark swapping optional

[Lothar Waßmann]

With a flash-based BBT there is no reason to move the Factory Bad
Block Marker from the data area buffer (to where it is mapped by the
GPMI NAND controller) to the OOB buffer. Thus, make this feature
configurable via DT. This is required for the Ka-Ro electronics
i.MX6 platforms.

Changes wrt. v2:
- added a warning for i.MX28 to the binding documentation
- fixed the gpmi_ecc_read_subpage() routine which turned on blockmark
  swapping unconditionally
- use !GPMI_IS_MX23() in place of this->swap_block_mark (which were
  synonymous the original code) in gpmi_ecc_read_oob() and
  gpmi_block_markbad()
- make nand-on-flash-bbt a prerequisite for this feature

patch added:
- add an additional option to turn of BB mark writing

Changes wrt. v3:
- added two code cleanup patches
- added a patch to turn of NAND_BBT_CREATE, if blockmark swapping is disabled

[PATCHv4 1/5] mtd: gpmi: remove useless (void *) type casts and spaces between type casts and variables

[Lothar Waßmann]

Signed-off-by: Lothar Wa?mann <LW@KARO-electronics.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index f638cd8..31032b1 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1180,7 +1180,7 @@ static int gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 
 		/* Handle block mark swapping. */
 		block_mark_swapping(this,
-				(void *) payload_virt, (void *) auxiliary_virt);
+				(void *)payload_virt, (void *)auxiliary_virt);
 	} else {
 		/*
 		 * If control arrives here, we're not doing block mark swapping,
@@ -1760,16 +1760,16 @@ err_out:
 static const struct of_device_id gpmi_nand_id_table[] = {
 	{
 		.compatible = "fsl,imx23-gpmi-nand",
-		.data = (void *)&gpmi_devdata_imx23,
+		.data = &gpmi_devdata_imx23,
 	}, {
 		.compatible = "fsl,imx28-gpmi-nand",
-		.data = (void *)&gpmi_devdata_imx28,
+		.data = &gpmi_devdata_imx28,
 	}, {
 		.compatible = "fsl,imx6q-gpmi-nand",
-		.data = (void *)&gpmi_devdata_imx6q,
+		.data = &gpmi_devdata_imx6q,
 	}, {
 		.compatible = "fsl,imx6sx-gpmi-nand",
-		.data = (void *)&gpmi_devdata_imx6sx,
+		.data = &gpmi_devdata_imx6sx,
 	}, {}
 };
 MODULE_DEVICE_TABLE(of, gpmi_nand_id_table);
-- 
1.7.10.4

[PATCHv4 2/5] mtd: gpmi: remove line breaks from error messages and improve wording

[Lothar Waßmann]

Signed-off-by: Lothar Wa?mann <LW@KARO-electronics.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 31032b1..16a533a 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -285,9 +285,8 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
 	geo->ecc_strength = get_ecc_strength(this);
 	if (!gpmi_check_ecc(this)) {
 		dev_err(this->dev,
-			"We can not support this nand chip."
-			" Its required ecc strength(%d) is beyond our"
-			" capability(%d).\n", geo->ecc_strength,
+			"required ecc strength of the NAND chip: %d is not supported by the GPMI controller (%d)\n",
+			geo->ecc_strength,
 			this->devdata->bch_max_ecc_strength);
 		return -EINVAL;
 	}
@@ -1597,8 +1596,9 @@ static int mx23_boot_init(struct gpmi_nand_data  *this)
 			dev_dbg(dev, "Transcribing mark in block %u\n", block);
 			ret = chip->block_markbad(mtd, byte);
 			if (ret)
-				dev_err(dev, "Failed to mark block bad with "
-							"ret %d\n", ret);
+				dev_err(dev,
+					"Failed to mark block bad with ret %d\n",
+					ret);
 		}
 	}
 
-- 
1.7.10.4

[PATCHv4 3/5] mtd: gpmi: make blockmark swapping optional

[Lothar Waßmann]

With a flash-based BBT there is no reason to move the Factory Bad
Block Marker from the data area buffer (to where it is mapped by the
GPMI NAND controller) to the OOB buffer. Thus, make this feature
configurable via DT. This is required for the Ka-Ro electronics
platforms.

In the original code 'this->swap_block_mark' was synonymous with
'!GPMI_IS_MX23()', so use the latter at the relevant places.

Signed-off-by: Lothar Wa?mann <LW@KARO-electronics.de>
---
 .../devicetree/bindings/mtd/gpmi-nand.txt          |   10 ++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c             |   51 ++++++++++++--------
 2 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
index 458d596..a011fdf 100644
--- a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
@@ -25,6 +25,16 @@ Optional properties:
                        discoverable or this property is not enabled,
                        the software may chooses an implementation-defined
                        ECC scheme.
+  - fsl,no-blockmark-swap: Don't swap the bad block marker from the OOB
+                       area with the byte in the data area but rely on the
+                       flash based BBT for identifying bad blocks.
+                       NOTE: this is only valid in conjunction with
+                             'nand-on-flash-bbt'.
+                       WARNING: on i.MX28 blockmark swapping cannot be
+                       disabled for the BootROM in the FCB. Thus,
+                       partitions written from Linux with this feature
+                       turned on may not be accessible by the BootROM
+                       code.
 
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 16a533a..959cb9b 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1081,6 +1081,7 @@ static int gpmi_ecc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	int first, last, marker_pos;
 	int ecc_parity_size;
 	int col = 0;
+	int old_swap_block_mark = this->swap_block_mark;
 
 	/* The size of ECC parity */
 	ecc_parity_size = geo->gf_len * geo->ecc_strength / 8;
@@ -1089,17 +1090,21 @@ static int gpmi_ecc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	first = offs / size;
 	last = (offs + len - 1) / size;
 
-	/*
-	 * Find the chunk which contains the Block Marker. If this chunk is
-	 * in the range of [first, last], we have to read out the whole page.
-	 * Why? since we had swapped the data at the position of Block Marker
-	 * to the metadata which is bound with the chunk 0.
-	 */
-	marker_pos = geo->block_mark_byte_offset / size;
-	if (last >= marker_pos && first <= marker_pos) {
-		dev_dbg(this->dev, "page:%d, first:%d, last:%d, marker at:%d\n",
+	if (this->swap_block_mark) {
+		/*
+		 * Find the chunk which contains the Block Marker.
+		 * If this chunk is in the range of [first, last],
+		 * we have to read out the whole page.
+		 * Why? since we had swapped the data at the position of Block
+		 * Marker to the metadata which is bound with the chunk 0.
+		 */
+		marker_pos = geo->block_mark_byte_offset / size;
+		if (last >= marker_pos && first <= marker_pos) {
+			dev_dbg(this->dev,
+				"page:%d, first:%d, last:%d, marker at:%d\n",
 				page, first, last, marker_pos);
-		return gpmi_ecc_read_page(mtd, chip, buf, 0, page);
+			return gpmi_ecc_read_page(mtd, chip, buf, 0, page);
+		}
 	}
 
 	meta = geo->metadata_size;
@@ -1145,7 +1150,7 @@ static int gpmi_ecc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	writel(r1_old, bch_regs + HW_BCH_FLASH0LAYOUT0);
 	writel(r2_old, bch_regs + HW_BCH_FLASH0LAYOUT1);
 	this->bch_geometry = old_geo;
-	this->swap_block_mark = true;
+	this->swap_block_mark = old_swap_block_mark;
 
 	return max_bitflips;
 }
@@ -1309,10 +1314,10 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 
 	/*
 	 * Now, we want to make sure the block mark is correct. In the
-	 * Swapping/Raw case, we already have it. Otherwise, we need to
-	 * explicitly read it.
+	 * non-transcribing case (!GPMI_IS_MX23()), we already have it.
+	 * Otherwise, we need to explicitly read it.
 	 */
-	if (!this->swap_block_mark) {
+	if (GPMI_IS_MX23(this)) {
 		/* Read the block mark into the first byte of the OOB buffer. */
 		chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
 		chip->oob_poi[0] = chip->read_byte(mtd);
@@ -1353,7 +1358,7 @@ static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)
 	chipnr = (int)(ofs >> chip->chip_shift);
 	chip->select_chip(mtd, chipnr);
 
-	column = this->swap_block_mark ? mtd->writesize : 0;
+	column = !GPMI_IS_MX23(this) ? mtd->writesize : 0;
 
 	/* Write the block mark. */
 	block_mark = this->data_buffer_dma;
@@ -1649,9 +1654,6 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
 	struct bch_geometry *bch_geo = &this->bch_geometry;
 	int ret;
 
-	/* Set up swap_block_mark, must be set before the gpmi_set_geometry() */
-	this->swap_block_mark = !GPMI_IS_MX23(this);
-
 	/* Set up the medium geometry */
 	ret = gpmi_set_geometry(this);
 	if (ret)
@@ -1715,9 +1717,20 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
 	chip->badblock_pattern	= &gpmi_bbt_descr;
 	chip->block_markbad	= gpmi_block_markbad;
 	chip->options		|= NAND_NO_SUBPAGE_WRITE;
-	if (of_get_nand_on_flash_bbt(this->dev->of_node))
+
+	/* Set up swap_block_mark, must be set before the gpmi_set_geometry() */
+	this->swap_block_mark = !GPMI_IS_MX23(this);
+
+	if (of_get_nand_on_flash_bbt(this->dev->of_node)) {
 		chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
 
+		if (of_property_read_bool(this->dev->of_node,
+						"fsl,no-blockmark-swap"))
+			this->swap_block_mark = false;
+	}
+	dev_dbg(this->dev, "Blockmark swapping %sabled\n",
+		this->swap_block_mark ? "en" : "dis");
+
 	/*
 	 * Allocate a temporary DMA buffer for reading ID in the
 	 * nand_scan_ident().
-- 
1.7.10.4

[PATCHv4 4/5] of/mtd/nand: add generic binding and helper for NAND_BBT_NO_OOB_BBM

[Lothar Waßmann]

add a boolean property 'nand-no-oob-bbm' and helper function to be
able to set the NAND_BBT_NO_OOB_BBM flag in DT capable NAND drivers
and use it for i.MX and MXS nand drivers.

Signed-off-by: Lothar Wa?mann <LW@KARO-electronics.de>
---
 Documentation/devicetree/bindings/mtd/nand.txt |    1 +
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c         |    3 +++
 drivers/mtd/nand/mxc_nand.c                    |    2 ++
 drivers/of/of_mtd.c                            |   12 ++++++++++++
 include/linux/of_mtd.h                         |    6 ++++++
 5 files changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
index b53f92e..e46bfbe 100644
--- a/Documentation/devicetree/bindings/mtd/nand.txt
+++ b/Documentation/devicetree/bindings/mtd/nand.txt
@@ -5,6 +5,7 @@
   "soft_bch".
 - nand-bus-width : 8 or 16 bus width if not present 8
 - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
+- nand-no-oob-bbm: boolean to disable writing bad block markers to flash
 
 - nand-ecc-strength: integer representing the number of bits to correct
 		     per ECC step.
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 959cb9b..37537b4 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1724,6 +1724,9 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
 	if (of_get_nand_on_flash_bbt(this->dev->of_node)) {
 		chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
 
+		if (of_get_nand_no_oob_bbm(this->dev->of_node))
+			chip->bbt_options |= NAND_BBT_NO_OOB_BBM;
+
 		if (of_property_read_bool(this->dev->of_node,
 						"fsl,no-blockmark-swap"))
 			this->swap_block_mark = false;
diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index dba262b..bb54a2a 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -1496,6 +1496,8 @@ static int mxcnd_probe(struct platform_device *pdev)
 		this->bbt_md = &bbt_mirror_descr;
 		/* update flash based bbt */
 		this->bbt_options |= NAND_BBT_USE_FLASH;
+		if (of_get_nand_no_oob_bbm(pdev->dev.of_node))
+			this->bbt_options |= NAND_BBT_NO_OOB_BBM;
 	}
 
 	init_completion(&host->op_completion);
diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c
index b7361ed..d947acc 100644
--- a/drivers/of/of_mtd.c
+++ b/drivers/of/of_mtd.c
@@ -117,3 +117,15 @@ bool of_get_nand_on_flash_bbt(struct device_node *np)
 	return of_property_read_bool(np, "nand-on-flash-bbt");
 }
 EXPORT_SYMBOL_GPL(of_get_nand_on_flash_bbt);
+
+/**
+ * of_get_nand_no_oob_bbm - Get nand no oob bbm for given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * return true if present, false otherwise
+ */
+bool of_get_nand_no_oob_bbm(struct device_node *np)
+{
+	return of_property_read_bool(np, "nand-no-oob-bbm");
+}
+EXPORT_SYMBOL_GPL(of_get_nand_no_oob_bbm);
diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h
index e266caa..6ece1a9 100644
--- a/include/linux/of_mtd.h
+++ b/include/linux/of_mtd.h
@@ -17,6 +17,7 @@ int of_get_nand_ecc_step_size(struct device_node *np);
 int of_get_nand_ecc_strength(struct device_node *np);
 int of_get_nand_bus_width(struct device_node *np);
 bool of_get_nand_on_flash_bbt(struct device_node *np);
+bool of_get_nand_no_oob_bbm(struct device_node *np);
 
 #else /* CONFIG_OF_MTD */
 
@@ -45,6 +46,11 @@ static inline bool of_get_nand_on_flash_bbt(struct device_node *np)
 	return false;
 }
 
+static inline bool of_get_nand_no_oob_bbm(struct device_node *np)
+{
+	return false;
+}
+
 #endif /* CONFIG_OF_MTD */
 
 #endif /* __LINUX_OF_MTD_H */
-- 
1.7.10.4

[PATCHv4 5/5] mtd: gpmi: prevent creating a new BBT when blockmark swapping is disabled

[Lothar Waßmann]

Without blockmark swapping, there is no use in creating a BBT from
scratch, so use a BBT descriptor with NAND_BBT_CREATE unset in this
case.

Signed-off-by: Lothar Wa?mann <LW@KARO-electronics.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 37537b4..fc710d7 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -43,6 +43,29 @@ static struct nand_bbt_descr gpmi_bbt_descr = {
 	.pattern	= scan_ff_pattern
 };
 
+static uint8_t bbt_pattern[] = {'B', 'b', 't', '0' };
+static uint8_t mirror_pattern[] = {'1', 't', 'b', 'B' };
+
+static struct nand_bbt_descr bbt_main_no_oob_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_WRITE |
+	NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP |
+	NAND_BBT_NO_OOB,
+	.len = 4,
+	.veroffs = 4,
+	.maxblocks = NAND_BBT_SCAN_MAXBLOCKS,
+	.pattern = bbt_pattern,
+};
+
+static struct nand_bbt_descr bbt_mirror_no_oob_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_WRITE |
+	NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP |
+	NAND_BBT_NO_OOB,
+	.len = 4,
+	.veroffs = 4,
+	.maxblocks = NAND_BBT_SCAN_MAXBLOCKS,
+	.pattern = mirror_pattern,
+};
+
 /*
  * We may change the layout if we can get the ECC info from the datasheet,
  * else we will use all the (page + OOB).
@@ -1728,8 +1751,11 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
 			chip->bbt_options |= NAND_BBT_NO_OOB_BBM;
 
 		if (of_property_read_bool(this->dev->of_node,
-						"fsl,no-blockmark-swap"))
+						"fsl,no-blockmark-swap")) {
 			this->swap_block_mark = false;
+			chip->bbt_td = &bbt_main_no_oob_descr;
+			chip->bbt_md = &bbt_mirror_no_oob_descr;
+		}
 	}
 	dev_dbg(this->dev, "Blockmark swapping %sabled\n",
 		this->swap_block_mark ? "en" : "dis");
-- 
1.7.10.4

[PATCHv4 5/5] mtd: gpmi: prevent creating a new BBT when blockmark swapping is disabled

[Brian Norris]

Hi Lothar,

On Thu, Jun 12, 2014 at 03:20:45PM +0200, Lothar Wa?mann wrote:
> Without blockmark swapping, there is no use in creating a BBT from
> scratch, so use a BBT descriptor with NAND_BBT_CREATE unset in this
> case.

I'm curious: what is your plan if there is no BBT available on your
device, or if it ever gets corrupted? IIUC, nand_bbt will just assume
you have no bad blocks, and it will never write a bad block table to
flash. This also means no subsequent discoverable bad blocks can be
recorded across power cycles, I believe.

Maybe you don't want to specify your own nand_bbt_descr's at all, but
you just need to set:

	chip->bbt_options |= NAND_BBT_CREATE_EMPTY | NAND_BBT_NO_OOB;

(Note: there's a little bit of fuzziness about NAND_BBT_* flags, where
some are targeted for the nand_chip::bbt_options field, and others
belong in struct nand_bbt_descr::options.)

But if for some reason we need to keep this patch, a comment below:

> Signed-off-by: Lothar Wa?mann <LW@KARO-electronics.de>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 37537b4..fc710d7 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -43,6 +43,29 @@ static struct nand_bbt_descr gpmi_bbt_descr = {
>  	.pattern	= scan_ff_pattern
>  };
>  
> +static uint8_t bbt_pattern[] = {'B', 'b', 't', '0' };
> +static uint8_t mirror_pattern[] = {'1', 't', 'b', 'B' };
> +
> +static struct nand_bbt_descr bbt_main_no_oob_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_WRITE |
> +	NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP |
> +	NAND_BBT_NO_OOB,

Please indent the above two lines a bit, preferably matching the
indentation of NAND_BBT_LASTBLOCK. It should be clear that this is a
continuation of the '.options' initialization.

> +	.len = 4,
> +	.veroffs = 4,
> +	.maxblocks = NAND_BBT_SCAN_MAXBLOCKS,
> +	.pattern = bbt_pattern,
> +};
> +
> +static struct nand_bbt_descr bbt_mirror_no_oob_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_WRITE |
> +	NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP |
> +	NAND_BBT_NO_OOB,

Same here.

> +	.len = 4,
> +	.veroffs = 4,
> +	.maxblocks = NAND_BBT_SCAN_MAXBLOCKS,
> +	.pattern = mirror_pattern,
> +};
> +
>  /*
>   * We may change the layout if we can get the ECC info from the datasheet,
>   * else we will use all the (page + OOB).
> @@ -1728,8 +1751,11 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
>  			chip->bbt_options |= NAND_BBT_NO_OOB_BBM;
>  
>  		if (of_property_read_bool(this->dev->of_node,
> -						"fsl,no-blockmark-swap"))
> +						"fsl,no-blockmark-swap")) {
>  			this->swap_block_mark = false;
> +			chip->bbt_td = &bbt_main_no_oob_descr;
> +			chip->bbt_md = &bbt_mirror_no_oob_descr;

My initial recommendation for this patch and the previous patch means
that you could just drop both patches and replace them with the
following:

			/* Comment here to explain why... */
			chip->bbt_options |= NAND_BBT_CREATE_EMPTY |
					     NAND_BBT_NO_OOB |
					     NAND_BBT_NO_OOB_BBM;
> +		}
>  	}
>  	dev_dbg(this->dev, "Blockmark swapping %sabled\n",
>  		this->swap_block_mark ? "en" : "dis");

Brian

[PATCHv4 5/5] mtd: gpmi: prevent creating a new BBT when blockmark swapping is disabled

[Lothar Waßmann]

Hi,

Brian Norris wrote:
> Hi Lothar,
> 
> On Thu, Jun 12, 2014 at 03:20:45PM +0200, Lothar Wa?mann wrote:
> > Without blockmark swapping, there is no use in creating a BBT from
> > scratch, so use a BBT descriptor with NAND_BBT_CREATE unset in this
> > case.
> 
> I'm curious: what is your plan if there is no BBT available on your
> device, or if it ever gets corrupted? IIUC, nand_bbt will just assume
> you have no bad blocks, and it will never write a bad block table to
> flash. This also means no subsequent discoverable bad blocks can be
> recorded across power cycles, I believe.
> 
That won't happen (unless it's not possible to create a BBT because all
the possible blocks for the BBT are bad), because the bootloader will
have created one before Linux is started.

> Maybe you don't want to specify your own nand_bbt_descr's at all, but
> you just need to set:
> 
> 	chip->bbt_options |= NAND_BBT_CREATE_EMPTY | NAND_BBT_NO_OOB;
> 
> (Note: there's a little bit of fuzziness about NAND_BBT_* flags, where
> some are targeted for the nand_chip::bbt_options field, and others
> belong in struct nand_bbt_descr::options.)
> 
> But if for some reason we need to keep this patch, a comment below:
> 
> > Signed-off-by: Lothar Wa?mann <LW@KARO-electronics.de>
> > ---
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   28 +++++++++++++++++++++++++++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > index 37537b4..fc710d7 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > @@ -43,6 +43,29 @@ static struct nand_bbt_descr gpmi_bbt_descr = {
> >  	.pattern	= scan_ff_pattern
> >  };
> >  
> > +static uint8_t bbt_pattern[] = {'B', 'b', 't', '0' };
> > +static uint8_t mirror_pattern[] = {'1', 't', 'b', 'B' };
> > +
> > +static struct nand_bbt_descr bbt_main_no_oob_descr = {
> > +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_WRITE |
> > +	NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP |
> > +	NAND_BBT_NO_OOB,
> 
> Please indent the above two lines a bit, preferably matching the
> indentation of NAND_BBT_LASTBLOCK. It should be clear that this is a
> continuation of the '.options' initialization.
> 
OK.

> > +	.len = 4,
> > +	.veroffs = 4,
> > +	.maxblocks = NAND_BBT_SCAN_MAXBLOCKS,
> > +	.pattern = bbt_pattern,
> > +};
> > +
> > +static struct nand_bbt_descr bbt_mirror_no_oob_descr = {
> > +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_WRITE |
> > +	NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP |
> > +	NAND_BBT_NO_OOB,
> 
> Same here.
> 
> > +	.len = 4,
> > +	.veroffs = 4,
> > +	.maxblocks = NAND_BBT_SCAN_MAXBLOCKS,
> > +	.pattern = mirror_pattern,
> > +};
> > +
> >  /*
> >   * We may change the layout if we can get the ECC info from the datasheet,
> >   * else we will use all the (page + OOB).
> > @@ -1728,8 +1751,11 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
> >  			chip->bbt_options |= NAND_BBT_NO_OOB_BBM;
> >  
> >  		if (of_property_read_bool(this->dev->of_node,
> > -						"fsl,no-blockmark-swap"))
> > +						"fsl,no-blockmark-swap")) {
> >  			this->swap_block_mark = false;
> > +			chip->bbt_td = &bbt_main_no_oob_descr;
> > +			chip->bbt_md = &bbt_mirror_no_oob_descr;
> 
> My initial recommendation for this patch and the previous patch means
> that you could just drop both patches and replace them with the
> following:
> 
> 			/* Comment here to explain why... */
> 			chip->bbt_options |= NAND_BBT_CREATE_EMPTY |
> 					     NAND_BBT_NO_OOB |
> 					     NAND_BBT_NO_OOB_BBM;
OK.

Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCHv4 4/5] of/mtd/nand: add generic binding and helper for NAND_BBT_NO_OOB_BBM

[Brian Norris]

(BTW, that's a mighty CC list you have! I'm not sure all CC'd parties
are interested in this series; e.g., Russel and the ARM list seem
unrelated)

Hi Lothar,

Sorry for the delay on this. I get busy enough that I can't/don't reply
to everything quickly...

On Thu, Jun 12, 2014 at 03:20:44PM +0200, Lothar Wa?mann wrote:
> add a boolean property 'nand-no-oob-bbm' and helper function to be
> able to set the NAND_BBT_NO_OOB_BBM flag in DT capable NAND drivers
> and use it for i.MX and MXS nand drivers.

If I'm understanding your previous conversations with Huang correctly,
you *must* use NAND_BBT_NO_OOB_BBM if you're going to use the
fsl,no-blockmark-swap option. Correct? If so, then you might not need
a separate 'nand-no-oob-bbm' binding; your driver should imply from
'fsl,no-blockmark-swap' that it must also enable NAND_BBT_NO_OOB_BBM.

Also, as I noted in [1], I don't really like exposing a ton of
individual boolean DT properties like this. (At least this property is
orthogonal to the bad block table; I was a little off-base in [1].)

Brian

[1] http://lists.infradead.org/pipermail/linux-mtd/2014-July/054764.html

> Signed-off-by: Lothar Wa?mann <LW@KARO-electronics.de>
> ---
>  Documentation/devicetree/bindings/mtd/nand.txt |    1 +
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c         |    3 +++
>  drivers/mtd/nand/mxc_nand.c                    |    2 ++
>  drivers/of/of_mtd.c                            |   12 ++++++++++++
>  include/linux/of_mtd.h                         |    6 ++++++
>  5 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
> index b53f92e..e46bfbe 100644
> --- a/Documentation/devicetree/bindings/mtd/nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
> @@ -5,6 +5,7 @@
>    "soft_bch".
>  - nand-bus-width : 8 or 16 bus width if not present 8
>  - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
> +- nand-no-oob-bbm: boolean to disable writing bad block markers to flash
>  
>  - nand-ecc-strength: integer representing the number of bits to correct
>  		     per ECC step.
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 959cb9b..37537b4 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1724,6 +1724,9 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
>  	if (of_get_nand_on_flash_bbt(this->dev->of_node)) {
>  		chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>  
> +		if (of_get_nand_no_oob_bbm(this->dev->of_node))
> +			chip->bbt_options |= NAND_BBT_NO_OOB_BBM;
> +
>  		if (of_property_read_bool(this->dev->of_node,
>  						"fsl,no-blockmark-swap"))
>  			this->swap_block_mark = false;
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index dba262b..bb54a2a 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -1496,6 +1496,8 @@ static int mxcnd_probe(struct platform_device *pdev)
>  		this->bbt_md = &bbt_mirror_descr;
>  		/* update flash based bbt */
>  		this->bbt_options |= NAND_BBT_USE_FLASH;
> +		if (of_get_nand_no_oob_bbm(pdev->dev.of_node))
> +			this->bbt_options |= NAND_BBT_NO_OOB_BBM;
>  	}
>  
>  	init_completion(&host->op_completion);
> diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c
> index b7361ed..d947acc 100644
> --- a/drivers/of/of_mtd.c
> +++ b/drivers/of/of_mtd.c
> @@ -117,3 +117,15 @@ bool of_get_nand_on_flash_bbt(struct device_node *np)
>  	return of_property_read_bool(np, "nand-on-flash-bbt");
>  }
>  EXPORT_SYMBOL_GPL(of_get_nand_on_flash_bbt);
> +
> +/**
> + * of_get_nand_no_oob_bbm - Get nand no oob bbm for given device_node
> + * @np:	Pointer to the given device_node
> + *
> + * return true if present, false otherwise
> + */
> +bool of_get_nand_no_oob_bbm(struct device_node *np)
> +{
> +	return of_property_read_bool(np, "nand-no-oob-bbm");
> +}
> +EXPORT_SYMBOL_GPL(of_get_nand_no_oob_bbm);
> diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h
> index e266caa..6ece1a9 100644
> --- a/include/linux/of_mtd.h
> +++ b/include/linux/of_mtd.h
> @@ -17,6 +17,7 @@ int of_get_nand_ecc_step_size(struct device_node *np);
>  int of_get_nand_ecc_strength(struct device_node *np);
>  int of_get_nand_bus_width(struct device_node *np);
>  bool of_get_nand_on_flash_bbt(struct device_node *np);
> +bool of_get_nand_no_oob_bbm(struct device_node *np);
>  
>  #else /* CONFIG_OF_MTD */
>  
> @@ -45,6 +46,11 @@ static inline bool of_get_nand_on_flash_bbt(struct device_node *np)
>  	return false;
>  }
>  
> +static inline bool of_get_nand_no_oob_bbm(struct device_node *np)
> +{
> +	return false;
> +}
> +
>  #endif /* CONFIG_OF_MTD */
>  
>  #endif /* __LINUX_OF_MTD_H */

[PATCHv4 4/5] of/mtd/nand: add generic binding and helper for NAND_BBT_NO_OOB_BBM

[Lothar Waßmann]

Hi,

Brian Norris wrote:
> (BTW, that's a mighty CC list you have! I'm not sure all CC'd parties
> are interested in this series; e.g., Russel and the ARM list seem
> unrelated)
> 
> Hi Lothar,
> 
> Sorry for the delay on this. I get busy enough that I can't/don't reply
> to everything quickly...
> 
> On Thu, Jun 12, 2014 at 03:20:44PM +0200, Lothar Wa?mann wrote:
> > add a boolean property 'nand-no-oob-bbm' and helper function to be
> > able to set the NAND_BBT_NO_OOB_BBM flag in DT capable NAND drivers
> > and use it for i.MX and MXS nand drivers.
> 
> If I'm understanding your previous conversations with Huang correctly,
> you *must* use NAND_BBT_NO_OOB_BBM if you're going to use the
> fsl,no-blockmark-swap option. Correct? If so, then you might not need
> a separate 'nand-no-oob-bbm' binding; your driver should imply from
> 'fsl,no-blockmark-swap' that it must also enable NAND_BBT_NO_OOB_BBM.
> 
no-blockmark-swap implies NO_OOB_BBM but NO_OOB_BBM may also be used
independent from no-blockmark-swap.

IMO writing a bad block marker to flash (which is prevented by
the NAND_BBT_NO_OOB_BBM flag) is a misinterpretation of the purpose of
the BB mark in the first place. The manufacturer guarantees that blocks
which are initially bad will have at least one zero bit in the position
of the BB mark. That's all to it.

There is no guarantee, that you will even be able to write any
deterministic data to a block that has turned bad due to wearout or
other flash defects. It is rather bogus to rely on data written to a
known bad block to reflect the state of the block.

> Also, as I noted in [1], I don't really like exposing a ton of
> individual boolean DT properties like this. (At least this property is
> orthogonal to the bad block table; I was a little off-base in [1].)
> 
How else should this information be conveyed to the flash drivers?


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCHv4 4/5] of/mtd/nand: add generic binding and helper for NAND_BBT_NO_OOB_BBM

[Brian Norris]

On Thu, Jul 24, 2014 at 08:49:15AM +0200, Lothar Wa?mann wrote:
> Brian Norris wrote:
> > On Thu, Jun 12, 2014 at 03:20:44PM +0200, Lothar Wa?mann wrote:
> > > add a boolean property 'nand-no-oob-bbm' and helper function to be
> > > able to set the NAND_BBT_NO_OOB_BBM flag in DT capable NAND drivers
> > > and use it for i.MX and MXS nand drivers.
> > 
> > If I'm understanding your previous conversations with Huang correctly,
> > you *must* use NAND_BBT_NO_OOB_BBM if you're going to use the
> > fsl,no-blockmark-swap option. Correct? If so, then you might not need
> > a separate 'nand-no-oob-bbm' binding; your driver should imply from
> > 'fsl,no-blockmark-swap' that it must also enable NAND_BBT_NO_OOB_BBM.
> > 
> no-blockmark-swap implies NO_OOB_BBM but NO_OOB_BBM may also be used
> independent from no-blockmark-swap.

Why would you want NO_OOB_BBM without no-blockmark-swap? If the block is
bad, why do you care what's written to it? (For that matter, why is it
ever important to use NO_OOB_BBM? At worst, the extra BB marks are
useless / written to the wrong place.)

> IMO writing a bad block marker to flash (which is prevented by
> the NAND_BBT_NO_OOB_BBM flag) is a misinterpretation of the purpose of
> the BB mark in the first place. The manufacturer guarantees that blocks
> which are initially bad will have at least one zero bit in the position
> of the BB mark. That's all to it.

Yes, it is a misinterpretation, and it's really irrelevant in many cases
whether or not the BB mark is written to each block's OOB. But it does
still provide some resilience in case the on-flash table ever gets
completely corrupted -- nand_bbt will rescan the flash for BB marks on
the next boot (and this will be totally broken--with or without
NO_OOB_BBM--for hardware like yours). Or to put it another way, it
supports some legacy scenarios without (AFAICT) any real negative
effects.

> There is no guarantee, that you will even be able to write any
> deterministic data to a block that has turned bad due to wearout or
> other flash defects.

Certainly. But that's not an argument against attempting.

> It is rather bogus to rely on data written to a
> known bad block to reflect the state of the block.

We don't "rely" on this. If the BBT (and its mirrors) never completely
fails, these marks are never used.

> > Also, as I noted in [1], I don't really like exposing a ton of
> > individual boolean DT properties like this. (At least this property is
> > orthogonal to the bad block table; I was a little off-base in [1].)
> > 
> How else should this information be conveyed to the flash drivers?

I'm not convinced the NO_OOB_BBM DT property is actually necessary at
all.

I was more concerned about bad block *table* properties, where I see
that at least some users (e.g. ST Micro's BCH NAND driver) expect a
different BBT format than the default, and we might begin to see extra
boolean flags for random bits of differentiation. This is apparently
still just a theoretical concern, though.

Brian

[PATCHv4 0/5] mtd: gpmi: make blockmark swapping optional

[Lothar Waßmann]

Hi,

Lothar Wa?mann wrote:
> With a flash-based BBT there is no reason to move the Factory Bad
> Block Marker from the data area buffer (to where it is mapped by the
> GPMI NAND controller) to the OOB buffer. Thus, make this feature
> configurable via DT. This is required for the Ka-Ro electronics
> i.MX6 platforms.
> 
> Changes wrt. v2:
> - added a warning for i.MX28 to the binding documentation
> - fixed the gpmi_ecc_read_subpage() routine which turned on blockmark
>   swapping unconditionally
> - use !GPMI_IS_MX23() in place of this->swap_block_mark (which were
>   synonymous the original code) in gpmi_ecc_read_oob() and
>   gpmi_block_markbad()
> - make nand-on-flash-bbt a prerequisite for this feature
> 
> patch added:
> - add an additional option to turn of BB mark writing
> 
> Changes wrt. v3:
> - added two code cleanup patches
> - added a patch to turn of NAND_BBT_CREATE, if blockmark swapping is disabled
> 
> 
any comments on this patchset?


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCHv4 0/5] mtd: gpmi: make blockmark swapping optional

[Brian Norris]

On Thu, Jun 12, 2014 at 03:20:40PM +0200, Lothar Wa?mann wrote:
> With a flash-based BBT there is no reason to move the Factory Bad
> Block Marker from the data area buffer (to where it is mapped by the
> GPMI NAND controller) to the OOB buffer. Thus, make this feature
> configurable via DT. This is required for the Ka-Ro electronics
> i.MX6 platforms.

Pushed patches 1-3 to l2-mtd.git. Thanks!

Brian