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

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]


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.

Signed-off-by: NeilBrown <neil@brown.name>
---
 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) */
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Marek Vasut]

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?

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?

> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  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

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5266 bytes --]

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@brown.name>
>> ---
>>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Marek Vasut]

On 04/08/2018 11:56 PM, NeilBrown wrote:
> 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.

Sorry for the delay.

> "Broken" is an emotive word :-)  Certain the board *doesn't* reset the NOR
> chip on reset.

It's not emotive, it is descriptive of what it really is. Such boards
which do not correctly reset the SPI NOR can, during ie. reset, get into
a state where the system is unbootable or corrupts the content of the
SPI NOR. This stuff came up over and over on the ML, it seems HW
designers never learn.

> 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).

I'm afraid this needs HW changes. The solution for SPI NORs without a
dedicated reset line is to just hard cut the power to them for a while
in case the CPU core reset out is asserted.

> 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")

This works when reloading the driver, but not when restarting the system.

> 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.

Yes, dedicated opcodes are the way to go ... although, it doesn't solve
the reset problem completely. Imagine your CPU ie. restarts during a
page program operation and then the BootROM sends data over the SPI.
Those data might get written into the SPI NOR, thus corrupting the content.

> 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.

Uh oh, I seems to remember something like this with the winbond flash.
I think this was also the reason why zynqmp couldn't boot from those,
because it was somehow weirdly configured by default.

> It looks like we need to clear the extended address register before
> reboot, either by:
>  - software-reset the flash at shutdown

Doesn't work if CPU resets without executing this hook.

>  - write '0' in the shutdown handler

See above

>  - write '0' after every transfer (or every transfer beyond 16M).

What happens if CPU resets during the transfer ? System fails to boot.

> Which would you prefer, or is there another option?
Neither, and I am really sorry I don't have a suggestion for you here.
But I think there might be something eluding me regarding this winbond
extended something register. How is the behavior of the chip different
exactly from a convention > 16 MiB SPI NOR (ie. Spansion one) ?

-- 
Best regards,
Marek Vasut

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 7747 bytes --]

On Mon, Apr 09 2018, Marek Vasut wrote:

> On 04/08/2018 11:56 PM, NeilBrown wrote:
>> 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.
>
> Sorry for the delay.
>
>> "Broken" is an emotive word :-)  Certain the board *doesn't* reset the NOR
>> chip on reset.
>
> It's not emotive, it is descriptive of what it really is. Such boards
> which do not correctly reset the SPI NOR can, during ie. reset, get into
> a state where the system is unbootable or corrupts the content of the
> SPI NOR. This stuff came up over and over on the ML, it seems HW
> designers never learn.

Ok, the board is broken.
Still, Linux has a history of even supporting broken hardware -  Spectre
and Meltdown for example.
I wouldn't propose a fix that harms the kernel in any way (hurts
maintainability or negatively affect other hardware), but I don't
think my proposed patch does.

>
>> 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).
>
> I'm afraid this needs HW changes. The solution for SPI NORs without a
> dedicated reset line is to just hard cut the power to them for a while
> in case the CPU core reset out is asserted.
>
>> 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")
>
> This works when reloading the driver, but not when restarting the system.

I don't understand what you are saying.
The second patch provides a ->shutdown function for the spi_driver.
This is called by spi_drv_shutdown which is called by the driver
->shutdown function, which is called by device_shutdown(), which
is only called by
  kernel_shutdown_prepare() and
  kernel_restart_prepare().

So it works exactly when restarting the system, and not when reloading
the driver.

>
>> 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.
>
> Yes, dedicated opcodes are the way to go ... although, it doesn't solve
> the reset problem completely. Imagine your CPU ie. restarts during a
> page program operation and then the BootROM sends data over the SPI.
> Those data might get written into the SPI NOR, thus corrupting the
> content.

What, apart from power loss, would restart the CPU??
I guess the watchdog could do that - so I would not enable the watchdog
timer on this board unless I could disable it during a NOR write cycle.

Even if I we cannot, in software, fix all the down-sides of not having a
reset line, I think there is still a case for fixing anything that can
easily be fixed.

>
>> 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.
>
> Uh oh, I seems to remember something like this with the winbond flash.
> I think this was also the reason why zynqmp couldn't boot from those,
> because it was somehow weirdly configured by default.

I've since discovered another aspect of the weirdness.
Switching from 4-byte mode to 3-byte mode sets the extended address
register to 1.  I'm happy to call that "broken hardware".
So for this chip, the correct sequence for switching to 3-byte mode is:
  - send the "switch to 3-byte mote" command
  - write 0 to the extended address register.
 

>
>> It looks like we need to clear the extended address register before
>> reboot, either by:
>>  - software-reset the flash at shutdown
>
> Doesn't work if CPU resets without executing this hook.
>
>>  - write '0' in the shutdown handler
>
> See above
>
>>  - write '0' after every transfer (or every transfer beyond 16M).
>
> What happens if CPU resets during the transfer ? System fails to boot.
>
>> Which would you prefer, or is there another option?
> Neither, and I am really sorry I don't have a suggestion for you here.
> But I think there might be something eluding me regarding this winbond
> extended something register. How is the behavior of the chip different
> exactly from a convention > 16 MiB SPI NOR (ie. Spansion one) ?

I'm sorry but I cannot answer that.  I don't have a Spansion chip and
I've never worked with nor flash before.
What I know is that in the Winbond W25Q256FV

- There is an "Extended address register" (EAR) which is used for the
  top byte when 3-byte addressing is used.
- The EAR is set to 1 when switching from 4-byte to 3-byte mode, and
  set to zero at power-on.
- When using a 4-byte-address opcode, the high byte is copied into
  the EAR on each access.

(I examined http://www.cypress.com/file/177966/download which seems to be
 a data sheet for the spansion S25FL256S and it describes a single bit
 in the BAR (Bank Address Register) which corresponds to the Winbond
 Extended address register.  Presumably is does not get updated
 implicitly in the same way that the Winbond EAR does)

Also on my board
- the SOC boots from the NOR flash in 3-byte mode
- the hold/reset line for the NOR flash is wired to 3v3.

I cannot defend any of this, nor can I change it.  I just want to make
it work as best I can.  The patch I provided does fix the one
problematic symptom that I can easily reproduce.  Do you have any
objection to the actual code?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Marek Vasut]

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.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  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 &&

I think this should be made winbond-specific, macronix flashes do not
suffer from this I think. Otherwise looks good.

> +		    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

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

[Marek Vasut]

On 04/10/2018 03:05 AM, NeilBrown wrote:

Skipping the previous discussion, I think there are actually two
problems here. One is the reset and the other is braindead behavior of
this flash. TLDR, you are right and this patch is needed (with minor
tweak to make it winbond-only and possibly clean up the conditions a
bit, maybe even pull it into separate function with some sensible name),
sorry for the noise.

I had to read up the datasheet and the discussion again, the behavior of
the flash is horrid.

[...]

-- 
Best regards,
Marek Vasut

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3301 bytes --]


Winbond spi-nor flash 32MB and larger have an 'Extended Address
Register' as one option for addressing beyond 16MB (Macronix
has the same concept, Spansion has EXTADD bits in the Bank Address
Register).

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.

So the documentation suggests clearing the EAR after switching to
3-byte mode.  Experimentation shows that the EAR is *always* one after
the switch to 3-byte mode, so clearing the EAR is mandatory at
shutdown for a subsequent 3-byte-addressed reboot to work.

Note that some SOCs (e.g. MT7621) do not assert a reset line at normal
reboot, so we cannot rely on hardware reset.  The MT7621 does assert a
reset line at watchdog-reset.

Signed-off-by: NeilBrown <neil@brown.name>
---

following a helpful discussion with Marek, I've revised the description
a little, and make the code change specific to winbond.
I've change the OP names to RDEAR and WREAR instead of RDXA and WRXA to
match names used in the Macronix documentation.  Winbond documentation
doesn't provide abbreviated OP names.

Thanks,
NeilBrown


 drivers/mtd/spi-nor/spi-nor.c | 13 +++++++++++++
 include/linux/mtd/spi-nor.h   |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d445a4d3b770..0d0af0acf8b9 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -284,6 +284,19 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 		if (need_wren)
 			write_disable(nor);
 
+		if (!status && !enable &&
+		    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
+			/* On Winbond W25Q256FV, leaving 4byte mode causes
+			 * the Extended Address Register to be set to 1, so all
+			 * 3-byte-address reads come from the second 16M.
+			 * We must clear the register to enable normal behavior.
+			 */
+			write_enable(nor);
+			nor->cmd_buf[0] = 0;
+			nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1);
+			write_disable(nor);
+		}
+
 		return status;
 	default:
 		/* Spansion style */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index de36969eb359..e60da0d34cc1 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_RDEAR		0xc8	/* Read Extended Address Register */
+#define SPINOR_OP_WREAR		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) */
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Boris Brezillon]

Hi Neil,

On Mon, 16 Apr 2018 09:42:30 +1000
NeilBrown <neil@brown.name> wrote:

> Winbond spi-nor flash 32MB and larger have an 'Extended Address
> Register' as one option for addressing beyond 16MB (Macronix
> has the same concept, Spansion has EXTADD bits in the Bank Address
> Register).
> 
> 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.
> 
> So the documentation suggests clearing the EAR after switching to
> 3-byte mode.  Experimentation shows that the EAR is *always* one after
> the switch to 3-byte mode, so clearing the EAR is mandatory at
> shutdown for a subsequent 3-byte-addressed reboot to work.
> 
> Note that some SOCs (e.g. MT7621) do not assert a reset line at normal
> reboot, so we cannot rely on hardware reset.  The MT7621 does assert a
> reset line at watchdog-reset.
> 
> Signed-off-by: NeilBrown <neil@brown.name>

We should probably backport the fix. Can you add a Fixes and Cc-stable
tag?

> ---
> 
> following a helpful discussion with Marek, I've revised the description
> a little, and make the code change specific to winbond.
> I've change the OP names to RDEAR and WREAR instead of RDXA and WRXA to
> match names used in the Macronix documentation.  Winbond documentation
> doesn't provide abbreviated OP names.
> 
> Thanks,
> NeilBrown
> 
> 
>  drivers/mtd/spi-nor/spi-nor.c | 13 +++++++++++++
>  include/linux/mtd/spi-nor.h   |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d445a4d3b770..0d0af0acf8b9 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -284,6 +284,19 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>  		if (need_wren)
>  			write_disable(nor);
>  
> +		if (!status && !enable &&
> +		    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
> +			/* On Winbond W25Q256FV, leaving 4byte mode causes

We use regular kernel-comment style in MTD:

			/*
			 * blablabla
			 */

Thanks,

Boris

> +			 * the Extended Address Register to be set to 1, so all
> +			 * 3-byte-address reads come from the second 16M.
> +			 * We must clear the register to enable normal behavior.
> +			 */
> +			write_enable(nor);
> +			nor->cmd_buf[0] = 0;
> +			nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1);
> +			write_disable(nor);
> +		}
> +
>  		return status;
>  	default:
>  		/* Spansion style */
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index de36969eb359..e60da0d34cc1 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_RDEAR		0xc8	/* Read Extended Address Register */
> +#define SPINOR_OP_WREAR		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) */

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3319 bytes --]


Winbond spi-nor flash 32MB and larger have an 'Extended Address
Register' as one option for addressing beyond 16MB (Macronix
has the same concept, Spansion has EXTADD bits in the Bank Address
Register).

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.

So the documentation suggests clearing the EAR after switching to
3-byte mode.  Experimentation shows that the EAR is *always* one after
the switch to 3-byte mode, so clearing the EAR is mandatory at
shutdown for a subsequent 3-byte-addressed reboot to work.

Note that some SOCs (e.g. MT7621) do not assert a reset line at normal
reboot, so we cannot rely on hardware reset.  The MT7621 does assert a
reset line at watchdog-reset.

This problem is not a regression.  However the commit identified below
added support for resetting an spi-nor chip at shutdown, but didn't
achieve the goal for all spi-nor chips.  So this patch can be seen as
fixing that one.

Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
Cc: stable@vger.kernel.org (v4.16)
Signed-off-by: NeilBrown <neil@brown.name>
---
 drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++++++
 include/linux/mtd/spi-nor.h   |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 5bfa36e95f35..42ae9a1529bb 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -284,6 +284,20 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 		if (need_wren)
 			write_disable(nor);
 
+		if (!status && !enable &&
+		    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
+			/*
+			 * On Winbond W25Q256FV, leaving 4byte mode causes
+			 * the Extended Address Register to be set to 1, so all
+			 * 3-byte-address reads come from the second 16M.
+			 * We must clear the register to enable normal behavior.
+			 */
+			write_enable(nor);
+			nor->cmd_buf[0] = 0;
+			nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1);
+			write_disable(nor);
+		}
+
 		return status;
 	default:
 		/* Spansion style */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index de36969eb359..e60da0d34cc1 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_RDEAR		0xc8	/* Read Extended Address Register */
+#define SPINOR_OP_WREAR		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) */
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4166 bytes --]

On Fri, Apr 20 2018, Boris Brezillon wrote:

> Hi Neil,
>
> On Mon, 16 Apr 2018 09:42:30 +1000
> NeilBrown <neil@brown.name> wrote:
>
>> Winbond spi-nor flash 32MB and larger have an 'Extended Address
>> Register' as one option for addressing beyond 16MB (Macronix
>> has the same concept, Spansion has EXTADD bits in the Bank Address
>> Register).
>> 
>> 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.
>> 
>> So the documentation suggests clearing the EAR after switching to
>> 3-byte mode.  Experimentation shows that the EAR is *always* one after
>> the switch to 3-byte mode, so clearing the EAR is mandatory at
>> shutdown for a subsequent 3-byte-addressed reboot to work.
>> 
>> Note that some SOCs (e.g. MT7621) do not assert a reset line at normal
>> reboot, so we cannot rely on hardware reset.  The MT7621 does assert a
>> reset line at watchdog-reset.
>> 
>> Signed-off-by: NeilBrown <neil@brown.name>
>
> We should probably backport the fix. Can you add a Fixes and Cc-stable
> tag?

It's a bit weird having Fixes when this isn't a regression, but I guess
it doesn't hurt.
I chose
  Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
as this patch it useless without that one.

I also fixed the comment and have resent.

Thanks,
NeilBrown

>
>> ---
>> 
>> following a helpful discussion with Marek, I've revised the description
>> a little, and make the code change specific to winbond.
>> I've change the OP names to RDEAR and WREAR instead of RDXA and WRXA to
>> match names used in the Macronix documentation.  Winbond documentation
>> doesn't provide abbreviated OP names.
>> 
>> Thanks,
>> NeilBrown
>> 
>> 
>>  drivers/mtd/spi-nor/spi-nor.c | 13 +++++++++++++
>>  include/linux/mtd/spi-nor.h   |  2 ++
>>  2 files changed, 15 insertions(+)
>> 
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index d445a4d3b770..0d0af0acf8b9 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -284,6 +284,19 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>  		if (need_wren)
>>  			write_disable(nor);
>>  
>> +		if (!status && !enable &&
>> +		    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
>> +			/* On Winbond W25Q256FV, leaving 4byte mode causes
>
> We use regular kernel-comment style in MTD:
>
> 			/*
> 			 * blablabla
> 			 */
>
> Thanks,
>
> Boris
>
>> +			 * the Extended Address Register to be set to 1, so all
>> +			 * 3-byte-address reads come from the second 16M.
>> +			 * We must clear the register to enable normal behavior.
>> +			 */
>> +			write_enable(nor);
>> +			nor->cmd_buf[0] = 0;
>> +			nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1);
>> +			write_disable(nor);
>> +		}
>> +
>>  		return status;
>>  	default:
>>  		/* Spansion style */
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index de36969eb359..e60da0d34cc1 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_RDEAR		0xc8	/* Read Extended Address Register */
>> +#define SPINOR_OP_WREAR		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) */

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Marek Vasut]

On 04/20/2018 11:26 PM, NeilBrown wrote:
> 
> Winbond spi-nor flash 32MB and larger have an 'Extended Address
> Register' as one option for addressing beyond 16MB (Macronix
> has the same concept, Spansion has EXTADD bits in the Bank Address
> Register).
> 
> 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.
> 
> So the documentation suggests clearing the EAR after switching to
> 3-byte mode.  Experimentation shows that the EAR is *always* one after
> the switch to 3-byte mode, so clearing the EAR is mandatory at
> shutdown for a subsequent 3-byte-addressed reboot to work.
> 
> Note that some SOCs (e.g. MT7621) do not assert a reset line at normal
> reboot, so we cannot rely on hardware reset.  The MT7621 does assert a
> reset line at watchdog-reset.
> 
> This problem is not a regression.  However the commit identified below
> added support for resetting an spi-nor chip at shutdown, but didn't
> achieve the goal for all spi-nor chips.  So this patch can be seen as
> fixing that one.
> 
> Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
> Cc: stable@vger.kernel.org (v4.16)
> Signed-off-by: NeilBrown <neil@brown.name>

Acked-by: Marek Vasut <marek.vasut@gmail.com>

> ---
>  drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++++++
>  include/linux/mtd/spi-nor.h   |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 5bfa36e95f35..42ae9a1529bb 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -284,6 +284,20 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>  		if (need_wren)
>  			write_disable(nor);
>  
> +		if (!status && !enable &&
> +		    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
> +			/*
> +			 * On Winbond W25Q256FV, leaving 4byte mode causes
> +			 * the Extended Address Register to be set to 1, so all
> +			 * 3-byte-address reads come from the second 16M.
> +			 * We must clear the register to enable normal behavior.
> +			 */
> +			write_enable(nor);
> +			nor->cmd_buf[0] = 0;
> +			nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1);
> +			write_disable(nor);
> +		}
> +
>  		return status;
>  	default:
>  		/* Spansion style */
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index de36969eb359..e60da0d34cc1 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_RDEAR		0xc8	/* Read Extended Address Register */
> +#define SPINOR_OP_WREAR		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

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

[Boris Brezillon]

On Sat, 21 Apr 2018 07:28:19 +1000
NeilBrown <neil@brown.name> wrote:

> On Fri, Apr 20 2018, Boris Brezillon wrote:
> 
> > Hi Neil,
> >
> > On Mon, 16 Apr 2018 09:42:30 +1000
> > NeilBrown <neil@brown.name> wrote:
> >  
> >> Winbond spi-nor flash 32MB and larger have an 'Extended Address
> >> Register' as one option for addressing beyond 16MB (Macronix
> >> has the same concept, Spansion has EXTADD bits in the Bank Address
> >> Register).
> >> 
> >> 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.
> >> 
> >> So the documentation suggests clearing the EAR after switching to
> >> 3-byte mode.  Experimentation shows that the EAR is *always* one after
> >> the switch to 3-byte mode, so clearing the EAR is mandatory at
> >> shutdown for a subsequent 3-byte-addressed reboot to work.
> >> 
> >> Note that some SOCs (e.g. MT7621) do not assert a reset line at normal
> >> reboot, so we cannot rely on hardware reset.  The MT7621 does assert a
> >> reset line at watchdog-reset.
> >> 
> >> Signed-off-by: NeilBrown <neil@brown.name>  
> >
> > We should probably backport the fix. Can you add a Fixes and Cc-stable
> > tag?  
> 
> It's a bit weird having Fixes when this isn't a regression, but I guess
> it doesn't hurt.

Well, I thought you wanted this patch to be backported to stable
releases, hence my suggestion. Of course it's not a regression, since
the bug is here from the beginning, but nonetheless it's fixing a bug.

> I chose
>   Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
> as this patch it useless without that one.

Not sure that's how Fixes should be used. IIUC, it should point to the
first commit where the bug was introduced, so I guess it's 0aa87b7563f1
("mtd: m25p80: add support for the windbond w25q256 chip") in this case.

Now, if you want to restrict the kernel releases this patch should be
backported to, you can use the '# vX.Y+' suffix in your Cc-stable tag.

> 
> I also fixed the comment and have resent.

Thanks.

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3237 bytes --]

On Sat, Apr 21 2018, Boris Brezillon wrote:

> On Sat, 21 Apr 2018 07:28:19 +1000
> NeilBrown <neil@brown.name> wrote:
>
>> On Fri, Apr 20 2018, Boris Brezillon wrote:
>> 
>> > Hi Neil,
>> >
>> > On Mon, 16 Apr 2018 09:42:30 +1000
>> > NeilBrown <neil@brown.name> wrote:
>> >  
>> >> Winbond spi-nor flash 32MB and larger have an 'Extended Address
>> >> Register' as one option for addressing beyond 16MB (Macronix
>> >> has the same concept, Spansion has EXTADD bits in the Bank Address
>> >> Register).
>> >> 
>> >> 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.
>> >> 
>> >> So the documentation suggests clearing the EAR after switching to
>> >> 3-byte mode.  Experimentation shows that the EAR is *always* one after
>> >> the switch to 3-byte mode, so clearing the EAR is mandatory at
>> >> shutdown for a subsequent 3-byte-addressed reboot to work.
>> >> 
>> >> Note that some SOCs (e.g. MT7621) do not assert a reset line at normal
>> >> reboot, so we cannot rely on hardware reset.  The MT7621 does assert a
>> >> reset line at watchdog-reset.
>> >> 
>> >> Signed-off-by: NeilBrown <neil@brown.name>  
>> >
>> > We should probably backport the fix. Can you add a Fixes and Cc-stable
>> > tag?  
>> 
>> It's a bit weird having Fixes when this isn't a regression, but I guess
>> it doesn't hurt.
>
> Well, I thought you wanted this patch to be backported to stable
> releases, hence my suggestion. Of course it's not a regression, since
> the bug is here from the beginning, but nonetheless it's fixing a bug.

I have not interest in this patch going to stable.  I'll be perfectly
happy once it lands upstream.

>
>> I chose
>>   Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
>> as this patch it useless without that one.
>
> Not sure that's how Fixes should be used. IIUC, it should point to the
> first commit where the bug was introduced, so I guess it's 0aa87b7563f1
> ("mtd: m25p80: add support for the windbond w25q256 chip") in this case.

That does make a certain sort of sense, but if you tried applying this
patch to that kernel, it wouldn't apply at all.  So it is hard to say
that it fixes anything there.

I think it is best to drop the fixes/stable tags.  It isn't a
regression, doesn't cause data corruption, doesn't make it possible to
crash the kernel, so it isn't really "stable" material in my mind.

Thanks,
NeilBrown


>
> Now, if you want to restrict the kernel releases this patch should be
> backported to, you can use the '# vX.Y+' suffix in your Cc-stable tag.
>
>> 
>> I also fixed the comment and have resent.
>
> Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]


Winbond spi-nor flash 32MB and larger have an 'Extended Address
Register' as one option for addressing beyond 16MB (Macronix
has the same concept, Spansion has EXTADD bits in the Bank Address
Register).

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.

So the documentation suggests clearing the EAR after switching to
3-byte mode.  Experimentation shows that the EAR is *always* one after
the switch to 3-byte mode, so clearing the EAR is mandatory at
shutdown for a subsequent 3-byte-addressed reboot to work.

Note that some SOCs (e.g. MT7621) do not assert a reset line at normal
reboot, so we cannot rely on hardware reset.  The MT7621 does assert a
reset line at watchdog-reset.

Acked-by: Marek Vasut <marek.vasut@gmail.com>
Signed-off-by: NeilBrown <neil@brown.name>
---

Changes since v3:
 Removed Fixes/stable tags. Added Acked-by from Marek.
Changes sinc3 v2:
  Fixed comment style.


 drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++++++
 include/linux/mtd/spi-nor.h   |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 5bfa36e95f35..42ae9a1529bb 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -284,6 +284,20 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 		if (need_wren)
 			write_disable(nor);
 
+		if (!status && !enable &&
+		    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
+			/*
+			 * On Winbond W25Q256FV, leaving 4byte mode causes
+			 * the Extended Address Register to be set to 1, so all
+			 * 3-byte-address reads come from the second 16M.
+			 * We must clear the register to enable normal behavior.
+			 */
+			write_enable(nor);
+			nor->cmd_buf[0] = 0;
+			nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1);
+			write_disable(nor);
+		}
+
 		return status;
 	default:
 		/* Spansion style */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index de36969eb359..e60da0d34cc1 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_RDEAR		0xc8	/* Read Extended Address Register */
+#define SPINOR_OP_WREAR		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) */
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Boris Brezillon]

On Sat, 21 Apr 2018 08:54:40 +1000
NeilBrown <neil@brown.name> wrote:

> Winbond spi-nor flash 32MB and larger have an 'Extended Address
> Register' as one option for addressing beyond 16MB (Macronix
> has the same concept, Spansion has EXTADD bits in the Bank Address
> Register).
> 
> 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.
> 
> So the documentation suggests clearing the EAR after switching to
> 3-byte mode.  Experimentation shows that the EAR is *always* one after
> the switch to 3-byte mode, so clearing the EAR is mandatory at
> shutdown for a subsequent 3-byte-addressed reboot to work.
> 
> Note that some SOCs (e.g. MT7621) do not assert a reset line at normal
> reboot, so we cannot rely on hardware reset.  The MT7621 does assert a
> reset line at watchdog-reset.
> 
> Acked-by: Marek Vasut <marek.vasut@gmail.com>
> Signed-off-by: NeilBrown <neil@brown.name>

Applied to spi-nor/next.

Thanks,

Boris

> ---
> 
> Changes since v3:
>  Removed Fixes/stable tags. Added Acked-by from Marek.
> Changes sinc3 v2:
>   Fixed comment style.
> 
> 
>  drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++++++
>  include/linux/mtd/spi-nor.h   |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 5bfa36e95f35..42ae9a1529bb 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -284,6 +284,20 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>  		if (need_wren)
>  			write_disable(nor);
>  
> +		if (!status && !enable &&
> +		    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
> +			/*
> +			 * On Winbond W25Q256FV, leaving 4byte mode causes
> +			 * the Extended Address Register to be set to 1, so all
> +			 * 3-byte-address reads come from the second 16M.
> +			 * We must clear the register to enable normal behavior.
> +			 */
> +			write_enable(nor);
> +			nor->cmd_buf[0] = 0;
> +			nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1);
> +			write_disable(nor);
> +		}
> +
>  		return status;
>  	default:
>  		/* Spansion style */
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index de36969eb359..e60da0d34cc1 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_RDEAR		0xc8	/* Read Extended Address Register */
> +#define SPINOR_OP_WREAR		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) */

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

[Brian Norris]

Hi,

I'm a little late to this thread, but I recently noticed (and
complained about) commit: 59b356ffd0b0 ("mtd: m25p80: restore the
status of SPI flash when exiting").

On Mon, Apr 9, 2018 at 6:05 PM, NeilBrown <neil@brown.name> wrote:
> On Mon, Apr 09 2018, Marek Vasut wrote:
>
>> On 04/08/2018 11:56 PM, NeilBrown wrote:
>>> 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.
>>
>> Sorry for the delay.
>>
>>> "Broken" is an emotive word :-)  Certain the board *doesn't* reset the NOR
>>> chip on reset.
>>
>> It's not emotive, it is descriptive of what it really is. Such boards
>> which do not correctly reset the SPI NOR can, during ie. reset, get into
>> a state where the system is unbootable or corrupts the content of the
>> SPI NOR. This stuff came up over and over on the ML, it seems HW
>> designers never learn.
>
> Ok, the board is broken.
> Still, Linux has a history of even supporting broken hardware -  Spectre
> and Meltdown for example.

Except those can generally be worked around at the expense of
performance. This is impossible to workaround completely without
hardware changes.

> I wouldn't propose a fix that harms the kernel in any way (hurts
> maintainability or negatively affect other hardware), but I don't
> think my proposed patch does.

No, yours doesn't, but the original patch (Commit: 59b356ffd0b0 ("mtd:
m25p80: restore the status of SPI flash when exiting")) started us
down the slippery slope. If things work 99% of the time, people often
fail to notice that they are broken for the 1%. Thus, that patch can
harm future development, where unwitting designers think things are
working fine and fail to fix their hardware. That's not to say
designers always notice even when things are really really broken, but
that patch makes the brokenness much harder to notice.

>>> 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).
>>
>> I'm afraid this needs HW changes. The solution for SPI NORs without a
>> dedicated reset line is to just hard cut the power to them for a while
>> in case the CPU core reset out is asserted.
>>
>>> 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")
>>
>> This works when reloading the driver, but not when restarting the system.
>
> I don't understand what you are saying.
> The second patch provides a ->shutdown function for the spi_driver.
> This is called by spi_drv_shutdown which is called by the driver
> ->shutdown function, which is called by device_shutdown(), which
> is only called by
>   kernel_shutdown_prepare() and
>   kernel_restart_prepare().
>
> So it works exactly when restarting the system, and not when reloading
> the driver.
>
>>
>>> 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.

I would (and already did) vote for removal. The shutdown() hook just
papers over bugs and leads people to think that it is a good solution.
There's a reason we rejected such patches repeatedly in the past. This
one slipped through.

Brian

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5426 bytes --]

On Mon, Jul 23 2018, Brian Norris wrote:

> Hi,
>
> I'm a little late to this thread, but I recently noticed (and
> complained about) commit: 59b356ffd0b0 ("mtd: m25p80: restore the
> status of SPI flash when exiting").
>
> On Mon, Apr 9, 2018 at 6:05 PM, NeilBrown <neil@brown.name> wrote:
>> On Mon, Apr 09 2018, Marek Vasut wrote:
>>
>>> On 04/08/2018 11:56 PM, NeilBrown wrote:
>>>> 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.
>>>
>>> Sorry for the delay.
>>>
>>>> "Broken" is an emotive word :-)  Certain the board *doesn't* reset the NOR
>>>> chip on reset.
>>>
>>> It's not emotive, it is descriptive of what it really is. Such boards
>>> which do not correctly reset the SPI NOR can, during ie. reset, get into
>>> a state where the system is unbootable or corrupts the content of the
>>> SPI NOR. This stuff came up over and over on the ML, it seems HW
>>> designers never learn.
>>
>> Ok, the board is broken.
>> Still, Linux has a history of even supporting broken hardware -  Spectre
>> and Meltdown for example.
>
> Except those can generally be worked around at the expense of
> performance. This is impossible to workaround completely without
> hardware changes.
>
>> I wouldn't propose a fix that harms the kernel in any way (hurts
>> maintainability or negatively affect other hardware), but I don't
>> think my proposed patch does.
>
> No, yours doesn't, but the original patch (Commit: 59b356ffd0b0 ("mtd:
> m25p80: restore the status of SPI flash when exiting")) started us
> down the slippery slope. If things work 99% of the time, people often
> fail to notice that they are broken for the 1%. Thus, that patch can
> harm future development, where unwitting designers think things are
> working fine and fail to fix their hardware. That's not to say
> designers always notice even when things are really really broken, but
> that patch makes the brokenness much harder to notice.
>
>>>> 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).
>>>
>>> I'm afraid this needs HW changes. The solution for SPI NORs without a
>>> dedicated reset line is to just hard cut the power to them for a while
>>> in case the CPU core reset out is asserted.
>>>
>>>> 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")
>>>
>>> This works when reloading the driver, but not when restarting the system.
>>
>> I don't understand what you are saying.
>> The second patch provides a ->shutdown function for the spi_driver.
>> This is called by spi_drv_shutdown which is called by the driver
>> ->shutdown function, which is called by device_shutdown(), which
>> is only called by
>>   kernel_shutdown_prepare() and
>>   kernel_restart_prepare().
>>
>> So it works exactly when restarting the system, and not when reloading
>> the driver.
>>
>>>
>>>> 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.
>
> I would (and already did) vote for removal. The shutdown() hook just
> papers over bugs and leads people to think that it is a good solution.
> There's a reason we rejected such patches repeatedly in the past. This
> one slipped through.

Hi Brian,
 thanks for your thoughts.
 Could you just clarify what you see as the end-game.
 Do you have an alternate approach which can provide reliability for the
 various hardware which currently seems to need these patches?
 Or do you propose that people with this hardware should suffer
 a measurably lower level of reliability than they currently enjoy?

Thanks,
NeilBrown

>
> Brian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Brian Norris]

Hi Neil,

On Mon, Jul 23, 2018 at 2:45 PM, NeilBrown <neil@brown.name> wrote:
> On Mon, Jul 23 2018, Brian Norris wrote:
>> On Mon, Apr 9, 2018 at 6:05 PM, NeilBrown <neil@brown.name> wrote:
>>> On Mon, Apr 09 2018, Marek Vasut wrote:
>>>> On 04/08/2018 11:56 PM, NeilBrown wrote:
>>>>> 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.
>>
>> I would (and already did) vote for removal. The shutdown() hook just
>> papers over bugs and leads people to think that it is a good solution.
>> There's a reason we rejected such patches repeatedly in the past. This
>> one slipped through.
>
> Hi Brian,
>  thanks for your thoughts.
>  Could you just clarify what you see as the end-game.
>  Do you have an alternate approach which can provide reliability for the
>  various hardware which currently seems to need these patches?
>  Or do you propose that people with this hardware should suffer
>  a measurably lower level of reliability than they currently enjoy?

I'd suggest following the original thread, which I resurrected:

[PATCHv3 2/2] mtd: m25p80: restore the status of SPI flash when exiting
https://lkml.org/lkml/2018/7/23/1207
https://patchwork.ozlabs.org/patch/845022/

I suppose I could CC you on future replies...

My current summary: I'd prefer the hack be much more narrowly applied,
with a big warning, if we apply it at all. But if we don't merge
something to narrow the use of the hack, then yes, I'd prefer a
degraded experience for crappy products over today's status quo.

Brian

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2337 bytes --]

On Mon, Jul 23 2018, Brian Norris wrote:

> Hi Neil,
>
> On Mon, Jul 23, 2018 at 2:45 PM, NeilBrown <neil@brown.name> wrote:
>> On Mon, Jul 23 2018, Brian Norris wrote:
>>> On Mon, Apr 9, 2018 at 6:05 PM, NeilBrown <neil@brown.name> wrote:
>>>> On Mon, Apr 09 2018, Marek Vasut wrote:
>>>>> On 04/08/2018 11:56 PM, NeilBrown wrote:
>>>>>> 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.
>>>
>>> I would (and already did) vote for removal. The shutdown() hook just
>>> papers over bugs and leads people to think that it is a good solution.
>>> There's a reason we rejected such patches repeatedly in the past. This
>>> one slipped through.
>>
>> Hi Brian,
>>  thanks for your thoughts.
>>  Could you just clarify what you see as the end-game.
>>  Do you have an alternate approach which can provide reliability for the
>>  various hardware which currently seems to need these patches?
>>  Or do you propose that people with this hardware should suffer
>>  a measurably lower level of reliability than they currently enjoy?
>
> I'd suggest following the original thread, which I resurrected:
>
> [PATCHv3 2/2] mtd: m25p80: restore the status of SPI flash when exiting
> https://lkml.org/lkml/2018/7/23/1207
> https://patchwork.ozlabs.org/patch/845022/

Thanks for the links.

>
> I suppose I could CC you on future replies...

No need (though I wouldn't object).  Thanks for the heads-up!

>
> My current summary: I'd prefer the hack be much more narrowly applied,
> with a big warning, if we apply it at all. But if we don't merge
> something to narrow the use of the hack, then yes, I'd prefer a
> degraded experience for crappy products over today's status quo.
>
I'm strongly against degrading experience - partly because it could be
my experiences, partly because it seems to go against the pragmatic
basis of Linux - we build this thing because it is useful.
I don't object to highly focuses handling of specific "quirks" - that
seems to be an established pattern in Linux.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]