| Message ID | 20180104090744.67654-1-mika.westerberg@linux.intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Cyrille Pitchen |
| Headers | show |
| Series | [1/3] spi-nor: intel-spi: Prefer WREN over other write enables | expand |
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
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.
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?
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.
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
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.
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);
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(-)