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@xxxxxxxxxxxxx> wrote:
>> On 05/08/2019 04:11 PM, Geert Uytterhoeven wrote:
>>> On Wed, May 8, 2019 at 12:44 PM <Tudor.Ambarus@xxxxxxxxxxxxx> 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)) {
