flash_lock issue for n25q 128mb spi nor part

flash_lock issue for n25q 128mb spi nor part

[John Garry]

Hi guys,

I am testing the following driver on top of Linus' master branch with a 
n25q128a11 part:

https://lore.kernel.org/linux-mtd/1572886297-45400-3-git-send-email-john.garry@huawei.com/

I am finding flash lock is not working. Even after a “successful” lock, 
flash_lock is reporting flash is unlocked. And I can still write to the 
flash.

I find that since 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on 
lock()/unlock()"), we're using a 16b SR for this part in the driver, but 
this part only has a 8b SR.

This hack fixes the problem for me:

@ -4691,7 +4691,7 @@ static void spi_nor_info_init_params(struct spi_nor 
*nor)
params->set_4byte = spansion_set_4byte;
params->setup = spi_nor_default_setup;
/* Default to 16-bit Write Status (01h) Command */
- nor->flags |= SNOR_F_HAS_16BIT_SR;
+// nor->flags |= SNOR_F_HAS_16BIT_SR;

Any idea what's going wrong?

Thanks,
John

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

Re: flash_lock issue for n25q 128mb spi nor part

[Tudor.Ambarus]

Hi, John,

On 12/2/19 7:28 PM, John Garry wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi guys,
> 
> I am testing the following driver on top of Linus' master branch with a
> n25q128a11 part:

I understand the you enabled locking support for this flash on your side,
because in mainline it is not yet there.

Be aware that this flash has BP3 support, and in mainline we support for now
just BP0-BP2, so just a partial lock can be achieved, and it depends on what you
tried to lock. It will be helpful to tell how you enabled the locking and what
commands did you use.

> 
> https://lore.kernel.org/linux-mtd/1572886297-45400-3-git-send-email-john.garry@huawei.com/
> 
> I am finding flash lock is not working. Even after a “successful” lock,
> flash_lock is reporting flash is unlocked. And I can still write to the
> flash.
> 
> I find that since 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on
> lock()/unlock()"), we're using a 16b SR for this part in the driver, but
> this part only has a 8b SR.
> 
> This hack fixes the problem for me:
> 
> @ -4691,7 +4691,7 @@ static void spi_nor_info_init_params(struct spi_nor
> *nor)
> params->set_4byte = spansion_set_4byte;
> params->setup = spi_nor_default_setup;
> /* Default to 16-bit Write Status (01h) Command */
> - nor->flags |= SNOR_F_HAS_16BIT_SR;
> +// nor->flags |= SNOR_F_HAS_16BIT_SR;

This means that bfpt is not parsed. You can confirm this by running with the
patch from below.

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f4afe123e9dc..735cd5202598 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3545,8 +3545,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_err(nor->dev, "bfpt.dwords[%d] = %08x\n", i,
+                       bfpt.dwords[i]);
+       }

        /* Number of address bytes. */
        switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
@@ -3649,6 +3652,9 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
        params->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
        params->page_size = 1U << params->page_size;

+       dev_err(nor->dev, "bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK =
%08x\n",
+               bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK);
+
        /* Quad Enable Requirements. */
        switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
        case BFPT_DWORD15_QER_NONE:
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

On 03/12/2019 09:45, Tudor.Ambarus@microchip.com wrote:
> Hi, John,

Hi Tudor,

Thanks for getting back to me.

> 
> On 12/2/19 7:28 PM, John Garry wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi guys,
>>
>> I am testing the following driver on top of Linus' master branch with a
>> n25q128a11 part:
> 
> I understand the you enabled locking support for this flash on your side,
> because in mainline it is not yet there.
> 
> Be aware that this flash has BP3 support, and in mainline we support for now
> just BP0-BP2, so just a partial lock can be achieved, and it depends on what you
> tried to lock. It will be helpful to tell how you enabled the locking and what
> commands did you use.

So I am simply using flash_lock -l /dev/mtd0, which will try to lock the 
whole device. Now I see what you're saying about BP3 and how complete 
locking is not available.

> 
>>
>> https://lore.kernel.org/linux-mtd/1572886297-45400-3-git-send-email-john.garry@huawei.com/
>>
>> I am finding flash lock is not working. Even after a “successful” lock,
>> flash_lock is reporting flash is unlocked. And I can still write to the
>> flash.
>>
>> I find that since 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on
>> lock()/unlock()"), we're using a 16b SR for this part in the driver, but
>> this part only has a 8b SR.
>>
>> This hack fixes the problem for me:
>>
>> @ -4691,7 +4691,7 @@ static void spi_nor_info_init_params(struct spi_nor
>> *nor)
>> params->set_4byte = spansion_set_4byte;
>> params->setup = spi_nor_default_setup;
>> /* Default to 16-bit Write Status (01h) Command */
>> - nor->flags |= SNOR_F_HAS_16BIT_SR;
>> +// nor->flags |= SNOR_F_HAS_16BIT_SR;
> 
> This means that bfpt is not parsed. You can confirm this by running with the
> patch from below.
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index f4afe123e9dc..735cd5202598 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -3545,8 +3545,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_err(nor->dev, "bfpt.dwords[%d] = %08x\n", i,
> +                       bfpt.dwords[i]);
> +       }
> 
>          /* Number of address bytes. */
>          switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
> @@ -3649,6 +3652,9 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>          params->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
>          params->page_size = 1U << params->page_size;
> 
> +       dev_err(nor->dev, "bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK =
> %08x\n",
> +               bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK);
> +
>          /* Quad Enable Requirements. */
>          switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
>          case BFPT_DWORD15_QER_NONE:
> 

john@ubuntu:~$ dmesg | grep spi
[   14.935740] spi-nor spi-PRP0001:00: bfpt.dwords[0] = fff920e5
[   14.941480] spi-nor spi-PRP0001:00: bfpt.dwords[1] = 07ffffff
[   14.947215] spi-nor spi-PRP0001:00: bfpt.dwords[2] = 6b27eb29
[   14.952949] spi-nor spi-PRP0001:00: bfpt.dwords[3] = bb273b27
[   14.958683] spi-nor spi-PRP0001:00: bfpt.dwords[4] = ffffffff
[   14.964417] spi-nor spi-PRP0001:00: bfpt.dwords[5] = bb27ffff
[   14.970150] spi-nor spi-PRP0001:00: bfpt.dwords[6] = eb29ffff
[   14.975884] spi-nor spi-PRP0001:00: bfpt.dwords[7] = d810200c
[   14.981618] spi-nor spi-PRP0001:00: bfpt.dwords[8] = 0000520f
[   14.987351] spi-nor spi-PRP0001:00: bfpt.dwords[9] = 00994a24
[   14.993085] spi-nor spi-PRP0001:00: bfpt.dwords[10] = c9038e8b
[   14.998906] spi-nor spi-PRP0001:00: bfpt.dwords[11] = 382701ac
[   15.004726] spi-nor spi-PRP0001:00: bfpt.dwords[12] = 757a757a
[   15.010547] spi-nor spi-PRP0001:00: bfpt.dwords[13] = 5cd5bdfb
[   15.016367] spi-nor spi-PRP0001:00: bfpt.dwords[14] = ff820f4a
[   15.022187] spi-nor spi-PRP0001:00: bfpt.dwords[15] = 00003d81

Thanks,
John


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

Re: flash_lock issue for n25q 128mb spi nor part

[Tudor.Ambarus]

On 12/3/19 12:31 PM, John Garry wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 03/12/2019 09:45, Tudor.Ambarus@microchip.com wrote:
>> Hi, John,
> 
> Hi Tudor,
> 
> Thanks for getting back to me.
> 
>>
>> On 12/2/19 7:28 PM, John Garry wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi guys,
>>>
>>> I am testing the following driver on top of Linus' master branch with a
>>> n25q128a11 part:
>>
>> I understand the you enabled locking support for this flash on your side,
>> because in mainline it is not yet there.
>>
>> Be aware that this flash has BP3 support, and in mainline we support for now
>> just BP0-BP2, so just a partial lock can be achieved, and it depends on what you
>> tried to lock. It will be helpful to tell how you enabled the locking and what
>> commands did you use.
> 
> So I am simply using flash_lock -l /dev/mtd0, which will try to lock the
> whole device. Now I see what you're saying about BP3 and how complete
> locking is not available.
> 
>>
>>>
>>> https://lore.kernel.org/linux-mtd/1572886297-45400-3-git-send-email-john.garry@huawei.com/
>>>
>>> I am finding flash lock is not working. Even after a “successful” lock,
>>> flash_lock is reporting flash is unlocked. And I can still write to the
>>> flash.
>>>
>>> I find that since 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on
>>> lock()/unlock()"), we're using a 16b SR for this part in the driver, but
>>> this part only has a 8b SR.
>>>
>>> This hack fixes the problem for me:
>>>
>>> @ -4691,7 +4691,7 @@ static void spi_nor_info_init_params(struct spi_nor
>>> *nor)
>>> params->set_4byte = spansion_set_4byte;
>>> params->setup = spi_nor_default_setup;
>>> /* Default to 16-bit Write Status (01h) Command */
>>> - nor->flags |= SNOR_F_HAS_16BIT_SR;
>>> +// nor->flags |= SNOR_F_HAS_16BIT_SR;
>>
>> This means that bfpt is not parsed. You can confirm this by running with the
>> patch from below.
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index f4afe123e9dc..735cd5202598 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -3545,8 +3545,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_err(nor->dev, "bfpt.dwords[%d] = %08x\n", i,
>> +                       bfpt.dwords[i]);
>> +       }
>>
>>          /* Number of address bytes. */
>>          switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
>> @@ -3649,6 +3652,9 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>>          params->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
>>          params->page_size = 1U << params->page_size;
>>
>> +       dev_err(nor->dev, "bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK =
>> %08x\n",
>> +               bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK);
>> +
>>          /* Quad Enable Requirements. */
>>          switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
>>          case BFPT_DWORD15_QER_NONE:
>>
> 
> john@ubuntu:~$ dmesg | grep spi
> [   14.935740] spi-nor spi-PRP0001:00: bfpt.dwords[0] = fff920e5
> [   14.941480] spi-nor spi-PRP0001:00: bfpt.dwords[1] = 07ffffff
> [   14.947215] spi-nor spi-PRP0001:00: bfpt.dwords[2] = 6b27eb29
> [   14.952949] spi-nor spi-PRP0001:00: bfpt.dwords[3] = bb273b27
> [   14.958683] spi-nor spi-PRP0001:00: bfpt.dwords[4] = ffffffff
> [   14.964417] spi-nor spi-PRP0001:00: bfpt.dwords[5] = bb27ffff
> [   14.970150] spi-nor spi-PRP0001:00: bfpt.dwords[6] = eb29ffff
> [   14.975884] spi-nor spi-PRP0001:00: bfpt.dwords[7] = d810200c
> [   14.981618] spi-nor spi-PRP0001:00: bfpt.dwords[8] = 0000520f
> [   14.987351] spi-nor spi-PRP0001:00: bfpt.dwords[9] = 00994a24
> [   14.993085] spi-nor spi-PRP0001:00: bfpt.dwords[10] = c9038e8b
> [   14.998906] spi-nor spi-PRP0001:00: bfpt.dwords[11] = 382701ac
> [   15.004726] spi-nor spi-PRP0001:00: bfpt.dwords[12] = 757a757a
> [   15.010547] spi-nor spi-PRP0001:00: bfpt.dwords[13] = 5cd5bdfb
> [   15.016367] spi-nor spi-PRP0001:00: bfpt.dwords[14] = ff820f4a
> [   15.022187] spi-nor spi-PRP0001:00: bfpt.dwords[15] = 00003d81
> 

This falls into the BFPT_DWORD15_QER_NONE case. If there's a correspondence
between the BFPT_DWORD15_QER_NONE and no 16-bit Write SR support, we can clear
the flag there, but JESD216D does not specify this :(.

Please enable CONFIG_DEBUG on top of the file to see dev_dbg messages. I'll try
to find a micron flash in the meantime.

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

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

On 03/12/2019 11:07, Tudor.Ambarus@microchip.com wrote:
>>> +               bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK);
>>> +
>>>           /* Quad Enable Requirements. */
>>>           switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
>>>           c

Hi Tudor,

ase BFPT_DWORD15_QER_NONE:
>>>
>> john@ubuntu:~$ dmesg | grep spi
>> [   14.935740] spi-nor spi-PRP0001:00: bfpt.dwords[0] = fff920e5
>> [   14.941480] spi-nor spi-PRP0001:00: bfpt.dwords[1] = 07ffffff
>> [   14.947215] spi-nor spi-PRP0001:00: bfpt.dwords[2] = 6b27eb29
>> [   14.952949] spi-nor spi-PRP0001:00: bfpt.dwords[3] = bb273b27
>> [   14.958683] spi-nor spi-PRP0001:00: bfpt.dwords[4] = ffffffff
>> [   14.964417] spi-nor spi-PRP0001:00: bfpt.dwords[5] = bb27ffff
>> [   14.970150] spi-nor spi-PRP0001:00: bfpt.dwords[6] = eb29ffff
>> [   14.975884] spi-nor spi-PRP0001:00: bfpt.dwords[7] = d810200c
>> [   14.981618] spi-nor spi-PRP0001:00: bfpt.dwords[8] = 0000520f
>> [   14.987351] spi-nor spi-PRP0001:00: bfpt.dwords[9] = 00994a24
>> [   14.993085] spi-nor spi-PRP0001:00: bfpt.dwords[10] = c9038e8b
>> [   14.998906] spi-nor spi-PRP0001:00: bfpt.dwords[11] = 382701ac
>> [   15.004726] spi-nor spi-PRP0001:00: bfpt.dwords[12] = 757a757a
>> [   15.010547] spi-nor spi-PRP0001:00: bfpt.dwords[13] = 5cd5bdfb
>> [   15.016367] spi-nor spi-PRP0001:00: bfpt.dwords[14] = ff820f4a
>> [   15.022187] spi-nor spi-PRP0001:00: bfpt.dwords[15] = 00003d81
>>
> This falls into the BFPT_DWORD15_QER_NONE case. If there's a correspondence
> between the BFPT_DWORD15_QER_NONE and no 16-bit Write SR support, we can clear
> the flag there, but JESD216D does not specify this:(.
> 
> Please enable CONFIG_DEBUG on top of the file to see dev_dbg messages. I'll try
> to find a micron flash in the meantime.
> 

 From what you say, the spec seems to be in conflict/unspecified with 
this micron part in terms of using a 16b SR, so I am not sure what a log 
can reveal.

Anyway here's what I got:

john@ubuntu:~$ dmesg | grep spi
[   14.934354] spi-nor spi-PRP0001:00: bfpt.dwords[0] = fff920e5
[   14.940094] spi-nor spi-PRP0001:00: bfpt.dwords[1] = 07ffffff
[   14.945828] spi-nor spi-PRP0001:00: bfpt.dwords[2] = 6b27eb29
[   14.951563] spi-nor spi-PRP0001:00: bfpt.dwords[3] = bb273b27
[   14.957297] spi-nor spi-PRP0001:00: bfpt.dwords[4] = ffffffff
[   14.963030] spi-nor spi-PRP0001:00: bfpt.dwords[5] = bb27ffff
[   14.968764] spi-nor spi-PRP0001:00: bfpt.dwords[6] = eb29ffff
[   14.974498] spi-nor spi-PRP0001:00: bfpt.dwords[7] = d810200c
[   14.980231] spi-nor spi-PRP0001:00: bfpt.dwords[8] = 0000520f
[   14.985965] spi-nor spi-PRP0001:00: bfpt.dwords[9] = 00994a24
[   14.991699] spi-nor spi-PRP0001:00: bfpt.dwords[10] = c9038e8b
[   14.997520] spi-nor spi-PRP0001:00: bfpt.dwords[11] = 382701ac
[   15.003340] spi-nor spi-PRP0001:00: bfpt.dwords[12] = 757a757a
[   15.009161] spi-nor spi-PRP0001:00: bfpt.dwords[13] = 5cd5bdfb
[   15.014981] spi-nor spi-PRP0001:00: bfpt.dwords[14] = ff820f4a
[   15.020801] spi-nor spi-PRP0001:00: bfpt.dwords[15] = 00003d81
[   15.026624] spi-nor spi-PRP0001:00: bfpt.dwords[BFPT_DWORD(15)] & 
BFPT_DWORD15_QER_MASK =00000000
[   15.035515] spi-nor spi-PRP0001:00: n25q128a11 (16384 Kbytes)
[   15.041250] spi-nor spi-PRP0001:00: mtd .name = spi-PRP0001:00, .size 
= 0x1000000 (16MiB), .erasesize = 0x00001000 (4KiB) .numeraseregions = 0
[   23.338259] spi-nor spi-PRP0001:00: from 0x00ff0000, len 512
[   23.338380] spi-nor spi-PRP0001:00: from 0x00ff0200, len 512
[   23.338497] spi-nor spi-PRP0001:00: from 0x00ff0400, len 512
[   23.338612] spi-nor spi-PRP0001:00: from 0x00ff0600, len 512
[   23.338728] spi-nor spi-PRP0001:00: from 0x00ff0800, len 512
[   23.338844] spi-nor spi-PRP0001:00: from 0x00ff0a00, len 512
[   23.338959] spi-nor spi-PRP0001:00: from 0x00ff0c00, len 512
[   23.339075] spi-nor spi-PRP0001:00: from 0x00ff0e00, len 512
[   23.339237] spi-nor spi-PRP0001:00: from 0x00ffe000, len 512
[   23.339359] spi-nor spi-PRP0001:00: from 0x00ffe200, len 512
[   23.339474] spi-nor spi-PRP0001:00: from 0x00ffe400, len 512
[   23.339589] spi-nor spi-PRP0001:00: from 0x00ffe600, len 512
[   23.339704] spi-nor spi-PRP0001:00: from 0x00ffe800, len 512
[   23.339818] spi-nor spi-PRP0001:00: from 0x00ffea00, len 512
[   23.339933] spi-nor spi-PRP0001:00: from 0x00ffec00, len 512
[   23.340047] spi-nor spi-PRP0001:00: from 0x00ffee00, len 512
[   23.340188] spi-nor spi-PRP0001:00: from 0x00000000, len 512
[   23.340306] spi-nor spi-PRP0001:00: from 0x00000200, len 512
[   23.340422] spi-nor spi-PRP0001:00: from 0x00000400, len 512
[   23.340539] spi-nor spi-PRP0001:00: from 0x00000600, len 512
[   23.3401:00: from 0x00000c00, len 512
[   23.340999] spi-nor spi-PRP0001:00: from 0x00000e00, len 512
[   23.341139] spi-nor spi-PRP0001:00: from 0x00001000, len 512
[   23.341257] spi-nor spi-PRP0001:00: from 0x00001200, len 512

[snip]

[   23.410270] spi-nor spi-PRP0001:00: from 0x0003ce00, len 512
[   23.410386] spi-nor spi-PRP0001:00: from 0x0003d000, len 512
[   23.410502] spi-nor spi-PRP0001:00: from 0x0003d200, len 512
[   23.410617] spi-nor spi-PRP0001:00: from 0x0003d400, len 512
[   23.410733] spi-nor spi-PRP0001:00: from 0x0003d600, len 512
[   23.410849] spi-nor spi-PRP0001:00: from 0x0003d800, len 512
[   23.410964] spi-nor spi-PRP0001:00: from 0x0003da00, len 512
[   23.411080] spi-nor spi-PRP0001:00: from 0x0003dc00, len 512
[   23.411196] spi-nor spi-PRP0001:00: from 0x0003de00, len 512
[   23.411312] spi-nor spi-PRP0001:00: from 0x0003e000, len 512
[   23.411428] spi-nor spi-PRP0001:00: from 0x0003e200, len 512
[   23.411543] spi-nor spi-PRP0001:00: from 0x0003e400, len 512
[   23.411659] spi-nor spi-PRP0001:00: from 0x0003e600, len 512
[   23.411775] spi-nor spi-PRP0001:00: from 0x0003e800, len 512
[   23.411891] spi-nor spi-PRP0001:00: from 0x0003ea00, len 512
[   23.412007] spi-nor spi-PRP0001:00: from 0x0003ec00, len 512
[   23.412122] spi-nor spi-PRP0001:00: from 0x0003ee00, len 512
[   23.412239] spi-nor spi-PRP0001:00: from 0x0003f000, len 512
[   23.412354] spi-nor spi-PRP0001:00: from 0x0003f200, len 512
[   23.412470] spi-nor spi-PRP0001:00: from 0x0003f400, len 512
[   23.412587] spi-nor spi-PRP0001:00: from 0x0003f600, len 512
[   23.412704] spi-nor spi-PRP0001:00: from 0x0003f800, len 512
[   23.412819] spi-nor spi-PRP0001:00: from 0x0003fa00, len 512
[   23.412935] spi-nor spi-PRP00 512
[   23.413257] spi-nor spi-PRP0001:00: from 0x00040000, len 512
[   23.413375] spi-nor spi-PRP0001:00: from 0x00040200, len 512
[   23.413490] spi-nor spi-PRP0001:00: from 0x00040400, len 512
[   23.413605] spi-nor spi-PRP0001:00: from 0x00040600, len 512
[   23.413720] spi-nor spi-PRP0001:00: from 0x00040800, len 512
[   23.413835] spi-nor spi-PRP0001:00: from 0x00040a00, len 512
[   23.413950] spi-nor spi-PRP0001:00: from 0x00040c00, len 512
[   23.414065] spi-nor spi-PRP0001:00: from 0x00040e00, len 512
[   23.414210] spi-nor spi-PRP0001:00: from 0x00200000, len 512
[   23.414328] spi-nor spi-PRP0001:00: from 0x00200200, len 512
[   23.414444] spi-nor spi-PRP0001:00: from 0x00200400, len 512
[   23.414560] spi-nor spi-PRP0001:00: from 0x00200600, len 512
[   23.414676] spi-nor spi-PRP0001:00: from 0x00200800, len 512
[   23.414792] spi-nor spi-PRP0001:00: from 0x00200a00, len 512
[   23.414908] spi-nor spi-PRP0001:00: from 0x00200c00, len 512
[   23.415024] spi-nor spi-PRP0001:00: from 0x00200e00, len 512
john@ubuntu:~$
john@ubuntu:~$
john@ubuntu:~$
john@ubuntu:~$ sudo su
[sudo] password for john:
root@ubuntu:/home/john# flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: unlocked
Return code: 0
root@ubuntu:/home/john# flash_lock -l /dev/mtd0 0x800000 0x800000
root@ubuntu:/home/john# flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: unlocked
Return code: 0
root@ubuntu:/home/john#


Thanks,
John

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

Re: flash_lock issue for n25q 128mb spi nor part

[Tudor.Ambarus]

On 12/3/19 1:44 PM, John Garry wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 03/12/2019 11:07, Tudor.Ambarus@microchip.com wrote:
>>>> +               bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK);
>>>> +
>>>>           /* Quad Enable Requirements. */
>>>>           switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
>>>>           c
> 
> Hi Tudor,
> 
> ase BFPT_DWORD15_QER_NONE:
>>>>
>>> john@ubuntu:~$ dmesg | grep spi
>>> [   14.935740] spi-nor spi-PRP0001:00: bfpt.dwords[0] = fff920e5
>>> [   14.941480] spi-nor spi-PRP0001:00: bfpt.dwords[1] = 07ffffff
>>> [   14.947215] spi-nor spi-PRP0001:00: bfpt.dwords[2] = 6b27eb29
>>> [   14.952949] spi-nor spi-PRP0001:00: bfpt.dwords[3] = bb273b27
>>> [   14.958683] spi-nor spi-PRP0001:00: bfpt.dwords[4] = ffffffff
>>> [   14.964417] spi-nor spi-PRP0001:00: bfpt.dwords[5] = bb27ffff
>>> [   14.970150] spi-nor spi-PRP0001:00: bfpt.dwords[6] = eb29ffff
>>> [   14.975884] spi-nor spi-PRP0001:00: bfpt.dwords[7] = d810200c
>>> [   14.981618] spi-nor spi-PRP0001:00: bfpt.dwords[8] = 0000520f
>>> [   14.987351] spi-nor spi-PRP0001:00: bfpt.dwords[9] = 00994a24
>>> [   14.993085] spi-nor spi-PRP0001:00: bfpt.dwords[10] = c9038e8b
>>> [   14.998906] spi-nor spi-PRP0001:00: bfpt.dwords[11] = 382701ac
>>> [   15.004726] spi-nor spi-PRP0001:00: bfpt.dwords[12] = 757a757a
>>> [   15.010547] spi-nor spi-PRP0001:00: bfpt.dwords[13] = 5cd5bdfb
>>> [   15.016367] spi-nor spi-PRP0001:00: bfpt.dwords[14] = ff820f4a
>>> [   15.022187] spi-nor spi-PRP0001:00: bfpt.dwords[15] = 00003d81
>>>
>> This falls into the BFPT_DWORD15_QER_NONE case. If there's a correspondence
>> between the BFPT_DWORD15_QER_NONE and no 16-bit Write SR support, we can clear
>> the flag there, but JESD216D does not specify this:(.
>>
>> Please enable CONFIG_DEBUG on top of the file to see dev_dbg messages. I'll try
>> to find a micron flash in the meantime.
>>
> 
> From what you say, the spec seems to be in conflict/unspecified with
> this micron part in terms of using a 16b SR, so I am not sure what a log
> can reveal.
> 
> Anyway here's what I got:
> 
> john@ubuntu:~$ dmesg | grep spi
> [   14.934354] spi-nor spi-PRP0001:00: bfpt.dwords[0] = fff920e5
> [   14.940094] spi-nor spi-PRP0001:00: bfpt.dwords[1] = 07ffffff
> [   14.945828] spi-nor spi-PRP0001:00: bfpt.dwords[2] = 6b27eb29
> [   14.951563] spi-nor spi-PRP0001:00: bfpt.dwords[3] = bb273b27
> [   14.957297] spi-nor spi-PRP0001:00: bfpt.dwords[4] = ffffffff
> [   14.963030] spi-nor spi-PRP0001:00: bfpt.dwords[5] = bb27ffff
> [   14.968764] spi-nor spi-PRP0001:00: bfpt.dwords[6] = eb29ffff
> [   14.974498] spi-nor spi-PRP0001:00: bfpt.dwords[7] = d810200c
> [   14.980231] spi-nor spi-PRP0001:00: bfpt.dwords[8] = 0000520f
> [   14.985965] spi-nor spi-PRP0001:00: bfpt.dwords[9] = 00994a24
> [   14.991699] spi-nor spi-PRP0001:00: bfpt.dwords[10] = c9038e8b
> [   14.997520] spi-nor spi-PRP0001:00: bfpt.dwords[11] = 382701ac
> [   15.003340] spi-nor spi-PRP0001:00: bfpt.dwords[12] = 757a757a
> [   15.009161] spi-nor spi-PRP0001:00: bfpt.dwords[13] = 5cd5bdfb
> [   15.014981] spi-nor spi-PRP0001:00: bfpt.dwords[14] = ff820f4a
> [   15.020801] spi-nor spi-PRP0001:00: bfpt.dwords[15] = 00003d81
> [   15.026624] spi-nor spi-PRP0001:00: bfpt.dwords[BFPT_DWORD(15)] &
> BFPT_DWORD15_QER_MASK =00000000
> [   15.035515] spi-nor spi-PRP0001:00: n25q128a11 (16384 Kbytes)
> [   15.041250] spi-nor spi-PRP0001:00: mtd .name = spi-PRP0001:00, .size
> = 0x1000000 (16MiB), .erasesize = 0x00001000 (4KiB) .numeraseregions = 0
> [   23.338259] spi-nor spi-PRP0001:00: from 0x00ff0000, len 512
> [   23.338380] spi-nor spi-PRP0001:00: from 0x00ff0200, len 512
> [   23.338497] spi-nor spi-PRP0001:00: from 0x00ff0400, len 512
> [   23.338612] spi-nor spi-PRP0001:00: from 0x00ff0600, len 512
> [   23.338728] spi-nor spi-PRP0001:00: from 0x00ff0800, len 512
> [   23.338844] spi-nor spi-PRP0001:00: from 0x00ff0a00, len 512
> [   23.338959] spi-nor spi-PRP0001:00: from 0x00ff0c00, len 512
> [   23.339075] spi-nor spi-PRP0001:00: from 0x00ff0e00, len 512
> [   23.339237] spi-nor spi-PRP0001:00: from 0x00ffe000, len 512
> [   23.339359] spi-nor spi-PRP0001:00: from 0x00ffe200, len 512
> [   23.339474] spi-nor spi-PRP0001:00: from 0x00ffe400, len 512
> [   23.339589] spi-nor spi-PRP0001:00: from 0x00ffe600, len 512
> [   23.339704] spi-nor spi-PRP0001:00: from 0x00ffe800, len 512
> [   23.339818] spi-nor spi-PRP0001:00: from 0x00ffea00, len 512
> [   23.339933] spi-nor spi-PRP0001:00: from 0x00ffec00, len 512
> [   23.340047] spi-nor spi-PRP0001:00: from 0x00ffee00, len 512
> [   23.340188] spi-nor spi-PRP0001:00: from 0x00000000, len 512
> [   23.340306] spi-nor spi-PRP0001:00: from 0x00000200, len 512
> [   23.340422] spi-nor spi-PRP0001:00: from 0x00000400, len 512
> [   23.340539] spi-nor spi-PRP0001:00: from 0x00000600, len 512
> [   23.3401:00: from 0x00000c00, len 512
> [   23.340999] spi-nor spi-PRP0001:00: from 0x00000e00, len 512
> [   23.341139] spi-nor spi-PRP0001:00: from 0x00001000, len 512
> [   23.341257] spi-nor spi-PRP0001:00: from 0x00001200, len 512
> 
> [snip]
> 
> [   23.410270] spi-nor spi-PRP0001:00: from 0x0003ce00, len 512
> [   23.410386] spi-nor spi-PRP0001:00: from 0x0003d000, len 512
> [   23.410502] spi-nor spi-PRP0001:00: from 0x0003d200, len 512
> [   23.410617] spi-nor spi-PRP0001:00: from 0x0003d400, len 512
> [   23.410733] spi-nor spi-PRP0001:00: from 0x0003d600, len 512
> [   23.410849] spi-nor spi-PRP0001:00: from 0x0003d800, len 512
> [   23.410964] spi-nor spi-PRP0001:00: from 0x0003da00, len 512
> [   23.411080] spi-nor spi-PRP0001:00: from 0x0003dc00, len 512
> [   23.411196] spi-nor spi-PRP0001:00: from 0x0003de00, len 512
> [   23.411312] spi-nor spi-PRP0001:00: from 0x0003e000, len 512
> [   23.411428] spi-nor spi-PRP0001:00: from 0x0003e200, len 512
> [   23.411543] spi-nor spi-PRP0001:00: from 0x0003e400, len 512
> [   23.411659] spi-nor spi-PRP0001:00: from 0x0003e600, len 512
> [   23.411775] spi-nor spi-PRP0001:00: from 0x0003e800, len 512
> [   23.411891] spi-nor spi-PRP0001:00: from 0x0003ea00, len 512
> [   23.412007] spi-nor spi-PRP0001:00: from 0x0003ec00, len 512
> [   23.412122] spi-nor spi-PRP0001:00: from 0x0003ee00, len 512
> [   23.412239] spi-nor spi-PRP0001:00: from 0x0003f000, len 512
> [   23.412354] spi-nor spi-PRP0001:00: from 0x0003f200, len 512
> [   23.412470] spi-nor spi-PRP0001:00: from 0x0003f400, len 512
> [   23.412587] spi-nor spi-PRP0001:00: from 0x0003f600, len 512
> [   23.412704] spi-nor spi-PRP0001:00: from 0x0003f800, len 512
> [   23.412819] spi-nor spi-PRP0001:00: from 0x0003fa00, len 512
> [   23.412935] spi-nor spi-PRP00 512
> [   23.413257] spi-nor spi-PRP0001:00: from 0x00040000, len 512
> [   23.413375] spi-nor spi-PRP0001:00: from 0x00040200, len 512
> [   23.413490] spi-nor spi-PRP0001:00: from 0x00040400, len 512
> [   23.413605] spi-nor spi-PRP0001:00: from 0x00040600, len 512
> [   23.413720] spi-nor spi-PRP0001:00: from 0x00040800, len 512
> [   23.413835] spi-nor spi-PRP0001:00: from 0x00040a00, len 512
> [   23.413950] spi-nor spi-PRP0001:00: from 0x00040c00, len 512
> [   23.414065] spi-nor spi-PRP0001:00: from 0x00040e00, len 512
> [   23.414210] spi-nor spi-PRP0001:00: from 0x00200000, len 512
> [   23.414328] spi-nor spi-PRP0001:00: from 0x00200200, len 512
> [   23.414444] spi-nor spi-PRP0001:00: from 0x00200400, len 512
> [   23.414560] spi-nor spi-PRP0001:00: from 0x00200600, len 512
> [   23.414676] spi-nor spi-PRP0001:00: from 0x00200800, len 512
> [   23.414792] spi-nor spi-PRP0001:00: from 0x00200a00, len 512
> [   23.414908] spi-nor spi-PRP0001:00: from 0x00200c00, len 512
> [   23.415024] spi-nor spi-PRP0001:00: from 0x00200e00, len 512
> john@ubuntu:~$
> john@ubuntu:~$
> john@ubuntu:~$
> john@ubuntu:~$ sudo su
> [sudo] password for john:
> root@ubuntu:/home/john# flash_lock -i /dev/mtd0
> Device: /dev/mtd0
> Start: 0
> Len: 0x1000000
> Lock status: unlocked
> Return code: 0
> root@ubuntu:/home/john# flash_lock -l /dev/mtd0 0x800000 0x800000
> root@ubuntu:/home/john# flash_lock -i /dev/mtd0
> Device: /dev/mtd0
> Start: 0
> Len: 0x1000000
> Lock status: unlocked
> Return code: 0
> root@ubuntu:/home/john#
> 

this should reveal what happens after a lock, with the 16 bit status reg write:

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f4afe123e9dc..f1490c7b5cb9 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1033,10 +1033,19 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
 
        sr_cr[0] = sr1;
 
+       dev_err(nor->dev, "before write: sr_cr[0] = %02x, sr_cr[1] = %02x\n",
+               sr_cr[0], sr_cr[1]);
+
        ret = spi_nor_write_sr(nor, sr_cr, 2);
        if (ret)
                return ret;
 
+       ret = spi_nor_read_sr(nor, &sr_cr[0]);
+       if (ret)
+               return ret;
+
+       dev_err(nor->dev, "read back sr1: sr_cr[0] = %02x\n", sr_cr[0]);
+
        if (nor->flags & SNOR_F_NO_READ_CR)
                return 0;
 
@@ -1046,6 +1055,8 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
        if (ret)
                return ret;
 
+       dev_err(nor->dev, "read back sr2: sr_cr[1] = %02x\n", sr_cr[1]);
+
        if (cr_written != sr_cr[1]) {
                dev_dbg(nor->dev, "CR: read back test failed\n");
                return -EIO;

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

Re: flash_lock issue for n25q 128mb spi nor part

[Tudor.Ambarus]

On 12/3/19 2:05 PM, Tudor.Ambarus@microchip.com wrote:
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index f4afe123e9dc..f1490c7b5cb9 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1033,10 +1033,19 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
> 
>         sr_cr[0] = sr1;
> 
> +       dev_err(nor->dev, "before write: sr_cr[0] = %02x, sr_cr[1] = %02x\n",
> +               sr_cr[0], sr_cr[1]);
> +
>         ret = spi_nor_write_sr(nor, sr_cr, 2);
>         if (ret)
>                 return ret;
> 
> +       ret = spi_nor_read_sr(nor, &sr_cr[0]);
> +       if (ret)
> +               return ret;
> +
> +       dev_err(nor->dev, "read back sr1: sr_cr[0] = %02x\n", sr_cr[0]);
> +
>         if (nor->flags & SNOR_F_NO_READ_CR)
>                 return 0;
> 
> @@ -1046,6 +1055,8 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
>         if (ret)
>                 return ret;
> 
> +       dev_err(nor->dev, "read back sr2: sr_cr[1] = %02x\n", sr_cr[1]);
> +
>         if (cr_written != sr_cr[1]) {
>                 dev_dbg(nor->dev, "CR: read back test failed\

On n25q256a I obtain:

root@sama5d2-xplained-sd:~# flash_lock -l /dev/mtd1
spi-nor spi1.0: before write: sr_cr[0] = 9e, sr_cr[1] = ff
spi-nor spi1.0: read back sr1: sr_cr[0] = 02
spi-nor spi1.0: read back sr2: sr_cr[1] = ff

the 16 bit write SR does not execute correctly and the WE remains set. If
neither of the micron flashes do not support the 16 bit write SR, we can add a
condition based on MFR. Let me check few datasheets.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

On 03/12/2019 12:27, Tudor.Ambarus@microchip.com wrote:
> 
> 
> On 12/3/19 2:05 PM, Tudor.Ambarus@microchip.com wrote:
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index f4afe123e9dc..f1490c7b5cb9 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1033,10 +1033,19 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
>>
>>          sr_cr[0] = sr1;
>>
>> +       dev_err(nor->dev, "before write: sr_cr[0] = %02x, sr_cr[1] = %02x\n",
>> +               sr_cr[0], sr_cr[1]);
>> +
>>          ret = spi_nor_write_sr(nor, sr_cr, 2);
>>          if (ret)
>>                  return ret;
>>
>> +       ret = spi_nor_read_sr(nor, &sr_cr[0]);
>> +       if (ret)
>> +               return ret;
>> +
>> +       dev_err(nor->dev, "read back sr1: sr_cr[0] = %02x\n", sr_cr[0]);
>> +
>>          if (nor->flags & SNOR_F_NO_READ_CR)
>>                  return 0;
>>
>> @@ -1046,6 +1055,8 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
>>          if (ret)
>>                  return ret;
>>
>> +       dev_err(nor->dev, "read back sr2: sr_cr[1] = %02x\n", sr_cr[1]);
>> +
>>          if (cr_written != sr_cr[1]) {
>>                  dev_dbg(nor->dev, "CR: read back test failed\
> 
> On n25q256a I obtain:
> 
> root@sama5d2-xplained-sd:~# flash_lock -l /dev/mtd1
> spi-nor spi1.0: before write: sr_cr[0] = 9e, sr_cr[1] = ff
> spi-nor spi1.0: read back sr1: sr_cr[0] = 02
> spi-nor spi1.0: read back sr2: sr_cr[1] = ff
> 

So here is what I get:

root@ubuntu:/home/john# flash_lock -l /dev/mtd0
[  109.037492] spi-nor spi-PRP0001:00: before write: sr_cr[0] = 9e, 
sr_cr[1] = 00
[  109.044769] spi-nor spi-PRP0001:00: read back sr1: sr_cr[0] = 02
[  109.050790] spi-nor spi-PRP0001:00: read back sr2: sr_cr[1] = 00


> the 16 bit write SR does not execute correctly and the WE remains set. If
> neither of the micron flashes do not support the 16 bit write SR, we can add a
> condition based on MFR. Let me check few datasheets.
> 

OK, thanks.

John


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

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

On 03/12/2019 12:35, John Garry wrote:
> On 03/12/2019 12:27, Tudor.Ambarus@microchip.com wrote:
>>
>>
>> On 12/3/19 2:05 PM, Tudor.Ambarus@microchip.com wrote:
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c 
>>> b/drivers/mtd/spi-nor/spi-nor.c
>>> index f4afe123e9dc..f1490c7b5cb9 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -1033,10 +1033,19 @@ static int 
>>> spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
>>>
>>>          sr_cr[0] = sr1;
>>>
>>> +       dev_err(nor->dev, "before write: sr_cr[0] = %02x, sr_cr[1] = 
>>> %02x\n",
>>> +               sr_cr[0], sr_cr[1]);
>>> +
>>>          ret = spi_nor_write_sr(nor, sr_cr, 2);
>>>          if (ret)
>>>                  return ret;
>>>
>>> +       ret = spi_nor_read_sr(nor, &sr_cr[0]);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       dev_err(nor->dev, "read back sr1: sr_cr[0] = %02x\n", sr_cr[0]);
>>> +
>>>          if (nor->flags & SNOR_F_NO_READ_CR)
>>>                  return 0;
>>>
>>> @@ -1046,6 +1055,8 @@ static int 
>>> spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
>>>          if (ret)
>>>                  return ret;
>>>
>>> +       dev_err(nor->dev, "read back sr2: sr_cr[1] = %02x\n", sr_cr[1]);
>>> +
>>>          if (cr_written != sr_cr[1]) {
>>>                  dev_dbg(nor->dev, "CR: read back test failed\
>>
>> On n25q256a I obtain:
>>
>> root@sama5d2-xplained-sd:~# flash_lock -l /dev/mtd1
>> spi-nor spi1.0: before write: sr_cr[0] = 9e, sr_cr[1] = ff
>> spi-nor spi1.0: read back sr1: sr_cr[0] = 02
>> spi-nor spi1.0: read back sr2: sr_cr[1] = ff
>>
> 
> So here is what I get:
> 
> root@ubuntu:/home/john# flash_lock -l /dev/mtd0
> [  109.037492] spi-nor spi-PRP0001:00: before write: sr_cr[0] = 9e, 
> sr_cr[1] = 00
> [  109.044769] spi-nor spi-PRP0001:00: read back sr1: sr_cr[0] = 02
> [  109.050790] spi-nor spi-PRP0001:00: read back sr2: sr_cr[1] = 00
> 
> 
>> the 16 bit write SR does not execute correctly and the WE remains set. If
>> neither of the micron flashes do not support the 16 bit write SR, we 
>> can add a
>> condition based on MFR. Let me check few datasheets.
>>
> 
> OK, thanks.

Hi Tudor,

Something else which I wanted to ask is why we don't check the FSR after 
a write command for that part?

Without it, if the flash is locked, a flash write reports no error, 
which could be maybe misleading.

A more real problem is that I find that SR.WEL is not cleared when we 
attempt to write when the flash is locked, and this causes issues when 
we attempt to reset the SR.

Here's an example flow (with my hack to stop using 16b SR method):

root@ubuntu:/home/john# flash_lock -l /dev/mtd0
root@ubuntu:/home/john# mtd_debug erase /dev/mtd0 0xe00000 4096
[   69.650642] spi-nor spi-PRP0001:00: at 0xe00000, len 4096
Erased 4096 bytes from address 0x00e00000 in flash
root@ubuntu:/home/john# mtd_debug write /dev/mtd0 0xe00000 4096 dump4096
[   77.093755] spi-nor spi-PRP0001:00: to 0x00e00000, len 4096
Copied 4096 bytes from dump4096 to address 0x00e00000 in flash
root@ubuntu:/home/john# mtd_debug read /dev/mtd0 0xe00000 4096 temp
[   82.162445] spi-nor spi-PRP0001:00: from 0x00e00000, len 4096
Copied 4096 bytes from address 0x00e00000 in flash to temp
root@ubuntu:/home/john# flash_lock -u /dev/mtd0
[   87.558435] spi-nor spi-PRP0001:00: SR1: read back test failed
flash_lock: error!: could not unlock device: /dev/mtd0

             error 5 (Input/output error)
root@ubuntu:/home/john#

Unlock reports an error as the the read back test in 
spi_nor_write_sr1_and_check() fails as the SR.WEL has never been cleared.

I'm just saying this while it's fresh in my head - I don't want to seem 
pushy :)

Thanks,
John

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

[PATCH] mtd: spi-nor: Fix the write Status Register on micron flashes

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Micron flashes do not support 16 bit writes on the Status Register.
According to micron datasheets, when using the Write Status Register
(01h) command, the chip select should be driven LOW and held LOW until
the eighth bit of the last data byte has been latched in, after which
it must be driven HIGH. If CS is not driven HIGH, the command is not
executed, flag status register error bits are not set, and the write enable
latch remains set to 1. This fixes the lock operations on micron flashes.

Reported-by: John Garry <john.garry@huawei.com>
Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f1490c7b5cb9..7e41493f69d8 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4607,6 +4607,7 @@ static void sst_set_default_init(struct spi_nor *nor)
 static void st_micron_set_default_init(struct spi_nor *nor)
 {
 	nor->flags |= SNOR_F_HAS_LOCK;
+	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
 	nor->params.quad_enable = NULL;
 	nor->params.set_4byte = st_micron_set_4byte;
 }
-- 
2.14.5


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

Re: flash_lock issue for n25q 128mb spi nor part

[Tudor.Ambarus]

> Hi Tudor,

Hi, John,

> 
> Something else which I wanted to ask is why we don't check the FSR after
> a write command for that part?

no particular reason, no one asked for it :).

> 
> Without it, if the flash is locked, a flash write reports no error,
> which could be maybe misleading.

true

> 
> A more real problem is that I find that SR.WEL is not cleared when we
> attempt to write when the flash is locked, and this causes issues when
> we attempt to reset the SR.
> 
> Here's an example flow (with my hack to stop using 16b SR method):
> 
> root@ubuntu:/home/john# flash_lock -l /dev/mtd0
> root@ubuntu:/home/john# mtd_debug erase /dev/mtd0 0xe00000 4096
> [   69.650642] spi-nor spi-PRP0001:00: at 0xe00000, len 4096
> Erased 4096 bytes from address 0x00e00000 in flash
> root@ubuntu:/home/john# mtd_debug write /dev/mtd0 0xe00000 4096 dump4096
> [   77.093755] spi-nor spi-PRP0001:00: to 0x00e00000, len 4096
> Copied 4096 bytes from dump4096 to address 0x00e00000 in flash
> root@ubuntu:/home/john# mtd_debug read /dev/mtd0 0xe00000 4096 temp
> [   82.162445] spi-nor spi-PRP0001:00: from 0x00e00000, len 4096
> Copied 4096 bytes from address 0x00e00000 in flash to temp
> root@ubuntu:/home/john# flash_lock -u /dev/mtd0
> [   87.558435] spi-nor spi-PRP0001:00: SR1: read back test failed
> flash_lock: error!: could not unlock device: /dev/mtd0
> 
>             error 5 (Input/output error)
> root@ubuntu:/home/john#
> 
> Unlock reports an error as the the read back test in
> spi_nor_write_sr1_and_check() fails as the SR.WEL has never been cleared.
> 

Interesting.


Does the following do the trick?

-       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K |
SPI_NOR_QUAD_READ) },
+       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K | USE_FSR |
+                             SPI_NOR_QUAD_READ) },

This can of course be extended to all micron flashes, if all support FSR, but
some documentation work has to be made.

> I'm just saying this while it's fresh in my head - I don't want to seem
> pushy :)

No worries, this is a good initiative, thanks.

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

[PATCH v2] mtd: spi-nor: Fix the writing of the Status Register on micron flashes

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Micron flashes do not support 16 bit writes on the Status Register.
According to micron datasheets, when using the Write Status Register
(01h) command, the chip select should be driven LOW and held LOW until
the eighth bit of the last data byte has been latched in, after which
it must be driven HIGH. If CS is not driven HIGH, the command is not
executed, flag status register error bits are not set, and the write enable
latch remains set to 1. This fixes the lock operations on micron flashes.

Reported-by: John Garry <john.garry@huawei.com>
Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v2: reword commit subject

 drivers/mtd/spi-nor/spi-nor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f1490c7b5cb9..7e41493f69d8 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4607,6 +4607,7 @@ static void sst_set_default_init(struct spi_nor *nor)
 static void st_micron_set_default_init(struct spi_nor *nor)
 {
 	nor->flags |= SNOR_F_HAS_LOCK;
+	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
 	nor->params.quad_enable = NULL;
 	nor->params.set_4byte = st_micron_set_4byte;
 }
-- 
2.14.5


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

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

>>
>> Here's an example flow (with my hack to stop using 16b SR method):
>>
>> root@ubuntu:/home/john# flash_lock -l /dev/mtd0
>> root@ubuntu:/home/john# mtd_debug erase /dev/mtd0 0xe00000 4096
>> [   69.650642] spi-nor spi-PRP0001:00: at 0xe00000, len 4096
>> Erased 4096 bytes from address 0x00e00000 in flash
>> root@ubuntu:/home/john# mtd_debug write /dev/mtd0 0xe00000 4096 dump4096
>> [   77.093755] spi-nor spi-PRP0001:00: to 0x00e00000, len 4096
>> Copied 4096 bytes from dump4096 to address 0x00e00000 in flash
>> root@ubuntu:/home/john# mtd_debug read /dev/mtd0 0xe00000 4096 temp
>> [   82.162445] spi-nor spi-PRP0001:00: from 0x00e00000, len 4096
>> Copied 4096 bytes from address 0x00e00000 in flash to temp
>> root@ubuntu:/home/john# flash_lock -u /dev/mtd0
>> [   87.558435] spi-nor spi-PRP0001:00: SR1: read back test failed
>> flash_lock: error!: could not unlock device: /dev/mtd0
>>
>>              error 5 (Input/output error)
>> root@ubuntu:/home/john#
>>
>> Unlock reports an error as the the read back test in
>> spi_nor_write_sr1_and_check() fails as the SR.WEL has never been cleared.
>>
> 
> Interesting.

I note that spi_nor_erase() exits with a call to 
spi_nor_write_disable(), yet spi_nor_write() doesn't - maybe we should 
add a similar call there.

> 
> 
> Does the following do the trick?
> 
> -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K |
> SPI_NOR_QUAD_READ) },
> +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K | USE_FSR |
> +                             SPI_NOR_QUAD_READ) },
> 

I'll give it a spin - along with your SR lock fix - when I get back on 
the HW again.

> This can of course be extended to all micron flashes, if all support FSR, but
> some documentation work has to be made.
> 
>> I'm just saying this while it's fresh in my head - I don't want to seem
>> pushy :)
> 
> No worries, this is a good initiative, thanks.
> 

Much appreciated,
John


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

Re: [PATCH v2] mtd: spi-nor: Fix the writing of the Status Register on micron flashes

[John Garry]

On 03/12/2019 14:50, Tudor.Ambarus@microchip.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Micron flashes do not support 16 bit writes on the Status Register.
> According to micron datasheets, when using the Write Status Register
> (01h) command, the chip select should be driven LOW and held LOW until
> the eighth bit of the last data byte has been latched in, after which
> it must be driven HIGH. If CS is not driven HIGH, the command is not
> executed, flag status register error bits are not set, and the write enable
> latch remains set to 1. This fixes the lock operations on micron flashes.
> 
> Reported-by: John Garry <john.garry@huawei.com>
> Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Thanks,

Tested-by: John Garry <john.garry@huawei.com>

> ---
> v2: reword commit subject
> 
>   drivers/mtd/spi-nor/spi-nor.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index f1490c7b5cb9..7e41493f69d8 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -4607,6 +4607,7 @@ static void sst_set_default_init(struct spi_nor *nor)
>   static void st_micron_set_default_init(struct spi_nor *nor)
>   {
>   	nor->flags |= SNOR_F_HAS_LOCK;
> +	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
>   	nor->params.quad_enable = NULL;
>   	nor->params.set_4byte = st_micron_set_4byte;
>   }
> 


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

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

On 03/12/2019 15:29, John Garry wrote:
>>>
>>> Here's an example flow (with my hack to stop using 16b SR method):
>>>
>>> root@ubuntu:/home/john# flash_lock -l /dev/mtd0
>>> root@ubuntu:/home/john# mtd_debug erase /dev/mtd0 0xe00000 4096
>>> [   69.650642] spi-nor spi-PRP0001:00: at 0xe00000, len 4096
>>> Erased 4096 bytes from address 0x00e00000 in flash
>>> root@ubuntu:/home/john# mtd_debug write /dev/mtd0 0xe00000 4096 dump4096
>>> [   77.093755] spi-nor spi-PRP0001:00: to 0x00e00000, len 4096
>>> Copied 4096 bytes from dump4096 to address 0x00e00000 in flash
>>> root@ubuntu:/home/john# mtd_debug read /dev/mtd0 0xe00000 4096 temp
>>> [   82.162445] spi-nor spi-PRP0001:00: from 0x00e00000, len 4096
>>> Copied 4096 bytes from address 0x00e00000 in flash to temp
>>> root@ubuntu:/home/john# flash_lock -u /dev/mtd0
>>> [   87.558435] spi-nor spi-PRP0001:00: SR1: read back test failed
>>> flash_lock: error!: could not unlock device: /dev/mtd0
>>>
>>>              error 5 (Input/output error)
>>> root@ubuntu:/home/john#
>>>
>>> Unlock reports an error as the the read back test in
>>> spi_nor_write_sr1_and_check() fails as the SR.WEL has never been 
>>> cleared.
>>>
>>
>> Interesting.
> 
> I note that spi_nor_erase() exits with a call to 
> spi_nor_write_disable(), yet spi_nor_write() doesn't - maybe we should 
> add a similar call there.

So this remedies that issue for me:

commit c90de583d81f7c5c6cb1c8c021108a0e7e244b91 (HEAD)
Author: John Garry <john.garry@huawei.com>
Date:   Wed Dec 4 10:26:49 2019 +0000

     Ensure we clear SR.WEL for any write failure due to locking

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index aeb3ad2dbfb8..3f12335eb20c 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2875,6 +2875,8 @@ static int spi_nor_write(struct mtd_info *mtd, 
loff_t to, size_t len,
                 i += written;
         }

+       ret = spi_nor_write_disable(nor);
+
  write_err:
         spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
         return ret;


> 
>>
>>
>> Does the following do the trick?
>>
>> -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K |
>> SPI_NOR_QUAD_READ) },
>> +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K | 
>> USE_FSR |
>> +                             SPI_NOR_QUAD_READ) },
>>

Yes, that works and it's nice to not have the silent failures:

root@ubuntu:/home/john# flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: locked
Return code: 1
root@ubuntu:/home/john#
root@ubuntu:/home/john# mtd_debug erase /dev/mtd0 0xe00000 4096
[   45.023353] spi-nor spi0.0: Erase operation failed.
[   45.028257] spi-nor spi0.0: Attempted to modify a protected sector.
MEMERASE: Input/output error
mtd_debug write /dev/mtd0 0xe00000 4096 dump4096
[   50.212013] spi-nor spi0.0: Program operation failed.
[   50.217084] spi-nor spi0.0: Attempted to modify a protected sector.
file_to_flash: write, size 0x1000, n 0x1000
write(): Input/output error
root@ubuntu:/home/john# mtd_debug read /dev/mtd0 0xe00000 4096 temp
Copied 4096 bytes from address 0x00e00000 in flash to temp
root@ubuntu:/home/john# flash_lock -u /dev/mtd0
flash_lock: error!: could not unlock device: /dev/mtd0

             error 5 (Input/output error)
root@ubuntu:/home/john# flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: unlocked
Return code: 0
root@ubuntu:/home/john# flash_lock -u /dev/mtd0
root@ubuntu:/home/john#

But, as you may see, it seems that my change to spi_nor_write() is still 
required to stop the first unlock failure message, but it needs to be 
relocated to after write_err label, as we now jump there for 
spi_nor_wait_till_ready() failure. I guess the equivalent relocation is 
also required for spi_nor_erase().

Or maybe spi_nor_wait_till_ready() should clear this flag always.

> 
> I'll give it a spin - along with your SR lock fix - when I get back on 
> the HW again.
> 
>> This can of course be extended to all micron flashes, if all support 
>> FSR, but
>> some documentation work has to be made.
>>

Thanks,
John

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

Re: flash_lock issue for n25q 128mb spi nor part

[Tudor.Ambarus]

Hi, John,

On 12/4/19 1:10 PM, John Garry wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 03/12/2019 15:29, John Garry wrote:
>>>>
>>>> Here's an example flow (with my hack to stop using 16b SR method):
>>>>
>>>> root@ubuntu:/home/john# flash_lock -l /dev/mtd0
>>>> root@ubuntu:/home/john# mtd_debug erase /dev/mtd0 0xe00000 4096
>>>> [   69.650642] spi-nor spi-PRP0001:00: at 0xe00000, len 4096
>>>> Erased 4096 bytes from address 0x00e00000 in flash
>>>> root@ubuntu:/home/john# mtd_debug write /dev/mtd0 0xe00000 4096 dump4096
>>>> [   77.093755] spi-nor spi-PRP0001:00: to 0x00e00000, len 4096
>>>> Copied 4096 bytes from dump4096 to address 0x00e00000 in flash
>>>> root@ubuntu:/home/john# mtd_debug read /dev/mtd0 0xe00000 4096 temp
>>>> [   82.162445] spi-nor spi-PRP0001:00: from 0x00e00000, len 4096
>>>> Copied 4096 bytes from address 0x00e00000 in flash to temp
>>>> root@ubuntu:/home/john# flash_lock -u /dev/mtd0
>>>> [   87.558435] spi-nor spi-PRP0001:00: SR1: read back test failed
>>>> flash_lock: error!: could not unlock device: /dev/mtd0
>>>>
>>>>              error 5 (Input/output error)
>>>> root@ubuntu:/home/john#
>>>>
>>>> Unlock reports an error as the the read back test in
>>>> spi_nor_write_sr1_and_check() fails as the SR.WEL has never been
>>>> cleared.
>>>>
>>>
>>> Interesting.
>>
>> I note that spi_nor_erase() exits with a call to
>> spi_nor_write_disable(), yet spi_nor_write() doesn't - maybe we should
>> add a similar call there.
> 
> So this remedies that issue for me:
> 
> commit c90de583d81f7c5c6cb1c8c021108a0e7e244b91 (HEAD)
> Author: John Garry <john.garry@huawei.com>
> Date:   Wed Dec 4 10:26:49 2019 +0000
> 
>     Ensure we clear SR.WEL for any write failure due to locking
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index aeb3ad2dbfb8..3f12335eb20c 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2875,6 +2875,8 @@ static int spi_nor_write(struct mtd_info *mtd,
> loff_t to, size_t len,
>                 i += written;
>         }
> 
> +       ret = spi_nor_write_disable(nor);
> +
>  write_err:
>         spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
>         return ret;
> 
> 
>>
>>>
>>>
>>> Does the following do the trick?
>>>
>>> -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K |
>>> SPI_NOR_QUAD_READ) },
>>> +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K |
>>> USE_FSR |
>>> +                             SPI_NOR_QUAD_READ) },
>>>
> 
> Yes, that works and it's nice to not have the silent failures:
> 
> root@ubuntu:/home/john# flash_lock -i /dev/mtd0
> Device: /dev/mtd0
> Start: 0
> Len: 0x1000000
> Lock status: locked
> Return code: 1
> root@ubuntu:/home/john#
> root@ubuntu:/home/john# mtd_debug erase /dev/mtd0 0xe00000 4096
> [   45.023353] spi-nor spi0.0: Erase operation failed.
> [   45.028257] spi-nor spi0.0: Attempted to modify a protected sector.
> MEMERASE: Input/output error
> mtd_debug write /dev/mtd0 0xe00000 4096 dump4096
> [   50.212013] spi-nor spi0.0: Program operation failed.
> [   50.217084] spi-nor spi0.0: Attempted to modify a protected sector.
> file_to_flash: write, size 0x1000, n 0x1000
> write(): Input/output error
> root@ubuntu:/home/john# mtd_debug read /dev/mtd0 0xe00000 4096 temp
> Copied 4096 bytes from address 0x00e00000 in flash to temp
> root@ubuntu:/home/john# flash_lock -u /dev/mtd0
> flash_lock: error!: could not unlock device: /dev/mtd0
> 
>             error 5 (Input/output error)
> root@ubuntu:/home/john# flash_lock -i /dev/mtd0
> Device: /dev/mtd0
> Start: 0
> Len: 0x1000000
> Lock status: unlocked
> Return code: 0
> root@ubuntu:/home/john# flash_lock -u /dev/mtd0
> root@ubuntu:/home/john#
> 
> But, as you may see, it seems that my change to spi_nor_write() is still
> required to stop the first unlock failure message, but it needs to be
> relocated to after write_err label, as we now jump there for
> spi_nor_wait_till_ready() failure. I guess the equivalent relocation is
> also required for spi_nor_erase().
> 
> Or maybe spi_nor_wait_till_ready() should clear this flag always.

I reproduced this on a n25q256a, with both erase and write. Did a lock,
an erase or write, and then the unlock raises an error on the read back test:
it receives 0x02 to write (the prev operation let the SR.WE set to 1),
and after write, it reads back 0x00 (which is correct, WE is de-asserted).

What is pretty strange is that Micron says about erase or program operations
that: "When the operation is in progress, the write in progress bit is set
to 1. The write enable latch bit is cleared to 0, whether the operation is
successful or not".

So what I guess it happens, is that when an erase/write command tries to
modify a software protected area, the flash completely ignores the command,
so no Write In Progress, and no clearing of the WE.

An idea would be to clear the WE in spi_nor_sr/fsr_ready() when errors.
I'll continue to debug/study this.

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

Re: flash_lock issue for n25q 128mb spi nor part

[Vignesh Raghavendra]

Hi Tudor,

On 12/16/2019 11:39 PM, Tudor.Ambarus@microchip.com wrote:
[...]

>>
>> But, as you may see, it seems that my change to spi_nor_write() is still
>> required to stop the first unlock failure message, but it needs to be
>> relocated to after write_err label, as we now jump there for
>> spi_nor_wait_till_ready() failure. I guess the equivalent relocation is
>> also required for spi_nor_erase().
>>
>> Or maybe spi_nor_wait_till_ready() should clear this flag always.
> 
> I reproduced this on a n25q256a, with both erase and write. Did a lock,
> an erase or write, and then the unlock raises an error on the read back test:
> it receives 0x02 to write (the prev operation let the SR.WE set to 1),
> and after write, it reads back 0x00 (which is correct, WE is de-asserted).
> 
> What is pretty strange is that Micron says about erase or program operations
> that: "When the operation is in progress, the write in progress bit is set
> to 1. The write enable latch bit is cleared to 0, whether the operation is
> successful or not".
> 
> So what I guess it happens, is that when an erase/write command tries to
> modify a software protected area, the flash completely ignores the command,
> so no Write In Progress, and no clearing of the WE.
> 


From PROGRAM Operations section of mt25q datasheet:

" When a command is applied to a protected sector, the command is not
executed, the write enable latch bit remains set to 1, and flag status
register bits 1 and 4 are set."

So, Data sheet is quite clear about this and SW would need to clear WEL
(if required) after write failure.

Regards
Vignesh

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

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

On 17/12/2019 08:57, Vignesh Raghavendra wrote:
> Hi Tudor,
> 
> On 12/16/2019 11:39 PM, Tudor.Ambarus@microchip.com wrote:
> [...]
> 
>>>
>>> But, as you may see, it seems that my change to spi_nor_write() is still
>>> required to stop the first unlock failure message, but it needs to be
>>> relocated to after write_err label, as we now jump there for
>>> spi_nor_wait_till_ready() failure. I guess the equivalent relocation is
>>> also required for spi_nor_erase().
>>>
>>> Or maybe spi_nor_wait_till_ready() should clear this flag always.
>>
>> I reproduced this on a n25q256a, with both erase and write. Did a lock,
>> an erase or write, and then the unlock raises an error on the read back test:
>> it receives 0x02 to write (the prev operation let the SR.WE set to 1),
>> and after write, it reads back 0x00 (which is correct, WE is de-asserted).
>>
>> What is pretty strange is that Micron says about erase or program operations
>> that: "When the operation is in progress, the write in progress bit is set
>> to 1. The write enable latch bit is cleared to 0, whether the operation is
>> successful or not".
>>
>> So what I guess it happens, is that when an erase/write command tries to
>> modify a software protected area, the flash completely ignores the command,
>> so no Write In Progress, and no clearing of the WE.
>>
> 
> 
>>From PROGRAM Operations section of mt25q datasheet:
> 
> " When a command is applied to a protected sector, the command is not
> executed, the write enable latch bit remains set to 1, and flag status
> register bits 1 and 4 are set."
> 
> So, Data sheet is quite clear about this and SW would need to clear WEL
> (if required) after write failure.

Yeah, I think that the datasheet could have been better written in that 
regard.

So about "whether the operation is successful or not" - I wonder what 
they mean by "successful". Does it mean simply that the command 
completes, even with error?

Thanks,
John

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

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

On 17/12/2019 08:57, Vignesh Raghavendra wrote:
> Hi Tudor,
> 
> On 12/16/2019 11:39 PM, Tudor.Ambarus@microchip.com wrote:
> [...]
> 
>>>
>>> But, as you may see, it seems that my change to spi_nor_write() is still
>>> required to stop the first unlock failure message, but it needs to be
>>> relocated to after write_err label, as we now jump there for
>>> spi_nor_wait_till_ready() failure. I guess the equivalent relocation is
>>> also required for spi_nor_erase().
>>>
>>> Or maybe spi_nor_wait_till_ready() should clear this flag always.
>>
>> I reproduced this on a n25q256a, with both erase and write. Did a lock,
>> an erase or write, and then the unlock raises an error on the read back test:
>> it receives 0x02 to write (the prev operation let the SR.WE set to 1),
>> and after write, it reads back 0x00 (which is correct, WE is de-asserted).
>>
>> What is pretty strange is that Micron says about erase or program operations
>> that: "When the operation is in progress, the write in progress bit is set
>> to 1. The write enable latch bit is cleared to 0, whether the operation is
>> successful or not".
>>
>> So what I guess it happens, is that when an erase/write command tries to
>> modify a software protected area, the flash completely ignores the command,
>> so no Write In Progress, and no clearing of the WE.
>>
> 
> 
>>From PROGRAM Operations section of mt25q datasheet:
> 
> " When a command is applied to a protected sector, the command is not
> executed, the write enable latch bit remains set to 1, and flag status
> register bits 1 and 4 are set."
> 
> So, Data sheet is quite clear about this and SW would need to clear WEL
> (if required) after write failure.
> 

Hi guys,

Apart from this issue, on another topic, is there any special reason 
which we don't support status register.BP3? Is it that it is not 
adjacent to BP2 in the register, so makes handling trickier?

I ran these commands, which show how this is broken:

root@ubuntu:/home/john# sudo flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: locked
Return code: 1
root@ubuntu:/home/john# sudo mtd_debug write /dev/mtd0 0xf80000 4096 
dump4096
[ 134.573819] spi-nor spi-PRP0001:00: Program operation failed.
[ 134.579567] spi-nor spi-PRP0001:00: Attempted to modify a protected 
sector.
file_to_flash: write, size 0x1000, n 0x1000
write(): Input/output error
root@ubuntu:/home/john# sudo mtd_debug read /dev/mtd0 0x100000 4096 temp
[ 105.657411] spi-nor spi-PRP0001:00: from 0x00100000, len 4096
Copied 4096 bytes from address 0x00100000 in flash to temp
root@ubuntu:/home/john# hexdump temp
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0001000
root@ubuntu:/home/john# sudo mtd_debug write /dev/mtd0 0x100000 4096 
dump4096
[ 140.161265] spi-nor spi-PRP0001:00: to 0x00100000, len 4096
Copied 4096 bytes from dump4096 to address 0x00100000 in flash
root@ubuntu:/home/john# sudo mtd_debug read /dev/mtd0 0x100000 4096 temp
[ 150.697181] spi-nor spi-PRP0001:00: from 0x00100000, len 4096
Copied 4096 bytes from address 0x00100000 in flash to temp
root@ubuntu:/home/john# diff temp dump4096
root@ubuntu:/home/john#

Here is are told that all sectors for the chip are locked, which is 
untrue, and we could execute the write command successfully on the 
bottom half.

Thanks,
John

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

Re: [PATCH v2] mtd: spi-nor: Fix the writing of the Status Register on micron flashes

[Miquel Raynal]

On Tue, 2019-12-03 at 14:50:01 UTC,  wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Micron flashes do not support 16 bit writes on the Status Register.
> According to micron datasheets, when using the Write Status Register
> (01h) command, the chip select should be driven LOW and held LOW until
> the eighth bit of the last data byte has been latched in, after which
> it must be driven HIGH. If CS is not driven HIGH, the command is not
> executed, flag status register error bits are not set, and the write enable
> latch remains set to 1. This fixes the lock operations on micron flashes.
> 
> Reported-by: John Garry <john.garry@huawei.com>
> Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Tested-by: John Garry <john.garry@huawei.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.

Miquel

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

Re: flash_lock issue for n25q 128mb spi nor part

[Tudor.Ambarus]

On Thursday, January 9, 2020 12:36:11 PM EET John Garry wrote:
> Apart from this issue, on another topic, is there any special reason

Ups, I forgot about this. I had a patch for it, let me find it.

> which we don't support status register.BP3? Is it that it is not
> adjacent to BP2 in the register, so makes handling trickier?

Jungseung worked in the past for adding BP3 support. He have just sent a new 
patch set for addressing this, feedback is welcomed:
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=152401

Cheers,
ta


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

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

On 10/01/2020 11:51, Tudor.Ambarus@microchip.com wrote:
> On Thursday, January 9, 2020 12:36:11 PM EET John Garry wrote:
>> Apart from this issue, on another topic, is there any special reason
> 
> Ups, I forgot about this. I had a patch for it, let me find it.

ok, cool

> 
>> which we don't support status register.BP3? Is it that it is not
>> adjacent to BP2 in the register, so makes handling trickier?
> 
> Jungseung worked in the past for adding BP3 support. He have just sent a new
> patch set for addressing this, feedback is welcomed:
> https://patchwork.ozlabs.org/project/linux-mtd/list/?series=152401

oh, great. I'll have a look.

Thanks,
John

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

Re: flash_lock issue for n25q 128mb spi nor part

[John Garry]

On 10/01/2020 11:56, John Garry wrote:
> On 10/01/2020 11:51, Tudor.Ambarus@microchip.com wrote:
>> On Thursday, January 9, 2020 12:36:11 PM EET John Garry wrote:
>>> Apart from this issue, on another topic, is there any special reason
>>
>> Ups, I forgot about this. I had a patch for it, let me find it.
> 
> ok, cool
> 
>>
>>> which we don't support status register.BP3? Is it that it is not
>>> adjacent to BP2 in the register, so makes handling trickier?
>>
>> Jungseung worked in the past for adding BP3 support. He have just sent 
>> a new
>> patch set for addressing this, feedback is welcomed:
>> https://patchwork.ozlabs.org/project/linux-mtd/list/?series=152401
> 
> oh, great. I'll have a look.
> 
> Thanks,
> John

Hi guys,

It turns out now that we may need support a macronix part also - 
mx25u12835f.

Do you know why flash locking is not supported yet in the kernel for 
macronix parts? Did no one get around to it? Or just a technical issue?

Cheers,
John


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

[RESEND PATCH 1/2] mtd: spi-nor: Clear WEL bit when erase or program errors occur

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

When an Erase or Program error occurs on a spansion/cypress or a
micron flash, the WEL bit remains set to one and should be cleared
with a WRDI command in order to protect against inadvertent writes
that can possible corrupt the contents of the memory.

Winbond, macronix, gd, etc., do not support the E_ERR and P_ERR bits in the
Status Register and always clear the WEL bit regardless of the outcome
of the erase or page program operation (ex w25q40bw, MX25L25635E).

Issue a WRDI command when erase or page program errors occur.

Reported-by: John Garry <john.garry@huawei.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index caf0c109cca0..4376d8ad5834 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -819,6 +819,17 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
 			dev_err(nor->dev, "Programming Error occurred\n");
 
 		spi_nor_clear_sr(nor);
+
+		/*
+		 * WEL bit remains set to one when an erase or page program
+		 * error occurs. Issue a Write Disable command to protect
+		 * against inadvertent writes that can possibly corrupt the
+		 * contents of the memory.
+		 */
+		ret = spi_nor_write_disable(nor);
+		if (ret)
+			return ret;
+
 		return -EIO;
 	}
 
@@ -875,6 +886,17 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
 			"Attempted to modify a protected sector.\n");
 
 		spi_nor_clear_fsr(nor);
+
+		/*
+		 * WEL bit remains set to one when an erase or page program
+		 * error occurs. Issue a Write Disable command to protect
+		 * against inadvertent writes that can possibly corrupt the
+		 * contents of the memory.
+		 */
+		ret = spi_nor_write_disable(nor);
+		if (ret)
+			return ret;
+
 		return -EIO;
 	}
 
-- 
2.23.0

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

[RESEND PATCH 2/2] mtd: spi-nor: Fix description of the sr_ready() return value

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

The functions return 1 if ready, 0 if not ready, -errno on errors.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 4376d8ad5834..604043ec95cc 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -759,7 +759,7 @@ static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
  * flash is ready for new commands.
  * @nor:	pointer to 'struct spi_nor'.
  *
- * Return: 0 on success, -errno otherwise.
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
  */
 static int s3an_sr_ready(struct spi_nor *nor)
 {
@@ -802,7 +802,7 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
  * for new commands.
  * @nor:	pointer to 'struct spi_nor'.
  *
- * Return: 0 on success, -errno otherwise.
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
  */
 static int spi_nor_sr_ready(struct spi_nor *nor)
 {
@@ -866,7 +866,7 @@ static void spi_nor_clear_fsr(struct spi_nor *nor)
  * ready for new commands.
  * @nor:	pointer to 'struct spi_nor'.
  *
- * Return: 0 on success, -errno otherwise.
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
  */
 static int spi_nor_fsr_ready(struct spi_nor *nor)
 {
@@ -907,7 +907,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
  * spi_nor_ready() - Query the flash to see if it is ready for new commands.
  * @nor:	pointer to 'struct spi_nor'.
  *
- * Return: 0 on success, -errno otherwise.
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
  */
 static int spi_nor_ready(struct spi_nor *nor)
 {
-- 
2.23.0

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

Re: [RESEND PATCH 1/2] mtd: spi-nor: Clear WEL bit when erase or program errors occur

[John Garry]

On 09/03/2020 10:15, Tudor.Ambarus@microchip.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> When an Erase or Program error occurs on a spansion/cypress or a
> micron flash, the WEL bit remains set to one and should be cleared
> with a WRDI command in order to protect against inadvertent writes
> that can possible corrupt the contents of the memory.
> 
> Winbond, macronix, gd, etc., do not support the E_ERR and P_ERR bits in the
> Status Register and always clear the WEL bit regardless of the outcome
> of the erase or page program operation (ex w25q40bw, MX25L25635E).
> 
> Issue a WRDI command when erase or page program errors occur.
> 
> Reported-by: John Garry <john.garry@huawei.com>

Previously I would get this unlock complaint:

root@ubuntu:/home/john# flash_lock -l /dev/mtd0
root@ubuntu:/home/john# sudo mtd_debug erase /dev/mtd0 0xf80000 4096
[  167.458647] spi-nor spi-PRP0001:00: Erase operation failed.
[  167.464248] spi-nor spi-PRP0001:00: Attempted to modify a protected 
sector.
MEMERASE: Input/output error
root@ubuntu:/home/john# flash_lock -u /dev/mtd0
flash_lock: error!: could not unlock device: /dev/mtd0

             error 5 (Input/output error)

Now, no such issue. Thanks

Tested-by: John Garry <john.garry@huawei.com>

> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>   drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index caf0c109cca0..4376d8ad5834 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -819,6 +819,17 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
>   			dev_err(nor->dev, "Programming Error occurred\n");
>   
>   		spi_nor_clear_sr(nor);
> +
> +		/*
> +		 * WEL bit remains set to one when an erase or page program
> +		 * error occurs. Issue a Write Disable command to protect
> +		 * against inadvertent writes that can possibly corrupt the
> +		 * contents of the memory.
> +		 */
> +		ret = spi_nor_write_disable(nor);
> +		if (ret)
> +			return ret;
> +
>   		return -EIO;
>   	}
>   
> @@ -875,6 +886,17 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
>   			"Attempted to modify a protected sector.\n");
>   
>   		spi_nor_clear_fsr(nor);
> +
> +		/*
> +		 * WEL bit remains set to one when an erase or page program
> +		 * error occurs. Issue a Write Disable command to protect
> +		 * against inadvertent writes that can possibly corrupt the
> +		 * contents of the memory.
> +		 */
> +		ret = spi_nor_write_disable(nor);
> +		if (ret)
> +			return ret;
> +
>   		return -EIO;
>   	}
>   
> 


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

Re: [RESEND PATCH 1/2] mtd: spi-nor: Clear WEL bit when erase or program errors occur

[Tudor.Ambarus]

On Monday, March 9, 2020 5:04:53 PM EET John Garry wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> On 09/03/2020 10:15, Tudor.Ambarus@microchip.com wrote:
> > From: Tudor Ambarus <tudor.ambarus@microchip.com>
> > 
> > When an Erase or Program error occurs on a spansion/cypress or a
> > micron flash, the WEL bit remains set to one and should be cleared
> > with a WRDI command in order to protect against inadvertent writes
> > that can possible corrupt the contents of the memory.
> > 
> > Winbond, macronix, gd, etc., do not support the E_ERR and P_ERR bits in
> > the
> > Status Register and always clear the WEL bit regardless of the outcome
> > of the erase or page program operation (ex w25q40bw, MX25L25635E).
> > 
> > Issue a WRDI command when erase or page program errors occur.
> > 
> > Reported-by: John Garry <john.garry@huawei.com>
> 
> Previously I would get this unlock complaint:
> 
> root@ubuntu:/home/john# flash_lock -l /dev/mtd0
> root@ubuntu:/home/john# sudo mtd_debug erase /dev/mtd0 0xf80000 4096
> [  167.458647] spi-nor spi-PRP0001:00: Erase operation failed.
> [  167.464248] spi-nor spi-PRP0001:00: Attempted to modify a protected
> sector.
> MEMERASE: Input/output error
> root@ubuntu:/home/john# flash_lock -u /dev/mtd0
> flash_lock: error!: could not unlock device: /dev/mtd0
> 
>              error 5 (Input/output error)
> 
> Now, no such issue. Thanks
> 
> Tested-by: John Garry <john.garry@huawei.com>
> 
> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> > ---
> > 
> >   drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)

Rebased and applied on top of spi-nor/next. Thanks.
ta


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