Re: [PATCH v4 2/3] spi-nor: s25fl512s supports region locking

[<Tudor.Ambarus@microchip.com>]

Geert,

Would you run this debug patch on top of mtd/next? I dumped the SR and CR before
and after the operations in cause.

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 73172d7f512b..20d0fdb1dc92 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -22,6 +22,8 @@
 #include <linux/spi/flash.h>
 #include <linux/mtd/spi-nor.h>

+#define DEBUG
+
 /* Define max times to check status register before we give up. */

 /*
@@ -200,7 +202,7 @@ struct sfdp_header {
  *         register does not modify status register 2.
  * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
  *         Read Status instruction 05h. Status register2 is read using
- *         instruction 35h. QE is set via Writ Status instruction 01h with
+ *         instruction 35h. QE is set via Write Status instruction 01h with
  *         two data bytes where bit 1 of the second byte is one.
  *         [...]
  */
@@ -2795,8 +2797,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 		return err;

 	/* Fix endianness of the BFPT DWORDs. */
-	for (i = 0; i < BFPT_DWORD_MAX; i++)
+	for (i = 0; i < BFPT_DWORD_MAX; i++) {
 		bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
+		dev_dbg(nor->dev, "bfpt.dwords[%d] = %08x\n", i + 1,
+			bfpt.dwords[i]);
+	}

 	/* Number of address bytes. */
 	switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
@@ -3532,8 +3537,10 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 	}

 	err = spi_nor_parse_bfpt(nor, bfpt_header, params);
-	if (err)
+	if (err) {
+		dev_dbg(dev, "failed to parse BFPT: err = %d\n", err);
 		goto exit;
+	}

 	/* Parse optional parameter tables. */
 	for (i = 0; i < header.nph; i++) {
@@ -3576,6 +3583,7 @@ static int spi_nor_init_params(struct spi_nor *nor,
 	struct spi_nor_erase_map *map = &nor->erase_map;
 	const struct flash_info *info = nor->info;
 	u8 i, erase_mask;
+	int ret;

 	/* Set legacy flash parameters as default. */
 	memset(params, 0, sizeof(*params));
@@ -3681,12 +3689,15 @@ static int spi_nor_init_params(struct spi_nor *nor,
 		memcpy(&sfdp_params, params, sizeof(sfdp_params));
 		memcpy(&prev_map, &nor->erase_map, sizeof(prev_map));

-		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
+		ret = spi_nor_parse_sfdp(nor, &sfdp_params);
+		if (ret) {
 			nor->addr_width = 0;
 			nor->flags &= ~SNOR_F_4B_OPCODES;
 			/* restore previous erase map */
 			memcpy(&nor->erase_map, &prev_map,
 			       sizeof(nor->erase_map));
+			dev_dbg(nor->dev, "%s sfdp parse failed, ret =%d\n",
+				__FUNCTION__, ret);
 		} else {
 			memcpy(params, &sfdp_params, sizeof(*params));
 		}
@@ -3908,9 +3919,67 @@ static int spi_nor_setup(struct spi_nor *nor,
 	return 0;
 }

+static int spi_nor_dump_sr_cr(struct spi_nor *nor)
+{
+	int ret = read_sr(nor);
+
+	dev_dbg(nor->dev, "SR = %08x\n", ret);
+        if (ret < 0) {
+                dev_err(nor->dev, "error while reading status register\n");
+                return ret;
+        }
+
+	ret = read_cr(nor);
+	dev_dbg(nor->dev, "CR = %08x\n", ret);
+        if (ret < 0) {
+                dev_err(nor->dev, "error while reading configuration register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int spi_nor_clear_block_protection(struct spi_nor *nor)
+{
+	int ret;
+	u8 val, mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+	ret = spi_nor_dump_sr_cr(nor);
+	if (ret)
+		return ret;
+
+	/* clear just the BP bits */
+	ret = read_sr(nor);
+        if (ret < 0) {
+                dev_err(nor->dev, "error while reading status register\n");
+                return ret;
+        }
+	val = ret;
+
+	ret = write_enable(nor);
+        if (ret < 0) {
+                dev_err(nor->dev, "error on write enable, err = %d\n", ret);
+                return ret;
+	}
+
+	ret = write_sr(nor, val & ~mask);
+        if (ret < 0) {
+                dev_err(nor->dev, "error on write_sr, err = %d\n", ret);
+                return ret;
+	}
+
+	ret = spi_nor_wait_till_ready(nor);
+        if (ret) {
+                dev_err(nor->dev, "timeout while writing SR, ret = %d\n", ret);
+		return spi_nor_dump_sr_cr(nor);
+        }
+
+	return 0;
+}
+
 static int spi_nor_init(struct spi_nor *nor)
 {
-	int err;
+	int err = 0, quad_err;

 	/*
 	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
@@ -3919,18 +3988,38 @@ static int spi_nor_init(struct spi_nor *nor)
 	if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL ||
 	    JEDEC_MFR(nor->info) == SNOR_MFR_INTEL ||
 	    JEDEC_MFR(nor->info) == SNOR_MFR_SST ||
-	    nor->info->flags & SPI_NOR_HAS_LOCK) {
-		write_enable(nor);
-		write_sr(nor, 0);
-		spi_nor_wait_till_ready(nor);
+	    nor->info->flags & SPI_NOR_HAS_LOCK)
+		/*
+		 * this should be done only on demand, not for every flash that
+		 * has SPI_NOR_HAS_LOCK set
+		 */
+		err = spi_nor_clear_block_protection(nor);
+	if (err) {
+		dev_err(nor->dev, "clearing BP bits failed, err = %d\n", err);
+		return err;
 	}

 	if (nor->quad_enable) {
-		err = nor->quad_enable(nor);
+		dev_dbg(nor->dev, "SR and CR before quad_enable:\n");
+
+		err = spi_nor_dump_sr_cr(nor);
 		if (err) {
-			dev_err(nor->dev, "quad mode not supported\n");
+			dev_err(nor->dev, "dump_sr_cr before quad enable fail, err = %d\n", err);
 			return err;
 		}
+
+		quad_err = nor->quad_enable(nor);
+		dev_dbg(nor->dev, "SR and CR after quad_enable:\n");
+		err = spi_nor_dump_sr_cr(nor);
+		if (err) {
+			dev_err(nor->dev, "dump_sr_cr after quad enable fail, err = %d\n", err);
+			return err;
+		}
+
+		if (quad_err) {
+			dev_err(nor->dev, "quad mode not supported, err = %d\n", quad_err);
+			return quad_err;
+		}
 	}

 	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {