[PATCH 00/10] spi-nor: intel-spi: Various fixes and enhancements

[PATCH 00/10] spi-nor: intel-spi: Various fixes and enhancements

[Bin Meng]

This series does several bug fixes and clean ups against the intel-spi
spi-nor driver, as well as enhancements to make the driver independent
on the underlying BIOS/bootloader.

At present the driver uses the HW sequencer for the read/write/erase on
all supported platforms, read_reg/write_reg for BXT, and the SW sequencer
for read_reg/write_reg for BYT/LPT. The way the driver uses the HW and SW
sequencer relies on some programmed register settings and hence creates
unneeded dependencies with the underlying BIOS/bootloader. For example,
the driver unfortunately does not work as expected when booting from
Intel Baytrail FSP based bootloaders like U-Boot, as the Baytrail FSP
does not set up some SPI controller settings to make the driver happy.
Now such limitation has been removed with this series.


Bin Meng (10):
  spi-nor: intel-spi: Fix number of protected range registers for
    BYT/LPT
  spi-nor: intel-spi: Remove useless 'buf' parameter in the HW/SW cycle
  spi-nor: intel-spi: Fix broken software sequencing codes
  spi-nor: intel-spi: Check transfer length in the HW/SW cycle
  spi-nor: intel-spi: Use SW sequencer for BYT/LPT
  spi-nor: intel-spi: Remove 'Atomic Cycle Sequence' in
    intel_spi_write()
  spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS
  spi-nor: intel-spi: Remove the unnecessary HSFSTS register RW
  spi-nor: intel-spi: Rename swseq to swseq_reg in 'struct intel_spi'
  spi-nor: intel-spi: Fall back to use SW sequencer to erase

 drivers/mtd/spi-nor/intel-spi.c | 209 +++++++++++++++++++++++++++++-----------
 1 file changed, 151 insertions(+), 58 deletions(-)

-- 
2.9.2

[PATCH 01/10] spi-nor: intel-spi: Fix number of protected range registers for BYT/LPT

[Bin Meng]

The number of protected range registers is not the same on BYT/LPT/
BXT. GPR0 only exists on Apollo Lake and its offset is reserved on
other platforms.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/intel-spi.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 8a596bf..e5b52e8 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -67,8 +67,6 @@
 #define PR_LIMIT_MASK			(0x3fff << PR_LIMIT_SHIFT)
 #define PR_RPE				BIT(15)
 #define PR_BASE_MASK			0x3fff
-/* Last PR is GPR0 */
-#define PR_NUM				(5 + 1)
 
 /* Offsets are from @ispi->sregs */
 #define SSFSTS_CTL			0x00
@@ -96,14 +94,17 @@
 #define BYT_BCR				0xfc
 #define BYT_BCR_WPD			BIT(0)
 #define BYT_FREG_NUM			5
+#define BYT_PR_NUM			5
 
 #define LPT_PR				0x74
 #define LPT_SSFSTS_CTL			0x90
 #define LPT_FREG_NUM			5
+#define LPT_PR_NUM			5
 
 #define BXT_PR				0x84
 #define BXT_SSFSTS_CTL			0xa0
 #define BXT_FREG_NUM			12
+#define BXT_PR_NUM			6
 
 #define INTEL_SPI_TIMEOUT		5000 /* ms */
 #define INTEL_SPI_FIFO_SZ		64
@@ -117,6 +118,7 @@
  * @pregs: Start of protection registers
  * @sregs: Start of software sequencer registers
  * @nregions: Maximum number of regions
+ * @pr_num: Maximum number of protected range registers
  * @writeable: Is the chip writeable
  * @swseq: Use SW sequencer in register reads/writes
  * @erase_64k: 64k erase supported
@@ -132,6 +134,7 @@ struct intel_spi {
 	void __iomem *pregs;
 	void __iomem *sregs;
 	size_t nregions;
+	size_t pr_num;
 	bool writeable;
 	bool swseq;
 	bool erase_64k;
@@ -167,7 +170,7 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
 	for (i = 0; i < ispi->nregions; i++)
 		dev_dbg(ispi->dev, "FREG(%d)=0x%08x\n", i,
 			readl(ispi->base + FREG(i)));
-	for (i = 0; i < PR_NUM; i++)
+	for (i = 0; i < ispi->pr_num; i++)
 		dev_dbg(ispi->dev, "PR(%d)=0x%08x\n", i,
 			readl(ispi->pregs + PR(i)));
 
@@ -182,7 +185,7 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
 		dev_dbg(ispi->dev, "BCR=0x%08x\n", readl(ispi->base + BYT_BCR));
 
 	dev_dbg(ispi->dev, "Protected regions:\n");
-	for (i = 0; i < PR_NUM; i++) {
+	for (i = 0; i < ispi->pr_num; i++) {
 		u32 base, limit;
 
 		value = readl(ispi->pregs + PR(i));
@@ -286,6 +289,7 @@ static int intel_spi_init(struct intel_spi *ispi)
 		ispi->sregs = ispi->base + BYT_SSFSTS_CTL;
 		ispi->pregs = ispi->base + BYT_PR;
 		ispi->nregions = BYT_FREG_NUM;
+		ispi->pr_num = BYT_PR_NUM;
 
 		if (writeable) {
 			/* Disable write protection */
@@ -305,12 +309,14 @@ static int intel_spi_init(struct intel_spi *ispi)
 		ispi->sregs = ispi->base + LPT_SSFSTS_CTL;
 		ispi->pregs = ispi->base + LPT_PR;
 		ispi->nregions = LPT_FREG_NUM;
+		ispi->pr_num = LPT_PR_NUM;
 		break;
 
 	case INTEL_SPI_BXT:
 		ispi->sregs = ispi->base + BXT_SSFSTS_CTL;
 		ispi->pregs = ispi->base + BXT_PR;
 		ispi->nregions = BXT_FREG_NUM;
+		ispi->pr_num = BXT_PR_NUM;
 		ispi->erase_64k = true;
 		break;
 
@@ -652,7 +658,7 @@ static bool intel_spi_is_protected(const struct intel_spi *ispi,
 {
 	int i;
 
-	for (i = 0; i < PR_NUM; i++) {
+	for (i = 0; i < ispi->pr_num; i++) {
 		u32 pr_base, pr_limit, pr_value;
 
 		pr_value = readl(ispi->pregs + PR(i));
-- 
2.9.2

[PATCH 02/10] spi-nor: intel-spi: Remove useless 'buf' parameter in the HW/SW cycle

[Bin Meng]

intel_spi_hw_cycle() and intel_spi_sw_cycle() don't use the parameter
'buf' at all. Remove it.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/intel-spi.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index e5b52e8..07626ca 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -377,8 +377,7 @@ static int intel_spi_opcode_index(struct intel_spi *ispi, u8 opcode)
 	return -EINVAL;
 }
 
-static int intel_spi_hw_cycle(struct intel_spi *ispi, u8 opcode, u8 *buf,
-			      int len)
+static int intel_spi_hw_cycle(struct intel_spi *ispi, u8 opcode, int len)
 {
 	u32 val, status;
 	int ret;
@@ -418,8 +417,7 @@ static int intel_spi_hw_cycle(struct intel_spi *ispi, u8 opcode, u8 *buf,
 	return 0;
 }
 
-static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, u8 *buf,
-			      int len)
+static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len)
 {
 	u32 val, status;
 	int ret;
@@ -456,9 +454,9 @@ static int intel_spi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 	writel(0, ispi->base + FADDR);
 
 	if (ispi->swseq)
-		ret = intel_spi_sw_cycle(ispi, opcode, buf, len);
+		ret = intel_spi_sw_cycle(ispi, opcode, len);
 	else
-		ret = intel_spi_hw_cycle(ispi, opcode, buf, len);
+		ret = intel_spi_hw_cycle(ispi, opcode, len);
 
 	if (ret)
 		return ret;
@@ -486,8 +484,8 @@ static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 		return ret;
 
 	if (ispi->swseq)
-		return intel_spi_sw_cycle(ispi, opcode, buf, len);
-	return intel_spi_hw_cycle(ispi, opcode, buf, len);
+		return intel_spi_sw_cycle(ispi, opcode, len);
+	return intel_spi_hw_cycle(ispi, opcode, len);
 }
 
 static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
-- 
2.9.2

[PATCH 03/10] spi-nor: intel-spi: Fix broken software sequencing codes

[Bin Meng]

There are two bugs in current intel_spi_sw_cycle():

- The 'data byte count' field should be the number of bytes
  transferred minus 1
- SSFSTS_CTL is the offset from ispi->sregs, not ispi->base

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/intel-spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 07626ca..263c6ab 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -426,7 +426,7 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len)
 	if (ret < 0)
 		return ret;
 
-	val = (len << SSFSTS_CTL_DBC_SHIFT) | SSFSTS_CTL_DS;
+	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;
@@ -436,7 +436,7 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len)
 	if (ret)
 		return ret;
 
-	status = readl(ispi->base + SSFSTS_CTL);
+	status = readl(ispi->sregs + SSFSTS_CTL);
 	if (status & SSFSTS_CTL_FCERR)
 		return -EIO;
 	else if (status & SSFSTS_CTL_AEL)
-- 
2.9.2

[PATCH 04/10] spi-nor: intel-spi: Check transfer length in the HW/SW cycle

[Bin Meng]

Intel SPI controller only has a 64 bytes FIFO. This adds a sanity
check before triggering any HW/SW sequencer work.

Additionally for the SW sequencer, if given data length is zero,
we should not mark the 'Data Cycle' bit.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/intel-spi.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 263c6ab..c4a9de6 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -399,6 +399,9 @@ static int intel_spi_hw_cycle(struct intel_spi *ispi, u8 opcode, int len)
 		return -EINVAL;
 	}
 
+	if (len > INTEL_SPI_FIFO_SZ)
+		return -EINVAL;
+
 	val |= (len - 1) << HSFSTS_CTL_FDBC_SHIFT;
 	val |= HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
 	val |= HSFSTS_CTL_FGO;
@@ -419,14 +422,19 @@ static int intel_spi_hw_cycle(struct intel_spi *ispi, u8 opcode, int len)
 
 static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len)
 {
-	u32 val, status;
+	u32 val = 0, status;
 	int ret;
 
 	ret = intel_spi_opcode_index(ispi, opcode);
 	if (ret < 0)
 		return ret;
 
-	val = ((len - 1) << SSFSTS_CTL_DBC_SHIFT) | SSFSTS_CTL_DS;
+	if (len > INTEL_SPI_FIFO_SZ)
+		return -EINVAL;
+
+	/* 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;
-- 
2.9.2

[PATCH 05/10] spi-nor: intel-spi: Use SW sequencer for BYT/LPT

[Bin Meng]

Baytrail/Lynx Point SPI controller's HW sequencer only supports basic
operations. This is determined by the chipset design, however current
codes try to use register values in OPMENU0/OPMENU1 to see whether SW
sequencer should be used, which is wrong. In fact OPMENU0/OPMENU1 can
remain unprogrammed by some bootloaders.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/intel-spi.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index c4a9de6..d0237fe 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -290,6 +290,7 @@ static int intel_spi_init(struct intel_spi *ispi)
 		ispi->pregs = ispi->base + BYT_PR;
 		ispi->nregions = BYT_FREG_NUM;
 		ispi->pr_num = BYT_PR_NUM;
+		ispi->swseq = true;
 
 		if (writeable) {
 			/* Disable write protection */
@@ -310,6 +311,7 @@ static int intel_spi_init(struct intel_spi *ispi)
 		ispi->pregs = ispi->base + LPT_PR;
 		ispi->nregions = LPT_FREG_NUM;
 		ispi->pr_num = LPT_PR_NUM;
+		ispi->swseq = true;
 		break;
 
 	case INTEL_SPI_BXT:
@@ -324,12 +326,24 @@ static int intel_spi_init(struct intel_spi *ispi)
 		return -EINVAL;
 	}
 
-	/* Disable #SMI generation */
+	/* Disable #SMI generation from HW sequencer */
 	val = readl(ispi->base + HSFSTS_CTL);
 	val &= ~HSFSTS_CTL_FSMIE;
 	writel(val, ispi->base + HSFSTS_CTL);
 
 	/*
+	 * Some controllers can only do basic operations using hardware
+	 * sequencer. All other operations are supposed to be carried out
+	 * using software sequencer.
+	 */
+	if (ispi->swseq) {
+		/* Disable #SMI generation from SW sequencer */
+		val = readl(ispi->sregs + SSFSTS_CTL);
+		val &= ~SSFSTS_CTL_FSMIE;
+		writel(val, ispi->sregs + SSFSTS_CTL);
+	}
+
+	/*
 	 * BIOS programs allowed opcodes and then locks down the register.
 	 * So read back what opcodes it decided to support. That's the set
 	 * we are going to support as well.
@@ -337,13 +351,6 @@ static int intel_spi_init(struct intel_spi *ispi)
 	opmenu0 = readl(ispi->sregs + OPMENU0);
 	opmenu1 = readl(ispi->sregs + OPMENU1);
 
-	/*
-	 * Some controllers can only do basic operations using hardware
-	 * sequencer. All other operations are supposed to be carried out
-	 * using software sequencer. If we find that BIOS has programmed
-	 * opcodes for the software sequencer we use that over the hardware
-	 * sequencer.
-	 */
 	if (opmenu0 && opmenu1) {
 		for (i = 0; i < ARRAY_SIZE(ispi->opcodes) / 2; i++) {
 			ispi->opcodes[i] = opmenu0 >> i * 8;
@@ -353,13 +360,6 @@ static int intel_spi_init(struct intel_spi *ispi)
 		val = readl(ispi->sregs + PREOP_OPTYPE);
 		ispi->preopcodes[0] = val;
 		ispi->preopcodes[1] = val >> 8;
-
-		/* Disable #SMI generation from SW sequencer */
-		val = readl(ispi->sregs + SSFSTS_CTL);
-		val &= ~SSFSTS_CTL_FSMIE;
-		writel(val, ispi->sregs + SSFSTS_CTL);
-
-		ispi->swseq = true;
 	}
 
 	intel_spi_dump_regs(ispi);
-- 
2.9.2

[PATCH 06/10] spi-nor: intel-spi: Remove 'Atomic Cycle Sequence' in intel_spi_write()

[Bin Meng]

So far intel_spi_write() uses the HW sequencer to do the write. But
the HW sequencer register HSFSTS_CTL does not have such a field for
'Atomic Cycle Sequence', remove it.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/intel-spi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index d0237fe..757b9f1 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -572,11 +572,6 @@ static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
 		val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
 		val |= (block_size - 1) << HSFSTS_CTL_FDBC_SHIFT;
 		val |= HSFSTS_CTL_FCYCLE_WRITE;
-
-		/* Write enable */
-		if (ispi->preopcodes[1] == SPINOR_OP_WREN)
-			val |= SSFSTS_CTL_SPOP;
-		val |= SSFSTS_CTL_ACS;
 		writel(val, ispi->base + HSFSTS_CTL);
 
 		ret = intel_spi_write_block(ispi, write_buf, block_size);
-- 
2.9.2

[PATCH 07/10] spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS

[Bin Meng]

At present the driver relies on valid OPMENU0/OPMENU1 register values
that are programmed by BIOS to function correctly. However in a real
world it's absolutely legitimate for a bootloader to leave these two
registers untouched. Intel FSP for Baytrail exactly does like this.
When we are booting from any Intel FSP based bootloaders like U-Boot,
the driver refuses to work.

We can of course program various flash opcodes in the OPMENU0/OPMENU1
registers, and such workaround can be added in either the bootloader
codes, or the kernel driver itself.

But a graceful solution would be to update the kernel driver to remove
such limitation of OPMENU0/1 register dependency. The SPI controller
settings are not locked under such configuration. So we can first check
the controller locking status, and if it is not locked that means the
driver job can be fulfilled by using a chosen OPMENU index to set up
the flash opcode every time.

While we are here, the missing 'Atomic Cycle Sequence' handling in the
SW sequencer codes is also added.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/intel-spi.c | 91 +++++++++++++++++++++++++++++------------
 1 file changed, 65 insertions(+), 26 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 757b9f1..07146ab 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -88,6 +88,11 @@
 #define OPMENU0				0x08
 #define OPMENU1				0x0c
 
+#define OPTYPE_READ_NO_ADDR		0
+#define OPTYPE_WRITE_NO_ADDR		1
+#define OPTYPE_READ_WITH_ADDR		2
+#define OPTYPE_WRITE_WITH_ADDR		3
+
 /* CPU specifics */
 #define BYT_PR				0x74
 #define BYT_SSFSTS_CTL			0x90
@@ -120,6 +125,7 @@
  * @nregions: Maximum number of regions
  * @pr_num: Maximum number of protected range registers
  * @writeable: Is the chip writeable
+ * @locked: Is SPI setting locked
  * @swseq: Use SW sequencer in register reads/writes
  * @erase_64k: 64k erase supported
  * @opcodes: Opcodes which are supported. This are programmed by BIOS
@@ -136,6 +142,7 @@ struct intel_spi {
 	size_t nregions;
 	size_t pr_num;
 	bool writeable;
+	bool locked;
 	bool swseq;
 	bool erase_64k;
 	u8 opcodes[8];
@@ -343,23 +350,29 @@ static int intel_spi_init(struct intel_spi *ispi)
 		writel(val, ispi->sregs + SSFSTS_CTL);
 	}
 
-	/*
-	 * BIOS programs allowed opcodes and then locks down the register.
-	 * So read back what opcodes it decided to support. That's the set
-	 * we are going to support as well.
-	 */
-	opmenu0 = readl(ispi->sregs + OPMENU0);
-	opmenu1 = readl(ispi->sregs + OPMENU1);
+	/* Check controller's lock status */
+	val = readl(ispi->base + HSFSTS_CTL);
+	ispi->locked = !!(val & HSFSTS_CTL_FLOCKDN);
 
-	if (opmenu0 && opmenu1) {
-		for (i = 0; i < ARRAY_SIZE(ispi->opcodes) / 2; i++) {
-			ispi->opcodes[i] = opmenu0 >> i * 8;
-			ispi->opcodes[i + 4] = opmenu1 >> i * 8;
-		}
+	if (ispi->locked) {
+		/*
+		 * BIOS programs allowed opcodes and then locks down the
+		 * register. So read back what opcodes it decided to support.
+		 * That's the set we are going to support as well.
+		 */
+		opmenu0 = readl(ispi->sregs + OPMENU0);
+		opmenu1 = readl(ispi->sregs + OPMENU1);
 
-		val = readl(ispi->sregs + PREOP_OPTYPE);
-		ispi->preopcodes[0] = val;
-		ispi->preopcodes[1] = val >> 8;
+		if (opmenu0 && opmenu1) {
+			for (i = 0; i < ARRAY_SIZE(ispi->opcodes) / 2; i++) {
+				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;
+		}
 	}
 
 	intel_spi_dump_regs(ispi);
@@ -367,14 +380,25 @@ static int intel_spi_init(struct intel_spi *ispi)
 	return 0;
 }
 
-static int intel_spi_opcode_index(struct intel_spi *ispi, u8 opcode)
+static int intel_spi_opcode_index(struct intel_spi *ispi, u8 opcode, int optype)
 {
 	int i;
+	int preop;
 
-	for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++)
-		if (ispi->opcodes[i] == opcode)
-			return i;
-	return -EINVAL;
+	if (ispi->locked) {
+		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++)
+			if (ispi->opcodes[i] == opcode)
+				return i;
+
+		return -EINVAL;
+	}
+
+	/* The lock is off, so just use index 0 */
+	writel(opcode, ispi->sregs + OPMENU0);
+	preop = readw(ispi->sregs + PREOP_OPTYPE);
+	writel(optype << 16 | preop, ispi->sregs + PREOP_OPTYPE);
+
+	return 0;
 }
 
 static int intel_spi_hw_cycle(struct intel_spi *ispi, u8 opcode, int len)
@@ -420,12 +444,14 @@ static int intel_spi_hw_cycle(struct intel_spi *ispi, u8 opcode, int len)
 	return 0;
 }
 
-static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len)
+static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len,
+			      int optype)
 {
 	u32 val = 0, status;
+	u16 preop;
 	int ret;
 
-	ret = intel_spi_opcode_index(ispi, opcode);
+	ret = intel_spi_opcode_index(ispi, opcode, optype);
 	if (ret < 0)
 		return ret;
 
@@ -438,6 +464,12 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, int len)
 	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;
+	}
 	writel(val, ispi->sregs + SSFSTS_CTL);
 
 	ret = intel_spi_wait_sw_busy(ispi);
@@ -462,7 +494,8 @@ static int intel_spi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 	writel(0, ispi->base + FADDR);
 
 	if (ispi->swseq)
-		ret = intel_spi_sw_cycle(ispi, opcode, len);
+		ret = intel_spi_sw_cycle(ispi, opcode, len,
+					 OPTYPE_READ_NO_ADDR);
 	else
 		ret = intel_spi_hw_cycle(ispi, opcode, len);
 
@@ -479,10 +512,15 @@ 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.
+	 * controller so skip it here now. If the controller is not locked,
+	 * program the opcode to the PREOP register for later use.
 	 */
-	if (opcode == SPINOR_OP_WREN)
+	if (opcode == SPINOR_OP_WREN) {
+		if (!ispi->locked)
+			writel(opcode, ispi->sregs + PREOP_OPTYPE);
+
 		return 0;
+	}
 
 	writel(0, ispi->base + FADDR);
 
@@ -492,7 +530,8 @@ static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 		return ret;
 
 	if (ispi->swseq)
-		return intel_spi_sw_cycle(ispi, opcode, len);
+		return intel_spi_sw_cycle(ispi, opcode, len,
+					  OPTYPE_WRITE_NO_ADDR);
 	return intel_spi_hw_cycle(ispi, opcode, len);
 }
 
-- 
2.9.2

[PATCH 08/10] spi-nor: intel-spi: Remove the unnecessary HSFSTS register RW

[Bin Meng]

There is no code that alters the HSFSTS register content in between
in intel_spi_write(). Remove the unnecessary RW to save some cycles.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/intel-spi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 07146ab..91ceef7 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -611,7 +611,6 @@ static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
 		val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
 		val |= (block_size - 1) << HSFSTS_CTL_FDBC_SHIFT;
 		val |= HSFSTS_CTL_FCYCLE_WRITE;
-		writel(val, ispi->base + HSFSTS_CTL);
 
 		ret = intel_spi_write_block(ispi, write_buf, block_size);
 		if (ret) {
@@ -620,8 +619,8 @@ static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
 		}
 
 		/* Start the write now */
-		val = readl(ispi->base + HSFSTS_CTL);
-		writel(val | HSFSTS_CTL_FGO, ispi->base + HSFSTS_CTL);
+		val |= HSFSTS_CTL_FGO;
+		writel(val, ispi->base + HSFSTS_CTL);
 
 		ret = intel_spi_wait_hw_busy(ispi);
 		if (ret) {
-- 
2.9.2

[PATCH 09/10] spi-nor: intel-spi: Rename swseq to swseq_reg in 'struct intel_spi'

[Bin Meng]

The ispi->swseq is used for register access. Let's rename it to
swseq_reg to better describe its usage.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/mtd/spi-nor/intel-spi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 91ceef7..5e7a389 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -126,7 +126,7 @@
  * @pr_num: Maximum number of protected range registers
  * @writeable: Is the chip writeable
  * @locked: Is SPI setting locked
- * @swseq: Use SW sequencer in register reads/writes
+ * @swseq_reg: Use SW sequencer in register reads/writes
  * @erase_64k: 64k erase supported
  * @opcodes: Opcodes which are supported. This are programmed by BIOS
  *           before it locks down the controller.
@@ -143,7 +143,7 @@ struct intel_spi {
 	size_t pr_num;
 	bool writeable;
 	bool locked;
-	bool swseq;
+	bool swseq_reg;
 	bool erase_64k;
 	u8 opcodes[8];
 	u8 preopcodes[2];
@@ -224,7 +224,7 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
 	}
 
 	dev_dbg(ispi->dev, "Using %cW sequencer for register access\n",
-		ispi->swseq ? 'S' : 'H');
+		ispi->swseq_reg ? 'S' : 'H');
 }
 
 /* Reads max INTEL_SPI_FIFO_SZ bytes from the device fifo */
@@ -297,7 +297,7 @@ static int intel_spi_init(struct intel_spi *ispi)
 		ispi->pregs = ispi->base + BYT_PR;
 		ispi->nregions = BYT_FREG_NUM;
 		ispi->pr_num = BYT_PR_NUM;
-		ispi->swseq = true;
+		ispi->swseq_reg = true;
 
 		if (writeable) {
 			/* Disable write protection */
@@ -318,7 +318,7 @@ static int intel_spi_init(struct intel_spi *ispi)
 		ispi->pregs = ispi->base + LPT_PR;
 		ispi->nregions = LPT_FREG_NUM;
 		ispi->pr_num = LPT_PR_NUM;
-		ispi->swseq = true;
+		ispi->swseq_reg = true;
 		break;
 
 	case INTEL_SPI_BXT:
@@ -343,7 +343,7 @@ static int intel_spi_init(struct intel_spi *ispi)
 	 * sequencer. All other operations are supposed to be carried out
 	 * using software sequencer.
 	 */
-	if (ispi->swseq) {
+	if (ispi->swseq_reg) {
 		/* Disable #SMI generation from SW sequencer */
 		val = readl(ispi->sregs + SSFSTS_CTL);
 		val &= ~SSFSTS_CTL_FSMIE;
@@ -493,7 +493,7 @@ static int intel_spi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 	/* Address of the first chip */
 	writel(0, ispi->base + FADDR);
 
-	if (ispi->swseq)
+	if (ispi->swseq_reg)
 		ret = intel_spi_sw_cycle(ispi, opcode, len,
 					 OPTYPE_READ_NO_ADDR);
 	else
@@ -529,7 +529,7 @@ static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 	if (ret)
 		return ret;
 
-	if (ispi->swseq)
+	if (ispi->swseq_reg)
 		return intel_spi_sw_cycle(ispi, opcode, len,
 					  OPTYPE_WRITE_NO_ADDR);
 	return intel_spi_hw_cycle(ispi, opcode, len);
-- 
2.9.2

[PATCH 10/10] spi-nor: intel-spi: Fall back to use SW sequencer to erase

[Bin Meng]

According to the datasheet, the HW sequencer has a predefined list
of opcodes, with only the erase opcode being programmable in LVSCC
and UVSCC registers. If these registers don't contain a valid erase
opcode (eg: BIOS does not program it), erase cannot be done using
the HW sequencer, even though the erase operation does not report
any error, the flash remains not erased.

If such register setting is detected, let's fall back to use the SW
sequencer to erase instead.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

 drivers/mtd/spi-nor/intel-spi.c | 50 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 5e7a389..f1e2c5f 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -111,6 +111,13 @@
 #define BXT_FREG_NUM			12
 #define BXT_PR_NUM			6
 
+#define LVSCC				0xc4
+#define UVSCC				0xc8
+#define ERASE_OPCODE_SHIFT		8
+#define ERASE_OPCODE_MASK		(0xff << ERASE_OPCODE_SHIFT)
+#define ERASE_64K_OPCODE_SHIFT		16
+#define ERASE_64K_OPCODE_MASK		(0xff << ERASE_OPCODE_SHIFT)
+
 #define INTEL_SPI_TIMEOUT		5000 /* ms */
 #define INTEL_SPI_FIFO_SZ		64
 
@@ -127,6 +134,7 @@
  * @writeable: Is the chip writeable
  * @locked: Is SPI setting locked
  * @swseq_reg: Use SW sequencer in register reads/writes
+ * @swseq_erase: Use SW sequencer in erase operation
  * @erase_64k: 64k erase supported
  * @opcodes: Opcodes which are supported. This are programmed by BIOS
  *           before it locks down the controller.
@@ -144,6 +152,7 @@ struct intel_spi {
 	bool writeable;
 	bool locked;
 	bool swseq_reg;
+	bool swseq_erase;
 	bool erase_64k;
 	u8 opcodes[8];
 	u8 preopcodes[2];
@@ -191,6 +200,9 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
 	if (ispi->info->type == INTEL_SPI_BYT)
 		dev_dbg(ispi->dev, "BCR=0x%08x\n", readl(ispi->base + BYT_BCR));
 
+	dev_dbg(ispi->dev, "LVSCC=0x%08x\n", readl(ispi->base + LVSCC));
+	dev_dbg(ispi->dev, "UVSCC=0x%08x\n", readl(ispi->base + UVSCC));
+
 	dev_dbg(ispi->dev, "Protected regions:\n");
 	for (i = 0; i < ispi->pr_num; i++) {
 		u32 base, limit;
@@ -225,6 +237,8 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
 
 	dev_dbg(ispi->dev, "Using %cW sequencer for register access\n",
 		ispi->swseq_reg ? 'S' : 'H');
+	dev_dbg(ispi->dev, "Using %cW sequencer for erase operation\n",
+		ispi->swseq_erase ? 'S' : 'H');
 }
 
 /* Reads max INTEL_SPI_FIFO_SZ bytes from the device fifo */
@@ -288,7 +302,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
 
 static int intel_spi_init(struct intel_spi *ispi)
 {
-	u32 opmenu0, opmenu1, val;
+	u32 opmenu0, opmenu1, lvscc, uvscc, val;
 	int i;
 
 	switch (ispi->info->type) {
@@ -339,6 +353,24 @@ static int intel_spi_init(struct intel_spi *ispi)
 	writel(val, ispi->base + HSFSTS_CTL);
 
 	/*
+	 * Determine whether erase operatoin should use HW or SW sequencer.
+	 *
+	 * The HW sequencer has a predefined list of opcodes, with only the
+	 * erase opcode being programmable in LVSCC and UVSCC registers.
+	 * If these registers don't contain a valid erase opcode, erase
+	 * cannot be done using HW sequencer.
+	 */
+	lvscc = readl(ispi->base + LVSCC);
+	uvscc = readl(ispi->base + UVSCC);
+	if (!(lvscc & ERASE_OPCODE_MASK) || !(uvscc & ERASE_OPCODE_MASK))
+		ispi->swseq_erase = true;
+	/* SPI controller on Intel BXT supports 64K erase opcode */
+	if (ispi->info->type == INTEL_SPI_BXT && !ispi->swseq_erase)
+		if (!(lvscc & ERASE_64K_OPCODE_MASK) ||
+		    !(uvscc & ERASE_64K_OPCODE_MASK))
+			ispi->erase_64k = false;
+
+	/*
 	 * Some controllers can only do basic operations using hardware
 	 * sequencer. All other operations are supposed to be carried out
 	 * using software sequencer.
@@ -665,6 +697,22 @@ static int intel_spi_erase(struct spi_nor *nor, loff_t offs)
 		erase_size = SZ_4K;
 	}
 
+	if (ispi->swseq_erase) {
+		while (len > 0) {
+			writel(offs, ispi->base + FADDR);
+
+			ret = intel_spi_sw_cycle(ispi, nor->erase_opcode,
+						 0, OPTYPE_WRITE_WITH_ADDR);
+			if (ret)
+				return ret;
+
+			offs += erase_size;
+			len -= erase_size;
+		}
+
+		return 0;
+	}
+
 	while (len > 0) {
 		writel(offs, ispi->base + FADDR);
 
-- 
2.9.2

Re: [PATCH 03/10] spi-nor: intel-spi: Fix broken software sequencing codes

[Mika Westerberg]

On Fri, Sep 01, 2017 at 01:00:34AM -0700, Bin Meng wrote:
> There are two bugs in current intel_spi_sw_cycle():
> 
> - The 'data byte count' field should be the number of bytes
>   transferred minus 1
> - SSFSTS_CTL is the offset from ispi->sregs, not ispi->base
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

This should be tagged for stable inclusion as well.

Re: [PATCH 10/10] spi-nor: intel-spi: Fall back to use SW sequencer to erase

[Mika Westerberg]

On Fri, Sep 01, 2017 at 01:00:41AM -0700, Bin Meng wrote:
>  	/*
> +	 * Determine whether erase operatoin should use HW or SW sequencer.
                                   ^^^^^^^^^
Typo

> +	 *
> +	 * The HW sequencer has a predefined list of opcodes, with only the
> +	 * erase opcode being programmable in LVSCC and UVSCC registers.
> +	 * If these registers don't contain a valid erase opcode, erase
> +	 * cannot be done using HW sequencer.
> +	 */

Re: [PATCH 01/10] spi-nor: intel-spi: Fix number of protected range registers for BYT/LPT

[Marek Vasut]

On 09/01/2017 10:00 AM, Bin Meng wrote:
> The number of protected range registers is not the same on BYT/LPT/
> BXT. GPR0 only exists on Apollo Lake and its offset is reserved on
> other platforms.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Can this use regmap ? What you're implementing here seems like regmap to me.

> ---
> 
>  drivers/mtd/spi-nor/intel-spi.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> index 8a596bf..e5b52e8 100644
> --- a/drivers/mtd/spi-nor/intel-spi.c
> +++ b/drivers/mtd/spi-nor/intel-spi.c
> @@ -67,8 +67,6 @@
>  #define PR_LIMIT_MASK			(0x3fff << PR_LIMIT_SHIFT)
>  #define PR_RPE				BIT(15)
>  #define PR_BASE_MASK			0x3fff
> -/* Last PR is GPR0 */
> -#define PR_NUM				(5 + 1)
>  
>  /* Offsets are from @ispi->sregs */
>  #define SSFSTS_CTL			0x00
> @@ -96,14 +94,17 @@
>  #define BYT_BCR				0xfc
>  #define BYT_BCR_WPD			BIT(0)
>  #define BYT_FREG_NUM			5
> +#define BYT_PR_NUM			5
>  
>  #define LPT_PR				0x74
>  #define LPT_SSFSTS_CTL			0x90
>  #define LPT_FREG_NUM			5
> +#define LPT_PR_NUM			5
>  
>  #define BXT_PR				0x84
>  #define BXT_SSFSTS_CTL			0xa0
>  #define BXT_FREG_NUM			12
> +#define BXT_PR_NUM			6
>  
>  #define INTEL_SPI_TIMEOUT		5000 /* ms */
>  #define INTEL_SPI_FIFO_SZ		64
> @@ -117,6 +118,7 @@
>   * @pregs: Start of protection registers
>   * @sregs: Start of software sequencer registers
>   * @nregions: Maximum number of regions
> + * @pr_num: Maximum number of protected range registers
>   * @writeable: Is the chip writeable
>   * @swseq: Use SW sequencer in register reads/writes
>   * @erase_64k: 64k erase supported
> @@ -132,6 +134,7 @@ struct intel_spi {
>  	void __iomem *pregs;
>  	void __iomem *sregs;
>  	size_t nregions;
> +	size_t pr_num;
>  	bool writeable;
>  	bool swseq;
>  	bool erase_64k;
> @@ -167,7 +170,7 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
>  	for (i = 0; i < ispi->nregions; i++)
>  		dev_dbg(ispi->dev, "FREG(%d)=0x%08x\n", i,
>  			readl(ispi->base + FREG(i)));
> -	for (i = 0; i < PR_NUM; i++)
> +	for (i = 0; i < ispi->pr_num; i++)
>  		dev_dbg(ispi->dev, "PR(%d)=0x%08x\n", i,
>  			readl(ispi->pregs + PR(i)));
>  
> @@ -182,7 +185,7 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
>  		dev_dbg(ispi->dev, "BCR=0x%08x\n", readl(ispi->base + BYT_BCR));
>  
>  	dev_dbg(ispi->dev, "Protected regions:\n");
> -	for (i = 0; i < PR_NUM; i++) {
> +	for (i = 0; i < ispi->pr_num; i++) {
>  		u32 base, limit;
>  
>  		value = readl(ispi->pregs + PR(i));
> @@ -286,6 +289,7 @@ static int intel_spi_init(struct intel_spi *ispi)
>  		ispi->sregs = ispi->base + BYT_SSFSTS_CTL;
>  		ispi->pregs = ispi->base + BYT_PR;
>  		ispi->nregions = BYT_FREG_NUM;
> +		ispi->pr_num = BYT_PR_NUM;
>  
>  		if (writeable) {
>  			/* Disable write protection */
> @@ -305,12 +309,14 @@ static int intel_spi_init(struct intel_spi *ispi)
>  		ispi->sregs = ispi->base + LPT_SSFSTS_CTL;
>  		ispi->pregs = ispi->base + LPT_PR;
>  		ispi->nregions = LPT_FREG_NUM;
> +		ispi->pr_num = LPT_PR_NUM;
>  		break;
>  
>  	case INTEL_SPI_BXT:
>  		ispi->sregs = ispi->base + BXT_SSFSTS_CTL;
>  		ispi->pregs = ispi->base + BXT_PR;
>  		ispi->nregions = BXT_FREG_NUM;
> +		ispi->pr_num = BXT_PR_NUM;
>  		ispi->erase_64k = true;
>  		break;
>  
> @@ -652,7 +658,7 @@ static bool intel_spi_is_protected(const struct intel_spi *ispi,
>  {
>  	int i;
>  
> -	for (i = 0; i < PR_NUM; i++) {
> +	for (i = 0; i < ispi->pr_num; i++) {
>  		u32 pr_base, pr_limit, pr_value;
>  
>  		pr_value = readl(ispi->pregs + PR(i));
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH 00/10] spi-nor: intel-spi: Various fixes and enhancements

[Mika Westerberg]

On Fri, Sep 01, 2017 at 01:00:31AM -0700, Bin Meng wrote:
> This series does several bug fixes and clean ups against the intel-spi
> spi-nor driver, as well as enhancements to make the driver independent
> on the underlying BIOS/bootloader.
> 
> At present the driver uses the HW sequencer for the read/write/erase on
> all supported platforms, read_reg/write_reg for BXT, and the SW sequencer
> for read_reg/write_reg for BYT/LPT. The way the driver uses the HW and SW
> sequencer relies on some programmed register settings and hence creates
> unneeded dependencies with the underlying BIOS/bootloader. For example,
> the driver unfortunately does not work as expected when booting from
> Intel Baytrail FSP based bootloaders like U-Boot, as the Baytrail FSP
> does not set up some SPI controller settings to make the driver happy.
> Now such limitation has been removed with this series.
> 
> 
> Bin Meng (10):
>   spi-nor: intel-spi: Fix number of protected range registers for
>     BYT/LPT
>   spi-nor: intel-spi: Remove useless 'buf' parameter in the HW/SW cycle
>   spi-nor: intel-spi: Fix broken software sequencing codes
>   spi-nor: intel-spi: Check transfer length in the HW/SW cycle
>   spi-nor: intel-spi: Use SW sequencer for BYT/LPT
>   spi-nor: intel-spi: Remove 'Atomic Cycle Sequence' in
>     intel_spi_write()
>   spi-nor: intel-spi: Don't assume OPMENU0/1 to be programmed by BIOS
>   spi-nor: intel-spi: Remove the unnecessary HSFSTS register RW
>   spi-nor: intel-spi: Rename swseq to swseq_reg in 'struct intel_spi'
>   spi-nor: intel-spi: Fall back to use SW sequencer to erase
> 
>  drivers/mtd/spi-nor/intel-spi.c | 209 +++++++++++++++++++++++++++++-----------
>  1 file changed, 151 insertions(+), 58 deletions(-)

Nice work!

For the whole series,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 01/10] spi-nor: intel-spi: Fix number of protected range registers for BYT/LPT

[Bin Meng]

Hi Marek,

On Fri, Sep 1, 2017 at 5:52 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 09/01/2017 10:00 AM, Bin Meng wrote:
>> The number of protected range registers is not the same on BYT/LPT/
>> BXT. GPR0 only exists on Apollo Lake and its offset is reserved on
>> other platforms.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> Can this use regmap ? What you're implementing here seems like regmap to me.

No. regmap does not apply here.

Regards,
Bin