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

Message ID 87sh7wrq8p.fsf@notabene.neil.brown.name
State Superseded
Delegated to: Boris Brezillon
Headers show
Series
  • [v2] mtd: spi-nor: clear Winbond Extended Address Reg on switch to 3-byte addressing.
Related show

Commit Message

NeilBrown April 15, 2018, 11:42 p.m.
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(+)

Comments

Boris Brezillon April 20, 2018, 7:54 p.m. | #1
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) */
NeilBrown April 20, 2018, 9:28 p.m. | #2
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) */
Boris Brezillon April 20, 2018, 10:10 p.m. | #3
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.
NeilBrown April 20, 2018, 10:51 p.m. | #4
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.

Patch

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) */