[PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[Felix Fietkau]

This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f.

This commit is breaking read access on at least s25fl064k, but also
possibly other Spansion flash chips.

Any mtd read seems to succeed, but simply returns a zero-filled buffer.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/mtd/spi-nor/spi-nor.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 4988390..8b8842e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	mutex_init(&nor->lock);
 
 	/*
-	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
-	 * with the software protection bits set
+	 * Atmel, SST and Intel/Numonyx serial nor tend to power
+	 * up with the software protection bits set
 	 */
 
 	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
 	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
-	    JEDEC_MFR(info) == SNOR_MFR_SST ||
-	    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
+	    JEDEC_MFR(info) == SNOR_MFR_SST) {
 		write_enable(nor);
 		write_sr(nor, 0);
 	}
-- 
2.2.2

Re: [PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[Brian Norris]

Hi Felix,

On Thu, Nov 26, 2015 at 05:27:01PM +0100, Felix Fietkau wrote:
> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f.
> 
> This commit is breaking read access on at least s25fl064k, but also
> possibly other Spansion flash chips.

This makes no sense. This patch is about Winbond flash, but you're
referencing Spansion flash in the commit subject. I suspect you have
either some more explaining, or some better investigation to do, because
I don't understand how the commit in question could be the problem for
you.

> Any mtd read seems to succeed, but simply returns a zero-filled buffer.

If this is the symptom you're looking at, I doubt you have the right
fix. Wild guess: something similar to this patch:

  http://patchwork.ozlabs.org/patch/547367/
  "mtd: spi-nor: wait until lock/unlock operations are ready"

Brian

> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 4988390..8b8842e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  	mutex_init(&nor->lock);
>  
>  	/*
> -	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
> -	 * with the software protection bits set
> +	 * Atmel, SST and Intel/Numonyx serial nor tend to power
> +	 * up with the software protection bits set
>  	 */
>  
>  	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>  	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
> -	    JEDEC_MFR(info) == SNOR_MFR_SST ||
> -	    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
> +	    JEDEC_MFR(info) == SNOR_MFR_SST) {
>  		write_enable(nor);
>  		write_sr(nor, 0);
>  	}
> -- 
> 2.2.2
> 

Re: [PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[Felix Fietkau]

On 2015-11-27 23:27, Brian Norris wrote:
> Hi Felix,
> 
> On Thu, Nov 26, 2015 at 05:27:01PM +0100, Felix Fietkau wrote:
>> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f.
>> 
>> This commit is breaking read access on at least s25fl064k, but also
>> possibly other Spansion flash chips.
> 
> This makes no sense. This patch is about Winbond flash, but you're
> referencing Spansion flash in the commit subject. I suspect you have
> either some more explaining, or some better investigation to do, because
> I don't understand how the commit in question could be the problem for
> you.
Take a look at these Spansion flash ids:

    { "s25fl004k",  INFO(0xef4013,      0,  64 * 1024,   8, SECT_4K |
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
    { "s25fl008k",  INFO(0xef4014,      0,  64 * 1024,  16, SECT_4K |
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
    { "s25fl016k",  INFO(0xef4015,      0,  64 * 1024,  32, SECT_4K |
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
    { "s25fl064k",  INFO(0xef4017,      0,  64 * 1024, 128, SECT_4K) },

Note how they start with 0xef (SNOR_MFR_WINBOND).

>> Any mtd read seems to succeed, but simply returns a zero-filled buffer.
> 
> If this is the symptom you're looking at, I doubt you have the right
> fix. Wild guess: something similar to this patch:
> 
>   http://patchwork.ozlabs.org/patch/547367/
>   "mtd: spi-nor: wait until lock/unlock operations are ready"
When I have access to a similar device again, I can give this fix a try.
If that doesn't work, then maybe we need to add an extra check to
exclude Spansion flashes from the codepath that checks for SNOR_MFR_WINBOND.

- Felix

Re: [PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[Brian Norris]

Hi,

On Sat, Nov 28, 2015 at 09:59:43AM +0100, Felix Fietkau wrote:
> On 2015-11-27 23:27, Brian Norris wrote:
> > On Thu, Nov 26, 2015 at 05:27:01PM +0100, Felix Fietkau wrote:
> >> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f.
> >> 
> >> This commit is breaking read access on at least s25fl064k, but also
> >> possibly other Spansion flash chips.
> > 
> > This makes no sense. This patch is about Winbond flash, but you're
> > referencing Spansion flash in the commit subject. I suspect you have
> > either some more explaining, or some better investigation to do, because
> > I don't understand how the commit in question could be the problem for
> > you.
> Take a look at these Spansion flash ids:
> 
>     { "s25fl004k",  INFO(0xef4013,      0,  64 * 1024,   8, SECT_4K |
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>     { "s25fl008k",  INFO(0xef4014,      0,  64 * 1024,  16, SECT_4K |
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>     { "s25fl016k",  INFO(0xef4015,      0,  64 * 1024,  32, SECT_4K |
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>     { "s25fl064k",  INFO(0xef4017,      0,  64 * 1024, 128, SECT_4K) },
> 
> Note how they start with 0xef (SNOR_MFR_WINBOND).

Ugh, that really sucks :(

Maybe we should do the revert [*] now, and fix it for the next release.
Maybe we can add something to the table entry to (optionally) specify
the manufacturer, so we can specify that even if these look like they're
Winbond, they're actually Spansion.

However, it seems like the lock/unlock features (e.g., top/bottom; 4K
sector ranges) supported vary significantly across not only vendors, but
generation. So maybe we just need some more opt-in flags in the table,
so we don't have to try to make blanket statements, like "all Winbond
flash support flash locking in SR."

Brian

[*] I have a comment for this patch. I'll reply to the top-level.

Re: [PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[Ezequiel Garcia]

On 26 November 2015 at 13:27, Felix Fietkau <nbd@openwrt.org> wrote:
> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f.
>
> This commit is breaking read access on at least s25fl064k, but also
> possibly other Spansion flash chips.
>
> Any mtd read seems to succeed, but simply returns a zero-filled buffer.
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 4988390..8b8842e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>         mutex_init(&nor->lock);
>
>         /*
> -        * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
> -        * with the software protection bits set
> +        * Atmel, SST and Intel/Numonyx serial nor tend to power
> +        * up with the software protection bits set
>          */
>
>         if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>             JEDEC_MFR(info) == SNOR_MFR_INTEL ||
> -           JEDEC_MFR(info) == SNOR_MFR_SST ||
> -           JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
> +           JEDEC_MFR(info) == SNOR_MFR_SST) {
>                 write_enable(nor);
>                 write_sr(nor, 0);
>         }

As Brian mentioned, this looks definitely fishy.

IIUC, the above statement is trying to unlock flashes that power-up as
locked. But you say your flash is still locked after booting? And
moreover, removing the unlock quirk fixes it?

I think a more complete description of your problem might help us fix
this the right way. Perhaps the above line clearing the status
register is wrong?
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[Felix Fietkau]

On 2015-11-28 22:56, Ezequiel Garcia wrote:
> On 26 November 2015 at 13:27, Felix Fietkau <nbd@openwrt.org> wrote:
>> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f.
>>
>> This commit is breaking read access on at least s25fl064k, but also
>> possibly other Spansion flash chips.
>>
>> Any mtd read seems to succeed, but simply returns a zero-filled buffer.
>>
>> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 4988390..8b8842e 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>         mutex_init(&nor->lock);
>>
>>         /*
>> -        * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>> -        * with the software protection bits set
>> +        * Atmel, SST and Intel/Numonyx serial nor tend to power
>> +        * up with the software protection bits set
>>          */
>>
>>         if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>>             JEDEC_MFR(info) == SNOR_MFR_INTEL ||
>> -           JEDEC_MFR(info) == SNOR_MFR_SST ||
>> -           JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
>> +           JEDEC_MFR(info) == SNOR_MFR_SST) {
>>                 write_enable(nor);
>>                 write_sr(nor, 0);
>>         }
> 
> As Brian mentioned, this looks definitely fishy.
> 
> IIUC, the above statement is trying to unlock flashes that power-up as
> locked. But you say your flash is still locked after booting? And
> moreover, removing the unlock quirk fixes it?
The flash isn't locked when the system boots, reads and writes work just
fine.

> I think a more complete description of your problem might help us fix
> this the right way. Perhaps the above line clearing the status
> register is wrong?
Without the write_enable/write_sr, the flash is readable and writable.
With it, any reads return only null-bytes as data. What other info about
the problem do you need?

I think it probably works just fine for Winbond flashes, but
unfortunately this also affects some Spansion flash chips with IDs that
start with 0xef (SNOR_MFR_WINBOND).

- Felix

Re: [PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[Ezequiel Garcia]

On 28 November 2015 at 19:12, Felix Fietkau <nbd@openwrt.org> wrote:
> On 2015-11-28 22:56, Ezequiel Garcia wrote:
>> On 26 November 2015 at 13:27, Felix Fietkau <nbd@openwrt.org> wrote:
>>> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f.
>>>
>>> This commit is breaking read access on at least s25fl064k, but also
>>> possibly other Spansion flash chips.
>>>
>>> Any mtd read seems to succeed, but simply returns a zero-filled buffer.
>>>
>>> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
>>> ---
>>>  drivers/mtd/spi-nor/spi-nor.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>> index 4988390..8b8842e 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>         mutex_init(&nor->lock);
>>>
>>>         /*
>>> -        * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>>> -        * with the software protection bits set
>>> +        * Atmel, SST and Intel/Numonyx serial nor tend to power
>>> +        * up with the software protection bits set
>>>          */
>>>
>>>         if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>>>             JEDEC_MFR(info) == SNOR_MFR_INTEL ||
>>> -           JEDEC_MFR(info) == SNOR_MFR_SST ||
>>> -           JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
>>> +           JEDEC_MFR(info) == SNOR_MFR_SST) {
>>>                 write_enable(nor);
>>>                 write_sr(nor, 0);
>>>         }
>>
>> As Brian mentioned, this looks definitely fishy.
>>
>> IIUC, the above statement is trying to unlock flashes that power-up as
>> locked. But you say your flash is still locked after booting? And
>> moreover, removing the unlock quirk fixes it?
> The flash isn't locked when the system boots, reads and writes work just
> fine.
>
>> I think a more complete description of your problem might help us fix
>> this the right way. Perhaps the above line clearing the status
>> register is wrong?
> Without the write_enable/write_sr, the flash is readable and writable.
> With it, any reads return only null-bytes as data. What other info about
> the problem do you need?
>

Well, printing the status register right before a read (i.e. when the
problem shows) might help understand what's going on.

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[Felix Fietkau]

On 2015-11-30 15:50, Ezequiel Garcia wrote:
> On 28 November 2015 at 19:12, Felix Fietkau <nbd@openwrt.org> wrote:
>> On 2015-11-28 22:56, Ezequiel Garcia wrote:
>>> On 26 November 2015 at 13:27, Felix Fietkau <nbd@openwrt.org> wrote:
>>>> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f.
>>>>
>>>> This commit is breaking read access on at least s25fl064k, but also
>>>> possibly other Spansion flash chips.
>>>>
>>>> Any mtd read seems to succeed, but simply returns a zero-filled buffer.
>>>>
>>>> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
>>>> ---
>>>>  drivers/mtd/spi-nor/spi-nor.c | 7 +++----
>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index 4988390..8b8842e 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>>         mutex_init(&nor->lock);
>>>>
>>>>         /*
>>>> -        * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>>>> -        * with the software protection bits set
>>>> +        * Atmel, SST and Intel/Numonyx serial nor tend to power
>>>> +        * up with the software protection bits set
>>>>          */
>>>>
>>>>         if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>>>>             JEDEC_MFR(info) == SNOR_MFR_INTEL ||
>>>> -           JEDEC_MFR(info) == SNOR_MFR_SST ||
>>>> -           JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
>>>> +           JEDEC_MFR(info) == SNOR_MFR_SST) {
>>>>                 write_enable(nor);
>>>>                 write_sr(nor, 0);
>>>>         }
>>>
>>> As Brian mentioned, this looks definitely fishy.
>>>
>>> IIUC, the above statement is trying to unlock flashes that power-up as
>>> locked. But you say your flash is still locked after booting? And
>>> moreover, removing the unlock quirk fixes it?
>> The flash isn't locked when the system boots, reads and writes work just
>> fine.
>>
>>> I think a more complete description of your problem might help us fix
>>> this the right way. Perhaps the above line clearing the status
>>> register is wrong?
>> Without the write_enable/write_sr, the flash is readable and writable.
>> With it, any reads return only null-bytes as data. What other info about
>> the problem do you need?
>>
> 
> Well, printing the status register right before a read (i.e. when the
> problem shows) might help understand what's going on.
Will do that once I have access to suitable test hardware again.

- Felix

Re: [PATCH] Revert "mtd: spi-nor: disable protection for Winbond flash at startup"

[Brian Norris]

+ others

Hi Felix,

On Thu, Nov 26, 2015 at 05:27:01PM +0100, Felix Fietkau wrote:
> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f.
> 
> This commit is breaking read access on at least s25fl064k, but also
> possibly other Spansion flash chips.
> 
> Any mtd read seems to succeed, but simply returns a zero-filled buffer.
> 
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 4988390..8b8842e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  	mutex_init(&nor->lock);
>  
>  	/*
> -	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
> -	 * with the software protection bits set
> +	 * Atmel, SST and Intel/Numonyx serial nor tend to power
> +	 * up with the software protection bits set
>  	 */
>  
>  	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>  	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
> -	    JEDEC_MFR(info) == SNOR_MFR_SST ||
> -	    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
> +	    JEDEC_MFR(info) == SNOR_MFR_SST) {
>  		write_enable(nor);
>  		write_sr(nor, 0);
>  	}

So I think we've more or less agreed that this part needs applied for
4.4 (to prevent regressions), and hopefully we can work out a good way
forward for 4.5, to keep Winbond support.

But, I think if this part is breaking you, then ioctl(MEMLOCK) would
also mess up your flash, and so you'll need something like this too:

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 3b2460efc019..3c88ebddf4db 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1236,8 +1236,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	mtd->_read = spi_nor_read;
 
 	/* NOR protection support for STmicro/Micron chips and similar */
-	if (JEDEC_MFR(info) == SNOR_MFR_MICRON ||
-	    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
+	if (JEDEC_MFR(info) == SNOR_MFR_MICRON) {
 		nor->flash_lock = stm_lock;
 		nor->flash_unlock = stm_unlock;
 		nor->flash_is_locked = stm_is_locked;

Do you think you can put together (and test) a patch with a fuller
description soon? Might mention the fact that Winbond and Spansion
shared vendor IDs on some parts. Then I can apply it for 4.4 and try for
a better solution on 4.5.

Thanks,
Brian