[PATCH 1/2] mtd: spi-nor: disable software protection for Macronix flash at startup

[PATCH 1/2] mtd: spi-nor: disable software protection for Macronix flash at startup

[Matthias Schiffer]

This patch has been tested in OpenWrt for a few months and seems to work
correctly.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 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 157841d..d681003 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1304,6 +1304,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 
 	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
 	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
+	    JEDEC_MFR(info) == SNOR_MFR_MACRONIX ||
 	    JEDEC_MFR(info) == SNOR_MFR_SST ||
 	    info->flags & SPI_NOR_HAS_LOCK) {
 		write_enable(nor);
-- 
2.8.2

[PATCH 2/2] mtd: spi-nor: disable software protection for Winbond flash at startup (again)

[Matthias Schiffer]

Support for unlocking Winbond flash had been originally added in c6fc217
("mtd: spi-nor: disable protection for Winbond flash at startup"), but was
reverted in 67b9bcd ("mtd: spi-nor: fix Spansion regressions (aliased with
Winbond)"), as some Spansion chips using the Winbond manufacturer ID
weren't working correctly with it.

The actual issue has been fixed meanwhile in 32321e9 ("mtd: spi-nor: wait
until lock/unlock operations are ready") and edf891e ("mtd: spi-nor: wait
for SR_WIP to clear on initial unlock"). After this fix, the Winbond flash
can be unlocked without causing havoc for Spansion chips.

We have verified that everything is working correctly on some Winbond chips
(like the w25q64, which is aliased with the Spansion s25fl064k) and
Spansion chips.

As an additional test, I also tried enabling the initial unlocking code for
the Spansion manufacturer ID. All tested Spansion flash chips showed the
same behaviour: without 32321e9 and edf891e, subsequent reads only returned
zeros (as reported by Felix Fietkau, leading to 67b9bcd), but with 32321e9
and edf891e, the chip continued to work correctly.

Cc: Felix Fietkau <nbd@nbd.name>
Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 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 d681003..a834e66 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1306,6 +1306,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
 	    JEDEC_MFR(info) == SNOR_MFR_MACRONIX ||
 	    JEDEC_MFR(info) == SNOR_MFR_SST ||
+	    JEDEC_MFR(info) == SNOR_MFR_WINBOND ||
 	    info->flags & SPI_NOR_HAS_LOCK) {
 		write_enable(nor);
 		write_sr(nor, 0);
-- 
2.8.2

Re: [PATCH 1/2] mtd: spi-nor: disable software protection for Macronix flash at startup

[Cyrille Pitchen]

Hi Matthias,

Le 18/05/2016 15:32, Matthias Schiffer a écrit :
> This patch has been tested in OpenWrt for a few months and seems to work
> correctly.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>  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 157841d..d681003 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1304,6 +1304,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  
>  	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>  	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
> +	    JEDEC_MFR(info) == SNOR_MFR_MACRONIX ||
>  	    JEDEC_MFR(info) == SNOR_MFR_SST ||
>  	    info->flags & SPI_NOR_HAS_LOCK) {
>  		write_enable(nor);
> 
The line following this patch chunk is "write_sr(nor, 0);" however, if I refer
to the Macronix mx25l25673g datasheet about the Status Register, bits[5:2]
(BP0, BP1, BP2 and BP3) are non-volatile and define the protected area.
Also bit6 (Quad Enable) is also non-volatile and is used to reassign #WP and
#Hold pins to IO2 and IO3 functions needed by Quad SPI protocols on many other
Macronix memories (indeed on the 73g part, the Quad Enable bit is always 1).

So you should not write 0 directly into the Status Register if you only want to
clear bit7 (Status Register Write Disable): use a read, modify, write sequence
instead.


Best regards,

Cyrille

Re: [PATCH 1/2] mtd: spi-nor: disable software protection for Macronix flash at startup

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 2769 bytes --]

On 05/23/2016 04:01 PM, Cyrille Pitchen wrote:
> Hi Matthias,
> 
> Le 18/05/2016 15:32, Matthias Schiffer a écrit :
>> This patch has been tested in OpenWrt for a few months and seems to work
>> correctly.
>>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>> ---
>>  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 157841d..d681003 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1304,6 +1304,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>  
>>  	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>>  	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
>> +	    JEDEC_MFR(info) == SNOR_MFR_MACRONIX ||
>>  	    JEDEC_MFR(info) == SNOR_MFR_SST ||
>>  	    info->flags & SPI_NOR_HAS_LOCK) {
>>  		write_enable(nor);
>>
> The line following this patch chunk is "write_sr(nor, 0);" however, if I refer
> to the Macronix mx25l25673g datasheet about the Status Register, bits[5:2]
> (BP0, BP1, BP2 and BP3) are non-volatile and define the protected area.
> Also bit6 (Quad Enable) is also non-volatile and is used to reassign #WP and
> #Hold pins to IO2 and IO3 functions needed by Quad SPI protocols on many other
> Macronix memories (indeed on the 73g part, the Quad Enable bit is always 1).
> 
> So you should not write 0 directly into the Status Register if you only want to
> clear bit7 (Status Register Write Disable): use a read, modify, write sequence
> instead.
> 
> 
> Best regards,
> 
> Cyrille
> 

Hi,
clearing the protected area (BP0..BP3) is exactly what this code is
supposed to do; it was already doing this for Atmel, Intel and SST flash,
and this patch series makes it behave the same for Macronix and Winbond.

I see that the mx25l25673g defaults to unprotected according to the
datasheet, but removing the protection again should not hurt. If there is
actually a problem with writing 0 to the Quad Enable bit, we can of course
make this conditional and/or just clear the relevant bits.

I've observed the mx25l6405d coming up with protection bits set (the
datasheet doesn't seem to specify the initial state of the bits). It's also
possible that the software protection is enabled by the bootloader, which I
can't replace; but I'd argue that the kernel should try to get the flash
chip into a known state on boot, thus always removing the software protection.

Most of the setup of the flash chip is done after the "write_sr(nor, 0);"
in spi_nor_scan(). Do you suspect that "write_sr(nor, 0);" might cause
problems anyways?

Regards,
Matthias


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] mtd: spi-nor: disable software protection for Macronix flash at startup

[Cyrille Pitchen]

Hi Matthias,

Le 23/05/2016 18:32, Matthias Schiffer a écrit :
> On 05/23/2016 04:01 PM, Cyrille Pitchen wrote:
>> Hi Matthias,
>>
>> Le 18/05/2016 15:32, Matthias Schiffer a écrit :
>>> This patch has been tested in OpenWrt for a few months and seems to work
>>> correctly.
>>>
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>>> ---
>>>  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 157841d..d681003 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -1304,6 +1304,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>  
>>>  	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>>>  	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
>>> +	    JEDEC_MFR(info) == SNOR_MFR_MACRONIX ||
>>>  	    JEDEC_MFR(info) == SNOR_MFR_SST ||
>>>  	    info->flags & SPI_NOR_HAS_LOCK) {
>>>  		write_enable(nor);
>>>
>> The line following this patch chunk is "write_sr(nor, 0);" however, if I refer
>> to the Macronix mx25l25673g datasheet about the Status Register, bits[5:2]
>> (BP0, BP1, BP2 and BP3) are non-volatile and define the protected area.
>> Also bit6 (Quad Enable) is also non-volatile and is used to reassign #WP and
>> #Hold pins to IO2 and IO3 functions needed by Quad SPI protocols on many other
>> Macronix memories (indeed on the 73g part, the Quad Enable bit is always 1).
>>
>> So you should not write 0 directly into the Status Register if you only want to
>> clear bit7 (Status Register Write Disable): use a read, modify, write sequence
>> instead.
>>
>>
>> Best regards,
>>
>> Cyrille
>>
> 
> Hi,
> clearing the protected area (BP0..BP3) is exactly what this code is
> supposed to do; it was already doing this for Atmel, Intel and SST flash,
> and this patch series makes it behave the same for Macronix and Winbond.
> 
> I see that the mx25l25673g defaults to unprotected according to the
> datasheet, but removing the protection again should not hurt. If there is
> actually a problem with writing 0 to the Quad Enable bit, we can of course
> make this conditional and/or just clear the relevant bits.
> 
> I've observed the mx25l6405d coming up with protection bits set (the
> datasheet doesn't seem to specify the initial state of the bits). It's also
> possible that the software protection is enabled by the bootloader, which I
> can't replace; but I'd argue that the kernel should try to get the flash
> chip into a known state on boot, thus always removing the software protection.
> 
> Most of the setup of the flash chip is done after the "write_sr(nor, 0);"
> in spi_nor_scan(). Do you suspect that "write_sr(nor, 0);" might cause
> problems anyways?

Yes I do: it's likely to prevent us from adding support to the Macronix QPI mode
which use the SPI 4-4-4 protocol. As all other Quad SPI protocols on Macronix
memories, it requires the Quad Enable bit to be set so the #Write Protect, #Hold,
and #Reset functions are disabled and the associated pins are reassigned to
IO2 and IO3 data lines.

Adding proper support of the QPI mode would allow us to handle the case where
the Macronix memory has already entered its QPI mode before spi_nor_scan() is
called. So previous commands (Read JEDEC ID 0xAF, ...) already use the SPI 4-4-4
protocol. Then if you clear the Quad Enable bit in the Status Register:
1 - I don't whether it safely makes the Macronix exit its QPI mode
2 - the spi-nor framework is already configured to use the SPI 4-4-4 but once
    the QE bit is cleared you could no longer use any Quad SPI protocol until
    you set this bit again.

Also let forget the QPI mode for now, the Quad Enable bit is non-volatile:
if you clear it then a spurious reset occurs before the spi-nor framework sets the bit
back, some bootloaders might find the Macronix memory in a unexpected state where
Quad SPI protocols don't work.

Besides, if you just want to clear the BPx bits, there is no need to also clear
the SRWD non-volatile bit at the same time.

I don't think it's good practice to modify non-volatile bits when not needed.

> 
> Regards,
> Matthias
> 

Regards,

Cyrille