[v2,1/2] spi-nor: intel-spi: Fix atomic sequence handling

Message ID 20180205113300.21270-1-mika.westerberg@linux.intel.com
State Under Review
Delegated to: Cyrille Pitchen
Headers show
Series
  • [v2,1/2] spi-nor: intel-spi: Fix atomic sequence handling
Related show

Commit Message

Mika Westerberg Feb. 5, 2018, 11:32 a.m.
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.

Furthermore we should not really just assume that WREN was issued in
previous call to intel_spi_write_reg() because that might not be the
case.

This updates driver to first check that the opcode is actually available
in PREOP_OPTYPE register and if not return error back to the spi-nor
core (if the controller is not locked we program it now). In addition we
save the opcode to ispi->atomic_preopcode field which is checked in next
call to intel_spi_sw_cycle() to actually enable atomic sequence using
the requested preopcode.

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
---
Changes from v1:

  - Added atomic_preopcode field and only when it is set, enable atomic
    sequence.
  - Return -EINVAL if the requested WREN operation cannot be found from
    PREOP register instead of silently pretending it is supported.
  - Updated patch subject to match more closely what is being fixed.

 drivers/mtd/spi-nor/intel-spi.c | 76 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 67 insertions(+), 9 deletions(-)

Comments

Mika Westerberg Feb. 27, 2018, 4:51 p.m. | #1
On Mon, Feb 05, 2018 at 02:32:59PM +0300, Mika Westerberg wrote:
> Changes from v1:
> 
>   - Added atomic_preopcode field and only when it is set, enable atomic
>     sequence.
>   - Return -EINVAL if the requested WREN operation cannot be found from
>     PREOP register instead of silently pretending it is supported.
>   - Updated patch subject to match more closely what is being fixed.

Any comments on these two? I did the changes Cyrille asked me to do.

Thanks!
Mika Westerberg April 3, 2018, 1:48 p.m. | #2
On Tue, Feb 27, 2018 at 06:51:42PM +0200, Mika Westerberg wrote:
> On Mon, Feb 05, 2018 at 02:32:59PM +0300, Mika Westerberg wrote:
> > Changes from v1:
> > 
> >   - Added atomic_preopcode field and only when it is set, enable atomic
> >     sequence.
> >   - Return -EINVAL if the requested WREN operation cannot be found from
> >     PREOP register instead of silently pretending it is supported.
> >   - Updated patch subject to match more closely what is being fixed.
> 
> Any comments on these two? I did the changes Cyrille asked me to do.

Ping. If no comments, could someone pick these up please.
Marek Vasut April 4, 2018, 8:21 a.m. | #3
On 04/03/2018 03:48 PM, Mika Westerberg wrote:
> On Tue, Feb 27, 2018 at 06:51:42PM +0200, Mika Westerberg wrote:
>> On Mon, Feb 05, 2018 at 02:32:59PM +0300, Mika Westerberg wrote:
>>> Changes from v1:
>>>
>>>   - Added atomic_preopcode field and only when it is set, enable atomic
>>>     sequence.
>>>   - Return -EINVAL if the requested WREN operation cannot be found from
>>>     PREOP register instead of silently pretending it is supported.
>>>   - Updated patch subject to match more closely what is being fixed.
>>
>> Any comments on these two? I did the changes Cyrille asked me to do.
> 
> Ping. If no comments, could someone pick these up please.
> 
Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
Boris Brezillon April 4, 2018, 7:22 p.m. | #4
On Wed, 4 Apr 2018 10:21:45 +0200
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 04/03/2018 03:48 PM, Mika Westerberg wrote:
> > On Tue, Feb 27, 2018 at 06:51:42PM +0200, Mika Westerberg wrote:  
> >> On Mon, Feb 05, 2018 at 02:32:59PM +0300, Mika Westerberg wrote:  
> >>> Changes from v1:
> >>>
> >>>   - Added atomic_preopcode field and only when it is set, enable atomic
> >>>     sequence.
> >>>   - Return -EINVAL if the requested WREN operation cannot be found from
> >>>     PREOP register instead of silently pretending it is supported.
> >>>   - Updated patch subject to match more closely what is being fixed.  
> >>
> >> Any comments on these two? I did the changes Cyrille asked me to do.  
> > 
> > Ping. If no comments, could someone pick these up please.
> >   
> Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
> 

Will queue this patches to mtd/fixes just after 4.17-rc1 is out. Sorry
for the huge delay.

Regards,

Boris

Patch

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 699951523179..8e98f4ab87c1 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -136,6 +136,7 @@ 
  * @swseq_reg: Use SW sequencer in register reads/writes
  * @swseq_erase: Use SW sequencer in erase operation
  * @erase_64k: 64k erase supported
+ * @atomic_preopcode: Holds preopcode when atomic sequence is requested
  * @opcodes: Opcodes which are supported. This are programmed by BIOS
  *           before it locks down the controller.
  */
@@ -153,6 +154,7 @@  struct intel_spi {
 	bool swseq_reg;
 	bool swseq_erase;
 	bool erase_64k;
+	u8 atomic_preopcode;
 	u8 opcodes[8];
 };
 
@@ -474,7 +476,7 @@  static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len,
 			      int optype)
 {
 	u32 val = 0, status;
-	u16 preop;
+	u8 atomic_preopcode;
 	int ret;
 
 	ret = intel_spi_opcode_index(ispi, opcode, optype);
@@ -484,17 +486,42 @@  static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len,
 	if (len > INTEL_SPI_FIFO_SZ)
 		return -EINVAL;
 
+	/*
+	 * Always clear it after each SW sequencer operation regardless
+	 * of whether it is successful or not.
+	 */
+	atomic_preopcode = ispi->atomic_preopcode;
+	ispi->atomic_preopcode = 0;
+
 	/* Only mark 'Data Cycle' bit when there is data to be transferred */
 	if (len > 0)
 		val = ((len - 1) << SSFSTS_CTL_DBC_SHIFT) | SSFSTS_CTL_DS;
 	val |= ret << SSFSTS_CTL_COP_SHIFT;
 	val |= SSFSTS_CTL_FCERR | SSFSTS_CTL_FDONE;
 	val |= SSFSTS_CTL_SCGO;
-	preop = readw(ispi->sregs + PREOP_OPTYPE);
-	if (preop) {
-		val |= SSFSTS_CTL_ACS;
-		if (preop >> 8)
-			val |= SSFSTS_CTL_SPOP;
+	if (atomic_preopcode) {
+		u16 preop;
+
+		switch (optype) {
+		case OPTYPE_WRITE_NO_ADDR:
+		case OPTYPE_WRITE_WITH_ADDR:
+			/* Pick matching preopcode for the atomic sequence */
+			preop = readw(ispi->sregs + PREOP_OPTYPE);
+			if ((preop & 0xff) == atomic_preopcode)
+				; /* Do nothing */
+			else if ((preop >> 8) == atomic_preopcode)
+				val |= SSFSTS_CTL_SPOP;
+			else
+				return -EINVAL;
+
+			/* Enable atomic sequence */
+			val |= SSFSTS_CTL_ACS;
+			break;
+
+		default:
+			return -EINVAL;
+		}
+
 	}
 	writel(val, ispi->sregs + SSFSTS_CTL);
 
@@ -538,13 +565,31 @@  static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 
 	/*
 	 * This is handled with atomic operation and preop code in Intel
-	 * controller so skip it here now. If the controller is not locked,
-	 * program the opcode to the PREOP register for later use.
+	 * controller so we only verify that it is available. If the
+	 * controller is not locked, program the opcode to the PREOP
+	 * register for later use.
+	 *
+	 * When hardware sequencer is used there is no need to program
+	 * any opcodes (it handles them automatically as part of a command).
 	 */
 	if (opcode == SPINOR_OP_WREN) {
-		if (!ispi->locked)
+		u16 preop;
+
+		if (!ispi->swseq_reg)
+			return 0;
+
+		preop = readw(ispi->sregs + PREOP_OPTYPE);
+		if ((preop & 0xff) != opcode && (preop >> 8) != opcode) {
+			if (ispi->locked)
+				return -EINVAL;
 			writel(opcode, ispi->sregs + PREOP_OPTYPE);
+		}
 
+		/*
+		 * This enables atomic sequence on next SW sycle. Will
+		 * be cleared after next operation.
+		 */
+		ispi->atomic_preopcode = opcode;
 		return 0;
 	}
 
@@ -569,6 +614,13 @@  static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
 	u32 val, status;
 	ssize_t ret;
 
+	/*
+	 * Atomic sequence is not expected with HW sequencer reads. Make
+	 * sure it is cleared regardless.
+	 */
+	if (WARN_ON_ONCE(ispi->atomic_preopcode))
+		ispi->atomic_preopcode = 0;
+
 	switch (nor->read_opcode) {
 	case SPINOR_OP_READ:
 	case SPINOR_OP_READ_FAST:
@@ -627,6 +679,9 @@  static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
 	u32 val, status;
 	ssize_t ret;
 
+	/* Not needed with HW sequencer write, make sure it is cleared */
+	ispi->atomic_preopcode = 0;
+
 	while (len > 0) {
 		block_size = min_t(size_t, len, INTEL_SPI_FIFO_SZ);
 
@@ -707,6 +762,9 @@  static int intel_spi_erase(struct spi_nor *nor, loff_t offs)
 		return 0;
 	}
 
+	/* Not needed with HW sequencer erase, make sure it is cleared */
+	ispi->atomic_preopcode = 0;
+
 	while (len > 0) {
 		writel(offs, ispi->base + FADDR);