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

[Matthias Schiffer]

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@xxxxxxxx>
>> Signed-off-by: Matthias Schiffer <mschiffer@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>  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: signature.asc
Description: OpenPGP digital signature