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

[<Tudor.Ambarus@microchip.com>]

Hi, Geert,

On 05/08/2019 08:00 PM, Geert Uytterhoeven wrote:
> External E-Mail
> 
> 
> Hi Tudor,>
> On Wed, May 8, 2019 at 4:23 PM <Tudor.Ambarus@microchip.com> wrote:
>> On 05/08/2019 04:11 PM, Geert Uytterhoeven wrote:
>>> On Wed, May 8, 2019 at 12:44 PM <Tudor.Ambarus@microchip.com> wrote:
>>>> Would you run this debug patch on top of mtd/next? I dumped the SR and CR before
>>>> and after the operations in cause.
>>>
>>> Thanks, giving it a try...
>>>
>>>> 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
>>>
>>> Should be moved to the top of the file, before dev_dbg() is defined.
>>>
>>> Result is:
>>>
>>> m25p80 spi0.0: bfpt.dwords[1] = ffffffff
>>> m25p80 spi0.0: bfpt.dwords[2] = ffffffff
>>> m25p80 spi0.0: bfpt.dwords[3] = ffffffff
>>> m25p80 spi0.0: bfpt.dwords[4] = ffffffff
>>> m25p80 spi0.0: bfpt.dwords[5] = ffffffff
>>> m25p80 spi0.0: bfpt.dwords[6] = ffffffff
>>> m25p80 spi0.0: bfpt.dwords[7] = ffffffff
>>> m25p80 spi0.0: bfpt.dwords[8] = ffffffff
>>> m25p80 spi0.0: bfpt.dwords[9] = ffffffff
>>> m25p80 spi0.0: bfpt.dwords[10] = 00000000
>>> m25p80 spi0.0: bfpt.dwords[11] = 00000000
>>> m25p80 spi0.0: bfpt.dwords[12] = 00000000
>>> m25p80 spi0.0: bfpt.dwords[13] = 00000000
>>> m25p80 spi0.0: bfpt.dwords[14] = 00000000
>>> m25p80 spi0.0: bfpt.dwords[15] = 00000000
>>> m25p80 spi0.0: bfpt.dwords[16] = 00000000
>>> m25p80 spi0.0: failed to parse BFPT: err = -22
>>> m25p80 spi0.0: spi_nor_init_params sfdp parse failed, ret =-22
>>> m25p80 spi0.0: SR = 00000000
>>> m25p80 spi0.0: CR = 00000002
>>> m25p80 spi0.0: Erase Error occurred
>>> m25p80 spi0.0: timeout while writing SR, ret = -5
>>> m25p80 spi0.0: SR = 000000ff
>>> m25p80 spi0.0: CR = 000000ff
>>
>> ff can mean that the lines are "in air", maybe the flash resets when we
>> write_sr(nor, 0)? How about adding a delay here to let the flash reset?
> 
> No difference after adding msleep(1000).
> 

When the configuration register QUAD bit CR[1] is 1, only the WRR command format
with 16 data bits may be used, WRR with 8 bits is not recognized and hence the
FFs. You probably set quad bit in u-boot, while others don't. We can verify this
assumption with the patch form below. Can you try it?

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 73172d7f512b..0d636eab3ebf 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2005, Intec Automation Inc.
  * Copyright (C) 2014, Freescale Semiconductor, Inc.
  */
+#define DEBUG

 #include <linux/err.h>
 #include <linux/errno.h>
@@ -200,7 +201,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 +2796,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 +3536,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 +3582,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 +3688,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 +3918,86 @@ 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 sr, cr, sr_cr[2] = {0};
+	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+	ret = read_cr(nor);
+	dev_dbg(nor->dev, "CR = %08x\n", ret);
+        if (ret < 0) {
+                dev_err(nor->dev, "error while reading CR\n");
+		return ret;
+	}
+	cr = ret;
+
+	if (cr & CR_QUAD_EN_SPAN) {
+		/* disable quad if already set, must do it with 16-bit WRR */
+		ret = write_sr_cr(nor, sr_cr);
+		if (ret) {
+			dev_err(nor->dev, "error diasbling quad mode\n");
+			return ret;
+		}
+
+		/* read back and check it */
+		ret = read_cr(nor);
+		if (ret >= 0 && (ret & CR_QUAD_EN_SPAN)) {
+			dev_err(nor->dev, "Spansion Quad bit not cleared\n");
+			return -EINVAL;
+		}
+	}
+
+	/* Clear BP bits with 8-bit WRR */
+	ret = read_sr(nor);
+	dev_dbg(nor->dev, "SR = %08x\n", ret);
+	if (ret < 0) {
+		dev_err(nor->dev, "error while reading SR\n");
+                return ret;
+	}
+	sr = 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, sr & ~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);
+}
+
 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 +4006,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)) {