[PATCH 1/3] spi-nor: intel-spi: Prefer WREN over other write enables

[PATCH 1/3] spi-nor: intel-spi: Prefer WREN over other write enables

[Mika Westerberg]

On many older systems using SW sequencer the PREOP_OPTYPE register
contains two preopcodes as following:

  PREOP_OPTYPE=0xf2785006

The last two bytes are the opcodes decoded to:

  0x50 - Write enable for volatile status register
  0x06 - Write enable

The former is used to modify volatile bits in the status register. For
non-volatile bits the latter is needed. Preopcodes are used in SW
sequencer to send one command "atomically" without anything else
interfering the transfer. The sequence that gets executed is:

  - Send preopcode (write enable) from PREOP_OPTYPE register
  - Send the actual SPI command
  - Poll busy bit in the status register (0x05, RDSR)

Commit 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be
programmed by BIOS") enabled atomic sequence handling but because both
preopcodes are programmed, the following happens:

  if (preop >> 8)
  	val |= SSFSTS_CTL_SPOP;

Since on these systems preop >> 8 == 0x50 we end up picking volatile
write enable instead. Because of this the actual write command is pretty
much NOP unless there is a WREN latched in the chip already.

Fix this by preferring WREN over other write enable preopcodes.

Fixes: 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS")
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index ef034d898a23..bba762aa0c8d 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -498,9 +498,17 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len,
 	val |= SSFSTS_CTL_SCGO;
 	preop = readw(ispi->sregs + PREOP_OPTYPE);
 	if (preop) {
-		val |= SSFSTS_CTL_ACS;
-		if (preop >> 8)
-			val |= SSFSTS_CTL_SPOP;
+		switch (optype) {
+		case OPTYPE_WRITE_NO_ADDR:
+		case OPTYPE_WRITE_WITH_ADDR:
+			/*
+			 * For writes prefer WREN over other write enable
+			 * opcodes.
+			 */
+			val |= SSFSTS_CTL_ACS;
+			if ((preop >> 8) == SPINOR_OP_WREN)
+				val |= SSFSTS_CTL_SPOP;
+		}
 	}
 	writel(val, ispi->sregs + SSFSTS_CTL);
 
-- 
2.15.1

[PATCH 2/3] spi-nor: intel-spi: Explicitly mark the driver as dangerous in Kconfig

[Mika Westerberg]

The driver is not meant for normal users at all but instead such users
who really know what they are doing and are able to build their own
kernel to enable it. Mark both driver Kconfig entries as dangerous to
make sure the driver is not accidentally enabled without understanding
possible issues in doing so.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/mtd/spi-nor/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 89da88e59121..f480b227a6b8 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -90,7 +90,7 @@ config SPI_INTEL_SPI
 	tristate
 
 config SPI_INTEL_SPI_PCI
-	tristate "Intel PCH/PCU SPI flash PCI driver"
+	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
 	depends on X86 && PCI
 	select SPI_INTEL_SPI
 	help
@@ -106,7 +106,7 @@ config SPI_INTEL_SPI_PCI
 	  will be called intel-spi-pci.
 
 config SPI_INTEL_SPI_PLATFORM
-	tristate "Intel PCH/PCU SPI flash platform driver"
+	tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
 	depends on X86
 	select SPI_INTEL_SPI
 	help
-- 
2.15.1

[PATCH 3/3] spi-nor: intel-spi: Remove unused preopcodes field

[Mika Westerberg]

This field is not used in the driver anymore so remove it.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/mtd/spi-nor/intel-spi.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index bba762aa0c8d..8d54da46da30 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -138,7 +138,6 @@
  * @erase_64k: 64k erase supported
  * @opcodes: Opcodes which are supported. This are programmed by BIOS
  *           before it locks down the controller.
- * @preopcodes: Preopcodes which are supported.
  */
 struct intel_spi {
 	struct device *dev;
@@ -155,7 +154,6 @@ struct intel_spi {
 	bool swseq_erase;
 	bool erase_64k;
 	u8 opcodes[8];
-	u8 preopcodes[2];
 };
 
 static bool writeable;
@@ -400,10 +398,6 @@ static int intel_spi_init(struct intel_spi *ispi)
 				ispi->opcodes[i] = opmenu0 >> i * 8;
 				ispi->opcodes[i + 4] = opmenu1 >> i * 8;
 			}
-
-			val = readl(ispi->sregs + PREOP_OPTYPE);
-			ispi->preopcodes[0] = val;
-			ispi->preopcodes[1] = val >> 8;
 		}
 	}
 
-- 
2.15.1

Re: [PATCH 3/3] spi-nor: intel-spi: Remove unused preopcodes field

[Cyrille Pitchen]

Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
> This field is not used in the driver anymore so remove it.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied to the spi-nor/next branch of linux-mtd

Thanks!

> ---
>  drivers/mtd/spi-nor/intel-spi.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> index bba762aa0c8d..8d54da46da30 100644
> --- a/drivers/mtd/spi-nor/intel-spi.c
> +++ b/drivers/mtd/spi-nor/intel-spi.c
> @@ -138,7 +138,6 @@
>   * @erase_64k: 64k erase supported
>   * @opcodes: Opcodes which are supported. This are programmed by BIOS
>   *           before it locks down the controller.
> - * @preopcodes: Preopcodes which are supported.
>   */
>  struct intel_spi {
>  	struct device *dev;
> @@ -155,7 +154,6 @@ struct intel_spi {
>  	bool swseq_erase;
>  	bool erase_64k;
>  	u8 opcodes[8];
> -	u8 preopcodes[2];
>  };
>  
>  static bool writeable;
> @@ -400,10 +398,6 @@ static int intel_spi_init(struct intel_spi *ispi)
>  				ispi->opcodes[i] = opmenu0 >> i * 8;
>  				ispi->opcodes[i + 4] = opmenu1 >> i * 8;
>  			}
> -
> -			val = readl(ispi->sregs + PREOP_OPTYPE);
> -			ispi->preopcodes[0] = val;
> -			ispi->preopcodes[1] = val >> 8;
>  		}
>  	}
>  
> 

Re: [PATCH 2/3] spi-nor: intel-spi: Explicitly mark the driver as dangerous in Kconfig

[Cyrille Pitchen]

Hi Mika,

Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
> The driver is not meant for normal users at all but instead such users
> who really know what they are doing and are able to build their own
> kernel to enable it. Mark both driver Kconfig entries as dangerous to
> make sure the driver is not accidentally enabled without understanding
> possible issues in doing so.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/mtd/spi-nor/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 89da88e59121..f480b227a6b8 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -90,7 +90,7 @@ config SPI_INTEL_SPI
>  	tristate
>  
>  config SPI_INTEL_SPI_PCI
> -	tristate "Intel PCH/PCU SPI flash PCI driver"
> +	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"

I guess it might be even better and safer to add "default n" too, don't
you agree? Adding only the "DANGEROUS" word would not prevent build
scripts to select this driver when creating a kernel config from scratch.

Best regards,

Cyrille

>  	depends on X86 && PCI
>  	select SPI_INTEL_SPI
>  	help
> @@ -106,7 +106,7 @@ config SPI_INTEL_SPI_PCI
>  	  will be called intel-spi-pci.
>  
>  config SPI_INTEL_SPI_PLATFORM
> -	tristate "Intel PCH/PCU SPI flash platform driver"
> +	tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
>  	depends on X86
>  	select SPI_INTEL_SPI
>  	help
> 

Re: [PATCH 2/3] spi-nor: intel-spi: Explicitly mark the driver as dangerous in Kconfig

[Boris Brezillon]

On Sun, 7 Jan 2018 21:32:46 +0100
Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote:

> Hi Mika,
> 
> Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
> > The driver is not meant for normal users at all but instead such users
> > who really know what they are doing and are able to build their own
> > kernel to enable it. Mark both driver Kconfig entries as dangerous to
> > make sure the driver is not accidentally enabled without understanding
> > possible issues in doing so.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/mtd/spi-nor/Kconfig | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> > index 89da88e59121..f480b227a6b8 100644
> > --- a/drivers/mtd/spi-nor/Kconfig
> > +++ b/drivers/mtd/spi-nor/Kconfig
> > @@ -90,7 +90,7 @@ config SPI_INTEL_SPI
> >  	tristate
> >  
> >  config SPI_INTEL_SPI_PCI
> > -	tristate "Intel PCH/PCU SPI flash PCI driver"
> > +	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"  
> 
> I guess it might be even better and safer to add "default n" too, don't
> you agree? Adding only the "DANGEROUS" word would not prevent build
> scripts to select this driver when creating a kernel config from scratch.

How about adding

	depends on EXPERT

?

> 
> Best regards,
> 
> Cyrille
> 
> >  	depends on X86 && PCI
> >  	select SPI_INTEL_SPI
> >  	help
> > @@ -106,7 +106,7 @@ config SPI_INTEL_SPI_PCI
> >  	  will be called intel-spi-pci.
> >  
> >  config SPI_INTEL_SPI_PLATFORM
> > -	tristate "Intel PCH/PCU SPI flash platform driver"
> > +	tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> >  	depends on X86
> >  	select SPI_INTEL_SPI
> >  	help
> >   
> 

Re: [PATCH 1/3] spi-nor: intel-spi: Prefer WREN over other write enables

[Cyrille Pitchen]

Hi Mika,

Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
> On many older systems using SW sequencer the PREOP_OPTYPE register
> contains two preopcodes as following:
> 
>   PREOP_OPTYPE=0xf2785006
> 
> The last two bytes are the opcodes decoded to:
> 
>   0x50 - Write enable for volatile status register
>   0x06 - Write enable
> 
> The former is used to modify volatile bits in the status register. For
> non-volatile bits the latter is needed. Preopcodes are used in SW
> sequencer to send one command "atomically" without anything else
> interfering the transfer. The sequence that gets executed is:
> 
>   - Send preopcode (write enable) from PREOP_OPTYPE register
>   - Send the actual SPI command
>   - Poll busy bit in the status register (0x05, RDSR)
> 
> Commit 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be
> programmed by BIOS") enabled atomic sequence handling but because both
> preopcodes are programmed, the following happens:
> 
>   if (preop >> 8)
>   	val |= SSFSTS_CTL_SPOP;
> 
> Since on these systems preop >> 8 == 0x50 we end up picking volatile
> write enable instead. Because of this the actual write command is pretty
> much NOP unless there is a WREN latched in the chip already.
>

I agree with you but doesn't it mean that the Sector/Block Erase command that should
have been sent before the Page Program commands was discarded too?
If so, the BIOS should not have been corrupted at all.

Honestly, I don't really understand what is the real root cause of the BIOS corruption
reported by Ubuntu users. This is the issue your patch is trying to fix, isn't it?

Does the BIOS corruption occur just by probing the SPI controller and its SPI flash
memory during the boot or does some userspace program is involved too?

> Fix this by preferring WREN over other write enable preopcodes.
> 
> Fixes: 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS")
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> index ef034d898a23..bba762aa0c8d 100644
> --- a/drivers/mtd/spi-nor/intel-spi.c
> +++ b/drivers/mtd/spi-nor/intel-spi.c
> @@ -498,9 +498,17 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len,
>  	val |= SSFSTS_CTL_SCGO;
>  	preop = readw(ispi->sregs + PREOP_OPTYPE);
>  	if (preop) {
> -		val |= SSFSTS_CTL_ACS;
> -		if (preop >> 8)
> -			val |= SSFSTS_CTL_SPOP;
> +		switch (optype) {
> +		case OPTYPE_WRITE_NO_ADDR:
> +		case OPTYPE_WRITE_WITH_ADDR:
> +			/*
> +			 * For writes prefer WREN over other write enable
> +			 * opcodes.
> +			 */
> +			val |= SSFSTS_CTL_ACS;
> +			if ((preop >> 8) == SPINOR_OP_WREN)
> +				val |= SSFSTS_CTL_SPOP;
> +		}
>  	}
>  	writel(val, ispi->sregs + SSFSTS_CTL);
>  
> 

I guess I have figured out how the SPI controller works but just to be sure, what are
SSFSTS_CTL_SPOP and SSFSTS_CTL_ACS used for?

Why before commit 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be
programmed by BIOS") were they not used at all? Does it mean that before Bin's patch,
when the controller was locked (by the BIOS?), the Linux driver never sent any Write Enable
command as pre op code, hence the SPI flash memory was read-only?

Why is ispi->locked not tested inside intel_spi_sw_cycle() at the same time as the preop
value? Do you try now to allow Page Program and Sector/Block Erase operations also when
ispi->lock is true?

Best regards,

Cyrille

Re: [PATCH 2/3] spi-nor: intel-spi: Explicitly mark the driver as dangerous in Kconfig

[Mika Westerberg]

On Sun, Jan 07, 2018 at 09:32:46PM +0100, Cyrille Pitchen wrote:
> Hi Mika,
> 
> Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
> > The driver is not meant for normal users at all but instead such users
> > who really know what they are doing and are able to build their own
> > kernel to enable it. Mark both driver Kconfig entries as dangerous to
> > make sure the driver is not accidentally enabled without understanding
> > possible issues in doing so.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/mtd/spi-nor/Kconfig | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> > index 89da88e59121..f480b227a6b8 100644
> > --- a/drivers/mtd/spi-nor/Kconfig
> > +++ b/drivers/mtd/spi-nor/Kconfig
> > @@ -90,7 +90,7 @@ config SPI_INTEL_SPI
> >  	tristate
> >  
> >  config SPI_INTEL_SPI_PCI
> > -	tristate "Intel PCH/PCU SPI flash PCI driver"
> > +	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> 
> I guess it might be even better and safer to add "default n" too, don't
> you agree? Adding only the "DANGEROUS" word would not prevent build
> scripts to select this driver when creating a kernel config from scratch.

But isn't it always "default n" unless otherwise stated?

Re: [PATCH 2/3] spi-nor: intel-spi: Explicitly mark the driver as dangerous in Kconfig

[Mika Westerberg]

On Sun, Jan 07, 2018 at 10:06:31PM +0100, Boris Brezillon wrote:
> On Sun, 7 Jan 2018 21:32:46 +0100
> Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote:
> 
> > Hi Mika,
> > 
> > Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
> > > The driver is not meant for normal users at all but instead such users
> > > who really know what they are doing and are able to build their own
> > > kernel to enable it. Mark both driver Kconfig entries as dangerous to
> > > make sure the driver is not accidentally enabled without understanding
> > > possible issues in doing so.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/mtd/spi-nor/Kconfig | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> > > index 89da88e59121..f480b227a6b8 100644
> > > --- a/drivers/mtd/spi-nor/Kconfig
> > > +++ b/drivers/mtd/spi-nor/Kconfig
> > > @@ -90,7 +90,7 @@ config SPI_INTEL_SPI
> > >  	tristate
> > >  
> > >  config SPI_INTEL_SPI_PCI
> > > -	tristate "Intel PCH/PCU SPI flash PCI driver"
> > > +	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"  
> > 
> > I guess it might be even better and safer to add "default n" too, don't
> > you agree? Adding only the "DANGEROUS" word would not prevent build
> > scripts to select this driver when creating a kernel config from scratch.
> 
> How about adding
> 
> 	depends on EXPERT
> 
> ?

It used to be like that but then it was pointed out by Arnd Bergman that
EXPERT is not about hiding drivers and got removed by b8cc0012917d
("spi-nor: intel-spi: Remove EXPERT dependency").

Re: [PATCH 1/3] spi-nor: intel-spi: Prefer WREN over other write enables

[Mika Westerberg]

On Mon, Jan 08, 2018 at 12:29:58AM +0100, Cyrille Pitchen wrote:
> Hi Mika,
> 
> Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
> > On many older systems using SW sequencer the PREOP_OPTYPE register
> > contains two preopcodes as following:
> > 
> >   PREOP_OPTYPE=0xf2785006
> > 
> > The last two bytes are the opcodes decoded to:
> > 
> >   0x50 - Write enable for volatile status register
> >   0x06 - Write enable
> > 
> > The former is used to modify volatile bits in the status register. For
> > non-volatile bits the latter is needed. Preopcodes are used in SW
> > sequencer to send one command "atomically" without anything else
> > interfering the transfer. The sequence that gets executed is:
> > 
> >   - Send preopcode (write enable) from PREOP_OPTYPE register
> >   - Send the actual SPI command
> >   - Poll busy bit in the status register (0x05, RDSR)
> > 
> > Commit 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be
> > programmed by BIOS") enabled atomic sequence handling but because both
> > preopcodes are programmed, the following happens:
> > 
> >   if (preop >> 8)
> >   	val |= SSFSTS_CTL_SPOP;
> > 
> > Since on these systems preop >> 8 == 0x50 we end up picking volatile
> > write enable instead. Because of this the actual write command is pretty
> > much NOP unless there is a WREN latched in the chip already.
> >
> 
> I agree with you but doesn't it mean that the Sector/Block Erase command that should
> have been sent before the Page Program commands was discarded too?

Yes

> If so, the BIOS should not have been corrupted at all.

Correct and there was never any BIOS corruption.

> Honestly, I don't really understand what is the real root cause of the BIOS corruption
> reported by Ubuntu users. This is the issue your patch is trying to fix, isn't it?

No, this patch does not try to fix that issue because it has
already been fixed with 9d63f17661e2 ("spi-nor: intel-spi: Fix broken
software sequencing codes") in september.

The root cause is explained here:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1734147/comments/330

To summarize. The BIOS programs all allowable opcodes to OPMENU0/1
registers and then locks them down. One of the opcodes was WRSR (Write
Status register) which is already against any recommendations
to have it there in the first place.

Nevertheless Linux clears the first status register when
SPI_NOR_HAS_LOCK is set for the serial flash. Because of the bug that
was fixed with 9d63f17661e2 we accidentally write an additional byte
from the FIFO (which is second byte of the JEDEC ID we just read) that
then goes to the second status register (because WRSR allows to write
all of them at once). If the second byte of the JEDEC ID happens to have
bit 6 set it means that now the serial flash CMP (complement) bit is
set. CMP=1 and BP0/1/2=0 means the whole flash is write protected.

The BIOS in question then cannot save settings anymore.

So the issue is actually not about any corruption but simply that the
chip is set to write protected from the status register.

Also note that it did not affect all the systems, mostly because of the
missing WREN handling. Normally the WRSR write would have been discarded
but if the BIOS before it handed of to the OS did WREN the write from
our WRSR took place and set the CMP bit to 1.

This is all fixable by software and in fact in the launcpad bug there
is another pre-built kernel image where we explictly clear both status
registers (it includes this patch) to make sure CMP is set back to 0.
That has recovered most of the affected systems AFAIK.

What this patch actually is about is to properly handle the WREN+WRSR
atomic sequence.

> Does the BIOS corruption occur just by probing the SPI controller and its SPI flash
> memory during the boot or does some userspace program is involved too?

See above.

> 
> > Fix this by preferring WREN over other write enable preopcodes.
> > 
> > Fixes: 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS")
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> > index ef034d898a23..bba762aa0c8d 100644
> > --- a/drivers/mtd/spi-nor/intel-spi.c
> > +++ b/drivers/mtd/spi-nor/intel-spi.c
> > @@ -498,9 +498,17 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len,
> >  	val |= SSFSTS_CTL_SCGO;
> >  	preop = readw(ispi->sregs + PREOP_OPTYPE);
> >  	if (preop) {
> > -		val |= SSFSTS_CTL_ACS;
> > -		if (preop >> 8)
> > -			val |= SSFSTS_CTL_SPOP;
> > +		switch (optype) {
> > +		case OPTYPE_WRITE_NO_ADDR:
> > +		case OPTYPE_WRITE_WITH_ADDR:
> > +			/*
> > +			 * For writes prefer WREN over other write enable
> > +			 * opcodes.
> > +			 */
> > +			val |= SSFSTS_CTL_ACS;
> > +			if ((preop >> 8) == SPINOR_OP_WREN)
> > +				val |= SSFSTS_CTL_SPOP;
> > +		}
> >  	}
> >  	writel(val, ispi->sregs + SSFSTS_CTL);
> >  
> > 
> 
> I guess I have figured out how the SPI controller works but just to be sure, what are
> SSFSTS_CTL_SPOP and SSFSTS_CTL_ACS used for?

_SPOP selects preopcode from two available (WREN and something else)

_ACS enables atomic sequence i.e execute preopcode + opcode + wait for
busy.

> Why before commit 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be
> programmed by BIOS") were they not used at all? Does it mean that before Bin's patch,
> when the controller was locked (by the BIOS?), the Linux driver never sent any Write Enable
> command as pre op code, hence the SPI flash memory was read-only?

Yes.

> Why is ispi->locked not tested inside intel_spi_sw_cycle() at the same time as the preop
> value? Do you try now to allow Page Program and Sector/Block Erase operations also when
> ispi->lock is true?

ispi->locked is never changed and it is not about write protection to
the serial flash but it means that certain controller registers (like
the OPMENU0/1) cannot be modified.

Re: [PATCH 1/3] spi-nor: intel-spi: Prefer WREN over other write enables

[Mika Westerberg]

On Thu, Jan 04, 2018 at 12:07:42PM +0300, Mika Westerberg wrote:
> On many older systems using SW sequencer the PREOP_OPTYPE register
> contains two preopcodes as following:
> 
>   PREOP_OPTYPE=0xf2785006
> 
> The last two bytes are the opcodes decoded to:
> 
>   0x50 - Write enable for volatile status register
>   0x06 - Write enable
> 
> The former is used to modify volatile bits in the status register. For
> non-volatile bits the latter is needed. Preopcodes are used in SW
> sequencer to send one command "atomically" without anything else
> interfering the transfer. The sequence that gets executed is:
> 
>   - Send preopcode (write enable) from PREOP_OPTYPE register
>   - Send the actual SPI command
>   - Poll busy bit in the status register (0x05, RDSR)
> 
> Commit 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be
> programmed by BIOS") enabled atomic sequence handling but because both
> preopcodes are programmed, the following happens:
> 
>   if (preop >> 8)
>   	val |= SSFSTS_CTL_SPOP;
> 
> Since on these systems preop >> 8 == 0x50 we end up picking volatile
> write enable instead. Because of this the actual write command is pretty
> much NOP unless there is a WREN latched in the chip already.
> 
> Fix this by preferring WREN over other write enable preopcodes.
> 
> Fixes: 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS")
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: stable@vger.kernel.org

Hi,

Any change getting these merged for v4.16?

Re: [PATCH 1/3] spi-nor: intel-spi: Prefer WREN over other write enables

[Boris Brezillon]

On Tue, 30 Jan 2018 12:50:13 +0200
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Thu, Jan 04, 2018 at 12:07:42PM +0300, Mika Westerberg wrote:
> > On many older systems using SW sequencer the PREOP_OPTYPE register
> > contains two preopcodes as following:
> > 
> >   PREOP_OPTYPE=0xf2785006
> > 
> > The last two bytes are the opcodes decoded to:
> > 
> >   0x50 - Write enable for volatile status register
> >   0x06 - Write enable
> > 
> > The former is used to modify volatile bits in the status register. For
> > non-volatile bits the latter is needed. Preopcodes are used in SW
> > sequencer to send one command "atomically" without anything else
> > interfering the transfer. The sequence that gets executed is:
> > 
> >   - Send preopcode (write enable) from PREOP_OPTYPE register
> >   - Send the actual SPI command
> >   - Poll busy bit in the status register (0x05, RDSR)
> > 
> > Commit 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be
> > programmed by BIOS") enabled atomic sequence handling but because both
> > preopcodes are programmed, the following happens:
> > 
> >   if (preop >> 8)
> >   	val |= SSFSTS_CTL_SPOP;
> > 
> > Since on these systems preop >> 8 == 0x50 we end up picking volatile
> > write enable instead. Because of this the actual write command is pretty
> > much NOP unless there is a WREN latched in the chip already.
> > 
> > Fix this by preferring WREN over other write enable preopcodes.
> > 
> > Fixes: 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS")
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: stable@vger.kernel.org  
> 
> Hi,
> 
> Any change getting these merged for v4.16?

It's a fix, so yes, I can queue it for -rc2. I just need Cyrille's ack.

Re: [PATCH 1/3] spi-nor: intel-spi: Prefer WREN over other write enables

[Cyrille Pitchen]

Hi Mika,

Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
> On many older systems using SW sequencer the PREOP_OPTYPE register
> contains two preopcodes as following:
> 
>   PREOP_OPTYPE=0xf2785006
> 
> The last two bytes are the opcodes decoded to:
> 
>   0x50 - Write enable for volatile status register
>   0x06 - Write enable
> 
> The former is used to modify volatile bits in the status register. For
> non-volatile bits the latter is needed. Preopcodes are used in SW
> sequencer to send one command "atomically" without anything else
> interfering the transfer. The sequence that gets executed is:
> 
>   - Send preopcode (write enable) from PREOP_OPTYPE register
>   - Send the actual SPI command
>   - Poll busy bit in the status register (0x05, RDSR)
> 
> Commit 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be
> programmed by BIOS") enabled atomic sequence handling but because both
> preopcodes are programmed, the following happens:
> 
>   if (preop >> 8)
>   	val |= SSFSTS_CTL_SPOP;
> 
> Since on these systems preop >> 8 == 0x50 we end up picking volatile
> write enable instead. Because of this the actual write command is pretty
> much NOP unless there is a WREN latched in the chip already.
> 
> Fix this by preferring WREN over other write enable preopcodes.
> 
> Fixes: 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS")
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> index ef034d898a23..bba762aa0c8d 100644
> --- a/drivers/mtd/spi-nor/intel-spi.c
> +++ b/drivers/mtd/spi-nor/intel-spi.c
> @@ -498,9 +498,17 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len,
>  	val |= SSFSTS_CTL_SCGO;
>  	preop = readw(ispi->sregs + PREOP_OPTYPE);
>  	if (preop) {
> -		val |= SSFSTS_CTL_ACS;
> -		if (preop >> 8)
> -			val |= SSFSTS_CTL_SPOP;
> +		switch (optype) {
> +		case OPTYPE_WRITE_NO_ADDR:
> +		case OPTYPE_WRITE_WITH_ADDR:
> +			/*
> +			 * For writes prefer WREN over other write enable
> +			 * opcodes.
> +			 */
> +			val |= SSFSTS_CTL_ACS;
> +			if ((preop >> 8) == SPINOR_OP_WREN)
> +				val |= SSFSTS_CTL_SPOP;
> +		}
>  	}
>  	writel(val, ispi->sregs + SSFSTS_CTL);
>  
> 

I generally strongly discourage the use of SPINOR_OP_* macros in any
SPI (flash) controller driver, because when some of them does so it
often means that driver tries to guess what spi-nor.c expects it to do
and then tries to tweak the requested SPI command.
It may work at some point but when we do some legitimate changes
inside spi-nor.c, the SPI (flash) controller driver may no longer know
what to do with new and unexpected SPI commands.

Definitely, such SPI (flash) controller driver is broken and is a real
pain to maintain. SPI (flash) controller drivers should be "dummy" and
just execute the SPI command as requested by spi-nor.c, without
tuning, modifying it or whatever.

It seems that Yogesh is currently cleaning the fsl-quadspi.c driver to
remove reference to SPINOR_OP_* macros, which is a really good thing.
So I would like the same thing to be done for the intel-spi.c driver.

More precisely, for a first step, I would like intel_spi_write_reg()
to be updated so the "if (opcode == SPINOR_OP_WREN) { ... }" chunk is
replaced by something like this:

1/ The SPI flash controller is locked

compare the opcode argument with the supported opcodes read from the
registers of the Intel SPI controller (OPMENU0, OPMENU1 or
PREOP[_OPTYPE]). If the op code was found, good! then just use it,
otherwise return -EINVAL instead of pretending that the SPI command
has been handled. Indeed, with the current source code,
intel_spi_write_reg() pretends that the Write Enable command was sent
but actually is not.

When this driver was introduced first, I thought that the hardware was
comparing the op code then automatically preprended some Write Enable
command when needed but based on the lastest commits, it seems the
finally the hardware doesn't work this way.

Hence if the sending of the Write Enable command, or any other SPI
command, has to be triggered by the software in some way, please don't
try to mystify spi-nor.c if the requested operation has not been
handled as expected.

2/ The SPI flash controller has not been locked

Write the requested opcode in whatever register you want (OPMENU0,
OPMENU1 or PREOP) then execute the SPI command as requested.
Once again, I would like to avoid any special case, like
SPINOR_OP_WREN. So maybe PREOP should not be used at all to prepend
Write Enable before the next to come SPI command but simply just use
OPMENU0 in all cases.


If you still need to use PREOP_OPTYPE from intel_spi_sw_cycle(), then
the value of the SSFSTS_CTL_ACS and/or SSFSTS_CTL_SPOP flags should
have been chosen from the very same place where it has been decided
that we will use the PREOP_OPTYPE to prepend some op code before the
next SPI command. It's too late once in intel_spi_sw_cycle() to guess
what opcode may need to be inserted.

The source code should not rely on the following assumption:

(PREOP_OPTYPE != 0) => "We want to send a Write Enable command"


For what I understand, it might be almost enough to just remove the
"if (opcode == SPINOR_OP_WREN) { .. }" chunk from intel_spi_write_reg()
and add replace the "if (preop)" test in intel_spi_sw_cycle() by
"if (!ispi->locked && preop)".

Also, at some point the 16 least significant bits of PREOP_OPTYPE should
be reset to zero, otherwise all SPI commands to follow would keep on
inserting op codes from the PREOP_OPTYPE register...

Finally, if intel_spi_sw_cycle() could be used in any cases, then please
remove intel_spi_hw_cycle() and its references to SPINOR_OP_* macros.


Best regards,

Cyrille

Re: [PATCH 1/3] spi-nor: intel-spi: Prefer WREN over other write enables

[Mika Westerberg]

On Thu, Feb 01, 2018 at 12:36:56PM +0100, Cyrille Pitchen wrote:
> Hi Mika,

Hi,

> Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
> > On many older systems using SW sequencer the PREOP_OPTYPE register
> > contains two preopcodes as following:
> > 
> >   PREOP_OPTYPE=0xf2785006
> > 
> > The last two bytes are the opcodes decoded to:
> > 
> >   0x50 - Write enable for volatile status register
> >   0x06 - Write enable
> > 
> > The former is used to modify volatile bits in the status register. For
> > non-volatile bits the latter is needed. Preopcodes are used in SW
> > sequencer to send one command "atomically" without anything else
> > interfering the transfer. The sequence that gets executed is:
> > 
> >   - Send preopcode (write enable) from PREOP_OPTYPE register
> >   - Send the actual SPI command
> >   - Poll busy bit in the status register (0x05, RDSR)
> > 
> > Commit 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be
> > programmed by BIOS") enabled atomic sequence handling but because both
> > preopcodes are programmed, the following happens:
> > 
> >   if (preop >> 8)
> >   	val |= SSFSTS_CTL_SPOP;
> > 
> > Since on these systems preop >> 8 == 0x50 we end up picking volatile
> > write enable instead. Because of this the actual write command is pretty
> > much NOP unless there is a WREN latched in the chip already.
> > 
> > Fix this by preferring WREN over other write enable preopcodes.
> > 
> > Fixes: 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS")
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> > index ef034d898a23..bba762aa0c8d 100644
> > --- a/drivers/mtd/spi-nor/intel-spi.c
> > +++ b/drivers/mtd/spi-nor/intel-spi.c
> > @@ -498,9 +498,17 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len,
> >  	val |= SSFSTS_CTL_SCGO;
> >  	preop = readw(ispi->sregs + PREOP_OPTYPE);
> >  	if (preop) {
> > -		val |= SSFSTS_CTL_ACS;
> > -		if (preop >> 8)
> > -			val |= SSFSTS_CTL_SPOP;
> > +		switch (optype) {
> > +		case OPTYPE_WRITE_NO_ADDR:
> > +		case OPTYPE_WRITE_WITH_ADDR:
> > +			/*
> > +			 * For writes prefer WREN over other write enable
> > +			 * opcodes.
> > +			 */
> > +			val |= SSFSTS_CTL_ACS;
> > +			if ((preop >> 8) == SPINOR_OP_WREN)
> > +				val |= SSFSTS_CTL_SPOP;
> > +		}
> >  	}
> >  	writel(val, ispi->sregs + SSFSTS_CTL);
> >  
> > 
> 
> I generally strongly discourage the use of SPINOR_OP_* macros in any
> SPI (flash) controller driver, because when some of them does so it
> often means that driver tries to guess what spi-nor.c expects it to do
> and then tries to tweak the requested SPI command.
> It may work at some point but when we do some legitimate changes
> inside spi-nor.c, the SPI (flash) controller driver may no longer know
> what to do with new and unexpected SPI commands.
> 
> Definitely, such SPI (flash) controller driver is broken and is a real
> pain to maintain. SPI (flash) controller drivers should be "dummy" and
> just execute the SPI command as requested by spi-nor.c, without
> tuning, modifying it or whatever.
> 
> It seems that Yogesh is currently cleaning the fsl-quadspi.c driver to
> remove reference to SPINOR_OP_* macros, which is a really good thing.
> So I would like the same thing to be done for the intel-spi.c driver.

I understand what you are after but unfortunately that cannot be
generally applied to all SPI controllers. For SW sequencing we pretty
much can do what you are saying (sans the WREN preop things) but for HW
sequencing which is what all modern controllers only support, we need to
translate from SPINOR_OP_* to whatever the controller supports. There is
no way to pass those things directly to the flash.

> More precisely, for a first step, I would like intel_spi_write_reg()
> to be updated so the "if (opcode == SPINOR_OP_WREN) { ... }" chunk is
> replaced by something like this:
> 
> 1/ The SPI flash controller is locked
> 
> compare the opcode argument with the supported opcodes read from the
> registers of the Intel SPI controller (OPMENU0, OPMENU1 or
> PREOP[_OPTYPE]). If the op code was found, good! then just use it,
> otherwise return -EINVAL instead of pretending that the SPI command
> has been handled. Indeed, with the current source code,
> intel_spi_write_reg() pretends that the Write Enable command was sent
> but actually is not.

Fair enough, I'll update the driver to do this.

> When this driver was introduced first, I thought that the hardware was
> comparing the op code then automatically preprended some Write Enable
> command when needed but based on the lastest commits, it seems the
> finally the hardware doesn't work this way.
> 
> Hence if the sending of the Write Enable command, or any other SPI
> command, has to be triggered by the software in some way, please don't
> try to mystify spi-nor.c if the requested operation has not been
> handled as expected.
> 
> 2/ The SPI flash controller has not been locked
> 
> Write the requested opcode in whatever register you want (OPMENU0,
> OPMENU1 or PREOP) then execute the SPI command as requested.
> Once again, I would like to avoid any special case, like
> SPINOR_OP_WREN. So maybe PREOP should not be used at all to prepend
> Write Enable before the next to come SPI command but simply just use
> OPMENU0 in all cases.

Unfortunately that does not work because WREN needs to be applied as
part of atomic sequence. There are other agents in the system (for
example GBe) that accesss the flash as well and we must use atomic
sequence with proper PREOP to make sure they don't mess with each other.

> If you still need to use PREOP_OPTYPE from intel_spi_sw_cycle(), then
> the value of the SSFSTS_CTL_ACS and/or SSFSTS_CTL_SPOP flags should
> have been chosen from the very same place where it has been decided
> that we will use the PREOP_OPTYPE to prepend some op code before the
> next SPI command. It's too late once in intel_spi_sw_cycle() to guess
> what opcode may need to be inserted.
> 
> The source code should not rely on the following assumption:
> 
> (PREOP_OPTYPE != 0) => "We want to send a Write Enable command"
> 
> 
> For what I understand, it might be almost enough to just remove the
> "if (opcode == SPINOR_OP_WREN) { .. }" chunk from intel_spi_write_reg()
> and add replace the "if (preop)" test in intel_spi_sw_cycle() by
> "if (!ispi->locked && preop)".

OK, I'll look into this and update the driver accordingly.

> Also, at some point the 16 least significant bits of PREOP_OPTYPE should
> be reset to zero, otherwise all SPI commands to follow would keep on
> inserting op codes from the PREOP_OPTYPE register...

It actually does not work like that. There is another register (upper
16-bits of that register) telling type of the command and only commands
that are "write" or "write with address" will use PREOP command. Even
when BIOS has locked everything the PREOP register contains those two
preopcodes.

> Finally, if intel_spi_sw_cycle() could be used in any cases, then please
> remove intel_spi_hw_cycle() and its references to SPINOR_OP_* macros.

Unfurtunately that's not possible. The recommended and more safe way is
to use HW sequencer instead and that's the only sequencer available in
modern and future systems AFAIK.