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

Message ID 20180205113300.21270-1-mika.westerberg@linux.intel.com
State Accepted
Delegated to: Boris Brezillon
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
Mika Westerberg May 18, 2018, 5:20 a.m. | #5
On Wed, Apr 04, 2018 at 09:22:38PM +0200, Boris Brezillon wrote:
> 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.

Hi,

Did these patches end up somewhere? I can't see them in current
v4.17-rc5.

Thanks!
Boris Brezillon May 18, 2018, 6:53 a.m. | #6
On Fri, 18 May 2018 08:20:17 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Wed, Apr 04, 2018 at 09:22:38PM +0200, Boris Brezillon wrote:
> > 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.  
> 
> Hi,
> 
> Did these patches end up somewhere? I can't see them in current
> v4.17-rc5.

Oops, my bad. I'll queue it in spi-nor/next (it's too late for -rc6).
Sorry for the inconvenience.
Boris Brezillon May 18, 2018, 8:01 p.m. | #7
On Mon,  5 Feb 2018 14:32:59 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> 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.
> 
> 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

Queued to spi-nor/next.

Thanks,

Boris

> ---
> 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(-)
> 
> 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);
>

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);