Re: [PATCH] mtd: spi-nor: clear Extended Address Reg on switch to 3-byte addressing.

[NeilBrown]

On Sun, Apr 08 2018, Marek Vasut wrote:

> On 04/08/2018 09:04 AM, NeilBrown wrote:
>> 
>> According to section
>>    8.2.7 Write Extended Address Register (C5h)
>> 
>> of the Winbond W25Q256FV data sheet (256M-BIT SPI flash)
>> 
>>    The Extended Address Register is only effective when the device is
>>    in the 3-Byte Address Mode.  When the device operates in the 4-Byte
>>    Address Mode (ADS=1), any command with address input of A31-A24
>>    will replace the Extended Address Register values. It is
>>    recommended to check and update the Extended Address Register if
>>    necessary when the device is switched from 4-Byte to 3-Byte Address
>>    Mode.
>> 
>> This patch adds code to implement that recommendation.  Without this,
>> my GNUBEE-PC1 will not successfully reboot, as the Extended Address
>> Register is left with a value of '1'. When the SOC attempts to read
>> (in 3-byte address mode) the boot loader, it reads from the wrong
>> location.
>
> Your board is broken by design and does not implement proper reset logic
> for the SPI NOR chip, right ? That is, when the CPU resets, the SPI NOR
> is left in some random undefined state instead of being reset too, yes?

Thanks for the reply.
"Broken" is an emotive word :-)  Certain the board *doesn't* reset the NOR
chip on reset.
However the NOR chip doesn't have a dedicated RESET pin. It has a pin
that defaults to "HOLD" and can be programmed to act as "RESET".  This
pin is tied to 3V3 on my board. If it were tied to a reset line, it
would still need code changes to work (or switch from the default).

A few month ago:
Commit: 8dee1d971af9 ("mtd: spi-nor: add an API to restore the status of SPI flash chip")
Commit: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")

were added to Linux.  They appear to be designed to address a very
similar situation to mine.  Unfortunately they aren't complete as the
code to disable 4-byte addressing doesn't follow documented requirements
(at least for winbond) and doesn't work as intended (at least in one
case - mine). This code should either be fixed (e.g. with my patch), or removed.

>
> Doesn't this chip support 4-byte addressing opcodes ? If so, we should
> use those and keep the chip in 3-byte addressing mode. Would that work?

Yes and no.
If I
-	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512,
+			  SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			  SPI_NOR_4B_OPCODES) },

then I can still read all the flash and it never gets switched to
4-byte mode.
However, if the last address read from the flash is beyond 16M, the
extended address register gets implicitly set to 1, and reboot doesn't
work.
i.e. the problem isn't 4-byte mode exactly.  The problem is the Extended
Address Register being set implicitly, and not being zero at reboot.
It looks like we need to clear the extended address register before
reboot, either by:
 - software-reset the flash at shutdown
 - write '0' in the shutdown handler
 - write '0' after every transfer (or every transfer beyond 16M).

Which would you prefer, or is there another option?

Thanks,
NeilBrown



>
>> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 10 ++++++++++
>>  include/linux/mtd/spi-nor.h   |  2 ++
>>  2 files changed, 12 insertions(+)
>> 
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index d445a4d3b770..c303bf0d2982 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -269,6 +269,7 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>  	int status;
>>  	bool need_wren = false;
>>  	u8 cmd;
>> +	u8 val;
>>  
>>  	switch (JEDEC_MFR(info)) {
>>  	case SNOR_MFR_MICRON:
>> @@ -283,6 +284,15 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>  		status = nor->write_reg(nor, cmd, NULL, 0);
>>  		if (need_wren)
>>  			write_disable(nor);
>> +		if (!status && !enable &&
>> +		    nor->read_reg(nor, SPINOR_OP_RDXA, &val, 1) == 0 &&
>> +		    val != 0) {
>> +			/* need to reset the Extended Address Register */
>> +			write_enable(nor);
>> +			val = 0;
>> +			nor->write_reg(nor, SPINOR_OP_WRXA, &val, 1);
>> +			write_disable(nor);
>> +		}
>>  
>>  		return status;
>>  	default:
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index de36969eb359..42954419cfdf 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -62,6 +62,8 @@
>>  #define SPINOR_OP_RDCR		0x35	/* Read configuration register */
>>  #define SPINOR_OP_RDFSR		0x70	/* Read flag status register */
>>  #define SPINOR_OP_CLFSR		0x50	/* Clear flag status register */
>> +#define SPINOR_OP_RDXA		0xc8	/* Read Extended Address Register */
>> +#define SPINOR_OP_WRXA		0xc5	/* Write Extended Address Register */
>>  
>>  /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
>>  #define SPINOR_OP_READ_4B	0x13	/* Read data bytes (low frequency) */
>> 
>
>
> -- 
> Best regards,
> Marek Vasut

Attachment: signature.asc
Description: PGP signature