spi-nor: intel-spi: Prefer WREN over other write enables
Subsystems (2)
PATCH 1/3 spi-nor: intel-spi: Prefer WREN over other write enables
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.
--- drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/mtd/spi-nor/intel-spi.c +11 -3
@@ -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 );
Signature · 1 line
2.15.1
PATCH 2/3 spi-nor: intel-spi: Explicitly mark the driver as dangerous in Kconfig
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.
--- drivers/mtd/spi-nor/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/mtd/spi-nor/Kconfig +2 -2
@@ -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
Signature · 1 line
2.15.1
Re: [PATCH 2/3] spi-nor: intel-spi: Explicitly mark the driver as dangerous in Kconfig
Hi Mika,
Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
Quoted 16 lines
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
Kconfigentries as dangerous to make sure the driver is not accidentally enabled without understanding possible issues in doing so.Signed-off-by Mika Westerberg <[email protected]>--- drivers/mtd/spi-nor/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/mtd/spi-nor/Kconfig +0
-0
drivers/mtd/spi-nor/Kconfig config SPI_INTEL_SPI · line 93
Quoted 5 lines
drivers/mtd/spi-nor/Kconfig +1
-1@@ -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
Quoted 13 lines
drivers/mtd/spi-nor/Kconfig +1
-1depends 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
Quoted 21 lines · On Sun, 7 Jan 2018 21:32:46 +0100 Cyrille Pitchen <[email protected]>…
Hi Mika,
Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
Quoted 18 lines
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
Kconfigentries as dangerous to make sure the driver is not accidentally enabled without understanding possible issues in doing so.Signed-off-by Mika Westerberg <[email protected]>--- drivers/mtd/spi-nor/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/mtd/spi-nor/Kconfig +0
-0@@ -90,7 +90,7 @@ config SPI_INTEL_SPI tristate
Quoted 7 lines
drivers/mtd/spi-nor/Kconfig config SPI_INTEL_SPI · line 93
Quoted 3 lines
drivers/mtd/spi-nor/Kconfig +1
-1config 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
?
Quoted 19 lines
Best regards,
Cyrille
Quoted 13 lines
drivers/mtd/spi-nor/Kconfig +1
-1depends 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
Quoted 32 lines · Boris Brezillon
Quoted 21 lines · On Sun, 7 Jan 2018 21:32:46 +0100 Cyrille Pitchen <[email protected]>…
Hi Mika,
Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
Quoted 18 lines
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
Kconfigentries as dangerous to make sure the driver is not accidentally enabled without understanding possible issues in doing so.Signed-off-by Mika Westerberg <[email protected]>--- drivers/mtd/spi-nor/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/mtd/spi-nor/Kconfig +0
-0@@ -90,7 +90,7 @@ config SPI_INTEL_SPI tristateQuoted 7 lines
drivers/mtd/spi-nor/Kconfig config SPI_INTEL_SPI · line 93
Quoted 3 lines
drivers/mtd/spi-nor/Kconfig +1
-1config 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.
Quoted 5 lines
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 2/3] spi-nor: intel-spi: Explicitly mark the driver as dangerous in Kconfig
In reply to Cyrille PitchenQuoted 21 lines · Cyrille Pitchen
Hi Mika,
Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
Quoted 18 lines
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
Kconfigentries as dangerous to make sure the driver is not accidentally enabled without understanding possible issues in doing so.Signed-off-by Mika Westerberg <[email protected]>--- drivers/mtd/spi-nor/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/mtd/spi-nor/Kconfig +0
-0@@ -90,7 +90,7 @@ config SPI_INTEL_SPI tristate
Quoted 7 lines
drivers/mtd/spi-nor/Kconfig config SPI_INTEL_SPI · line 93
Quoted 3 lines
drivers/mtd/spi-nor/Kconfig +1
-1config 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?
PATCH 3/3 spi-nor: intel-spi: Remove unused preopcodes field
In reply to Mika WesterbergThis field is not used in the driver anymore so remove it.
--- drivers/mtd/spi-nor/intel-spi.c | 6 ------ 1 file changed, 6 deletions(-)
drivers/mtd/spi-nor/intel-spi.c +0 -6
@@ -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 ; } }
Signature · 1 line
2.15.1
Cyrille Pitchen
AppliedRe: [PATCH 3/3] spi-nor: intel-spi: Remove unused preopcodes field
Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
Quoted 3 lines
This field is not used in the driver anymore so remove it.
Signed-off-by Mika Westerberg <[email protected]>
Applied to the spi-nor/next branch of linux-mtd
Thanks!
Quoted 36 lines
--- drivers/mtd/spi-nor/intel-spi.c | 6 ------ 1 file changed, 6 deletions(-)drivers/mtd/spi-nor/intel-spi.c +0
-6@@ -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 ; } }
Hi Mika,
Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
Quoted 23 lines
On many older systems using SW sequencer the
PREOP_OPTYPEregister contains two preopcodes as following:PREOP_OPTYPE=0xf2785006
The last two bytes are the opcodes decoded to:
0x50- Write enable for volatile status register0x06- Write enableThe 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_OPTYPEregister
- 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:
Quoted 7 lines
if (preop >> 8) val |= SSFSTS_CTL_SPOP;Since on these systems preop >> 8 ==
0x50we 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?
Quoted 25 lines
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 <[email protected]>--- drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/mtd/spi-nor/intel-spi.c +4
-3@@ -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 : +/*
drivers/mtd/spi-nor/intel-spi.c static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len, · line 514
Quoted 10 lines
drivers/mtd/spi-nor/intel-spi.c +7
-0+* 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
Quoted 29 lines · Cyrille Pitchen
Hi Mika,
Le 04/01/2018 à 10:07, Mika Westerberg a écrit :
Quoted 26 lines
On many older systems using SW sequencer the
PREOP_OPTYPEregister contains two preopcodes as following:PREOP_OPTYPE=0xf2785006
The last two bytes are the opcodes decoded to:
0x50- Write enable for volatile status register0x06- Write enableThe 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_OPTYPEregister
- 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;
Quoted 7 lines
Quoted 4 lines
Since on these systems preop >> 8 ==
0x50we 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
Quoted 1 line
If so, the BIOS should not have been corrupted at all.
Correct and there was never any BIOS corruption.
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 31 lines
Quoted 31 lines
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 <[email protected]>--- drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/mtd/spi-nor/intel-spi.c +10
-3@@ -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 ;
Quoted 8 lines
drivers/mtd/spi-nor/intel-spi.c static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len, · line 514
Quoted 5 lines
drivers/mtd/spi-nor/intel-spi.c +1
-0+ } }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_SPOPandSSFSTS_CTL_ACSused for?
_SPOP selects preopcode from two available (WREN and something else)
_ACS enables atomic sequence i.e execute preopcode + opcode + wait for busy.
Quoted 4 lines
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.
Quoted 3 lines
Why is
ispi->lockednot tested insideintel_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 whenispi->lockis 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.
Quoted 30 lines · Mika Westerberg
On many older systems using SW sequencer the
PREOP_OPTYPEregister contains two preopcodes as following:PREOP_OPTYPE=0xf2785006
The last two bytes are the opcodes decoded to:
0x50- Write enable for volatile status register0x06- Write enableThe 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_OPTYPEregister
- 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 ==
0x50we 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.
Quoted 5 lines
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 <[email protected]>
Hi,
Any change getting these merged for v4.16?
Quoted 33 lines · On Tue, 30 Jan 2018 12:50:13 +0200 Mika Westerberg…
Quoted 32 lines · Mika Westerberg
On many older systems using SW sequencer the
PREOP_OPTYPEregister contains two preopcodes as following:PREOP_OPTYPE=0xf2785006
The last two bytes are the opcodes decoded to:
0x50- Write enable for volatile status register0x06- Write enableThe 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_OPTYPEregister
- 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 ==
0x50we 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.
Quoted 7 lines
Quoted 3 lines
Fixes 8c473dd61bb5 ("spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS")Signed-off-by Mika Westerberg <[email protected]>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.
Hi Mika,
Quoted 55 lines · Mika Westerberg
On many older systems using SW sequencer the
PREOP_OPTYPEregister contains two preopcodes as following:PREOP_OPTYPE=0xf2785006
The last two bytes are the opcodes decoded to:
0x50- Write enable for volatile status register0x06- Write enableThe 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_OPTYPEregister
- 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 ==
0x50we 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 <[email protected]>--- drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/mtd/spi-nor/intel-spi.c +4
-3@@ -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 : +/*
drivers/mtd/spi-nor/intel-spi.c static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len, · line 514
Quoted 10 lines
drivers/mtd/spi-nor/intel-spi.c +7
-0+* 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
Quoted 1 line · Cyrille Pitchen
Hi Mika,
Hi,
Quoted 75 lines · Mika Westerberg
Quoted 55 lines
On many older systems using SW sequencer the
PREOP_OPTYPEregister contains two preopcodes as following:PREOP_OPTYPE=0xf2785006
The last two bytes are the opcodes decoded to:
0x50- Write enable for volatile status register0x06- Write enableThe 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_OPTYPEregister
- 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 ==
0x50we 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 <[email protected]>--- drivers/mtd/spi-nor/intel-spi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/mtd/spi-nor/intel-spi.c +4
-3@@ -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 : +/* drivers/mtd/spi-nor/intel-spi.c static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len, · line 514
Quoted 10 lines
drivers/mtd/spi-nor/intel-spi.c +7
-0+* 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.cexpects 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 insidespi-nor.c, the SPI (flash) controller driver may no longer know what to do with new and unexpected SPI commands.
Quoted 8 lines
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.cdriver to remove reference to SPINOR_OP_* macros, which is a really good thing. So I would like the same thing to be done for theintel-spi.cdriver.
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.
Quoted 6 lines
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
Quoted 7 lines
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
-EINVALinstead 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.
Quoted 10 lines
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.cif the requested operation has not been handled as expected.
Quoted 8 lines
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.
Quoted 9 lines
If you still need to use
PREOP_OPTYPEfromintel_spi_sw_cycle(), then the value of theSSFSTS_CTL_ACSand/orSSFSTS_CTL_SPOPflags should have been chosen from the very same place where it has been decided that we will use thePREOP_OPTYPEto prepend some op code before the next SPI command. It's too late once inintel_spi_sw_cycle()to guess what opcode may need to be inserted.The source code should not rely on the following assumption:
Quoted 7 lines
(
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 fromintel_spi_write_reg()and add replace the "if (preop)" test inintel_spi_sw_cycle()by "if (!ispi->locked && preop)".
OK, I'll look into this and update the driver accordingly.
Quoted 3 lines
Also, at some point the 16 least significant bits of
PREOP_OPTYPEshould be reset to zero, otherwise all SPI commands to follow would keep on inserting op codes from thePREOP_OPTYPEregister...
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.
Quoted 2 lines
Finally, if
intel_spi_sw_cycle()could be used in any cases, then please removeintel_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.