[PATCH 0/3] mtd: spi-nor / spi / MFD: Convert intel-spi to SPI MEM

[PATCH 0/3] mtd: spi-nor / spi / MFD: Convert intel-spi to SPI MEM

[Mika Westerberg]

Hi all,

Based on discussion on the patch I sent some time ago here:

  http://lists.infradead.org/pipermail/linux-mtd/2021-June/086867.html

it turns out that the preferred way to deal with the SPI flash controller
drivers is through SPI MEM which is part of Linux SPI subsystem.

This series does that for the intel-spi driver. This also renames the
driver to follow the convention used in the SPI subsystem. The first patch
improves the write protection handling to be slightly more safer. The
following two patches do the conversion itself. Note the Intel SPI flash
controller only allows commands such as read, write and so on and it
internally uses whatever addressing etc. it figured from the SFDP on the
flash device.

I have tested this series on two systems with hardware sequencer (full
erase, write) and on a laptop with software sequencer (read only since I
don't have any means to restore the flash contents if something goes
wrong).

Let me know if it is better to send the full patches without rename
detection.

Mika Westerberg (3):
  mtd: spi-nor: intel-spi: Disable write protection only if asked
  mtd: spi-nor: intel-spi: Convert to SPI MEM
  Documentation / MTD: Rename the intel-spi driver

 Documentation/driver-api/mtd/index.rst        |   2 +-
 .../mtd/{intel-spi.rst => spi-intel.rst}      |   8 +-
 drivers/mfd/lpc_ich.c                         |  59 ++-
 drivers/mtd/spi-nor/controllers/Kconfig       |  36 --
 drivers/mtd/spi-nor/controllers/Makefile      |   3 -
 drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
 drivers/spi/Kconfig                           |  38 ++
 drivers/spi/Makefile                          |   3 +
 .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  49 ++-
 .../spi-intel-platform.c}                     |  21 +-
 .../intel-spi.c => spi/spi-intel.c}           | 341 +++++++++++-------
 drivers/spi/spi-intel.h                       |  19 +
 include/linux/mfd/lpc_ich.h                   |   2 +-
 .../x86/{intel-spi.h => spi-intel.h}          |  12 +-
 14 files changed, 353 insertions(+), 261 deletions(-)
 rename Documentation/driver-api/mtd/{intel-spi.rst => spi-intel.rst} (94%)
 delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
 rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (84%)
 rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
 rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (78%)
 create mode 100644 drivers/spi/spi-intel.h
 rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (64%)

-- 
2.33.0

[PATCH 0/3] mtd: spi-nor / spi / MFD: Convert intel-spi to SPI MEM

[Mika Westerberg]

Hi all,

Based on discussion on the patch I sent some time ago here:

  http://lists.infradead.org/pipermail/linux-mtd/2021-June/086867.html

it turns out that the preferred way to deal with the SPI flash controller
drivers is through SPI MEM which is part of Linux SPI subsystem.

This series does that for the intel-spi driver. This also renames the
driver to follow the convention used in the SPI subsystem. The first patch
improves the write protection handling to be slightly more safer. The
following two patches do the conversion itself. Note the Intel SPI flash
controller only allows commands such as read, write and so on and it
internally uses whatever addressing etc. it figured from the SFDP on the
flash device.

I have tested this series on two systems with hardware sequencer (full
erase, write) and on a laptop with software sequencer (read only since I
don't have any means to restore the flash contents if something goes
wrong).

Let me know if it is better to send the full patches without rename
detection.

Mika Westerberg (3):
  mtd: spi-nor: intel-spi: Disable write protection only if asked
  mtd: spi-nor: intel-spi: Convert to SPI MEM
  Documentation / MTD: Rename the intel-spi driver

 Documentation/driver-api/mtd/index.rst        |   2 +-
 .../mtd/{intel-spi.rst => spi-intel.rst}      |   8 +-
 drivers/mfd/lpc_ich.c                         |  59 ++-
 drivers/mtd/spi-nor/controllers/Kconfig       |  36 --
 drivers/mtd/spi-nor/controllers/Makefile      |   3 -
 drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
 drivers/spi/Kconfig                           |  38 ++
 drivers/spi/Makefile                          |   3 +
 .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  49 ++-
 .../spi-intel-platform.c}                     |  21 +-
 .../intel-spi.c => spi/spi-intel.c}           | 341 +++++++++++-------
 drivers/spi/spi-intel.h                       |  19 +
 include/linux/mfd/lpc_ich.h                   |   2 +-
 .../x86/{intel-spi.h => spi-intel.h}          |  12 +-
 14 files changed, 353 insertions(+), 261 deletions(-)
 rename Documentation/driver-api/mtd/{intel-spi.rst => spi-intel.rst} (94%)
 delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
 rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (84%)
 rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
 rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (78%)
 create mode 100644 drivers/spi/spi-intel.h
 rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (64%)

-- 
2.33.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mika Westerberg]

Currently the driver tries to disable the BIOS write protection
automatically even if this is not what the user wants. For this reason
modify the driver so that by default it does not touch the write
protection. Only if specifically asked by the user (setting writeable=1
command line parameter) the driver tries to disable the BIOS write
protection.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/mfd/lpc_ich.c                         | 59 +++++++++++++++++--
 .../mtd/spi-nor/controllers/intel-spi-pci.c   | 29 +++++----
 drivers/mtd/spi-nor/controllers/intel-spi.c   | 41 ++++++-------
 include/linux/platform_data/x86/intel-spi.h   |  6 +-
 4 files changed, 96 insertions(+), 39 deletions(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index f10e53187f67..9ffab9aafd81 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -63,6 +63,8 @@
 #define SPIBASE_BYT		0x54
 #define SPIBASE_BYT_SZ		512
 #define SPIBASE_BYT_EN		BIT(1)
+#define BYT_BCR			0xfc
+#define BYT_BCR_WPD		BIT(0)
 
 #define SPIBASE_LPT		0x3800
 #define SPIBASE_LPT_SZ		512
@@ -1084,12 +1086,57 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 	return ret;
 }
 
+static bool lpc_ich_byt_set_writeable(void __iomem *base, void *data)
+{
+	u32 val;
+
+	val = readl(base + BYT_BCR);
+	if (!(val & BYT_BCR_WPD)) {
+		val |= BYT_BCR_WPD;
+		writel(val, base + BYT_BCR);
+		val = readl(base + BYT_BCR);
+	}
+
+	return val & BYT_BCR_WPD;
+}
+
+static bool lpc_ich_lpt_set_writeable(void __iomem *base, void *data)
+{
+	struct pci_dev *pdev = data;
+	u32 bcr;
+
+	pci_read_config_dword(pdev, BCR, &bcr);
+	if (!(bcr & BCR_WPD)) {
+		bcr |= BCR_WPD;
+		pci_write_config_dword(pdev, BCR, bcr);
+		pci_read_config_dword(pdev, BCR, &bcr);
+	}
+
+	return bcr & BCR_WPD;
+}
+
+static bool lpc_ich_bxt_set_writeable(void __iomem *base, void *data)
+{
+	unsigned int spi = PCI_DEVFN(13, 2);
+	struct pci_bus *bus = data;
+	u32 bcr;
+
+	pci_bus_read_config_dword(bus, spi, BCR, &bcr);
+	if (!(bcr & BCR_WPD)) {
+		bcr |= BCR_WPD;
+		pci_bus_write_config_dword(bus, spi, BCR, bcr);
+		pci_bus_read_config_dword(bus, spi, BCR, &bcr);
+	}
+
+	return bcr & BCR_WPD;
+}
+
 static int lpc_ich_init_spi(struct pci_dev *dev)
 {
 	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
 	struct resource *res = &intel_spi_res[0];
 	struct intel_spi_boardinfo *info;
-	u32 spi_base, rcba, bcr;
+	u32 spi_base, rcba;
 
 	info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -1103,6 +1150,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 		if (spi_base & SPIBASE_BYT_EN) {
 			res->start = spi_base & ~(SPIBASE_BYT_SZ - 1);
 			res->end = res->start + SPIBASE_BYT_SZ - 1;
+
+			info->set_writeable = lpc_ich_byt_set_writeable;
 		}
 		break;
 
@@ -1113,8 +1162,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 			res->start = spi_base + SPIBASE_LPT;
 			res->end = res->start + SPIBASE_LPT_SZ - 1;
 
-			pci_read_config_dword(dev, BCR, &bcr);
-			info->writeable = !!(bcr & BCR_WPD);
+			info->set_writeable = lpc_ich_lpt_set_writeable;
+			info->data = dev;
 		}
 		break;
 
@@ -1135,8 +1184,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 			res->start = spi_base & 0xfffffff0;
 			res->end = res->start + SPIBASE_APL_SZ - 1;
 
-			pci_bus_read_config_dword(bus, spi, BCR, &bcr);
-			info->writeable = !!(bcr & BCR_WPD);
+			info->set_writeable = lpc_ich_bxt_set_writeable;
+			info->data = bus;
 		}
 
 		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
index 1bc53b8bb88a..508f7ca098ef 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
@@ -16,12 +16,30 @@
 #define BCR		0xdc
 #define BCR_WPD		BIT(0)
 
+static bool intel_spi_pci_set_writeable(void __iomem *base, void *data)
+{
+	struct pci_dev *pdev = data;
+	u32 bcr;
+
+	/* Try to make the chip read/write */
+	pci_read_config_dword(pdev, BCR, &bcr);
+	if (!(bcr & BCR_WPD)) {
+		bcr |= BCR_WPD;
+		pci_write_config_dword(pdev, BCR, bcr);
+		pci_read_config_dword(pdev, BCR, &bcr);
+	}
+
+	return bcr & BCR_WPD;
+}
+
 static const struct intel_spi_boardinfo bxt_info = {
 	.type = INTEL_SPI_BXT,
+	.set_writeable = intel_spi_pci_set_writeable,
 };
 
 static const struct intel_spi_boardinfo cnl_info = {
 	.type = INTEL_SPI_CNL,
+	.set_writeable = intel_spi_pci_set_writeable,
 };
 
 static int intel_spi_pci_probe(struct pci_dev *pdev,
@@ -29,7 +47,6 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
 {
 	struct intel_spi_boardinfo *info;
 	struct intel_spi *ispi;
-	u32 bcr;
 	int ret;
 
 	ret = pcim_enable_device(pdev);
@@ -41,15 +58,7 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
 	if (!info)
 		return -ENOMEM;
 
-	/* Try to make the chip read/write */
-	pci_read_config_dword(pdev, BCR, &bcr);
-	if (!(bcr & BCR_WPD)) {
-		bcr |= BCR_WPD;
-		pci_write_config_dword(pdev, BCR, bcr);
-		pci_read_config_dword(pdev, BCR, &bcr);
-	}
-	info->writeable = !!(bcr & BCR_WPD);
-
+	info->data = pdev;
 	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
 	if (IS_ERR(ispi))
 		return PTR_ERR(ispi);
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
index a413892ff449..f35597cbea0c 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
@@ -131,7 +131,6 @@
  * @sregs: Start of software sequencer registers
  * @nregions: Maximum number of regions
  * @pr_num: Maximum number of protected range registers
- * @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
@@ -149,7 +148,6 @@ struct intel_spi {
 	void __iomem *sregs;
 	size_t nregions;
 	size_t pr_num;
-	bool writeable;
 	bool locked;
 	bool swseq_reg;
 	bool swseq_erase;
@@ -304,6 +302,14 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
 				  INTEL_SPI_TIMEOUT * 1000);
 }
 
+static bool intel_spi_set_writeable(struct intel_spi *ispi)
+{
+	if (!ispi->info->set_writeable)
+		return false;
+
+	return ispi->info->set_writeable(ispi->base, ispi->info->data);
+}
+
 static int intel_spi_init(struct intel_spi *ispi)
 {
 	u32 opmenu0, opmenu1, lvscc, uvscc, val;
@@ -316,19 +322,6 @@ static int intel_spi_init(struct intel_spi *ispi)
 		ispi->nregions = BYT_FREG_NUM;
 		ispi->pr_num = BYT_PR_NUM;
 		ispi->swseq_reg = true;
-
-		if (writeable) {
-			/* Disable write protection */
-			val = readl(ispi->base + BYT_BCR);
-			if (!(val & BYT_BCR_WPD)) {
-				val |= BYT_BCR_WPD;
-				writel(val, ispi->base + BYT_BCR);
-				val = readl(ispi->base + BYT_BCR);
-			}
-
-			ispi->writeable = !!(val & BYT_BCR_WPD);
-		}
-
 		break;
 
 	case INTEL_SPI_LPT:
@@ -358,6 +351,12 @@ static int intel_spi_init(struct intel_spi *ispi)
 		return -EINVAL;
 	}
 
+	/* Try to disable write protection if user asked to do so */
+	if (writeable && !intel_spi_set_writeable(ispi)) {
+		dev_warn(ispi->dev, "can't disable chip write protection\n");
+		writeable = false;
+	}
+
 	/* Disable #SMI generation from HW sequencer */
 	val = readl(ispi->base + HSFSTS_CTL);
 	val &= ~HSFSTS_CTL_FSMIE;
@@ -884,9 +883,12 @@ static void intel_spi_fill_partition(struct intel_spi *ispi,
 		/*
 		 * If any of the regions have protection bits set, make the
 		 * whole partition read-only to be on the safe side.
+		 *
+		 * Also if the user did not ask the chip to be writeable
+		 * mask the bit too.
 		 */
-		if (intel_spi_is_protected(ispi, base, limit))
-			ispi->writeable = false;
+		if (!writeable || intel_spi_is_protected(ispi, base, limit))
+			part->mask_flags |= MTD_WRITEABLE;
 
 		end = (limit << 12) + 4096;
 		if (end > part->size)
@@ -927,7 +929,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
 
 	ispi->dev = dev;
 	ispi->info = info;
-	ispi->writeable = info->writeable;
 
 	ret = intel_spi_init(ispi);
 	if (ret)
@@ -945,10 +946,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
 
 	intel_spi_fill_partition(ispi, &part);
 
-	/* Prevent writes if not explicitly enabled */
-	if (!ispi->writeable || !writeable)
-		ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
-
 	ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/include/linux/platform_data/x86/intel-spi.h b/include/linux/platform_data/x86/intel-spi.h
index 7f53a5c6f35e..7dda3f690465 100644
--- a/include/linux/platform_data/x86/intel-spi.h
+++ b/include/linux/platform_data/x86/intel-spi.h
@@ -19,11 +19,13 @@ enum intel_spi_type {
 /**
  * struct intel_spi_boardinfo - Board specific data for Intel SPI driver
  * @type: Type which this controller is compatible with
- * @writeable: The chip is writeable
+ * @set_writeable: Try to make the chip writeable (optional)
+ * @data: Data to be passed to @set_writeable can be %NULL
  */
 struct intel_spi_boardinfo {
 	enum intel_spi_type type;
-	bool writeable;
+	bool (*set_writeable)(void __iomem *base, void *data);
+	void *data;
 };
 
 #endif /* INTEL_SPI_PDATA_H */
-- 
2.33.0

[PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mika Westerberg]

Currently the driver tries to disable the BIOS write protection
automatically even if this is not what the user wants. For this reason
modify the driver so that by default it does not touch the write
protection. Only if specifically asked by the user (setting writeable=1
command line parameter) the driver tries to disable the BIOS write
protection.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/mfd/lpc_ich.c                         | 59 +++++++++++++++++--
 .../mtd/spi-nor/controllers/intel-spi-pci.c   | 29 +++++----
 drivers/mtd/spi-nor/controllers/intel-spi.c   | 41 ++++++-------
 include/linux/platform_data/x86/intel-spi.h   |  6 +-
 4 files changed, 96 insertions(+), 39 deletions(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index f10e53187f67..9ffab9aafd81 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -63,6 +63,8 @@
 #define SPIBASE_BYT		0x54
 #define SPIBASE_BYT_SZ		512
 #define SPIBASE_BYT_EN		BIT(1)
+#define BYT_BCR			0xfc
+#define BYT_BCR_WPD		BIT(0)
 
 #define SPIBASE_LPT		0x3800
 #define SPIBASE_LPT_SZ		512
@@ -1084,12 +1086,57 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 	return ret;
 }
 
+static bool lpc_ich_byt_set_writeable(void __iomem *base, void *data)
+{
+	u32 val;
+
+	val = readl(base + BYT_BCR);
+	if (!(val & BYT_BCR_WPD)) {
+		val |= BYT_BCR_WPD;
+		writel(val, base + BYT_BCR);
+		val = readl(base + BYT_BCR);
+	}
+
+	return val & BYT_BCR_WPD;
+}
+
+static bool lpc_ich_lpt_set_writeable(void __iomem *base, void *data)
+{
+	struct pci_dev *pdev = data;
+	u32 bcr;
+
+	pci_read_config_dword(pdev, BCR, &bcr);
+	if (!(bcr & BCR_WPD)) {
+		bcr |= BCR_WPD;
+		pci_write_config_dword(pdev, BCR, bcr);
+		pci_read_config_dword(pdev, BCR, &bcr);
+	}
+
+	return bcr & BCR_WPD;
+}
+
+static bool lpc_ich_bxt_set_writeable(void __iomem *base, void *data)
+{
+	unsigned int spi = PCI_DEVFN(13, 2);
+	struct pci_bus *bus = data;
+	u32 bcr;
+
+	pci_bus_read_config_dword(bus, spi, BCR, &bcr);
+	if (!(bcr & BCR_WPD)) {
+		bcr |= BCR_WPD;
+		pci_bus_write_config_dword(bus, spi, BCR, bcr);
+		pci_bus_read_config_dword(bus, spi, BCR, &bcr);
+	}
+
+	return bcr & BCR_WPD;
+}
+
 static int lpc_ich_init_spi(struct pci_dev *dev)
 {
 	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
 	struct resource *res = &intel_spi_res[0];
 	struct intel_spi_boardinfo *info;
-	u32 spi_base, rcba, bcr;
+	u32 spi_base, rcba;
 
 	info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -1103,6 +1150,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 		if (spi_base & SPIBASE_BYT_EN) {
 			res->start = spi_base & ~(SPIBASE_BYT_SZ - 1);
 			res->end = res->start + SPIBASE_BYT_SZ - 1;
+
+			info->set_writeable = lpc_ich_byt_set_writeable;
 		}
 		break;
 
@@ -1113,8 +1162,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 			res->start = spi_base + SPIBASE_LPT;
 			res->end = res->start + SPIBASE_LPT_SZ - 1;
 
-			pci_read_config_dword(dev, BCR, &bcr);
-			info->writeable = !!(bcr & BCR_WPD);
+			info->set_writeable = lpc_ich_lpt_set_writeable;
+			info->data = dev;
 		}
 		break;
 
@@ -1135,8 +1184,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 			res->start = spi_base & 0xfffffff0;
 			res->end = res->start + SPIBASE_APL_SZ - 1;
 
-			pci_bus_read_config_dword(bus, spi, BCR, &bcr);
-			info->writeable = !!(bcr & BCR_WPD);
+			info->set_writeable = lpc_ich_bxt_set_writeable;
+			info->data = bus;
 		}
 
 		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
index 1bc53b8bb88a..508f7ca098ef 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
@@ -16,12 +16,30 @@
 #define BCR		0xdc
 #define BCR_WPD		BIT(0)
 
+static bool intel_spi_pci_set_writeable(void __iomem *base, void *data)
+{
+	struct pci_dev *pdev = data;
+	u32 bcr;
+
+	/* Try to make the chip read/write */
+	pci_read_config_dword(pdev, BCR, &bcr);
+	if (!(bcr & BCR_WPD)) {
+		bcr |= BCR_WPD;
+		pci_write_config_dword(pdev, BCR, bcr);
+		pci_read_config_dword(pdev, BCR, &bcr);
+	}
+
+	return bcr & BCR_WPD;
+}
+
 static const struct intel_spi_boardinfo bxt_info = {
 	.type = INTEL_SPI_BXT,
+	.set_writeable = intel_spi_pci_set_writeable,
 };
 
 static const struct intel_spi_boardinfo cnl_info = {
 	.type = INTEL_SPI_CNL,
+	.set_writeable = intel_spi_pci_set_writeable,
 };
 
 static int intel_spi_pci_probe(struct pci_dev *pdev,
@@ -29,7 +47,6 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
 {
 	struct intel_spi_boardinfo *info;
 	struct intel_spi *ispi;
-	u32 bcr;
 	int ret;
 
 	ret = pcim_enable_device(pdev);
@@ -41,15 +58,7 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
 	if (!info)
 		return -ENOMEM;
 
-	/* Try to make the chip read/write */
-	pci_read_config_dword(pdev, BCR, &bcr);
-	if (!(bcr & BCR_WPD)) {
-		bcr |= BCR_WPD;
-		pci_write_config_dword(pdev, BCR, bcr);
-		pci_read_config_dword(pdev, BCR, &bcr);
-	}
-	info->writeable = !!(bcr & BCR_WPD);
-
+	info->data = pdev;
 	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
 	if (IS_ERR(ispi))
 		return PTR_ERR(ispi);
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
index a413892ff449..f35597cbea0c 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
@@ -131,7 +131,6 @@
  * @sregs: Start of software sequencer registers
  * @nregions: Maximum number of regions
  * @pr_num: Maximum number of protected range registers
- * @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
@@ -149,7 +148,6 @@ struct intel_spi {
 	void __iomem *sregs;
 	size_t nregions;
 	size_t pr_num;
-	bool writeable;
 	bool locked;
 	bool swseq_reg;
 	bool swseq_erase;
@@ -304,6 +302,14 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
 				  INTEL_SPI_TIMEOUT * 1000);
 }
 
+static bool intel_spi_set_writeable(struct intel_spi *ispi)
+{
+	if (!ispi->info->set_writeable)
+		return false;
+
+	return ispi->info->set_writeable(ispi->base, ispi->info->data);
+}
+
 static int intel_spi_init(struct intel_spi *ispi)
 {
 	u32 opmenu0, opmenu1, lvscc, uvscc, val;
@@ -316,19 +322,6 @@ static int intel_spi_init(struct intel_spi *ispi)
 		ispi->nregions = BYT_FREG_NUM;
 		ispi->pr_num = BYT_PR_NUM;
 		ispi->swseq_reg = true;
-
-		if (writeable) {
-			/* Disable write protection */
-			val = readl(ispi->base + BYT_BCR);
-			if (!(val & BYT_BCR_WPD)) {
-				val |= BYT_BCR_WPD;
-				writel(val, ispi->base + BYT_BCR);
-				val = readl(ispi->base + BYT_BCR);
-			}
-
-			ispi->writeable = !!(val & BYT_BCR_WPD);
-		}
-
 		break;
 
 	case INTEL_SPI_LPT:
@@ -358,6 +351,12 @@ static int intel_spi_init(struct intel_spi *ispi)
 		return -EINVAL;
 	}
 
+	/* Try to disable write protection if user asked to do so */
+	if (writeable && !intel_spi_set_writeable(ispi)) {
+		dev_warn(ispi->dev, "can't disable chip write protection\n");
+		writeable = false;
+	}
+
 	/* Disable #SMI generation from HW sequencer */
 	val = readl(ispi->base + HSFSTS_CTL);
 	val &= ~HSFSTS_CTL_FSMIE;
@@ -884,9 +883,12 @@ static void intel_spi_fill_partition(struct intel_spi *ispi,
 		/*
 		 * If any of the regions have protection bits set, make the
 		 * whole partition read-only to be on the safe side.
+		 *
+		 * Also if the user did not ask the chip to be writeable
+		 * mask the bit too.
 		 */
-		if (intel_spi_is_protected(ispi, base, limit))
-			ispi->writeable = false;
+		if (!writeable || intel_spi_is_protected(ispi, base, limit))
+			part->mask_flags |= MTD_WRITEABLE;
 
 		end = (limit << 12) + 4096;
 		if (end > part->size)
@@ -927,7 +929,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
 
 	ispi->dev = dev;
 	ispi->info = info;
-	ispi->writeable = info->writeable;
 
 	ret = intel_spi_init(ispi);
 	if (ret)
@@ -945,10 +946,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
 
 	intel_spi_fill_partition(ispi, &part);
 
-	/* Prevent writes if not explicitly enabled */
-	if (!ispi->writeable || !writeable)
-		ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
-
 	ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/include/linux/platform_data/x86/intel-spi.h b/include/linux/platform_data/x86/intel-spi.h
index 7f53a5c6f35e..7dda3f690465 100644
--- a/include/linux/platform_data/x86/intel-spi.h
+++ b/include/linux/platform_data/x86/intel-spi.h
@@ -19,11 +19,13 @@ enum intel_spi_type {
 /**
  * struct intel_spi_boardinfo - Board specific data for Intel SPI driver
  * @type: Type which this controller is compatible with
- * @writeable: The chip is writeable
+ * @set_writeable: Try to make the chip writeable (optional)
+ * @data: Data to be passed to @set_writeable can be %NULL
  */
 struct intel_spi_boardinfo {
 	enum intel_spi_type type;
-	bool writeable;
+	bool (*set_writeable)(void __iomem *base, void *data);
+	void *data;
 };
 
 #endif /* INTEL_SPI_PDATA_H */
-- 
2.33.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mauro Lima]

Hi all!

On Thu, Sep 30, 2021 at 7:07 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Currently the driver tries to disable the BIOS write protection
> automatically even if this is not what the user wants. For this reason
> modify the driver so that by default it does not touch the write
> protection. Only if specifically asked by the user (setting writeable=1
> command line parameter) the driver tries to disable the BIOS write
> protection.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/mfd/lpc_ich.c                         | 59 +++++++++++++++++--
>  .../mtd/spi-nor/controllers/intel-spi-pci.c   | 29 +++++----
>  drivers/mtd/spi-nor/controllers/intel-spi.c   | 41 ++++++-------
>  include/linux/platform_data/x86/intel-spi.h   |  6 +-
>  4 files changed, 96 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index f10e53187f67..9ffab9aafd81 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -63,6 +63,8 @@
>  #define SPIBASE_BYT            0x54
>  #define SPIBASE_BYT_SZ         512
>  #define SPIBASE_BYT_EN         BIT(1)
> +#define BYT_BCR                        0xfc
> +#define BYT_BCR_WPD            BIT(0)
>
>  #define SPIBASE_LPT            0x3800
>  #define SPIBASE_LPT_SZ         512
> @@ -1084,12 +1086,57 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>         return ret;
>  }
>
> +static bool lpc_ich_byt_set_writeable(void __iomem *base, void *data)
> +{
> +       u32 val;
> +
> +       val = readl(base + BYT_BCR);
> +       if (!(val & BYT_BCR_WPD)) {
> +               val |= BYT_BCR_WPD;
> +               writel(val, base + BYT_BCR);
> +               val = readl(base + BYT_BCR);
> +       }
> +
> +       return val & BYT_BCR_WPD;
> +}
> +
> +static bool lpc_ich_lpt_set_writeable(void __iomem *base, void *data)
> +{
> +       struct pci_dev *pdev = data;
> +       u32 bcr;
> +
> +       pci_read_config_dword(pdev, BCR, &bcr);
> +       if (!(bcr & BCR_WPD)) {
> +               bcr |= BCR_WPD;
> +               pci_write_config_dword(pdev, BCR, bcr);
> +               pci_read_config_dword(pdev, BCR, &bcr);
> +       }
> +
> +       return bcr & BCR_WPD;
> +}
> +
> +static bool lpc_ich_bxt_set_writeable(void __iomem *base, void *data)
> +{
> +       unsigned int spi = PCI_DEVFN(13, 2);
> +       struct pci_bus *bus = data;
> +       u32 bcr;
> +
> +       pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> +       if (!(bcr & BCR_WPD)) {
> +               bcr |= BCR_WPD;
> +               pci_bus_write_config_dword(bus, spi, BCR, bcr);
> +               pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> +       }
> +
> +       return bcr & BCR_WPD;
> +}
> +
>  static int lpc_ich_init_spi(struct pci_dev *dev)
>  {
>         struct lpc_ich_priv *priv = pci_get_drvdata(dev);
>         struct resource *res = &intel_spi_res[0];
>         struct intel_spi_boardinfo *info;
> -       u32 spi_base, rcba, bcr;
> +       u32 spi_base, rcba;
>
>         info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
>         if (!info)
> @@ -1103,6 +1150,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>                 if (spi_base & SPIBASE_BYT_EN) {
>                         res->start = spi_base & ~(SPIBASE_BYT_SZ - 1);
>                         res->end = res->start + SPIBASE_BYT_SZ - 1;
> +
> +                       info->set_writeable = lpc_ich_byt_set_writeable;
>                 }
>                 break;
>
> @@ -1113,8 +1162,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>                         res->start = spi_base + SPIBASE_LPT;
>                         res->end = res->start + SPIBASE_LPT_SZ - 1;
>
> -                       pci_read_config_dword(dev, BCR, &bcr);
> -                       info->writeable = !!(bcr & BCR_WPD);
> +                       info->set_writeable = lpc_ich_lpt_set_writeable;
> +                       info->data = dev;
>                 }
>                 break;
>
> @@ -1135,8 +1184,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>                         res->start = spi_base & 0xfffffff0;
>                         res->end = res->start + SPIBASE_APL_SZ - 1;
>
> -                       pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> -                       info->writeable = !!(bcr & BCR_WPD);
> +                       info->set_writeable = lpc_ich_bxt_set_writeable;
> +                       info->data = bus;
>                 }
>
>                 pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> index 1bc53b8bb88a..508f7ca098ef 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -16,12 +16,30 @@
>  #define BCR            0xdc
>  #define BCR_WPD                BIT(0)
>
> +static bool intel_spi_pci_set_writeable(void __iomem *base, void *data)
> +{
> +       struct pci_dev *pdev = data;
> +       u32 bcr;
> +
> +       /* Try to make the chip read/write */
> +       pci_read_config_dword(pdev, BCR, &bcr);
> +       if (!(bcr & BCR_WPD)) {
> +               bcr |= BCR_WPD;
> +               pci_write_config_dword(pdev, BCR, bcr);
> +               pci_read_config_dword(pdev, BCR, &bcr);
> +       }
> +
> +       return bcr & BCR_WPD;
> +}
> +
>  static const struct intel_spi_boardinfo bxt_info = {
>         .type = INTEL_SPI_BXT,
> +       .set_writeable = intel_spi_pci_set_writeable,
>  };
>
>  static const struct intel_spi_boardinfo cnl_info = {
>         .type = INTEL_SPI_CNL,
> +       .set_writeable = intel_spi_pci_set_writeable,
>  };
>
>  static int intel_spi_pci_probe(struct pci_dev *pdev,
> @@ -29,7 +47,6 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
>  {
>         struct intel_spi_boardinfo *info;
>         struct intel_spi *ispi;
> -       u32 bcr;
>         int ret;
>
>         ret = pcim_enable_device(pdev);
> @@ -41,15 +58,7 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
>         if (!info)
>                 return -ENOMEM;
>
> -       /* Try to make the chip read/write */
> -       pci_read_config_dword(pdev, BCR, &bcr);
> -       if (!(bcr & BCR_WPD)) {
> -               bcr |= BCR_WPD;
> -               pci_write_config_dword(pdev, BCR, bcr);
> -               pci_read_config_dword(pdev, BCR, &bcr);
> -       }
> -       info->writeable = !!(bcr & BCR_WPD);
> -
> +       info->data = pdev;
>         ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
>         if (IS_ERR(ispi))
>                 return PTR_ERR(ispi);
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> index a413892ff449..f35597cbea0c 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> @@ -131,7 +131,6 @@
>   * @sregs: Start of software sequencer registers
>   * @nregions: Maximum number of regions
>   * @pr_num: Maximum number of protected range registers
> - * @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
> @@ -149,7 +148,6 @@ struct intel_spi {
>         void __iomem *sregs;
>         size_t nregions;
>         size_t pr_num;
> -       bool writeable;
>         bool locked;
>         bool swseq_reg;
>         bool swseq_erase;
> @@ -304,6 +302,14 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
>                                   INTEL_SPI_TIMEOUT * 1000);
>  }
>
> +static bool intel_spi_set_writeable(struct intel_spi *ispi)
> +{
> +       if (!ispi->info->set_writeable)
> +               return false;
> +
> +       return ispi->info->set_writeable(ispi->base, ispi->info->data);
> +}
> +
>  static int intel_spi_init(struct intel_spi *ispi)
>  {
>         u32 opmenu0, opmenu1, lvscc, uvscc, val;
> @@ -316,19 +322,6 @@ static int intel_spi_init(struct intel_spi *ispi)
>                 ispi->nregions = BYT_FREG_NUM;
>                 ispi->pr_num = BYT_PR_NUM;
>                 ispi->swseq_reg = true;
> -
> -               if (writeable) {
> -                       /* Disable write protection */
> -                       val = readl(ispi->base + BYT_BCR);
> -                       if (!(val & BYT_BCR_WPD)) {
> -                               val |= BYT_BCR_WPD;
> -                               writel(val, ispi->base + BYT_BCR);
> -                               val = readl(ispi->base + BYT_BCR);
> -                       }
> -
> -                       ispi->writeable = !!(val & BYT_BCR_WPD);
> -               }
> -
>                 break;
>
>         case INTEL_SPI_LPT:
> @@ -358,6 +351,12 @@ static int intel_spi_init(struct intel_spi *ispi)
>                 return -EINVAL;
>         }
>
> +       /* Try to disable write protection if user asked to do so */
> +       if (writeable && !intel_spi_set_writeable(ispi)) {
> +               dev_warn(ispi->dev, "can't disable chip write protection\n");
> +               writeable = false;
> +       }
> +
>         /* Disable #SMI generation from HW sequencer */
>         val = readl(ispi->base + HSFSTS_CTL);
>         val &= ~HSFSTS_CTL_FSMIE;
> @@ -884,9 +883,12 @@ static void intel_spi_fill_partition(struct intel_spi *ispi,
>                 /*
>                  * If any of the regions have protection bits set, make the
>                  * whole partition read-only to be on the safe side.
> +                *
> +                * Also if the user did not ask the chip to be writeable
> +                * mask the bit too.
>                  */
> -               if (intel_spi_is_protected(ispi, base, limit))
> -                       ispi->writeable = false;
> +               if (!writeable || intel_spi_is_protected(ispi, base, limit))
> +                       part->mask_flags |= MTD_WRITEABLE;
>
>                 end = (limit << 12) + 4096;
>                 if (end > part->size)
> @@ -927,7 +929,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
>
>         ispi->dev = dev;
>         ispi->info = info;
> -       ispi->writeable = info->writeable;
>
>         ret = intel_spi_init(ispi);
>         if (ret)
> @@ -945,10 +946,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
>
>         intel_spi_fill_partition(ispi, &part);
>
> -       /* Prevent writes if not explicitly enabled */
> -       if (!ispi->writeable || !writeable)
> -               ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
> -
>         ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
>         if (ret)
>                 return ERR_PTR(ret);
> diff --git a/include/linux/platform_data/x86/intel-spi.h b/include/linux/platform_data/x86/intel-spi.h
> index 7f53a5c6f35e..7dda3f690465 100644
> --- a/include/linux/platform_data/x86/intel-spi.h
> +++ b/include/linux/platform_data/x86/intel-spi.h
> @@ -19,11 +19,13 @@ enum intel_spi_type {
>  /**
>   * struct intel_spi_boardinfo - Board specific data for Intel SPI driver
>   * @type: Type which this controller is compatible with
> - * @writeable: The chip is writeable
> + * @set_writeable: Try to make the chip writeable (optional)
> + * @data: Data to be passed to @set_writeable can be %NULL
>   */
>  struct intel_spi_boardinfo {
>         enum intel_spi_type type;
> -       bool writeable;
> +       bool (*set_writeable)(void __iomem *base, void *data);
> +       void *data;
>  };
>
>  #endif /* INTEL_SPI_PDATA_H */
> --
> 2.33.0
>
Question for maintainers: With these changes is it safe to remove the
*(DANGEROUS)* tag from menuconfig?
Thanks, Mauro.

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mauro Lima]

Hi all!

On Thu, Sep 30, 2021 at 7:07 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Currently the driver tries to disable the BIOS write protection
> automatically even if this is not what the user wants. For this reason
> modify the driver so that by default it does not touch the write
> protection. Only if specifically asked by the user (setting writeable=1
> command line parameter) the driver tries to disable the BIOS write
> protection.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/mfd/lpc_ich.c                         | 59 +++++++++++++++++--
>  .../mtd/spi-nor/controllers/intel-spi-pci.c   | 29 +++++----
>  drivers/mtd/spi-nor/controllers/intel-spi.c   | 41 ++++++-------
>  include/linux/platform_data/x86/intel-spi.h   |  6 +-
>  4 files changed, 96 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index f10e53187f67..9ffab9aafd81 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -63,6 +63,8 @@
>  #define SPIBASE_BYT            0x54
>  #define SPIBASE_BYT_SZ         512
>  #define SPIBASE_BYT_EN         BIT(1)
> +#define BYT_BCR                        0xfc
> +#define BYT_BCR_WPD            BIT(0)
>
>  #define SPIBASE_LPT            0x3800
>  #define SPIBASE_LPT_SZ         512
> @@ -1084,12 +1086,57 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>         return ret;
>  }
>
> +static bool lpc_ich_byt_set_writeable(void __iomem *base, void *data)
> +{
> +       u32 val;
> +
> +       val = readl(base + BYT_BCR);
> +       if (!(val & BYT_BCR_WPD)) {
> +               val |= BYT_BCR_WPD;
> +               writel(val, base + BYT_BCR);
> +               val = readl(base + BYT_BCR);
> +       }
> +
> +       return val & BYT_BCR_WPD;
> +}
> +
> +static bool lpc_ich_lpt_set_writeable(void __iomem *base, void *data)
> +{
> +       struct pci_dev *pdev = data;
> +       u32 bcr;
> +
> +       pci_read_config_dword(pdev, BCR, &bcr);
> +       if (!(bcr & BCR_WPD)) {
> +               bcr |= BCR_WPD;
> +               pci_write_config_dword(pdev, BCR, bcr);
> +               pci_read_config_dword(pdev, BCR, &bcr);
> +       }
> +
> +       return bcr & BCR_WPD;
> +}
> +
> +static bool lpc_ich_bxt_set_writeable(void __iomem *base, void *data)
> +{
> +       unsigned int spi = PCI_DEVFN(13, 2);
> +       struct pci_bus *bus = data;
> +       u32 bcr;
> +
> +       pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> +       if (!(bcr & BCR_WPD)) {
> +               bcr |= BCR_WPD;
> +               pci_bus_write_config_dword(bus, spi, BCR, bcr);
> +               pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> +       }
> +
> +       return bcr & BCR_WPD;
> +}
> +
>  static int lpc_ich_init_spi(struct pci_dev *dev)
>  {
>         struct lpc_ich_priv *priv = pci_get_drvdata(dev);
>         struct resource *res = &intel_spi_res[0];
>         struct intel_spi_boardinfo *info;
> -       u32 spi_base, rcba, bcr;
> +       u32 spi_base, rcba;
>
>         info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
>         if (!info)
> @@ -1103,6 +1150,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>                 if (spi_base & SPIBASE_BYT_EN) {
>                         res->start = spi_base & ~(SPIBASE_BYT_SZ - 1);
>                         res->end = res->start + SPIBASE_BYT_SZ - 1;
> +
> +                       info->set_writeable = lpc_ich_byt_set_writeable;
>                 }
>                 break;
>
> @@ -1113,8 +1162,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>                         res->start = spi_base + SPIBASE_LPT;
>                         res->end = res->start + SPIBASE_LPT_SZ - 1;
>
> -                       pci_read_config_dword(dev, BCR, &bcr);
> -                       info->writeable = !!(bcr & BCR_WPD);
> +                       info->set_writeable = lpc_ich_lpt_set_writeable;
> +                       info->data = dev;
>                 }
>                 break;
>
> @@ -1135,8 +1184,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>                         res->start = spi_base & 0xfffffff0;
>                         res->end = res->start + SPIBASE_APL_SZ - 1;
>
> -                       pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> -                       info->writeable = !!(bcr & BCR_WPD);
> +                       info->set_writeable = lpc_ich_bxt_set_writeable;
> +                       info->data = bus;
>                 }
>
>                 pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> index 1bc53b8bb88a..508f7ca098ef 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -16,12 +16,30 @@
>  #define BCR            0xdc
>  #define BCR_WPD                BIT(0)
>
> +static bool intel_spi_pci_set_writeable(void __iomem *base, void *data)
> +{
> +       struct pci_dev *pdev = data;
> +       u32 bcr;
> +
> +       /* Try to make the chip read/write */
> +       pci_read_config_dword(pdev, BCR, &bcr);
> +       if (!(bcr & BCR_WPD)) {
> +               bcr |= BCR_WPD;
> +               pci_write_config_dword(pdev, BCR, bcr);
> +               pci_read_config_dword(pdev, BCR, &bcr);
> +       }
> +
> +       return bcr & BCR_WPD;
> +}
> +
>  static const struct intel_spi_boardinfo bxt_info = {
>         .type = INTEL_SPI_BXT,
> +       .set_writeable = intel_spi_pci_set_writeable,
>  };
>
>  static const struct intel_spi_boardinfo cnl_info = {
>         .type = INTEL_SPI_CNL,
> +       .set_writeable = intel_spi_pci_set_writeable,
>  };
>
>  static int intel_spi_pci_probe(struct pci_dev *pdev,
> @@ -29,7 +47,6 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
>  {
>         struct intel_spi_boardinfo *info;
>         struct intel_spi *ispi;
> -       u32 bcr;
>         int ret;
>
>         ret = pcim_enable_device(pdev);
> @@ -41,15 +58,7 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
>         if (!info)
>                 return -ENOMEM;
>
> -       /* Try to make the chip read/write */
> -       pci_read_config_dword(pdev, BCR, &bcr);
> -       if (!(bcr & BCR_WPD)) {
> -               bcr |= BCR_WPD;
> -               pci_write_config_dword(pdev, BCR, bcr);
> -               pci_read_config_dword(pdev, BCR, &bcr);
> -       }
> -       info->writeable = !!(bcr & BCR_WPD);
> -
> +       info->data = pdev;
>         ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
>         if (IS_ERR(ispi))
>                 return PTR_ERR(ispi);
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> index a413892ff449..f35597cbea0c 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> @@ -131,7 +131,6 @@
>   * @sregs: Start of software sequencer registers
>   * @nregions: Maximum number of regions
>   * @pr_num: Maximum number of protected range registers
> - * @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
> @@ -149,7 +148,6 @@ struct intel_spi {
>         void __iomem *sregs;
>         size_t nregions;
>         size_t pr_num;
> -       bool writeable;
>         bool locked;
>         bool swseq_reg;
>         bool swseq_erase;
> @@ -304,6 +302,14 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
>                                   INTEL_SPI_TIMEOUT * 1000);
>  }
>
> +static bool intel_spi_set_writeable(struct intel_spi *ispi)
> +{
> +       if (!ispi->info->set_writeable)
> +               return false;
> +
> +       return ispi->info->set_writeable(ispi->base, ispi->info->data);
> +}
> +
>  static int intel_spi_init(struct intel_spi *ispi)
>  {
>         u32 opmenu0, opmenu1, lvscc, uvscc, val;
> @@ -316,19 +322,6 @@ static int intel_spi_init(struct intel_spi *ispi)
>                 ispi->nregions = BYT_FREG_NUM;
>                 ispi->pr_num = BYT_PR_NUM;
>                 ispi->swseq_reg = true;
> -
> -               if (writeable) {
> -                       /* Disable write protection */
> -                       val = readl(ispi->base + BYT_BCR);
> -                       if (!(val & BYT_BCR_WPD)) {
> -                               val |= BYT_BCR_WPD;
> -                               writel(val, ispi->base + BYT_BCR);
> -                               val = readl(ispi->base + BYT_BCR);
> -                       }
> -
> -                       ispi->writeable = !!(val & BYT_BCR_WPD);
> -               }
> -
>                 break;
>
>         case INTEL_SPI_LPT:
> @@ -358,6 +351,12 @@ static int intel_spi_init(struct intel_spi *ispi)
>                 return -EINVAL;
>         }
>
> +       /* Try to disable write protection if user asked to do so */
> +       if (writeable && !intel_spi_set_writeable(ispi)) {
> +               dev_warn(ispi->dev, "can't disable chip write protection\n");
> +               writeable = false;
> +       }
> +
>         /* Disable #SMI generation from HW sequencer */
>         val = readl(ispi->base + HSFSTS_CTL);
>         val &= ~HSFSTS_CTL_FSMIE;
> @@ -884,9 +883,12 @@ static void intel_spi_fill_partition(struct intel_spi *ispi,
>                 /*
>                  * If any of the regions have protection bits set, make the
>                  * whole partition read-only to be on the safe side.
> +                *
> +                * Also if the user did not ask the chip to be writeable
> +                * mask the bit too.
>                  */
> -               if (intel_spi_is_protected(ispi, base, limit))
> -                       ispi->writeable = false;
> +               if (!writeable || intel_spi_is_protected(ispi, base, limit))
> +                       part->mask_flags |= MTD_WRITEABLE;
>
>                 end = (limit << 12) + 4096;
>                 if (end > part->size)
> @@ -927,7 +929,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
>
>         ispi->dev = dev;
>         ispi->info = info;
> -       ispi->writeable = info->writeable;
>
>         ret = intel_spi_init(ispi);
>         if (ret)
> @@ -945,10 +946,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
>
>         intel_spi_fill_partition(ispi, &part);
>
> -       /* Prevent writes if not explicitly enabled */
> -       if (!ispi->writeable || !writeable)
> -               ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
> -
>         ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
>         if (ret)
>                 return ERR_PTR(ret);
> diff --git a/include/linux/platform_data/x86/intel-spi.h b/include/linux/platform_data/x86/intel-spi.h
> index 7f53a5c6f35e..7dda3f690465 100644
> --- a/include/linux/platform_data/x86/intel-spi.h
> +++ b/include/linux/platform_data/x86/intel-spi.h
> @@ -19,11 +19,13 @@ enum intel_spi_type {
>  /**
>   * struct intel_spi_boardinfo - Board specific data for Intel SPI driver
>   * @type: Type which this controller is compatible with
> - * @writeable: The chip is writeable
> + * @set_writeable: Try to make the chip writeable (optional)
> + * @data: Data to be passed to @set_writeable can be %NULL
>   */
>  struct intel_spi_boardinfo {
>         enum intel_spi_type type;
> -       bool writeable;
> +       bool (*set_writeable)(void __iomem *base, void *data);
> +       void *data;
>  };
>
>  #endif /* INTEL_SPI_PDATA_H */
> --
> 2.33.0
>
Question for maintainers: With these changes is it safe to remove the
*(DANGEROUS)* tag from menuconfig?
Thanks, Mauro.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mika Westerberg]

Hi,

On Fri, Oct 01, 2021 at 05:23:23PM -0300, Mauro Lima wrote:
> Question for maintainers: With these changes is it safe to remove the
> *(DANGEROUS)* tag from menuconfig?

I don't think we want to remove that. This driver is not for regular
users, at least in its current form.

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mika Westerberg]

Hi,

On Fri, Oct 01, 2021 at 05:23:23PM -0300, Mauro Lima wrote:
> Question for maintainers: With these changes is it safe to remove the
> *(DANGEROUS)* tag from menuconfig?

I don't think we want to remove that. This driver is not for regular
users, at least in its current form.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mauro Lima]

Hi Mika

On Mon, Oct 4, 2021 at 2:18 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Fri, Oct 01, 2021 at 05:23:23PM -0300, Mauro Lima wrote:
> > Question for maintainers: With these changes is it safe to remove the
> > *(DANGEROUS)* tag from menuconfig?
>
> I don't think we want to remove that. This driver is not for regular
> users, at least in its current form.
Do we know why this is still dangerous for the user?
My plan is to make a sys/class driver to query write protection status
of the SPI, this will be
used by fwupd to gather information about vendors, also should be easy
for the user to use
'cat' and see the information from userspace. For this to be possible
we need kernel engineers
to compile the kernel with this driver enabled, but the (DANGEROUS)
tag is a no go for most
of them.
Is there anything I can do to make this possible?
Thanks

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mauro Lima]

Hi Mika

On Mon, Oct 4, 2021 at 2:18 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Fri, Oct 01, 2021 at 05:23:23PM -0300, Mauro Lima wrote:
> > Question for maintainers: With these changes is it safe to remove the
> > *(DANGEROUS)* tag from menuconfig?
>
> I don't think we want to remove that. This driver is not for regular
> users, at least in its current form.
Do we know why this is still dangerous for the user?
My plan is to make a sys/class driver to query write protection status
of the SPI, this will be
used by fwupd to gather information about vendors, also should be easy
for the user to use
'cat' and see the information from userspace. For this to be possible
we need kernel engineers
to compile the kernel with this driver enabled, but the (DANGEROUS)
tag is a no go for most
of them.
Is there anything I can do to make this possible?
Thanks

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mika Westerberg]

Hi,

On Tue, Oct 12, 2021 at 03:49:22PM -0300, Mauro Lima wrote:
> Hi Mika
> 
> On Mon, Oct 4, 2021 at 2:18 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Hi,
> >
> > On Fri, Oct 01, 2021 at 05:23:23PM -0300, Mauro Lima wrote:
> > > Question for maintainers: With these changes is it safe to remove the
> > > *(DANGEROUS)* tag from menuconfig?
> >
> > I don't think we want to remove that. This driver is not for regular
> > users, at least in its current form.
> Do we know why this is still dangerous for the user?

There was a bug in the driver in the past (that was already fixed but it
did not yet reach the stable trees or something like that). At this
unfortunate time there was no DANGEROUS in the name so Ubuntu kernel
went and enabled it. Combined with the bug certain Lenovo laptops BIOS
turned into read-only which prevented booting from non-default devices.

This happened even when the driver did not do any "write" or "erase"
operations, just clearing the status register or so.

We don't want that to happen again.

> My plan is to make a sys/class driver to query write protection status
> of the SPI, this will be
> used by fwupd to gather information about vendors, also should be easy
> for the user to use
> 'cat' and see the information from userspace. For this to be possible
> we need kernel engineers
> to compile the kernel with this driver enabled, but the (DANGEROUS)
> tag is a no go for most
> of them.
> Is there anything I can do to make this possible?

IMHO we can make certain parts of the driver, that are known not to
cause any issues available without the DANGEROUS. I mean the controller
exposes some information that I think you are intersted in and that does
not cause anything to be sent to the flash chip itself.

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mika Westerberg]

Hi,

On Tue, Oct 12, 2021 at 03:49:22PM -0300, Mauro Lima wrote:
> Hi Mika
> 
> On Mon, Oct 4, 2021 at 2:18 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Hi,
> >
> > On Fri, Oct 01, 2021 at 05:23:23PM -0300, Mauro Lima wrote:
> > > Question for maintainers: With these changes is it safe to remove the
> > > *(DANGEROUS)* tag from menuconfig?
> >
> > I don't think we want to remove that. This driver is not for regular
> > users, at least in its current form.
> Do we know why this is still dangerous for the user?

There was a bug in the driver in the past (that was already fixed but it
did not yet reach the stable trees or something like that). At this
unfortunate time there was no DANGEROUS in the name so Ubuntu kernel
went and enabled it. Combined with the bug certain Lenovo laptops BIOS
turned into read-only which prevented booting from non-default devices.

This happened even when the driver did not do any "write" or "erase"
operations, just clearing the status register or so.

We don't want that to happen again.

> My plan is to make a sys/class driver to query write protection status
> of the SPI, this will be
> used by fwupd to gather information about vendors, also should be easy
> for the user to use
> 'cat' and see the information from userspace. For this to be possible
> we need kernel engineers
> to compile the kernel with this driver enabled, but the (DANGEROUS)
> tag is a no go for most
> of them.
> Is there anything I can do to make this possible?

IMHO we can make certain parts of the driver, that are known not to
cause any issues available without the DANGEROUS. I mean the controller
exposes some information that I think you are intersted in and that does
not cause anything to be sent to the flash chip itself.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mauro Lima]

Hi!
On Wed, Oct 13, 2021 at 6:04 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Tue, Oct 12, 2021 at 03:49:22PM -0300, Mauro Lima wrote:
> > Hi Mika
> >
> > On Mon, Oct 4, 2021 at 2:18 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Oct 01, 2021 at 05:23:23PM -0300, Mauro Lima wrote:
> > > > Question for maintainers: With these changes is it safe to remove the
> > > > *(DANGEROUS)* tag from menuconfig?
> > >
> > > I don't think we want to remove that. This driver is not for regular
> > > users, at least in its current form.
> > Do we know why this is still dangerous for the user?
>
> There was a bug in the driver in the past (that was already fixed but it
> did not yet reach the stable trees or something like that). At this
> unfortunate time there was no DANGEROUS in the name so Ubuntu kernel
> went and enabled it. Combined with the bug certain Lenovo laptops BIOS
> turned into read-only which prevented booting from non-default devices.
>
> This happened even when the driver did not do any "write" or "erase"
> operations, just clearing the status register or so.
>
> We don't want that to happen again.
>
> > My plan is to make a sys/class driver to query write protection status
> > of the SPI, this will be
> > used by fwupd to gather information about vendors, also should be easy
> > for the user to use
> > 'cat' and see the information from userspace. For this to be possible
> > we need kernel engineers
> > to compile the kernel with this driver enabled, but the (DANGEROUS)
> > tag is a no go for most
> > of them.
> > Is there anything I can do to make this possible?
>
> IMHO we can make certain parts of the driver, that are known not to
> cause any issues available without the DANGEROUS. I mean the controller
> exposes some information that I think you are intersted in and that does
> not cause anything to be sent to the flash chip itself.
This will work for me.
Thanks!

Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Mauro Lima]

Hi!
On Wed, Oct 13, 2021 at 6:04 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Tue, Oct 12, 2021 at 03:49:22PM -0300, Mauro Lima wrote:
> > Hi Mika
> >
> > On Mon, Oct 4, 2021 at 2:18 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Oct 01, 2021 at 05:23:23PM -0300, Mauro Lima wrote:
> > > > Question for maintainers: With these changes is it safe to remove the
> > > > *(DANGEROUS)* tag from menuconfig?
> > >
> > > I don't think we want to remove that. This driver is not for regular
> > > users, at least in its current form.
> > Do we know why this is still dangerous for the user?
>
> There was a bug in the driver in the past (that was already fixed but it
> did not yet reach the stable trees or something like that). At this
> unfortunate time there was no DANGEROUS in the name so Ubuntu kernel
> went and enabled it. Combined with the bug certain Lenovo laptops BIOS
> turned into read-only which prevented booting from non-default devices.
>
> This happened even when the driver did not do any "write" or "erase"
> operations, just clearing the status register or so.
>
> We don't want that to happen again.
>
> > My plan is to make a sys/class driver to query write protection status
> > of the SPI, this will be
> > used by fwupd to gather information about vendors, also should be easy
> > for the user to use
> > 'cat' and see the information from userspace. For this to be possible
> > we need kernel engineers
> > to compile the kernel with this driver enabled, but the (DANGEROUS)
> > tag is a no go for most
> > of them.
> > Is there anything I can do to make this possible?
>
> IMHO we can make certain parts of the driver, that are known not to
> cause any issues available without the DANGEROUS. I mean the controller
> exposes some information that I think you are intersted in and that does
> not cause anything to be sent to the flash chip itself.
This will work for me.
Thanks!

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

The preferred way to implement SPI-NOR controller drivers is through SPI
subsubsystem utilizing the SPI MEM core functions. This converts the
Intel SPI flash controller driver over the SPI MEM by moving the driver
from SPI-NOR subsystem to SPI subsystem and in one go make it use the
SPI MEM functions. The driver name will be changed from intel-spi to
spi-intel to match the convention used in the SPI subsystem.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/mtd/spi-nor/controllers/Kconfig       |  36 ---
 drivers/mtd/spi-nor/controllers/Makefile      |   3 -
 drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
 drivers/spi/Kconfig                           |  38 +++
 drivers/spi/Makefile                          |   3 +
 .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  20 +-
 .../spi-intel-platform.c}                     |  21 +-
 .../intel-spi.c => spi/spi-intel.c}           | 300 +++++++++++-------
 drivers/spi/spi-intel.h                       |  19 ++
 include/linux/mfd/lpc_ich.h                   |   2 +-
 .../x86/{intel-spi.h => spi-intel.h}          |   6 +-
 11 files changed, 252 insertions(+), 217 deletions(-)
 delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
 rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (86%)
 rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
 rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (80%)
 create mode 100644 drivers/spi/spi-intel.h
 rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (89%)

diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
index 5c0e0ec2e6d1..50f4f3484d42 100644
--- a/drivers/mtd/spi-nor/controllers/Kconfig
+++ b/drivers/mtd/spi-nor/controllers/Kconfig
@@ -26,39 +26,3 @@ config SPI_NXP_SPIFI
 	  SPIFI is a specialized controller for connecting serial SPI
 	  Flash. Enable this option if you have a device with a SPIFI
 	  controller and want to access the Flash as a mtd device.
-
-config SPI_INTEL_SPI
-	tristate
-
-config SPI_INTEL_SPI_PCI
-	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
-	depends on X86 && PCI
-	select SPI_INTEL_SPI
-	help
-	  This enables PCI support for the Intel PCH/PCU SPI controller in
-	  master mode. This controller is present in modern Intel hardware
-	  and is used to hold BIOS and other persistent settings. Using
-	  this driver it is possible to upgrade BIOS directly from Linux.
-
-	  Say N here unless you know what you are doing. Overwriting the
-	  SPI flash may render the system unbootable.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called intel-spi-pci.
-
-config SPI_INTEL_SPI_PLATFORM
-	tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
-	depends on X86
-	select SPI_INTEL_SPI
-	help
-	  This enables platform support for the Intel PCH/PCU SPI
-	  controller in master mode. This controller is present in modern
-	  Intel hardware and is used to hold BIOS and other persistent
-	  settings. Using this driver it is possible to upgrade BIOS
-	  directly from Linux.
-
-	  Say N here unless you know what you are doing. Overwriting the
-	  SPI flash may render the system unbootable.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called intel-spi-platform.
diff --git a/drivers/mtd/spi-nor/controllers/Makefile b/drivers/mtd/spi-nor/controllers/Makefile
index e7abba491d98..6e2a1dc68466 100644
--- a/drivers/mtd/spi-nor/controllers/Makefile
+++ b/drivers/mtd/spi-nor/controllers/Makefile
@@ -2,6 +2,3 @@
 obj-$(CONFIG_SPI_ASPEED_SMC)	+= aspeed-smc.o
 obj-$(CONFIG_SPI_HISI_SFC)	+= hisi-sfc.o
 obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
-obj-$(CONFIG_SPI_INTEL_SPI)	+= intel-spi.o
-obj-$(CONFIG_SPI_INTEL_SPI_PCI)	+= intel-spi-pci.o
-obj-$(CONFIG_SPI_INTEL_SPI_PLATFORM)	+= intel-spi-platform.o
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.h b/drivers/mtd/spi-nor/controllers/intel-spi.h
deleted file mode 100644
index f2871179fd34..000000000000
--- a/drivers/mtd/spi-nor/controllers/intel-spi.h
+++ /dev/null
@@ -1,21 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Intel PCH/PCU SPI flash driver.
- *
- * Copyright (C) 2016, Intel Corporation
- * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
- */
-
-#ifndef INTEL_SPI_H
-#define INTEL_SPI_H
-
-#include <linux/platform_data/x86/intel-spi.h>
-
-struct intel_spi;
-struct resource;
-
-struct intel_spi *intel_spi_probe(struct device *dev,
-	struct resource *mem, const struct intel_spi_boardinfo *info);
-int intel_spi_remove(struct intel_spi *ispi);
-
-#endif /* INTEL_SPI_H */
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 83e352b0c8f9..8c9d5a119046 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -406,6 +406,44 @@ config SPI_IMX
 	help
 	  This enables support for the Freescale i.MX SPI controllers.
 
+config SPI_INTEL
+	tristate
+
+config SPI_INTEL_PCI
+	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
+	depends on PCI && (X86 || COMPILE_TEST)
+	depends on SPI_MEM
+	select SPI_INTEL
+	help
+	  This enables PCI support for the Intel PCH/PCU SPI controller in
+	  master mode. This controller is present in modern Intel hardware
+	  and is used to hold BIOS and other persistent settings. Using
+	  this driver it is possible to upgrade BIOS directly from Linux.
+
+	  Say N here unless you know what you are doing. Overwriting the
+	  SPI flash may render the system unbootable.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called spi-intel-pci.
+
+config SPI_INTEL_PLATFORM
+	tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
+	depends on X86 || COMPILE_TEST
+	depends on SPI_MEM
+	select SPI_INTEL
+	help
+	  This enables platform support for the Intel PCH/PCU SPI
+	  controller in master mode. This controller is present in modern
+	  Intel hardware and is used to hold BIOS and other persistent
+	  settings. Using this driver it is possible to upgrade BIOS
+	  directly from Linux.
+
+	  Say N here unless you know what you are doing. Overwriting the
+	  SPI flash may render the system unbootable.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called spi-intel-platform.
+
 config SPI_JCORE
 	tristate "J-Core SPI Master"
 	depends on OF && (SUPERH || COMPILE_TEST)
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 699db95c8441..070918a79362 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -59,6 +59,9 @@ obj-$(CONFIG_SPI_HISI_KUNPENG)		+= spi-hisi-kunpeng.o
 obj-$(CONFIG_SPI_HISI_SFC_V3XX)		+= spi-hisi-sfc-v3xx.o
 obj-$(CONFIG_SPI_IMG_SPFI)		+= spi-img-spfi.o
 obj-$(CONFIG_SPI_IMX)			+= spi-imx.o
+obj-$(CONFIG_SPI_INTEL)			+= spi-intel.o
+obj-$(CONFIG_SPI_INTEL_PCI)		+= spi-intel-pci.o
+obj-$(CONFIG_SPI_INTEL_PLATFORM)	+= spi-intel-platform.o
 obj-$(CONFIG_SPI_LANTIQ_SSC)		+= spi-lantiq-ssc.o
 obj-$(CONFIG_SPI_JCORE)			+= spi-jcore.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/spi/spi-intel-pci.c
similarity index 86%
rename from drivers/mtd/spi-nor/controllers/intel-spi-pci.c
rename to drivers/spi/spi-intel-pci.c
index 508f7ca098ef..a177b10bc288 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
+++ b/drivers/spi/spi-intel-pci.c
@@ -2,16 +2,14 @@
 /*
  * Intel PCH/PCU SPI flash PCI driver.
  *
- * Copyright (C) 2016, Intel Corporation
+ * Copyright (C) 2016 - 2021 Intel Corporation
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
-#include <linux/ioport.h>
-#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 
-#include "intel-spi.h"
+#include "spi-intel.h"
 
 #define BCR		0xdc
 #define BCR_WPD		BIT(0)
@@ -46,7 +44,6 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
 			       const struct pci_device_id *id)
 {
 	struct intel_spi_boardinfo *info;
-	struct intel_spi *ispi;
 	int ret;
 
 	ret = pcim_enable_device(pdev);
@@ -59,17 +56,7 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
 		return -ENOMEM;
 
 	info->data = pdev;
-	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
-	if (IS_ERR(ispi))
-		return PTR_ERR(ispi);
-
-	pci_set_drvdata(pdev, ispi);
-	return 0;
-}
-
-static void intel_spi_pci_remove(struct pci_dev *pdev)
-{
-	intel_spi_remove(pci_get_drvdata(pdev));
+	return intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
 }
 
 static const struct pci_device_id intel_spi_pci_ids[] = {
@@ -98,7 +85,6 @@ static struct pci_driver intel_spi_pci_driver = {
 	.name = "intel-spi",
 	.id_table = intel_spi_pci_ids,
 	.probe = intel_spi_pci_probe,
-	.remove = intel_spi_pci_remove,
 };
 
 module_pci_driver(intel_spi_pci_driver);
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-platform.c b/drivers/spi/spi-intel-platform.c
similarity index 65%
rename from drivers/mtd/spi-nor/controllers/intel-spi-platform.c
rename to drivers/spi/spi-intel-platform.c
index f80f1086f928..293e03f239de 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-platform.c
+++ b/drivers/spi/spi-intel-platform.c
@@ -2,20 +2,18 @@
 /*
  * Intel PCH/PCU SPI flash platform driver.
  *
- * Copyright (C) 2016, Intel Corporation
+ * Copyright (C) 2016 - 2021 Intel Corporation
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
-#include <linux/ioport.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
-#include "intel-spi.h"
+#include "spi-intel.h"
 
 static int intel_spi_platform_probe(struct platform_device *pdev)
 {
 	struct intel_spi_boardinfo *info;
-	struct intel_spi *ispi;
 	struct resource *mem;
 
 	info = dev_get_platdata(&pdev->dev);
@@ -23,24 +21,11 @@ static int intel_spi_platform_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	ispi = intel_spi_probe(&pdev->dev, mem, info);
-	if (IS_ERR(ispi))
-		return PTR_ERR(ispi);
-
-	platform_set_drvdata(pdev, ispi);
-	return 0;
-}
-
-static int intel_spi_platform_remove(struct platform_device *pdev)
-{
-	struct intel_spi *ispi = platform_get_drvdata(pdev);
-
-	return intel_spi_remove(ispi);
+	return intel_spi_probe(&pdev->dev, mem, info);
 }
 
 static struct platform_driver intel_spi_platform_driver = {
 	.probe = intel_spi_platform_probe,
-	.remove = intel_spi_platform_remove,
 	.driver = {
 		.name = "intel-spi",
 	},
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/spi/spi-intel.c
similarity index 80%
rename from drivers/mtd/spi-nor/controllers/intel-spi.c
rename to drivers/spi/spi-intel.c
index f35597cbea0c..e299710bafa9 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi.c
+++ b/drivers/spi/spi-intel.c
@@ -2,21 +2,19 @@
 /*
  * Intel PCH/PCU SPI flash driver.
  *
- * Copyright (C) 2016, Intel Corporation
+ * Copyright (C) 2016 - 2021 Intel Corporation
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
-#include <linux/err.h>
-#include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
-#include <linux/sched.h>
-#include <linux/sizes.h>
-#include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/spi-nor.h>
+#include <linux/spi/flash.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
 
-#include "intel-spi.h"
+#include "spi-intel.h"
 
 /* Offsets are from @ispi->base */
 #define BFPREG				0x00
@@ -92,8 +90,6 @@
 /* CPU specifics */
 #define BYT_PR				0x74
 #define BYT_SSFSTS_CTL			0x90
-#define BYT_BCR				0xfc
-#define BYT_BCR_WPD			BIT(0)
 #define BYT_FREG_NUM			5
 #define BYT_PR_NUM			5
 
@@ -125,10 +121,10 @@
  * struct intel_spi - Driver private data
  * @dev: Device pointer
  * @info: Pointer to board specific info
- * @nor: SPI NOR layer structure
  * @base: Beginning of MMIO space
  * @pregs: Start of protection registers
  * @sregs: Start of software sequencer registers
+ * @master: Pointer to the SPI controller structure
  * @nregions: Maximum number of regions
  * @pr_num: Maximum number of protected range registers
  * @locked: Is SPI setting locked
@@ -142,10 +138,10 @@
 struct intel_spi {
 	struct device *dev;
 	const struct intel_spi_boardinfo *info;
-	struct spi_nor nor;
 	void __iomem *base;
 	void __iomem *pregs;
 	void __iomem *sregs;
+	struct spi_controller *master;
 	size_t nregions;
 	size_t pr_num;
 	bool locked;
@@ -199,9 +195,6 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
 			readl(ispi->sregs + OPMENU1));
 	}
 
-	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));
 
@@ -419,7 +412,6 @@ static int intel_spi_init(struct intel_spi *ispi)
 	}
 
 	intel_spi_dump_regs(ispi);
-
 	return 0;
 }
 
@@ -553,10 +545,9 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, size_t len,
 	return 0;
 }
 
-static int intel_spi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
+static int intel_spi_read_reg(struct intel_spi *ispi, u8 opcode, void *buf,
 			      size_t len)
 {
-	struct intel_spi *ispi = nor->priv;
 	int ret;
 
 	/* Address of the first chip */
@@ -574,10 +565,9 @@ static int intel_spi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
 	return intel_spi_read_block(ispi, buf, len);
 }
 
-static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, const u8 *buf,
+static int intel_spi_write_reg(struct intel_spi *ispi, u8 opcode, const void *buf,
 			       size_t len)
 {
-	struct intel_spi *ispi = nor->priv;
 	int ret;
 
 	/*
@@ -632,10 +622,9 @@ static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, const u8 *buf,
 	return intel_spi_hw_cycle(ispi, opcode, len);
 }
 
-static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
-			      u_char *read_buf)
+static ssize_t intel_spi_read(struct intel_spi *ispi, u32 from, size_t len,
+			      void *read_buf)
 {
-	struct intel_spi *ispi = nor->priv;
 	size_t block_size, retlen = 0;
 	u32 val, status;
 	ssize_t ret;
@@ -647,16 +636,6 @@ static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
 	if (WARN_ON_ONCE(ispi->atomic_preopcode))
 		ispi->atomic_preopcode = 0;
 
-	switch (nor->read_opcode) {
-	case SPINOR_OP_READ:
-	case SPINOR_OP_READ_FAST:
-	case SPINOR_OP_READ_4B:
-	case SPINOR_OP_READ_FAST_4B:
-		break;
-	default:
-		return -EINVAL;
-	}
-
 	while (len > 0) {
 		block_size = min_t(size_t, len, INTEL_SPI_FIFO_SZ);
 
@@ -685,7 +664,7 @@ static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
 			ret = -EACCES;
 
 		if (ret < 0) {
-			dev_err(ispi->dev, "read error: %llx: %#x\n", from,
+			dev_err(ispi->dev, "read error: %x: %#x\n", from,
 				status);
 			return ret;
 		}
@@ -703,10 +682,9 @@ static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
 	return retlen;
 }
 
-static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
-			       const u_char *write_buf)
+static ssize_t intel_spi_write(struct intel_spi *ispi, u32 to, size_t len,
+			       const void *write_buf)
 {
-	struct intel_spi *ispi = nor->priv;
 	size_t block_size, retlen = 0;
 	u32 val, status;
 	ssize_t ret;
@@ -752,7 +730,7 @@ static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
 			ret = -EACCES;
 
 		if (ret < 0) {
-			dev_err(ispi->dev, "write error: %llx: %#x\n", to,
+			dev_err(ispi->dev, "write error: %x: %#x\n", to,
 				status);
 			return ret;
 		}
@@ -766,68 +744,150 @@ static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
 	return retlen;
 }
 
-static int intel_spi_erase(struct spi_nor *nor, loff_t offs)
+static int intel_spi_erase(struct intel_spi *ispi, u8 opcode, u32 offs)
 {
-	size_t erase_size, len = nor->mtd.erasesize;
-	struct intel_spi *ispi = nor->priv;
 	u32 val, status, cmd;
 	int ret;
 
-	/* If the hardware can do 64k erase use that when possible */
-	if (len >= SZ_64K && ispi->erase_64k) {
+	switch (opcode) {
+	case SPINOR_OP_SE:
 		cmd = HSFSTS_CTL_FCYCLE_ERASE_64K;
-		erase_size = SZ_64K;
-	} else {
+		break;
+
+	case SPINOR_OP_BE_4K:
 		cmd = HSFSTS_CTL_FCYCLE_ERASE;
-		erase_size = SZ_4K;
+		break;
+
+	default:
+		return -EINVAL;
 	}
 
+	writel(offs, ispi->base + FADDR);
+
 	if (ispi->swseq_erase) {
-		while (len > 0) {
-			writel(offs, ispi->base + FADDR);
+		ret = intel_spi_sw_cycle(ispi, opcode, 0,
+					 OPTYPE_WRITE_WITH_ADDR);
+		return ret ? ret : 0;
+	}
+
+	/* Not needed with HW sequencer erase, make sure it is cleared */
+	ispi->atomic_preopcode = 0;
+
+	val = readl(ispi->base + HSFSTS_CTL);
+	val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
+	val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
+	val |= cmd;
+	val |= HSFSTS_CTL_FGO;
+	writel(val, ispi->base + HSFSTS_CTL);
+
+	ret = intel_spi_wait_hw_busy(ispi);
+	if (ret)
+		return ret;
+
+	status = readl(ispi->base + HSFSTS_CTL);
+	if (status & HSFSTS_CTL_FCERR)
+		return -EIO;
+	else if (status & HSFSTS_CTL_AEL)
+		return -EACCES;
+
+	return 0;
+}
 
-			ret = intel_spi_sw_cycle(ispi, nor->erase_opcode,
-						 0, OPTYPE_WRITE_WITH_ADDR);
-			if (ret)
-				return ret;
+static bool intel_spi_supports_mem_op(struct spi_mem *mem,
+				      const struct spi_mem_op *op)
+{
+	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
 
-			offs += erase_size;
-			len -= erase_size;
+	if (op->cmd.dtr)
+		return false;
+
+	if (ispi->swseq_reg && ispi->locked) {
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++) {
+			if (ispi->opcodes[i] == op->cmd.opcode)
+				return true;
 		}
 
-		return 0;
+		return false;
 	}
 
-	/* Not needed with HW sequencer erase, make sure it is cleared */
-	ispi->atomic_preopcode = 0;
+	switch (op->cmd.opcode) {
+	case SPINOR_OP_RDID:
+	case SPINOR_OP_RDSR:
+	case SPINOR_OP_WRSR:
+		return true;
 
-	while (len > 0) {
-		writel(offs, ispi->base + FADDR);
+	case SPINOR_OP_READ:
+	case SPINOR_OP_READ_FAST:
+	case SPINOR_OP_READ_4B:
+	case SPINOR_OP_READ_FAST_4B:
+		return true;
 
-		val = readl(ispi->base + HSFSTS_CTL);
-		val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
-		val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
-		val |= cmd;
-		val |= HSFSTS_CTL_FGO;
-		writel(val, ispi->base + HSFSTS_CTL);
+	case SPINOR_OP_PP:
+	case SPINOR_OP_PP_4B:
+	case SPINOR_OP_WREN:
+	case SPINOR_OP_WRDI:
+		return true;
 
-		ret = intel_spi_wait_hw_busy(ispi);
-		if (ret)
-			return ret;
+	case SPINOR_OP_SE:
+	case SPINOR_OP_SE_4B:
+		return ispi->erase_64k;
 
-		status = readl(ispi->base + HSFSTS_CTL);
-		if (status & HSFSTS_CTL_FCERR)
-			return -EIO;
-		else if (status & HSFSTS_CTL_AEL)
-			return -EACCES;
+	case SPINOR_OP_BE_4K:
+	case SPINOR_OP_BE_4K_4B:
+		return true;
+	}
+
+	return false;
+}
 
-		offs += erase_size;
-		len -= erase_size;
+static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
+	size_t nbytes = op->data.nbytes;
+	u8 opcode = op->cmd.opcode;
+
+	if (op->addr.nbytes) {
+		if  (op->data.dir == SPI_MEM_DATA_IN)
+			return intel_spi_read(ispi, op->addr.val, nbytes,
+					      op->data.buf.in);
+		else if (op->data.dir == SPI_MEM_DATA_OUT)
+			return intel_spi_write(ispi, op->addr.val, nbytes,
+					       op->data.buf.out);
+		else if (op->data.dir == SPI_MEM_NO_DATA)
+			return intel_spi_erase(ispi, opcode, op->addr.val);
+	} else {
+		if  (op->data.dir == SPI_MEM_DATA_IN)
+			return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
+						  nbytes);
+		else if (op->data.dir == SPI_MEM_DATA_OUT)
+			return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
+						   nbytes);
+		else if (op->data.dir == SPI_MEM_NO_DATA)
+			return intel_spi_write_reg(ispi, opcode, NULL, 0);
 	}
 
-	return 0;
+	return -EINVAL;
+}
+
+static const char *intel_spi_get_name(struct spi_mem *mem)
+{
+	const struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
+
+	/*
+	 * Return name of the flash controller device to be compatible
+	 * with the MTD version.
+	 */
+	return dev_name(ispi->dev);
 }
 
+static const struct spi_controller_mem_ops intel_spi_mem_ops = {
+	.supports_op = intel_spi_supports_mem_op,
+	.exec_op = intel_spi_exec_mem_op,
+	.get_name = intel_spi_get_name,
+};
+
 static bool intel_spi_is_protected(const struct intel_spi *ispi,
 				   unsigned int base, unsigned int limit)
 {
@@ -896,70 +956,74 @@ static void intel_spi_fill_partition(struct intel_spi *ispi,
 	}
 }
 
-static const struct spi_nor_controller_ops intel_spi_controller_ops = {
-	.read_reg = intel_spi_read_reg,
-	.write_reg = intel_spi_write_reg,
-	.read = intel_spi_read,
-	.write = intel_spi_write,
-	.erase = intel_spi_erase,
-};
+static int intel_spi_populate_chip(struct intel_spi *ispi)
+{
+	struct flash_platform_data *pdata;
+	struct spi_board_info chip;
+
+	pdata = devm_kzalloc(ispi->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->nr_parts = 1;
+	pdata->parts = devm_kcalloc(ispi->dev, sizeof(*pdata->parts),
+				    pdata->nr_parts, GFP_KERNEL);
+	if (!pdata->parts)
+		return -ENOMEM;
+
+	intel_spi_fill_partition(ispi, pdata->parts);
+
+	memset(&chip, 0, sizeof(chip));
+	snprintf(chip.modalias, 8, "spi-nor");
+	chip.platform_data = pdata;
+
+	return spi_new_device(ispi->master, &chip) ? 0 : -ENODEV;
+}
 
-struct intel_spi *intel_spi_probe(struct device *dev,
-	struct resource *mem, const struct intel_spi_boardinfo *info)
+/**
+ * intel_spi_probe() - Probe the Intel SPI flash controller
+ * @dev: Pointer to the parent device
+ * @mem: MMIO resource
+ * @info: Platform spefific information
+ *
+ * Probes Intel SPI flash controller and creates the flash chip device.
+ * Returns %0 on success and negative errno in case of failure.
+ */
+int intel_spi_probe(struct device *dev, struct resource *mem,
+		    const struct intel_spi_boardinfo *info)
 {
-	const struct spi_nor_hwcaps hwcaps = {
-		.mask = SNOR_HWCAPS_READ |
-			SNOR_HWCAPS_READ_FAST |
-			SNOR_HWCAPS_PP,
-	};
-	struct mtd_partition part;
+	struct spi_controller *master;
 	struct intel_spi *ispi;
 	int ret;
 
-	if (!info || !mem)
-		return ERR_PTR(-EINVAL);
+	master = devm_spi_alloc_master(dev, sizeof(*ispi));
+	if (!master)
+		return -ENOMEM;
+
+	master->mem_ops = &intel_spi_mem_ops;
 
-	ispi = devm_kzalloc(dev, sizeof(*ispi), GFP_KERNEL);
-	if (!ispi)
-		return ERR_PTR(-ENOMEM);
+	ispi = spi_master_get_devdata(master);
 
 	ispi->base = devm_ioremap_resource(dev, mem);
 	if (IS_ERR(ispi->base))
-		return ERR_CAST(ispi->base);
+		return PTR_ERR(ispi->base);
 
 	ispi->dev = dev;
+	ispi->master = master;
 	ispi->info = info;
 
 	ret = intel_spi_init(ispi);
 	if (ret)
-		return ERR_PTR(ret);
-
-	ispi->nor.dev = ispi->dev;
-	ispi->nor.priv = ispi;
-	ispi->nor.controller_ops = &intel_spi_controller_ops;
-
-	ret = spi_nor_scan(&ispi->nor, NULL, &hwcaps);
-	if (ret) {
-		dev_info(dev, "failed to locate the chip\n");
-		return ERR_PTR(ret);
-	}
-
-	intel_spi_fill_partition(ispi, &part);
+		return ret;
 
-	ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
+	ret = devm_spi_register_master(dev, master);
 	if (ret)
-		return ERR_PTR(ret);
+		return ret;
 
-	return ispi;
+	return intel_spi_populate_chip(ispi);
 }
 EXPORT_SYMBOL_GPL(intel_spi_probe);
 
-int intel_spi_remove(struct intel_spi *ispi)
-{
-	return mtd_device_unregister(&ispi->nor.mtd);
-}
-EXPORT_SYMBOL_GPL(intel_spi_remove);
-
 MODULE_DESCRIPTION("Intel PCH/PCU SPI flash core driver");
 MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/spi/spi-intel.h b/drivers/spi/spi-intel.h
new file mode 100644
index 000000000000..40f947f6941a
--- /dev/null
+++ b/drivers/spi/spi-intel.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Intel PCH/PCU SPI flash driver.
+ *
+ * Copyright (C) 2016 - 2021, Intel Corporation
+ * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
+ */
+
+#ifndef SPI_INTEL_H
+#define SPI_INTEL_H
+
+#include <linux/platform_data/x86/spi-intel.h>
+
+struct resource;
+
+int intel_spi_probe(struct device *dev, struct resource *mem,
+		    const struct intel_spi_boardinfo *info);
+
+#endif /* SPI_INTEL_H */
diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
index 39967a5eca6d..ea4a4b1b246a 100644
--- a/include/linux/mfd/lpc_ich.h
+++ b/include/linux/mfd/lpc_ich.h
@@ -8,7 +8,7 @@
 #ifndef LPC_ICH_H
 #define LPC_ICH_H
 
-#include <linux/platform_data/x86/intel-spi.h>
+#include <linux/platform_data/x86/spi-intel.h>
 
 /* GPIO resources */
 #define ICH_RES_GPIO	0
diff --git a/include/linux/platform_data/x86/intel-spi.h b/include/linux/platform_data/x86/spi-intel.h
similarity index 89%
rename from include/linux/platform_data/x86/intel-spi.h
rename to include/linux/platform_data/x86/spi-intel.h
index 7dda3f690465..a512ec37abbb 100644
--- a/include/linux/platform_data/x86/intel-spi.h
+++ b/include/linux/platform_data/x86/spi-intel.h
@@ -6,8 +6,8 @@
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
-#ifndef INTEL_SPI_PDATA_H
-#define INTEL_SPI_PDATA_H
+#ifndef SPI_INTEL_PDATA_H
+#define SPI_INTEL_PDATA_H
 
 enum intel_spi_type {
 	INTEL_SPI_BYT = 1,
@@ -28,4 +28,4 @@ struct intel_spi_boardinfo {
 	void *data;
 };
 
-#endif /* INTEL_SPI_PDATA_H */
+#endif /* SPI_INTEL_PDATA_H */
-- 
2.33.0

[PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

The preferred way to implement SPI-NOR controller drivers is through SPI
subsubsystem utilizing the SPI MEM core functions. This converts the
Intel SPI flash controller driver over the SPI MEM by moving the driver
from SPI-NOR subsystem to SPI subsystem and in one go make it use the
SPI MEM functions. The driver name will be changed from intel-spi to
spi-intel to match the convention used in the SPI subsystem.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/mtd/spi-nor/controllers/Kconfig       |  36 ---
 drivers/mtd/spi-nor/controllers/Makefile      |   3 -
 drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
 drivers/spi/Kconfig                           |  38 +++
 drivers/spi/Makefile                          |   3 +
 .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  20 +-
 .../spi-intel-platform.c}                     |  21 +-
 .../intel-spi.c => spi/spi-intel.c}           | 300 +++++++++++-------
 drivers/spi/spi-intel.h                       |  19 ++
 include/linux/mfd/lpc_ich.h                   |   2 +-
 .../x86/{intel-spi.h => spi-intel.h}          |   6 +-
 11 files changed, 252 insertions(+), 217 deletions(-)
 delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
 rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (86%)
 rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
 rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (80%)
 create mode 100644 drivers/spi/spi-intel.h
 rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (89%)

diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
index 5c0e0ec2e6d1..50f4f3484d42 100644
--- a/drivers/mtd/spi-nor/controllers/Kconfig
+++ b/drivers/mtd/spi-nor/controllers/Kconfig
@@ -26,39 +26,3 @@ config SPI_NXP_SPIFI
 	  SPIFI is a specialized controller for connecting serial SPI
 	  Flash. Enable this option if you have a device with a SPIFI
 	  controller and want to access the Flash as a mtd device.
-
-config SPI_INTEL_SPI
-	tristate
-
-config SPI_INTEL_SPI_PCI
-	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
-	depends on X86 && PCI
-	select SPI_INTEL_SPI
-	help
-	  This enables PCI support for the Intel PCH/PCU SPI controller in
-	  master mode. This controller is present in modern Intel hardware
-	  and is used to hold BIOS and other persistent settings. Using
-	  this driver it is possible to upgrade BIOS directly from Linux.
-
-	  Say N here unless you know what you are doing. Overwriting the
-	  SPI flash may render the system unbootable.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called intel-spi-pci.
-
-config SPI_INTEL_SPI_PLATFORM
-	tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
-	depends on X86
-	select SPI_INTEL_SPI
-	help
-	  This enables platform support for the Intel PCH/PCU SPI
-	  controller in master mode. This controller is present in modern
-	  Intel hardware and is used to hold BIOS and other persistent
-	  settings. Using this driver it is possible to upgrade BIOS
-	  directly from Linux.
-
-	  Say N here unless you know what you are doing. Overwriting the
-	  SPI flash may render the system unbootable.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called intel-spi-platform.
diff --git a/drivers/mtd/spi-nor/controllers/Makefile b/drivers/mtd/spi-nor/controllers/Makefile
index e7abba491d98..6e2a1dc68466 100644
--- a/drivers/mtd/spi-nor/controllers/Makefile
+++ b/drivers/mtd/spi-nor/controllers/Makefile
@@ -2,6 +2,3 @@
 obj-$(CONFIG_SPI_ASPEED_SMC)	+= aspeed-smc.o
 obj-$(CONFIG_SPI_HISI_SFC)	+= hisi-sfc.o
 obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
-obj-$(CONFIG_SPI_INTEL_SPI)	+= intel-spi.o
-obj-$(CONFIG_SPI_INTEL_SPI_PCI)	+= intel-spi-pci.o
-obj-$(CONFIG_SPI_INTEL_SPI_PLATFORM)	+= intel-spi-platform.o
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.h b/drivers/mtd/spi-nor/controllers/intel-spi.h
deleted file mode 100644
index f2871179fd34..000000000000
--- a/drivers/mtd/spi-nor/controllers/intel-spi.h
+++ /dev/null
@@ -1,21 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Intel PCH/PCU SPI flash driver.
- *
- * Copyright (C) 2016, Intel Corporation
- * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
- */
-
-#ifndef INTEL_SPI_H
-#define INTEL_SPI_H
-
-#include <linux/platform_data/x86/intel-spi.h>
-
-struct intel_spi;
-struct resource;
-
-struct intel_spi *intel_spi_probe(struct device *dev,
-	struct resource *mem, const struct intel_spi_boardinfo *info);
-int intel_spi_remove(struct intel_spi *ispi);
-
-#endif /* INTEL_SPI_H */
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 83e352b0c8f9..8c9d5a119046 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -406,6 +406,44 @@ config SPI_IMX
 	help
 	  This enables support for the Freescale i.MX SPI controllers.
 
+config SPI_INTEL
+	tristate
+
+config SPI_INTEL_PCI
+	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
+	depends on PCI && (X86 || COMPILE_TEST)
+	depends on SPI_MEM
+	select SPI_INTEL
+	help
+	  This enables PCI support for the Intel PCH/PCU SPI controller in
+	  master mode. This controller is present in modern Intel hardware
+	  and is used to hold BIOS and other persistent settings. Using
+	  this driver it is possible to upgrade BIOS directly from Linux.
+
+	  Say N here unless you know what you are doing. Overwriting the
+	  SPI flash may render the system unbootable.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called spi-intel-pci.
+
+config SPI_INTEL_PLATFORM
+	tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
+	depends on X86 || COMPILE_TEST
+	depends on SPI_MEM
+	select SPI_INTEL
+	help
+	  This enables platform support for the Intel PCH/PCU SPI
+	  controller in master mode. This controller is present in modern
+	  Intel hardware and is used to hold BIOS and other persistent
+	  settings. Using this driver it is possible to upgrade BIOS
+	  directly from Linux.
+
+	  Say N here unless you know what you are doing. Overwriting the
+	  SPI flash may render the system unbootable.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called spi-intel-platform.
+
 config SPI_JCORE
 	tristate "J-Core SPI Master"
 	depends on OF && (SUPERH || COMPILE_TEST)
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 699db95c8441..070918a79362 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -59,6 +59,9 @@ obj-$(CONFIG_SPI_HISI_KUNPENG)		+= spi-hisi-kunpeng.o
 obj-$(CONFIG_SPI_HISI_SFC_V3XX)		+= spi-hisi-sfc-v3xx.o
 obj-$(CONFIG_SPI_IMG_SPFI)		+= spi-img-spfi.o
 obj-$(CONFIG_SPI_IMX)			+= spi-imx.o
+obj-$(CONFIG_SPI_INTEL)			+= spi-intel.o
+obj-$(CONFIG_SPI_INTEL_PCI)		+= spi-intel-pci.o
+obj-$(CONFIG_SPI_INTEL_PLATFORM)	+= spi-intel-platform.o
 obj-$(CONFIG_SPI_LANTIQ_SSC)		+= spi-lantiq-ssc.o
 obj-$(CONFIG_SPI_JCORE)			+= spi-jcore.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/spi/spi-intel-pci.c
similarity index 86%
rename from drivers/mtd/spi-nor/controllers/intel-spi-pci.c
rename to drivers/spi/spi-intel-pci.c
index 508f7ca098ef..a177b10bc288 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
+++ b/drivers/spi/spi-intel-pci.c
@@ -2,16 +2,14 @@
 /*
  * Intel PCH/PCU SPI flash PCI driver.
  *
- * Copyright (C) 2016, Intel Corporation
+ * Copyright (C) 2016 - 2021 Intel Corporation
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
-#include <linux/ioport.h>
-#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 
-#include "intel-spi.h"
+#include "spi-intel.h"
 
 #define BCR		0xdc
 #define BCR_WPD		BIT(0)
@@ -46,7 +44,6 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
 			       const struct pci_device_id *id)
 {
 	struct intel_spi_boardinfo *info;
-	struct intel_spi *ispi;
 	int ret;
 
 	ret = pcim_enable_device(pdev);
@@ -59,17 +56,7 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
 		return -ENOMEM;
 
 	info->data = pdev;
-	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
-	if (IS_ERR(ispi))
-		return PTR_ERR(ispi);
-
-	pci_set_drvdata(pdev, ispi);
-	return 0;
-}
-
-static void intel_spi_pci_remove(struct pci_dev *pdev)
-{
-	intel_spi_remove(pci_get_drvdata(pdev));
+	return intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
 }
 
 static const struct pci_device_id intel_spi_pci_ids[] = {
@@ -98,7 +85,6 @@ static struct pci_driver intel_spi_pci_driver = {
 	.name = "intel-spi",
 	.id_table = intel_spi_pci_ids,
 	.probe = intel_spi_pci_probe,
-	.remove = intel_spi_pci_remove,
 };
 
 module_pci_driver(intel_spi_pci_driver);
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-platform.c b/drivers/spi/spi-intel-platform.c
similarity index 65%
rename from drivers/mtd/spi-nor/controllers/intel-spi-platform.c
rename to drivers/spi/spi-intel-platform.c
index f80f1086f928..293e03f239de 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-platform.c
+++ b/drivers/spi/spi-intel-platform.c
@@ -2,20 +2,18 @@
 /*
  * Intel PCH/PCU SPI flash platform driver.
  *
- * Copyright (C) 2016, Intel Corporation
+ * Copyright (C) 2016 - 2021 Intel Corporation
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
-#include <linux/ioport.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
-#include "intel-spi.h"
+#include "spi-intel.h"
 
 static int intel_spi_platform_probe(struct platform_device *pdev)
 {
 	struct intel_spi_boardinfo *info;
-	struct intel_spi *ispi;
 	struct resource *mem;
 
 	info = dev_get_platdata(&pdev->dev);
@@ -23,24 +21,11 @@ static int intel_spi_platform_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	ispi = intel_spi_probe(&pdev->dev, mem, info);
-	if (IS_ERR(ispi))
-		return PTR_ERR(ispi);
-
-	platform_set_drvdata(pdev, ispi);
-	return 0;
-}
-
-static int intel_spi_platform_remove(struct platform_device *pdev)
-{
-	struct intel_spi *ispi = platform_get_drvdata(pdev);
-
-	return intel_spi_remove(ispi);
+	return intel_spi_probe(&pdev->dev, mem, info);
 }
 
 static struct platform_driver intel_spi_platform_driver = {
 	.probe = intel_spi_platform_probe,
-	.remove = intel_spi_platform_remove,
 	.driver = {
 		.name = "intel-spi",
 	},
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/spi/spi-intel.c
similarity index 80%
rename from drivers/mtd/spi-nor/controllers/intel-spi.c
rename to drivers/spi/spi-intel.c
index f35597cbea0c..e299710bafa9 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi.c
+++ b/drivers/spi/spi-intel.c
@@ -2,21 +2,19 @@
 /*
  * Intel PCH/PCU SPI flash driver.
  *
- * Copyright (C) 2016, Intel Corporation
+ * Copyright (C) 2016 - 2021 Intel Corporation
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
-#include <linux/err.h>
-#include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
-#include <linux/sched.h>
-#include <linux/sizes.h>
-#include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/spi-nor.h>
+#include <linux/spi/flash.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
 
-#include "intel-spi.h"
+#include "spi-intel.h"
 
 /* Offsets are from @ispi->base */
 #define BFPREG				0x00
@@ -92,8 +90,6 @@
 /* CPU specifics */
 #define BYT_PR				0x74
 #define BYT_SSFSTS_CTL			0x90
-#define BYT_BCR				0xfc
-#define BYT_BCR_WPD			BIT(0)
 #define BYT_FREG_NUM			5
 #define BYT_PR_NUM			5
 
@@ -125,10 +121,10 @@
  * struct intel_spi - Driver private data
  * @dev: Device pointer
  * @info: Pointer to board specific info
- * @nor: SPI NOR layer structure
  * @base: Beginning of MMIO space
  * @pregs: Start of protection registers
  * @sregs: Start of software sequencer registers
+ * @master: Pointer to the SPI controller structure
  * @nregions: Maximum number of regions
  * @pr_num: Maximum number of protected range registers
  * @locked: Is SPI setting locked
@@ -142,10 +138,10 @@
 struct intel_spi {
 	struct device *dev;
 	const struct intel_spi_boardinfo *info;
-	struct spi_nor nor;
 	void __iomem *base;
 	void __iomem *pregs;
 	void __iomem *sregs;
+	struct spi_controller *master;
 	size_t nregions;
 	size_t pr_num;
 	bool locked;
@@ -199,9 +195,6 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
 			readl(ispi->sregs + OPMENU1));
 	}
 
-	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));
 
@@ -419,7 +412,6 @@ static int intel_spi_init(struct intel_spi *ispi)
 	}
 
 	intel_spi_dump_regs(ispi);
-
 	return 0;
 }
 
@@ -553,10 +545,9 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, size_t len,
 	return 0;
 }
 
-static int intel_spi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
+static int intel_spi_read_reg(struct intel_spi *ispi, u8 opcode, void *buf,
 			      size_t len)
 {
-	struct intel_spi *ispi = nor->priv;
 	int ret;
 
 	/* Address of the first chip */
@@ -574,10 +565,9 @@ static int intel_spi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
 	return intel_spi_read_block(ispi, buf, len);
 }
 
-static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, const u8 *buf,
+static int intel_spi_write_reg(struct intel_spi *ispi, u8 opcode, const void *buf,
 			       size_t len)
 {
-	struct intel_spi *ispi = nor->priv;
 	int ret;
 
 	/*
@@ -632,10 +622,9 @@ static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, const u8 *buf,
 	return intel_spi_hw_cycle(ispi, opcode, len);
 }
 
-static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
-			      u_char *read_buf)
+static ssize_t intel_spi_read(struct intel_spi *ispi, u32 from, size_t len,
+			      void *read_buf)
 {
-	struct intel_spi *ispi = nor->priv;
 	size_t block_size, retlen = 0;
 	u32 val, status;
 	ssize_t ret;
@@ -647,16 +636,6 @@ static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
 	if (WARN_ON_ONCE(ispi->atomic_preopcode))
 		ispi->atomic_preopcode = 0;
 
-	switch (nor->read_opcode) {
-	case SPINOR_OP_READ:
-	case SPINOR_OP_READ_FAST:
-	case SPINOR_OP_READ_4B:
-	case SPINOR_OP_READ_FAST_4B:
-		break;
-	default:
-		return -EINVAL;
-	}
-
 	while (len > 0) {
 		block_size = min_t(size_t, len, INTEL_SPI_FIFO_SZ);
 
@@ -685,7 +664,7 @@ static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
 			ret = -EACCES;
 
 		if (ret < 0) {
-			dev_err(ispi->dev, "read error: %llx: %#x\n", from,
+			dev_err(ispi->dev, "read error: %x: %#x\n", from,
 				status);
 			return ret;
 		}
@@ -703,10 +682,9 @@ static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
 	return retlen;
 }
 
-static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
-			       const u_char *write_buf)
+static ssize_t intel_spi_write(struct intel_spi *ispi, u32 to, size_t len,
+			       const void *write_buf)
 {
-	struct intel_spi *ispi = nor->priv;
 	size_t block_size, retlen = 0;
 	u32 val, status;
 	ssize_t ret;
@@ -752,7 +730,7 @@ static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
 			ret = -EACCES;
 
 		if (ret < 0) {
-			dev_err(ispi->dev, "write error: %llx: %#x\n", to,
+			dev_err(ispi->dev, "write error: %x: %#x\n", to,
 				status);
 			return ret;
 		}
@@ -766,68 +744,150 @@ static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
 	return retlen;
 }
 
-static int intel_spi_erase(struct spi_nor *nor, loff_t offs)
+static int intel_spi_erase(struct intel_spi *ispi, u8 opcode, u32 offs)
 {
-	size_t erase_size, len = nor->mtd.erasesize;
-	struct intel_spi *ispi = nor->priv;
 	u32 val, status, cmd;
 	int ret;
 
-	/* If the hardware can do 64k erase use that when possible */
-	if (len >= SZ_64K && ispi->erase_64k) {
+	switch (opcode) {
+	case SPINOR_OP_SE:
 		cmd = HSFSTS_CTL_FCYCLE_ERASE_64K;
-		erase_size = SZ_64K;
-	} else {
+		break;
+
+	case SPINOR_OP_BE_4K:
 		cmd = HSFSTS_CTL_FCYCLE_ERASE;
-		erase_size = SZ_4K;
+		break;
+
+	default:
+		return -EINVAL;
 	}
 
+	writel(offs, ispi->base + FADDR);
+
 	if (ispi->swseq_erase) {
-		while (len > 0) {
-			writel(offs, ispi->base + FADDR);
+		ret = intel_spi_sw_cycle(ispi, opcode, 0,
+					 OPTYPE_WRITE_WITH_ADDR);
+		return ret ? ret : 0;
+	}
+
+	/* Not needed with HW sequencer erase, make sure it is cleared */
+	ispi->atomic_preopcode = 0;
+
+	val = readl(ispi->base + HSFSTS_CTL);
+	val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
+	val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
+	val |= cmd;
+	val |= HSFSTS_CTL_FGO;
+	writel(val, ispi->base + HSFSTS_CTL);
+
+	ret = intel_spi_wait_hw_busy(ispi);
+	if (ret)
+		return ret;
+
+	status = readl(ispi->base + HSFSTS_CTL);
+	if (status & HSFSTS_CTL_FCERR)
+		return -EIO;
+	else if (status & HSFSTS_CTL_AEL)
+		return -EACCES;
+
+	return 0;
+}
 
-			ret = intel_spi_sw_cycle(ispi, nor->erase_opcode,
-						 0, OPTYPE_WRITE_WITH_ADDR);
-			if (ret)
-				return ret;
+static bool intel_spi_supports_mem_op(struct spi_mem *mem,
+				      const struct spi_mem_op *op)
+{
+	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
 
-			offs += erase_size;
-			len -= erase_size;
+	if (op->cmd.dtr)
+		return false;
+
+	if (ispi->swseq_reg && ispi->locked) {
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++) {
+			if (ispi->opcodes[i] == op->cmd.opcode)
+				return true;
 		}
 
-		return 0;
+		return false;
 	}
 
-	/* Not needed with HW sequencer erase, make sure it is cleared */
-	ispi->atomic_preopcode = 0;
+	switch (op->cmd.opcode) {
+	case SPINOR_OP_RDID:
+	case SPINOR_OP_RDSR:
+	case SPINOR_OP_WRSR:
+		return true;
 
-	while (len > 0) {
-		writel(offs, ispi->base + FADDR);
+	case SPINOR_OP_READ:
+	case SPINOR_OP_READ_FAST:
+	case SPINOR_OP_READ_4B:
+	case SPINOR_OP_READ_FAST_4B:
+		return true;
 
-		val = readl(ispi->base + HSFSTS_CTL);
-		val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
-		val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
-		val |= cmd;
-		val |= HSFSTS_CTL_FGO;
-		writel(val, ispi->base + HSFSTS_CTL);
+	case SPINOR_OP_PP:
+	case SPINOR_OP_PP_4B:
+	case SPINOR_OP_WREN:
+	case SPINOR_OP_WRDI:
+		return true;
 
-		ret = intel_spi_wait_hw_busy(ispi);
-		if (ret)
-			return ret;
+	case SPINOR_OP_SE:
+	case SPINOR_OP_SE_4B:
+		return ispi->erase_64k;
 
-		status = readl(ispi->base + HSFSTS_CTL);
-		if (status & HSFSTS_CTL_FCERR)
-			return -EIO;
-		else if (status & HSFSTS_CTL_AEL)
-			return -EACCES;
+	case SPINOR_OP_BE_4K:
+	case SPINOR_OP_BE_4K_4B:
+		return true;
+	}
+
+	return false;
+}
 
-		offs += erase_size;
-		len -= erase_size;
+static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
+	size_t nbytes = op->data.nbytes;
+	u8 opcode = op->cmd.opcode;
+
+	if (op->addr.nbytes) {
+		if  (op->data.dir == SPI_MEM_DATA_IN)
+			return intel_spi_read(ispi, op->addr.val, nbytes,
+					      op->data.buf.in);
+		else if (op->data.dir == SPI_MEM_DATA_OUT)
+			return intel_spi_write(ispi, op->addr.val, nbytes,
+					       op->data.buf.out);
+		else if (op->data.dir == SPI_MEM_NO_DATA)
+			return intel_spi_erase(ispi, opcode, op->addr.val);
+	} else {
+		if  (op->data.dir == SPI_MEM_DATA_IN)
+			return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
+						  nbytes);
+		else if (op->data.dir == SPI_MEM_DATA_OUT)
+			return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
+						   nbytes);
+		else if (op->data.dir == SPI_MEM_NO_DATA)
+			return intel_spi_write_reg(ispi, opcode, NULL, 0);
 	}
 
-	return 0;
+	return -EINVAL;
+}
+
+static const char *intel_spi_get_name(struct spi_mem *mem)
+{
+	const struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
+
+	/*
+	 * Return name of the flash controller device to be compatible
+	 * with the MTD version.
+	 */
+	return dev_name(ispi->dev);
 }
 
+static const struct spi_controller_mem_ops intel_spi_mem_ops = {
+	.supports_op = intel_spi_supports_mem_op,
+	.exec_op = intel_spi_exec_mem_op,
+	.get_name = intel_spi_get_name,
+};
+
 static bool intel_spi_is_protected(const struct intel_spi *ispi,
 				   unsigned int base, unsigned int limit)
 {
@@ -896,70 +956,74 @@ static void intel_spi_fill_partition(struct intel_spi *ispi,
 	}
 }
 
-static const struct spi_nor_controller_ops intel_spi_controller_ops = {
-	.read_reg = intel_spi_read_reg,
-	.write_reg = intel_spi_write_reg,
-	.read = intel_spi_read,
-	.write = intel_spi_write,
-	.erase = intel_spi_erase,
-};
+static int intel_spi_populate_chip(struct intel_spi *ispi)
+{
+	struct flash_platform_data *pdata;
+	struct spi_board_info chip;
+
+	pdata = devm_kzalloc(ispi->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->nr_parts = 1;
+	pdata->parts = devm_kcalloc(ispi->dev, sizeof(*pdata->parts),
+				    pdata->nr_parts, GFP_KERNEL);
+	if (!pdata->parts)
+		return -ENOMEM;
+
+	intel_spi_fill_partition(ispi, pdata->parts);
+
+	memset(&chip, 0, sizeof(chip));
+	snprintf(chip.modalias, 8, "spi-nor");
+	chip.platform_data = pdata;
+
+	return spi_new_device(ispi->master, &chip) ? 0 : -ENODEV;
+}
 
-struct intel_spi *intel_spi_probe(struct device *dev,
-	struct resource *mem, const struct intel_spi_boardinfo *info)
+/**
+ * intel_spi_probe() - Probe the Intel SPI flash controller
+ * @dev: Pointer to the parent device
+ * @mem: MMIO resource
+ * @info: Platform spefific information
+ *
+ * Probes Intel SPI flash controller and creates the flash chip device.
+ * Returns %0 on success and negative errno in case of failure.
+ */
+int intel_spi_probe(struct device *dev, struct resource *mem,
+		    const struct intel_spi_boardinfo *info)
 {
-	const struct spi_nor_hwcaps hwcaps = {
-		.mask = SNOR_HWCAPS_READ |
-			SNOR_HWCAPS_READ_FAST |
-			SNOR_HWCAPS_PP,
-	};
-	struct mtd_partition part;
+	struct spi_controller *master;
 	struct intel_spi *ispi;
 	int ret;
 
-	if (!info || !mem)
-		return ERR_PTR(-EINVAL);
+	master = devm_spi_alloc_master(dev, sizeof(*ispi));
+	if (!master)
+		return -ENOMEM;
+
+	master->mem_ops = &intel_spi_mem_ops;
 
-	ispi = devm_kzalloc(dev, sizeof(*ispi), GFP_KERNEL);
-	if (!ispi)
-		return ERR_PTR(-ENOMEM);
+	ispi = spi_master_get_devdata(master);
 
 	ispi->base = devm_ioremap_resource(dev, mem);
 	if (IS_ERR(ispi->base))
-		return ERR_CAST(ispi->base);
+		return PTR_ERR(ispi->base);
 
 	ispi->dev = dev;
+	ispi->master = master;
 	ispi->info = info;
 
 	ret = intel_spi_init(ispi);
 	if (ret)
-		return ERR_PTR(ret);
-
-	ispi->nor.dev = ispi->dev;
-	ispi->nor.priv = ispi;
-	ispi->nor.controller_ops = &intel_spi_controller_ops;
-
-	ret = spi_nor_scan(&ispi->nor, NULL, &hwcaps);
-	if (ret) {
-		dev_info(dev, "failed to locate the chip\n");
-		return ERR_PTR(ret);
-	}
-
-	intel_spi_fill_partition(ispi, &part);
+		return ret;
 
-	ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
+	ret = devm_spi_register_master(dev, master);
 	if (ret)
-		return ERR_PTR(ret);
+		return ret;
 
-	return ispi;
+	return intel_spi_populate_chip(ispi);
 }
 EXPORT_SYMBOL_GPL(intel_spi_probe);
 
-int intel_spi_remove(struct intel_spi *ispi)
-{
-	return mtd_device_unregister(&ispi->nor.mtd);
-}
-EXPORT_SYMBOL_GPL(intel_spi_remove);
-
 MODULE_DESCRIPTION("Intel PCH/PCU SPI flash core driver");
 MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/spi/spi-intel.h b/drivers/spi/spi-intel.h
new file mode 100644
index 000000000000..40f947f6941a
--- /dev/null
+++ b/drivers/spi/spi-intel.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Intel PCH/PCU SPI flash driver.
+ *
+ * Copyright (C) 2016 - 2021, Intel Corporation
+ * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
+ */
+
+#ifndef SPI_INTEL_H
+#define SPI_INTEL_H
+
+#include <linux/platform_data/x86/spi-intel.h>
+
+struct resource;
+
+int intel_spi_probe(struct device *dev, struct resource *mem,
+		    const struct intel_spi_boardinfo *info);
+
+#endif /* SPI_INTEL_H */
diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
index 39967a5eca6d..ea4a4b1b246a 100644
--- a/include/linux/mfd/lpc_ich.h
+++ b/include/linux/mfd/lpc_ich.h
@@ -8,7 +8,7 @@
 #ifndef LPC_ICH_H
 #define LPC_ICH_H
 
-#include <linux/platform_data/x86/intel-spi.h>
+#include <linux/platform_data/x86/spi-intel.h>
 
 /* GPIO resources */
 #define ICH_RES_GPIO	0
diff --git a/include/linux/platform_data/x86/intel-spi.h b/include/linux/platform_data/x86/spi-intel.h
similarity index 89%
rename from include/linux/platform_data/x86/intel-spi.h
rename to include/linux/platform_data/x86/spi-intel.h
index 7dda3f690465..a512ec37abbb 100644
--- a/include/linux/platform_data/x86/intel-spi.h
+++ b/include/linux/platform_data/x86/spi-intel.h
@@ -6,8 +6,8 @@
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
-#ifndef INTEL_SPI_PDATA_H
-#define INTEL_SPI_PDATA_H
+#ifndef SPI_INTEL_PDATA_H
+#define SPI_INTEL_PDATA_H
 
 enum intel_spi_type {
 	INTEL_SPI_BYT = 1,
@@ -28,4 +28,4 @@ struct intel_spi_boardinfo {
 	void *data;
 };
 
-#endif /* INTEL_SPI_PDATA_H */
+#endif /* SPI_INTEL_PDATA_H */
-- 
2.33.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Pratyush Yadav]

On 30/09/21 01:07PM, Mika Westerberg wrote:
> The preferred way to implement SPI-NOR controller drivers is through SPI
> subsubsystem utilizing the SPI MEM core functions. This converts the
> Intel SPI flash controller driver over the SPI MEM by moving the driver
> from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> SPI MEM functions. The driver name will be changed from intel-spi to
> spi-intel to match the convention used in the SPI subsystem.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/mtd/spi-nor/controllers/Kconfig       |  36 ---
>  drivers/mtd/spi-nor/controllers/Makefile      |   3 -
>  drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
>  drivers/spi/Kconfig                           |  38 +++
>  drivers/spi/Makefile                          |   3 +
>  .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  20 +-
>  .../spi-intel-platform.c}                     |  21 +-
>  .../intel-spi.c => spi/spi-intel.c}           | 300 +++++++++++-------
>  drivers/spi/spi-intel.h                       |  19 ++
>  include/linux/mfd/lpc_ich.h                   |   2 +-
>  .../x86/{intel-spi.h => spi-intel.h}          |   6 +-
>  11 files changed, 252 insertions(+), 217 deletions(-)
>  delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
>  rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (86%)
>  rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
>  rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (80%)
>  create mode 100644 drivers/spi/spi-intel.h
>  rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (89%)
> 
[...]
> +static bool intel_spi_supports_mem_op(struct spi_mem *mem,
> +				      const struct spi_mem_op *op)
> +{
> +	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
>  
> -			offs += erase_size;
> -			len -= erase_size;
> +	if (op->cmd.dtr)
> +		return false;
> +
> +	if (ispi->swseq_reg && ispi->locked) {
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++) {
> +			if (ispi->opcodes[i] == op->cmd.opcode)
> +				return true;
>  		}
>  
> -		return 0;
> +		return false;
>  	}
>  
> -	/* Not needed with HW sequencer erase, make sure it is cleared */
> -	ispi->atomic_preopcode = 0;
> +	switch (op->cmd.opcode) {
> +	case SPINOR_OP_RDID:
> +	case SPINOR_OP_RDSR:
> +	case SPINOR_OP_WRSR:
> +		return true;
>  
> -	while (len > 0) {
> -		writel(offs, ispi->base + FADDR);
> +	case SPINOR_OP_READ:
> +	case SPINOR_OP_READ_FAST:
> +	case SPINOR_OP_READ_4B:
> +	case SPINOR_OP_READ_FAST_4B:
> +		return true;

I think the checks need to be stricter here. For example, I don't see 
you pass in protocol width (dual, quad, octal, etc.) to intel_spi_read() 
so I assume it can only do a certain protocol. You need to make sure 
that other protocols are rejected here.

Similarly, you also don't pass in dummy cycles to intel_spi_read(), so I 
assume it can only do a fix number of dummy cycles. Need to make sure 
you reject unsupported ones. Same for other opcodes/cases.

>  
> -		val = readl(ispi->base + HSFSTS_CTL);
> -		val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> -		val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
> -		val |= cmd;
> -		val |= HSFSTS_CTL_FGO;
> -		writel(val, ispi->base + HSFSTS_CTL);
> +	case SPINOR_OP_PP:
> +	case SPINOR_OP_PP_4B:
> +	case SPINOR_OP_WREN:
> +	case SPINOR_OP_WRDI:
> +		return true;
>  
> -		ret = intel_spi_wait_hw_busy(ispi);
> -		if (ret)
> -			return ret;
> +	case SPINOR_OP_SE:
> +	case SPINOR_OP_SE_4B:
> +		return ispi->erase_64k;
>  
> -		status = readl(ispi->base + HSFSTS_CTL);
> -		if (status & HSFSTS_CTL_FCERR)
> -			return -EIO;
> -		else if (status & HSFSTS_CTL_AEL)
> -			return -EACCES;
> +	case SPINOR_OP_BE_4K:
> +	case SPINOR_OP_BE_4K_4B:
> +		return true;
> +	}
> +
> +	return false;
> +}
>  
> -		offs += erase_size;
> -		len -= erase_size;
> +static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> +	size_t nbytes = op->data.nbytes;
> +	u8 opcode = op->cmd.opcode;
> +
> +	if (op->addr.nbytes) {
> +		if  (op->data.dir == SPI_MEM_DATA_IN)
> +			return intel_spi_read(ispi, op->addr.val, nbytes,
> +					      op->data.buf.in);
> +		else if (op->data.dir == SPI_MEM_DATA_OUT)
> +			return intel_spi_write(ispi, op->addr.val, nbytes,
> +					       op->data.buf.out);
> +		else if (op->data.dir == SPI_MEM_NO_DATA)
> +			return intel_spi_erase(ispi, opcode, op->addr.val);
> +	} else {
> +		if  (op->data.dir == SPI_MEM_DATA_IN)
> +			return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
> +						  nbytes);
> +		else if (op->data.dir == SPI_MEM_DATA_OUT)
> +			return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
> +						   nbytes);
> +		else if (op->data.dir == SPI_MEM_NO_DATA)
> +			return intel_spi_write_reg(ispi, opcode, NULL, 0);
>  	}
>  
> -	return 0;
> +	return -EINVAL;
> +}
> +
[...]

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Pratyush Yadav]

On 30/09/21 01:07PM, Mika Westerberg wrote:
> The preferred way to implement SPI-NOR controller drivers is through SPI
> subsubsystem utilizing the SPI MEM core functions. This converts the
> Intel SPI flash controller driver over the SPI MEM by moving the driver
> from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> SPI MEM functions. The driver name will be changed from intel-spi to
> spi-intel to match the convention used in the SPI subsystem.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/mtd/spi-nor/controllers/Kconfig       |  36 ---
>  drivers/mtd/spi-nor/controllers/Makefile      |   3 -
>  drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
>  drivers/spi/Kconfig                           |  38 +++
>  drivers/spi/Makefile                          |   3 +
>  .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  20 +-
>  .../spi-intel-platform.c}                     |  21 +-
>  .../intel-spi.c => spi/spi-intel.c}           | 300 +++++++++++-------
>  drivers/spi/spi-intel.h                       |  19 ++
>  include/linux/mfd/lpc_ich.h                   |   2 +-
>  .../x86/{intel-spi.h => spi-intel.h}          |   6 +-
>  11 files changed, 252 insertions(+), 217 deletions(-)
>  delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
>  rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (86%)
>  rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
>  rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (80%)
>  create mode 100644 drivers/spi/spi-intel.h
>  rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (89%)
> 
[...]
> +static bool intel_spi_supports_mem_op(struct spi_mem *mem,
> +				      const struct spi_mem_op *op)
> +{
> +	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
>  
> -			offs += erase_size;
> -			len -= erase_size;
> +	if (op->cmd.dtr)
> +		return false;
> +
> +	if (ispi->swseq_reg && ispi->locked) {
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++) {
> +			if (ispi->opcodes[i] == op->cmd.opcode)
> +				return true;
>  		}
>  
> -		return 0;
> +		return false;
>  	}
>  
> -	/* Not needed with HW sequencer erase, make sure it is cleared */
> -	ispi->atomic_preopcode = 0;
> +	switch (op->cmd.opcode) {
> +	case SPINOR_OP_RDID:
> +	case SPINOR_OP_RDSR:
> +	case SPINOR_OP_WRSR:
> +		return true;
>  
> -	while (len > 0) {
> -		writel(offs, ispi->base + FADDR);
> +	case SPINOR_OP_READ:
> +	case SPINOR_OP_READ_FAST:
> +	case SPINOR_OP_READ_4B:
> +	case SPINOR_OP_READ_FAST_4B:
> +		return true;

I think the checks need to be stricter here. For example, I don't see 
you pass in protocol width (dual, quad, octal, etc.) to intel_spi_read() 
so I assume it can only do a certain protocol. You need to make sure 
that other protocols are rejected here.

Similarly, you also don't pass in dummy cycles to intel_spi_read(), so I 
assume it can only do a fix number of dummy cycles. Need to make sure 
you reject unsupported ones. Same for other opcodes/cases.

>  
> -		val = readl(ispi->base + HSFSTS_CTL);
> -		val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> -		val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
> -		val |= cmd;
> -		val |= HSFSTS_CTL_FGO;
> -		writel(val, ispi->base + HSFSTS_CTL);
> +	case SPINOR_OP_PP:
> +	case SPINOR_OP_PP_4B:
> +	case SPINOR_OP_WREN:
> +	case SPINOR_OP_WRDI:
> +		return true;
>  
> -		ret = intel_spi_wait_hw_busy(ispi);
> -		if (ret)
> -			return ret;
> +	case SPINOR_OP_SE:
> +	case SPINOR_OP_SE_4B:
> +		return ispi->erase_64k;
>  
> -		status = readl(ispi->base + HSFSTS_CTL);
> -		if (status & HSFSTS_CTL_FCERR)
> -			return -EIO;
> -		else if (status & HSFSTS_CTL_AEL)
> -			return -EACCES;
> +	case SPINOR_OP_BE_4K:
> +	case SPINOR_OP_BE_4K_4B:
> +		return true;
> +	}
> +
> +	return false;
> +}
>  
> -		offs += erase_size;
> -		len -= erase_size;
> +static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> +	size_t nbytes = op->data.nbytes;
> +	u8 opcode = op->cmd.opcode;
> +
> +	if (op->addr.nbytes) {
> +		if  (op->data.dir == SPI_MEM_DATA_IN)
> +			return intel_spi_read(ispi, op->addr.val, nbytes,
> +					      op->data.buf.in);
> +		else if (op->data.dir == SPI_MEM_DATA_OUT)
> +			return intel_spi_write(ispi, op->addr.val, nbytes,
> +					       op->data.buf.out);
> +		else if (op->data.dir == SPI_MEM_NO_DATA)
> +			return intel_spi_erase(ispi, opcode, op->addr.val);
> +	} else {
> +		if  (op->data.dir == SPI_MEM_DATA_IN)
> +			return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
> +						  nbytes);
> +		else if (op->data.dir == SPI_MEM_DATA_OUT)
> +			return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
> +						   nbytes);
> +		else if (op->data.dir == SPI_MEM_NO_DATA)
> +			return intel_spi_write_reg(ispi, opcode, NULL, 0);
>  	}
>  
> -	return 0;
> +	return -EINVAL;
> +}
> +
[...]

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

Hi,

On Mon, Oct 04, 2021 at 03:22:41PM +0530, Pratyush Yadav wrote:
> On 30/09/21 01:07PM, Mika Westerberg wrote:
> > The preferred way to implement SPI-NOR controller drivers is through SPI
> > subsubsystem utilizing the SPI MEM core functions. This converts the
> > Intel SPI flash controller driver over the SPI MEM by moving the driver
> > from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> > SPI MEM functions. The driver name will be changed from intel-spi to
> > spi-intel to match the convention used in the SPI subsystem.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/mtd/spi-nor/controllers/Kconfig       |  36 ---
> >  drivers/mtd/spi-nor/controllers/Makefile      |   3 -
> >  drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
> >  drivers/spi/Kconfig                           |  38 +++
> >  drivers/spi/Makefile                          |   3 +
> >  .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  20 +-
> >  .../spi-intel-platform.c}                     |  21 +-
> >  .../intel-spi.c => spi/spi-intel.c}           | 300 +++++++++++-------
> >  drivers/spi/spi-intel.h                       |  19 ++
> >  include/linux/mfd/lpc_ich.h                   |   2 +-
> >  .../x86/{intel-spi.h => spi-intel.h}          |   6 +-
> >  11 files changed, 252 insertions(+), 217 deletions(-)
> >  delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
> >  rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (86%)
> >  rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
> >  rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (80%)
> >  create mode 100644 drivers/spi/spi-intel.h
> >  rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (89%)
> > 
> [...]
> > +static bool intel_spi_supports_mem_op(struct spi_mem *mem,
> > +				      const struct spi_mem_op *op)
> > +{
> > +	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> >  
> > -			offs += erase_size;
> > -			len -= erase_size;
> > +	if (op->cmd.dtr)
> > +		return false;
> > +
> > +	if (ispi->swseq_reg && ispi->locked) {
> > +		int i;
> > +
> > +		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++) {
> > +			if (ispi->opcodes[i] == op->cmd.opcode)
> > +				return true;
> >  		}
> >  
> > -		return 0;
> > +		return false;
> >  	}
> >  
> > -	/* Not needed with HW sequencer erase, make sure it is cleared */
> > -	ispi->atomic_preopcode = 0;
> > +	switch (op->cmd.opcode) {
> > +	case SPINOR_OP_RDID:
> > +	case SPINOR_OP_RDSR:
> > +	case SPINOR_OP_WRSR:
> > +		return true;
> >  
> > -	while (len > 0) {
> > -		writel(offs, ispi->base + FADDR);
> > +	case SPINOR_OP_READ:
> > +	case SPINOR_OP_READ_FAST:
> > +	case SPINOR_OP_READ_4B:
> > +	case SPINOR_OP_READ_FAST_4B:
> > +		return true;
> 
> I think the checks need to be stricter here. For example, I don't see 
> you pass in protocol width (dual, quad, octal, etc.) to intel_spi_read() 
> so I assume it can only do a certain protocol. You need to make sure 
> that other protocols are rejected here.
> 
> Similarly, you also don't pass in dummy cycles to intel_spi_read(), so I 
> assume it can only do a fix number of dummy cycles. Need to make sure 
> you reject unsupported ones. Same for other opcodes/cases.

Unfortunately there is no way to tell the controller any of these. It
simply does "read" or "write" (as we command it) and internally then
uses whatever it got from the SFDP tables of the flash chip. That's why
we just claim to support all these operations and let the controller do
its thing (whatever it is).

Hope this clarifies.

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

Hi,

On Mon, Oct 04, 2021 at 03:22:41PM +0530, Pratyush Yadav wrote:
> On 30/09/21 01:07PM, Mika Westerberg wrote:
> > The preferred way to implement SPI-NOR controller drivers is through SPI
> > subsubsystem utilizing the SPI MEM core functions. This converts the
> > Intel SPI flash controller driver over the SPI MEM by moving the driver
> > from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> > SPI MEM functions. The driver name will be changed from intel-spi to
> > spi-intel to match the convention used in the SPI subsystem.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/mtd/spi-nor/controllers/Kconfig       |  36 ---
> >  drivers/mtd/spi-nor/controllers/Makefile      |   3 -
> >  drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
> >  drivers/spi/Kconfig                           |  38 +++
> >  drivers/spi/Makefile                          |   3 +
> >  .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  20 +-
> >  .../spi-intel-platform.c}                     |  21 +-
> >  .../intel-spi.c => spi/spi-intel.c}           | 300 +++++++++++-------
> >  drivers/spi/spi-intel.h                       |  19 ++
> >  include/linux/mfd/lpc_ich.h                   |   2 +-
> >  .../x86/{intel-spi.h => spi-intel.h}          |   6 +-
> >  11 files changed, 252 insertions(+), 217 deletions(-)
> >  delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
> >  rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (86%)
> >  rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
> >  rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (80%)
> >  create mode 100644 drivers/spi/spi-intel.h
> >  rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (89%)
> > 
> [...]
> > +static bool intel_spi_supports_mem_op(struct spi_mem *mem,
> > +				      const struct spi_mem_op *op)
> > +{
> > +	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> >  
> > -			offs += erase_size;
> > -			len -= erase_size;
> > +	if (op->cmd.dtr)
> > +		return false;
> > +
> > +	if (ispi->swseq_reg && ispi->locked) {
> > +		int i;
> > +
> > +		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++) {
> > +			if (ispi->opcodes[i] == op->cmd.opcode)
> > +				return true;
> >  		}
> >  
> > -		return 0;
> > +		return false;
> >  	}
> >  
> > -	/* Not needed with HW sequencer erase, make sure it is cleared */
> > -	ispi->atomic_preopcode = 0;
> > +	switch (op->cmd.opcode) {
> > +	case SPINOR_OP_RDID:
> > +	case SPINOR_OP_RDSR:
> > +	case SPINOR_OP_WRSR:
> > +		return true;
> >  
> > -	while (len > 0) {
> > -		writel(offs, ispi->base + FADDR);
> > +	case SPINOR_OP_READ:
> > +	case SPINOR_OP_READ_FAST:
> > +	case SPINOR_OP_READ_4B:
> > +	case SPINOR_OP_READ_FAST_4B:
> > +		return true;
> 
> I think the checks need to be stricter here. For example, I don't see 
> you pass in protocol width (dual, quad, octal, etc.) to intel_spi_read() 
> so I assume it can only do a certain protocol. You need to make sure 
> that other protocols are rejected here.
> 
> Similarly, you also don't pass in dummy cycles to intel_spi_read(), so I 
> assume it can only do a fix number of dummy cycles. Need to make sure 
> you reject unsupported ones. Same for other opcodes/cases.

Unfortunately there is no way to tell the controller any of these. It
simply does "read" or "write" (as we command it) and internally then
uses whatever it got from the SFDP tables of the flash chip. That's why
we just claim to support all these operations and let the controller do
its thing (whatever it is).

Hope this clarifies.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Pratyush Yadav]

On 04/10/21 01:07PM, Mika Westerberg wrote:
> Hi,
> 
> On Mon, Oct 04, 2021 at 03:22:41PM +0530, Pratyush Yadav wrote:
> > On 30/09/21 01:07PM, Mika Westerberg wrote:
> > > The preferred way to implement SPI-NOR controller drivers is through SPI
> > > subsubsystem utilizing the SPI MEM core functions. This converts the
> > > Intel SPI flash controller driver over the SPI MEM by moving the driver
> > > from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> > > SPI MEM functions. The driver name will be changed from intel-spi to
> > > spi-intel to match the convention used in the SPI subsystem.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/mtd/spi-nor/controllers/Kconfig       |  36 ---
> > >  drivers/mtd/spi-nor/controllers/Makefile      |   3 -
> > >  drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
> > >  drivers/spi/Kconfig                           |  38 +++
> > >  drivers/spi/Makefile                          |   3 +
> > >  .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  20 +-
> > >  .../spi-intel-platform.c}                     |  21 +-
> > >  .../intel-spi.c => spi/spi-intel.c}           | 300 +++++++++++-------
> > >  drivers/spi/spi-intel.h                       |  19 ++
> > >  include/linux/mfd/lpc_ich.h                   |   2 +-
> > >  .../x86/{intel-spi.h => spi-intel.h}          |   6 +-
> > >  11 files changed, 252 insertions(+), 217 deletions(-)
> > >  delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
> > >  rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (86%)
> > >  rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
> > >  rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (80%)
> > >  create mode 100644 drivers/spi/spi-intel.h
> > >  rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (89%)
> > > 
> > [...]
> > > +static bool intel_spi_supports_mem_op(struct spi_mem *mem,
> > > +				      const struct spi_mem_op *op)
> > > +{
> > > +	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> > >  
> > > -			offs += erase_size;
> > > -			len -= erase_size;
> > > +	if (op->cmd.dtr)
> > > +		return false;
> > > +
> > > +	if (ispi->swseq_reg && ispi->locked) {
> > > +		int i;
> > > +
> > > +		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++) {
> > > +			if (ispi->opcodes[i] == op->cmd.opcode)
> > > +				return true;
> > >  		}
> > >  
> > > -		return 0;
> > > +		return false;
> > >  	}
> > >  
> > > -	/* Not needed with HW sequencer erase, make sure it is cleared */
> > > -	ispi->atomic_preopcode = 0;
> > > +	switch (op->cmd.opcode) {
> > > +	case SPINOR_OP_RDID:
> > > +	case SPINOR_OP_RDSR:
> > > +	case SPINOR_OP_WRSR:
> > > +		return true;
> > >  
> > > -	while (len > 0) {
> > > -		writel(offs, ispi->base + FADDR);
> > > +	case SPINOR_OP_READ:
> > > +	case SPINOR_OP_READ_FAST:
> > > +	case SPINOR_OP_READ_4B:
> > > +	case SPINOR_OP_READ_FAST_4B:
> > > +		return true;
> > 
> > I think the checks need to be stricter here. For example, I don't see 
> > you pass in protocol width (dual, quad, octal, etc.) to intel_spi_read() 
> > so I assume it can only do a certain protocol. You need to make sure 
> > that other protocols are rejected here.
> > 
> > Similarly, you also don't pass in dummy cycles to intel_spi_read(), so I 
> > assume it can only do a fix number of dummy cycles. Need to make sure 
> > you reject unsupported ones. Same for other opcodes/cases.
> 
> Unfortunately there is no way to tell the controller any of these. It
> simply does "read" or "write" (as we command it) and internally then
> uses whatever it got from the SFDP tables of the flash chip. That's why
> we just claim to support all these operations and let the controller do
> its thing (whatever it is).

That is not ideal. SPI NOR uses this to negotiate the best available 
protocol with the controller. Say you have a flash that is capable of 
octal mode but the controller can only support quad mode. Your driver 
will happily tell SPI NOR that it can support octal mode. I think you 
should check the SPI mode bits to make sure the protocol bus width is 
supported at least (see spi_check_buswidth_req() in spi-mem.c).

As for opcodes, is there no way to find out what opcodes the controller 
discovered via SFDP? Maybe we can't change them, but can we at least 
take a peek at them?

I think this has problems similar to the Cadence xSPI controller [0].

Sorry I only replied to this after you posted a new version. It got lost 
in the heap of emails in my inbox :-(

[0] https://patchwork.kernel.org/project/spi-devel-general/patch/1630499858-20456-1-git-send-email-pthombar@cadence.com/

> 
> Hope this clarifies.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Pratyush Yadav]

On 04/10/21 01:07PM, Mika Westerberg wrote:
> Hi,
> 
> On Mon, Oct 04, 2021 at 03:22:41PM +0530, Pratyush Yadav wrote:
> > On 30/09/21 01:07PM, Mika Westerberg wrote:
> > > The preferred way to implement SPI-NOR controller drivers is through SPI
> > > subsubsystem utilizing the SPI MEM core functions. This converts the
> > > Intel SPI flash controller driver over the SPI MEM by moving the driver
> > > from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> > > SPI MEM functions. The driver name will be changed from intel-spi to
> > > spi-intel to match the convention used in the SPI subsystem.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/mtd/spi-nor/controllers/Kconfig       |  36 ---
> > >  drivers/mtd/spi-nor/controllers/Makefile      |   3 -
> > >  drivers/mtd/spi-nor/controllers/intel-spi.h   |  21 --
> > >  drivers/spi/Kconfig                           |  38 +++
> > >  drivers/spi/Makefile                          |   3 +
> > >  .../intel-spi-pci.c => spi/spi-intel-pci.c}   |  20 +-
> > >  .../spi-intel-platform.c}                     |  21 +-
> > >  .../intel-spi.c => spi/spi-intel.c}           | 300 +++++++++++-------
> > >  drivers/spi/spi-intel.h                       |  19 ++
> > >  include/linux/mfd/lpc_ich.h                   |   2 +-
> > >  .../x86/{intel-spi.h => spi-intel.h}          |   6 +-
> > >  11 files changed, 252 insertions(+), 217 deletions(-)
> > >  delete mode 100644 drivers/mtd/spi-nor/controllers/intel-spi.h
> > >  rename drivers/{mtd/spi-nor/controllers/intel-spi-pci.c => spi/spi-intel-pci.c} (86%)
> > >  rename drivers/{mtd/spi-nor/controllers/intel-spi-platform.c => spi/spi-intel-platform.c} (65%)
> > >  rename drivers/{mtd/spi-nor/controllers/intel-spi.c => spi/spi-intel.c} (80%)
> > >  create mode 100644 drivers/spi/spi-intel.h
> > >  rename include/linux/platform_data/x86/{intel-spi.h => spi-intel.h} (89%)
> > > 
> > [...]
> > > +static bool intel_spi_supports_mem_op(struct spi_mem *mem,
> > > +				      const struct spi_mem_op *op)
> > > +{
> > > +	struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> > >  
> > > -			offs += erase_size;
> > > -			len -= erase_size;
> > > +	if (op->cmd.dtr)
> > > +		return false;
> > > +
> > > +	if (ispi->swseq_reg && ispi->locked) {
> > > +		int i;
> > > +
> > > +		for (i = 0; i < ARRAY_SIZE(ispi->opcodes); i++) {
> > > +			if (ispi->opcodes[i] == op->cmd.opcode)
> > > +				return true;
> > >  		}
> > >  
> > > -		return 0;
> > > +		return false;
> > >  	}
> > >  
> > > -	/* Not needed with HW sequencer erase, make sure it is cleared */
> > > -	ispi->atomic_preopcode = 0;
> > > +	switch (op->cmd.opcode) {
> > > +	case SPINOR_OP_RDID:
> > > +	case SPINOR_OP_RDSR:
> > > +	case SPINOR_OP_WRSR:
> > > +		return true;
> > >  
> > > -	while (len > 0) {
> > > -		writel(offs, ispi->base + FADDR);
> > > +	case SPINOR_OP_READ:
> > > +	case SPINOR_OP_READ_FAST:
> > > +	case SPINOR_OP_READ_4B:
> > > +	case SPINOR_OP_READ_FAST_4B:
> > > +		return true;
> > 
> > I think the checks need to be stricter here. For example, I don't see 
> > you pass in protocol width (dual, quad, octal, etc.) to intel_spi_read() 
> > so I assume it can only do a certain protocol. You need to make sure 
> > that other protocols are rejected here.
> > 
> > Similarly, you also don't pass in dummy cycles to intel_spi_read(), so I 
> > assume it can only do a fix number of dummy cycles. Need to make sure 
> > you reject unsupported ones. Same for other opcodes/cases.
> 
> Unfortunately there is no way to tell the controller any of these. It
> simply does "read" or "write" (as we command it) and internally then
> uses whatever it got from the SFDP tables of the flash chip. That's why
> we just claim to support all these operations and let the controller do
> its thing (whatever it is).

That is not ideal. SPI NOR uses this to negotiate the best available 
protocol with the controller. Say you have a flash that is capable of 
octal mode but the controller can only support quad mode. Your driver 
will happily tell SPI NOR that it can support octal mode. I think you 
should check the SPI mode bits to make sure the protocol bus width is 
supported at least (see spi_check_buswidth_req() in spi-mem.c).

As for opcodes, is there no way to find out what opcodes the controller 
discovered via SFDP? Maybe we can't change them, but can we at least 
take a peek at them?

I think this has problems similar to the Cadence xSPI controller [0].

Sorry I only replied to this after you posted a new version. It got lost 
in the heap of emails in my inbox :-(

[0] https://patchwork.kernel.org/project/spi-devel-general/patch/1630499858-20456-1-git-send-email-pthombar@cadence.com/

> 
> Hope this clarifies.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

Hi,

On Thu, Oct 07, 2021 at 06:06:23PM +0530, Pratyush Yadav wrote:
> > Unfortunately there is no way to tell the controller any of these. It
> > simply does "read" or "write" (as we command it) and internally then
> > uses whatever it got from the SFDP tables of the flash chip. That's why
> > we just claim to support all these operations and let the controller do
> > its thing (whatever it is).
> 
> That is not ideal. SPI NOR uses this to negotiate the best available 
> protocol with the controller. Say you have a flash that is capable of 
> octal mode but the controller can only support quad mode. Your driver 
> will happily tell SPI NOR that it can support octal mode. I think you 
> should check the SPI mode bits to make sure the protocol bus width is 
> supported at least (see spi_check_buswidth_req() in spi-mem.c).

Okay, I'll see if I can add that check somewhere.

> As for opcodes, is there no way to find out what opcodes the controller 
> discovered via SFDP? Maybe we can't change them, but can we at least 
> take a peek at them?

AFAICT no. The controller only allows "higher" level commands like read,
write, erase but does not expose any of that to software. You can see
yourself if you want, the spec is here:
 
  https://cdrdv2.intel.com/v1/dl/getContent/636174

Page 403 has the control register.

> I think this has problems similar to the Cadence xSPI controller [0].

Probably but I would not call these "problems" - it is how the
controller is designed. This one is meant only for SPI-NOR flash access,
typically used by the BIOS. It is by no means general purpose SPI
controller (as you can see from the datasheet). The BIOS does need the
full SPI stack, it just issues these simple commands and let's the
controller figure out what actually needs to be done.

> Sorry I only replied to this after you posted a new version. It got lost 
> in the heap of emails in my inbox :-(

No worries :)

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

Hi,

On Thu, Oct 07, 2021 at 06:06:23PM +0530, Pratyush Yadav wrote:
> > Unfortunately there is no way to tell the controller any of these. It
> > simply does "read" or "write" (as we command it) and internally then
> > uses whatever it got from the SFDP tables of the flash chip. That's why
> > we just claim to support all these operations and let the controller do
> > its thing (whatever it is).
> 
> That is not ideal. SPI NOR uses this to negotiate the best available 
> protocol with the controller. Say you have a flash that is capable of 
> octal mode but the controller can only support quad mode. Your driver 
> will happily tell SPI NOR that it can support octal mode. I think you 
> should check the SPI mode bits to make sure the protocol bus width is 
> supported at least (see spi_check_buswidth_req() in spi-mem.c).

Okay, I'll see if I can add that check somewhere.

> As for opcodes, is there no way to find out what opcodes the controller 
> discovered via SFDP? Maybe we can't change them, but can we at least 
> take a peek at them?

AFAICT no. The controller only allows "higher" level commands like read,
write, erase but does not expose any of that to software. You can see
yourself if you want, the spec is here:
 
  https://cdrdv2.intel.com/v1/dl/getContent/636174

Page 403 has the control register.

> I think this has problems similar to the Cadence xSPI controller [0].

Probably but I would not call these "problems" - it is how the
controller is designed. This one is meant only for SPI-NOR flash access,
typically used by the BIOS. It is by no means general purpose SPI
controller (as you can see from the datasheet). The BIOS does need the
full SPI stack, it just issues these simple commands and let's the
controller figure out what actually needs to be done.

> Sorry I only replied to this after you posted a new version. It got lost 
> in the heap of emails in my inbox :-(

No worries :)

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Pratyush Yadav]

On 07/10/21 07:46PM, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Oct 07, 2021 at 06:06:23PM +0530, Pratyush Yadav wrote:
> > > Unfortunately there is no way to tell the controller any of these. It
> > > simply does "read" or "write" (as we command it) and internally then
> > > uses whatever it got from the SFDP tables of the flash chip. That's why
> > > we just claim to support all these operations and let the controller do
> > > its thing (whatever it is).
> > 
> > That is not ideal. SPI NOR uses this to negotiate the best available 
> > protocol with the controller. Say you have a flash that is capable of 
> > octal mode but the controller can only support quad mode. Your driver 
> > will happily tell SPI NOR that it can support octal mode. I think you 
> > should check the SPI mode bits to make sure the protocol bus width is 
> > supported at least (see spi_check_buswidth_req() in spi-mem.c).
> 
> Okay, I'll see if I can add that check somewhere.
> 
> > As for opcodes, is there no way to find out what opcodes the controller 
> > discovered via SFDP? Maybe we can't change them, but can we at least 
> > take a peek at them?
> 
> AFAICT no. The controller only allows "higher" level commands like read,
> write, erase but does not expose any of that to software. You can see
> yourself if you want, the spec is here:
>  
>   https://cdrdv2.intel.com/v1/dl/getContent/636174
> 
> Page 403 has the control register.

I do see 4k and 64k erase opcode fields on the BIOS_SFDP1_VSSC1 register 
(page 428), but I am not sure what exactly it is for. Anyway, that 
doesn't really do much for us I think.

> 
> > I think this has problems similar to the Cadence xSPI controller [0].
> 
> Probably but I would not call these "problems" - it is how the
> controller is designed. This one is meant only for SPI-NOR flash access,
> typically used by the BIOS. It is by no means general purpose SPI
> controller (as you can see from the datasheet). The BIOS does need the
> full SPI stack, it just issues these simple commands and let's the
> controller figure out what actually needs to be done.

The problem is not the controller itself. It is perfectly fine piece to 
have a controller like this IMO. The problem is how do we make it fit 
into the SPI MEM model, which seems to be designed for general purpose 
controllers only. This problem is shared with this and the Cadence xSPI 
controller.

> 
> > Sorry I only replied to this after you posted a new version. It got lost 
> > in the heap of emails in my inbox :-(
> 
> No worries :)
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Pratyush Yadav]

On 07/10/21 07:46PM, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Oct 07, 2021 at 06:06:23PM +0530, Pratyush Yadav wrote:
> > > Unfortunately there is no way to tell the controller any of these. It
> > > simply does "read" or "write" (as we command it) and internally then
> > > uses whatever it got from the SFDP tables of the flash chip. That's why
> > > we just claim to support all these operations and let the controller do
> > > its thing (whatever it is).
> > 
> > That is not ideal. SPI NOR uses this to negotiate the best available 
> > protocol with the controller. Say you have a flash that is capable of 
> > octal mode but the controller can only support quad mode. Your driver 
> > will happily tell SPI NOR that it can support octal mode. I think you 
> > should check the SPI mode bits to make sure the protocol bus width is 
> > supported at least (see spi_check_buswidth_req() in spi-mem.c).
> 
> Okay, I'll see if I can add that check somewhere.
> 
> > As for opcodes, is there no way to find out what opcodes the controller 
> > discovered via SFDP? Maybe we can't change them, but can we at least 
> > take a peek at them?
> 
> AFAICT no. The controller only allows "higher" level commands like read,
> write, erase but does not expose any of that to software. You can see
> yourself if you want, the spec is here:
>  
>   https://cdrdv2.intel.com/v1/dl/getContent/636174
> 
> Page 403 has the control register.

I do see 4k and 64k erase opcode fields on the BIOS_SFDP1_VSSC1 register 
(page 428), but I am not sure what exactly it is for. Anyway, that 
doesn't really do much for us I think.

> 
> > I think this has problems similar to the Cadence xSPI controller [0].
> 
> Probably but I would not call these "problems" - it is how the
> controller is designed. This one is meant only for SPI-NOR flash access,
> typically used by the BIOS. It is by no means general purpose SPI
> controller (as you can see from the datasheet). The BIOS does need the
> full SPI stack, it just issues these simple commands and let's the
> controller figure out what actually needs to be done.

The problem is not the controller itself. It is perfectly fine piece to 
have a controller like this IMO. The problem is how do we make it fit 
into the SPI MEM model, which seems to be designed for general purpose 
controllers only. This problem is shared with this and the Cadence xSPI 
controller.

> 
> > Sorry I only replied to this after you posted a new version. It got lost 
> > in the heap of emails in my inbox :-(
> 
> No worries :)
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

Hi,

On Thu, Oct 07, 2021 at 11:30:31PM +0530, Pratyush Yadav wrote:
> > Probably but I would not call these "problems" - it is how the
> > controller is designed. This one is meant only for SPI-NOR flash access,
> > typically used by the BIOS. It is by no means general purpose SPI
> > controller (as you can see from the datasheet). The BIOS does need the
> > full SPI stack, it just issues these simple commands and let's the
> > controller figure out what actually needs to be done.
> 
> The problem is not the controller itself. It is perfectly fine piece to 
> have a controller like this IMO. The problem is how do we make it fit 
> into the SPI MEM model, which seems to be designed for general purpose 
> controllers only. This problem is shared with this and the Cadence xSPI 
> controller.

Fully agree. IMHO trying to shoehorn driver like this into a generic SPI
subsystem does not make much sense to me. These things can only talk to
SPI-NOR chips, nothing else.

Do you have any suggestions how to solve this "problem" so we can move
forward with the drivers?

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

Hi,

On Thu, Oct 07, 2021 at 11:30:31PM +0530, Pratyush Yadav wrote:
> > Probably but I would not call these "problems" - it is how the
> > controller is designed. This one is meant only for SPI-NOR flash access,
> > typically used by the BIOS. It is by no means general purpose SPI
> > controller (as you can see from the datasheet). The BIOS does need the
> > full SPI stack, it just issues these simple commands and let's the
> > controller figure out what actually needs to be done.
> 
> The problem is not the controller itself. It is perfectly fine piece to 
> have a controller like this IMO. The problem is how do we make it fit 
> into the SPI MEM model, which seems to be designed for general purpose 
> controllers only. This problem is shared with this and the Cadence xSPI 
> controller.

Fully agree. IMHO trying to shoehorn driver like this into a generic SPI
subsystem does not make much sense to me. These things can only talk to
SPI-NOR chips, nothing else.

Do you have any suggestions how to solve this "problem" so we can move
forward with the drivers?

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Pratyush Yadav]

On 08/10/21 12:02PM, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Oct 07, 2021 at 11:30:31PM +0530, Pratyush Yadav wrote:
> > > Probably but I would not call these "problems" - it is how the
> > > controller is designed. This one is meant only for SPI-NOR flash access,
> > > typically used by the BIOS. It is by no means general purpose SPI
> > > controller (as you can see from the datasheet). The BIOS does need the
> > > full SPI stack, it just issues these simple commands and let's the
> > > controller figure out what actually needs to be done.
> > 
> > The problem is not the controller itself. It is perfectly fine piece to 
> > have a controller like this IMO. The problem is how do we make it fit 
> > into the SPI MEM model, which seems to be designed for general purpose 
> > controllers only. This problem is shared with this and the Cadence xSPI 
> > controller.
> 
> Fully agree. IMHO trying to shoehorn driver like this into a generic SPI
> subsystem does not make much sense to me. These things can only talk to
> SPI-NOR chips, nothing else.
> 
> Do you have any suggestions how to solve this "problem" so we can move
> forward with the drivers?

No, I unfortunately don't. But I suppose the same problem existed with 
the old driver as well. It ignored nor->read_opcode, etc. and did its 
own thing, so at least I don't expect things to get much worse with the 
SPI MEM model.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Pratyush Yadav]

On 08/10/21 12:02PM, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Oct 07, 2021 at 11:30:31PM +0530, Pratyush Yadav wrote:
> > > Probably but I would not call these "problems" - it is how the
> > > controller is designed. This one is meant only for SPI-NOR flash access,
> > > typically used by the BIOS. It is by no means general purpose SPI
> > > controller (as you can see from the datasheet). The BIOS does need the
> > > full SPI stack, it just issues these simple commands and let's the
> > > controller figure out what actually needs to be done.
> > 
> > The problem is not the controller itself. It is perfectly fine piece to 
> > have a controller like this IMO. The problem is how do we make it fit 
> > into the SPI MEM model, which seems to be designed for general purpose 
> > controllers only. This problem is shared with this and the Cadence xSPI 
> > controller.
> 
> Fully agree. IMHO trying to shoehorn driver like this into a generic SPI
> subsystem does not make much sense to me. These things can only talk to
> SPI-NOR chips, nothing else.
> 
> Do you have any suggestions how to solve this "problem" so we can move
> forward with the drivers?

No, I unfortunately don't. But I suppose the same problem existed with 
the old driver as well. It ignored nor->read_opcode, etc. and did its 
own thing, so at least I don't expect things to get much worse with the 
SPI MEM model.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Andy Shevchenko]

On Thu, Sep 30, 2021 at 1:08 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> The preferred way to implement SPI-NOR controller drivers is through SPI
> subsubsystem utilizing the SPI MEM core functions. This converts the
> Intel SPI flash controller driver over the SPI MEM by moving the driver
> from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> SPI MEM functions. The driver name will be changed from intel-spi to
> spi-intel to match the convention used in the SPI subsystem.

...

> +config SPI_INTEL_PCI
> +       tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> +       depends on PCI && (X86 || COMPILE_TEST)

Perhaps two entries, one of which will be the same as for platform case?

> +       depends on SPI_MEM
> +       select SPI_INTEL
> +       help
> +         This enables PCI support for the Intel PCH/PCU SPI controller in
> +         master mode. This controller is present in modern Intel hardware
> +         and is used to hold BIOS and other persistent settings. Using
> +         this driver it is possible to upgrade BIOS directly from Linux.
> +
> +         Say N here unless you know what you are doing. Overwriting the
> +         SPI flash may render the system unbootable.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called spi-intel-pci.
> +
> +config SPI_INTEL_PLATFORM
> +       tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> +       depends on X86 || COMPILE_TEST
> +       depends on SPI_MEM
> +       select SPI_INTEL
> +       help
> +         This enables platform support for the Intel PCH/PCU SPI
> +         controller in master mode. This controller is present in modern
> +         Intel hardware and is used to hold BIOS and other persistent
> +         settings. Using this driver it is possible to upgrade BIOS
> +         directly from Linux.
> +
> +         Say N here unless you know what you are doing. Overwriting the
> +         SPI flash may render the system unbootable.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called spi-intel-platform.

...

+ Blank line ?

>  #include <linux/mtd/partitions.h>
>  #include <linux/mtd/spi-nor.h>

+ Blank line?

> +#include <linux/spi/flash.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>

The rationale is to show that we use two sub(sub)sytems here.

...

> -                       dev_err(ispi->dev, "read error: %llx: %#x\n", from,
> +                       dev_err(ispi->dev, "read error: %x: %#x\n", from,
>                                 status);

Now one line?

...

> -                       dev_err(ispi->dev, "write error: %llx: %#x\n", to,
> +                       dev_err(ispi->dev, "write error: %x: %#x\n", to,
>                                 status);

Ditto.

...

> +               ret = intel_spi_sw_cycle(ispi, opcode, 0,
> +                                        OPTYPE_WRITE_WITH_ADDR);
> +               return ret ? ret : 0;

Why not simply return intel_spi_dw_cycle(...); ?

...

> +       val = readl(ispi->base + HSFSTS_CTL);
> +       val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> +       val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;

> +       val |= cmd;
> +       val |= HSFSTS_CTL_FGO;

Maybe swap these lines to group constants?

...

> +       status = readl(ispi->base + HSFSTS_CTL);
> +       if (status & HSFSTS_CTL_FCERR)
> +               return -EIO;

> +       else if (status & HSFSTS_CTL_AEL)

Redundant 'else'

> +               return -EACCES;

...

> +static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +       struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> +       size_t nbytes = op->data.nbytes;
> +       u8 opcode = op->cmd.opcode;
> +
> +       if (op->addr.nbytes) {
> +               if  (op->data.dir == SPI_MEM_DATA_IN)
> +                       return intel_spi_read(ispi, op->addr.val, nbytes,
> +                                             op->data.buf.in);

> +               else if (op->data.dir == SPI_MEM_DATA_OUT)

Redundant 'else' here and nearby.

> +                       return intel_spi_write(ispi, op->addr.val, nbytes,
> +                                              op->data.buf.out);
> +               else if (op->data.dir == SPI_MEM_NO_DATA)
> +                       return intel_spi_erase(ispi, opcode, op->addr.val);
> +       } else {
> +               if  (op->data.dir == SPI_MEM_DATA_IN)
> +                       return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
> +                                                 nbytes);
> +               else if (op->data.dir == SPI_MEM_DATA_OUT)
> +                       return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
> +                                                  nbytes);
> +               else if (op->data.dir == SPI_MEM_NO_DATA)
> +                       return intel_spi_write_reg(ispi, opcode, NULL, 0);
>         }

> +       return -EINVAL;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Andy Shevchenko]

On Thu, Sep 30, 2021 at 1:08 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> The preferred way to implement SPI-NOR controller drivers is through SPI
> subsubsystem utilizing the SPI MEM core functions. This converts the
> Intel SPI flash controller driver over the SPI MEM by moving the driver
> from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> SPI MEM functions. The driver name will be changed from intel-spi to
> spi-intel to match the convention used in the SPI subsystem.

...

> +config SPI_INTEL_PCI
> +       tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> +       depends on PCI && (X86 || COMPILE_TEST)

Perhaps two entries, one of which will be the same as for platform case?

> +       depends on SPI_MEM
> +       select SPI_INTEL
> +       help
> +         This enables PCI support for the Intel PCH/PCU SPI controller in
> +         master mode. This controller is present in modern Intel hardware
> +         and is used to hold BIOS and other persistent settings. Using
> +         this driver it is possible to upgrade BIOS directly from Linux.
> +
> +         Say N here unless you know what you are doing. Overwriting the
> +         SPI flash may render the system unbootable.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called spi-intel-pci.
> +
> +config SPI_INTEL_PLATFORM
> +       tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> +       depends on X86 || COMPILE_TEST
> +       depends on SPI_MEM
> +       select SPI_INTEL
> +       help
> +         This enables platform support for the Intel PCH/PCU SPI
> +         controller in master mode. This controller is present in modern
> +         Intel hardware and is used to hold BIOS and other persistent
> +         settings. Using this driver it is possible to upgrade BIOS
> +         directly from Linux.
> +
> +         Say N here unless you know what you are doing. Overwriting the
> +         SPI flash may render the system unbootable.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called spi-intel-platform.

...

+ Blank line ?

>  #include <linux/mtd/partitions.h>
>  #include <linux/mtd/spi-nor.h>

+ Blank line?

> +#include <linux/spi/flash.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>

The rationale is to show that we use two sub(sub)sytems here.

...

> -                       dev_err(ispi->dev, "read error: %llx: %#x\n", from,
> +                       dev_err(ispi->dev, "read error: %x: %#x\n", from,
>                                 status);

Now one line?

...

> -                       dev_err(ispi->dev, "write error: %llx: %#x\n", to,
> +                       dev_err(ispi->dev, "write error: %x: %#x\n", to,
>                                 status);

Ditto.

...

> +               ret = intel_spi_sw_cycle(ispi, opcode, 0,
> +                                        OPTYPE_WRITE_WITH_ADDR);
> +               return ret ? ret : 0;

Why not simply return intel_spi_dw_cycle(...); ?

...

> +       val = readl(ispi->base + HSFSTS_CTL);
> +       val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> +       val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;

> +       val |= cmd;
> +       val |= HSFSTS_CTL_FGO;

Maybe swap these lines to group constants?

...

> +       status = readl(ispi->base + HSFSTS_CTL);
> +       if (status & HSFSTS_CTL_FCERR)
> +               return -EIO;

> +       else if (status & HSFSTS_CTL_AEL)

Redundant 'else'

> +               return -EACCES;

...

> +static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +       struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> +       size_t nbytes = op->data.nbytes;
> +       u8 opcode = op->cmd.opcode;
> +
> +       if (op->addr.nbytes) {
> +               if  (op->data.dir == SPI_MEM_DATA_IN)
> +                       return intel_spi_read(ispi, op->addr.val, nbytes,
> +                                             op->data.buf.in);

> +               else if (op->data.dir == SPI_MEM_DATA_OUT)

Redundant 'else' here and nearby.

> +                       return intel_spi_write(ispi, op->addr.val, nbytes,
> +                                              op->data.buf.out);
> +               else if (op->data.dir == SPI_MEM_NO_DATA)
> +                       return intel_spi_erase(ispi, opcode, op->addr.val);
> +       } else {
> +               if  (op->data.dir == SPI_MEM_DATA_IN)
> +                       return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
> +                                                 nbytes);
> +               else if (op->data.dir == SPI_MEM_DATA_OUT)
> +                       return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
> +                                                  nbytes);
> +               else if (op->data.dir == SPI_MEM_NO_DATA)
> +                       return intel_spi_write_reg(ispi, opcode, NULL, 0);
>         }

> +       return -EINVAL;
> +}

-- 
With Best Regards,
Andy Shevchenko

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

Hi Andy,

On Mon, Oct 04, 2021 at 05:29:47PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 30, 2021 at 1:08 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > The preferred way to implement SPI-NOR controller drivers is through SPI
> > subsubsystem utilizing the SPI MEM core functions. This converts the
> > Intel SPI flash controller driver over the SPI MEM by moving the driver
> > from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> > SPI MEM functions. The driver name will be changed from intel-spi to
> > spi-intel to match the convention used in the SPI subsystem.
> 
> ...
> 
> > +config SPI_INTEL_PCI
> > +       tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > +       depends on PCI && (X86 || COMPILE_TEST)
> 
> Perhaps two entries, one of which will be the same as for platform case?

Sure will do that in v2.

> > +       depends on SPI_MEM
> > +       select SPI_INTEL
> > +       help
> > +         This enables PCI support for the Intel PCH/PCU SPI controller in
> > +         master mode. This controller is present in modern Intel hardware
> > +         and is used to hold BIOS and other persistent settings. Using
> > +         this driver it is possible to upgrade BIOS directly from Linux.
> > +
> > +         Say N here unless you know what you are doing. Overwriting the
> > +         SPI flash may render the system unbootable.
> > +
> > +         To compile this driver as a module, choose M here: the module
> > +         will be called spi-intel-pci.
> > +
> > +config SPI_INTEL_PLATFORM
> > +       tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> > +       depends on X86 || COMPILE_TEST
> > +       depends on SPI_MEM
> > +       select SPI_INTEL
> > +       help
> > +         This enables platform support for the Intel PCH/PCU SPI
> > +         controller in master mode. This controller is present in modern
> > +         Intel hardware and is used to hold BIOS and other persistent
> > +         settings. Using this driver it is possible to upgrade BIOS
> > +         directly from Linux.
> > +
> > +         Say N here unless you know what you are doing. Overwriting the
> > +         SPI flash may render the system unbootable.
> > +
> > +         To compile this driver as a module, choose M here: the module
> > +         will be called spi-intel-platform.
> 
> ...
> 
> + Blank line ?

OK

> 
> >  #include <linux/mtd/partitions.h>
> >  #include <linux/mtd/spi-nor.h>
> 
> + Blank line?

OK

> > +#include <linux/spi/flash.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/spi-mem.h>
> 
> The rationale is to show that we use two sub(sub)sytems here.

Yup, got it.

> ...
> 
> > -                       dev_err(ispi->dev, "read error: %llx: %#x\n", from,
> > +                       dev_err(ispi->dev, "read error: %x: %#x\n", from,
> >                                 status);
> 
> Now one line?
> 
> ...
> 
> > -                       dev_err(ispi->dev, "write error: %llx: %#x\n", to,
> > +                       dev_err(ispi->dev, "write error: %x: %#x\n", to,
> >                                 status);
> 
> Ditto.

Sure for both.
> 
> ...
> 
> > +               ret = intel_spi_sw_cycle(ispi, opcode, 0,
> > +                                        OPTYPE_WRITE_WITH_ADDR);
> > +               return ret ? ret : 0;
> 
> Why not simply return intel_spi_dw_cycle(...); ?

Good point. Will fix that.

> ...
> 
> > +       val = readl(ispi->base + HSFSTS_CTL);
> > +       val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> > +       val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
> 
> > +       val |= cmd;
> > +       val |= HSFSTS_CTL_FGO;
> 
> Maybe swap these lines to group constants?

OK

> ...
> 
> > +       status = readl(ispi->base + HSFSTS_CTL);
> > +       if (status & HSFSTS_CTL_FCERR)
> > +               return -EIO;
> 
> > +       else if (status & HSFSTS_CTL_AEL)
> 
> Redundant 'else'

Right.

> > +               return -EACCES;
> 
> ...
> 
> > +static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> > +{
> > +       struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> > +       size_t nbytes = op->data.nbytes;
> > +       u8 opcode = op->cmd.opcode;
> > +
> > +       if (op->addr.nbytes) {
> > +               if  (op->data.dir == SPI_MEM_DATA_IN)
> > +                       return intel_spi_read(ispi, op->addr.val, nbytes,
> > +                                             op->data.buf.in);
> 
> > +               else if (op->data.dir == SPI_MEM_DATA_OUT)
> 
> Redundant 'else' here and nearby.

Right. I'll fix these in v2.

> > +                       return intel_spi_write(ispi, op->addr.val, nbytes,
> > +                                              op->data.buf.out);
> > +               else if (op->data.dir == SPI_MEM_NO_DATA)
> > +                       return intel_spi_erase(ispi, opcode, op->addr.val);
> > +       } else {
> > +               if  (op->data.dir == SPI_MEM_DATA_IN)
> > +                       return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
> > +                                                 nbytes);
> > +               else if (op->data.dir == SPI_MEM_DATA_OUT)
> > +                       return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
> > +                                                  nbytes);
> > +               else if (op->data.dir == SPI_MEM_NO_DATA)
> > +                       return intel_spi_write_reg(ispi, opcode, NULL, 0);
> >         }
> 
> > +       return -EINVAL;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Mika Westerberg]

Hi Andy,

On Mon, Oct 04, 2021 at 05:29:47PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 30, 2021 at 1:08 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > The preferred way to implement SPI-NOR controller drivers is through SPI
> > subsubsystem utilizing the SPI MEM core functions. This converts the
> > Intel SPI flash controller driver over the SPI MEM by moving the driver
> > from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> > SPI MEM functions. The driver name will be changed from intel-spi to
> > spi-intel to match the convention used in the SPI subsystem.
> 
> ...
> 
> > +config SPI_INTEL_PCI
> > +       tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > +       depends on PCI && (X86 || COMPILE_TEST)
> 
> Perhaps two entries, one of which will be the same as for platform case?

Sure will do that in v2.

> > +       depends on SPI_MEM
> > +       select SPI_INTEL
> > +       help
> > +         This enables PCI support for the Intel PCH/PCU SPI controller in
> > +         master mode. This controller is present in modern Intel hardware
> > +         and is used to hold BIOS and other persistent settings. Using
> > +         this driver it is possible to upgrade BIOS directly from Linux.
> > +
> > +         Say N here unless you know what you are doing. Overwriting the
> > +         SPI flash may render the system unbootable.
> > +
> > +         To compile this driver as a module, choose M here: the module
> > +         will be called spi-intel-pci.
> > +
> > +config SPI_INTEL_PLATFORM
> > +       tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> > +       depends on X86 || COMPILE_TEST
> > +       depends on SPI_MEM
> > +       select SPI_INTEL
> > +       help
> > +         This enables platform support for the Intel PCH/PCU SPI
> > +         controller in master mode. This controller is present in modern
> > +         Intel hardware and is used to hold BIOS and other persistent
> > +         settings. Using this driver it is possible to upgrade BIOS
> > +         directly from Linux.
> > +
> > +         Say N here unless you know what you are doing. Overwriting the
> > +         SPI flash may render the system unbootable.
> > +
> > +         To compile this driver as a module, choose M here: the module
> > +         will be called spi-intel-platform.
> 
> ...
> 
> + Blank line ?

OK

> 
> >  #include <linux/mtd/partitions.h>
> >  #include <linux/mtd/spi-nor.h>
> 
> + Blank line?

OK

> > +#include <linux/spi/flash.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/spi-mem.h>
> 
> The rationale is to show that we use two sub(sub)sytems here.

Yup, got it.

> ...
> 
> > -                       dev_err(ispi->dev, "read error: %llx: %#x\n", from,
> > +                       dev_err(ispi->dev, "read error: %x: %#x\n", from,
> >                                 status);
> 
> Now one line?
> 
> ...
> 
> > -                       dev_err(ispi->dev, "write error: %llx: %#x\n", to,
> > +                       dev_err(ispi->dev, "write error: %x: %#x\n", to,
> >                                 status);
> 
> Ditto.

Sure for both.
> 
> ...
> 
> > +               ret = intel_spi_sw_cycle(ispi, opcode, 0,
> > +                                        OPTYPE_WRITE_WITH_ADDR);
> > +               return ret ? ret : 0;
> 
> Why not simply return intel_spi_dw_cycle(...); ?

Good point. Will fix that.

> ...
> 
> > +       val = readl(ispi->base + HSFSTS_CTL);
> > +       val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> > +       val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
> 
> > +       val |= cmd;
> > +       val |= HSFSTS_CTL_FGO;
> 
> Maybe swap these lines to group constants?

OK

> ...
> 
> > +       status = readl(ispi->base + HSFSTS_CTL);
> > +       if (status & HSFSTS_CTL_FCERR)
> > +               return -EIO;
> 
> > +       else if (status & HSFSTS_CTL_AEL)
> 
> Redundant 'else'

Right.

> > +               return -EACCES;
> 
> ...
> 
> > +static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> > +{
> > +       struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> > +       size_t nbytes = op->data.nbytes;
> > +       u8 opcode = op->cmd.opcode;
> > +
> > +       if (op->addr.nbytes) {
> > +               if  (op->data.dir == SPI_MEM_DATA_IN)
> > +                       return intel_spi_read(ispi, op->addr.val, nbytes,
> > +                                             op->data.buf.in);
> 
> > +               else if (op->data.dir == SPI_MEM_DATA_OUT)
> 
> Redundant 'else' here and nearby.

Right. I'll fix these in v2.

> > +                       return intel_spi_write(ispi, op->addr.val, nbytes,
> > +                                              op->data.buf.out);
> > +               else if (op->data.dir == SPI_MEM_NO_DATA)
> > +                       return intel_spi_erase(ispi, opcode, op->addr.val);
> > +       } else {
> > +               if  (op->data.dir == SPI_MEM_DATA_IN)
> > +                       return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
> > +                                                 nbytes);
> > +               else if (op->data.dir == SPI_MEM_DATA_OUT)
> > +                       return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
> > +                                                  nbytes);
> > +               else if (op->data.dir == SPI_MEM_NO_DATA)
> > +                       return intel_spi_write_reg(ispi, opcode, NULL, 0);
> >         }
> 
> > +       return -EINVAL;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/3] Documentation / MTD: Rename the intel-spi driver

[Mika Westerberg]

Since the driver is renamed (and moved) update the BIOS upgrade guide
accordingly from intel-spi to intel-spi. Keep the guide under MTD
documentation because this is pretty much still about MTD and SPI-NOR.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 Documentation/driver-api/mtd/index.rst                    | 2 +-
 .../driver-api/mtd/{intel-spi.rst => spi-intel.rst}       | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)
 rename Documentation/driver-api/mtd/{intel-spi.rst => spi-intel.rst} (94%)

diff --git a/Documentation/driver-api/mtd/index.rst b/Documentation/driver-api/mtd/index.rst
index 436ba5a851d7..6a4278f409d7 100644
--- a/Documentation/driver-api/mtd/index.rst
+++ b/Documentation/driver-api/mtd/index.rst
@@ -7,6 +7,6 @@ Memory Technology Device (MTD)
 .. toctree::
    :maxdepth: 1
 
-   intel-spi
+   spi-intel
    nand_ecc
    spi-nor
diff --git a/Documentation/driver-api/mtd/intel-spi.rst b/Documentation/driver-api/mtd/spi-intel.rst
similarity index 94%
rename from Documentation/driver-api/mtd/intel-spi.rst
rename to Documentation/driver-api/mtd/spi-intel.rst
index 0465f6879262..df854f20ead1 100644
--- a/Documentation/driver-api/mtd/intel-spi.rst
+++ b/Documentation/driver-api/mtd/spi-intel.rst
@@ -1,5 +1,5 @@
 ==============================
-Upgrading BIOS using intel-spi
+Upgrading BIOS using spi-intel
 ==============================
 
 Many Intel CPUs like Baytrail and Braswell include SPI serial flash host
@@ -11,12 +11,12 @@ avoid accidental (or on purpose) overwrite of the content.
 Not all manufacturers protect the SPI serial flash, mainly because it
 allows upgrading the BIOS image directly from an OS.
 
-The intel-spi driver makes it possible to read and write the SPI serial
+The spi-intel driver makes it possible to read and write the SPI serial
 flash, if certain protection bits are not set and locked. If it finds
 any of them set, the whole MTD device is made read-only to prevent
 partial overwrites. By default the driver exposes SPI serial flash
 contents as read-only but it can be changed from kernel command line,
-passing "intel-spi.writeable=1".
+passing "spi_intel.writeable=1".
 
 Please keep in mind that overwriting the BIOS image on SPI serial flash
 might render the machine unbootable and requires special equipment like
@@ -32,7 +32,7 @@ Linux.
     serial flash. Distros like Debian and Fedora have this prepackaged with
     name "mtd-utils".
 
- 3) Add "intel-spi.writeable=1" to the kernel command line and reboot
+ 3) Add "spi_intel.writeable=1" to the kernel command line and reboot
     the board (you can also reload the driver passing "writeable=1" as
     module parameter to modprobe).
 
-- 
2.33.0

[PATCH 3/3] Documentation / MTD: Rename the intel-spi driver

[Mika Westerberg]

Since the driver is renamed (and moved) update the BIOS upgrade guide
accordingly from intel-spi to intel-spi. Keep the guide under MTD
documentation because this is pretty much still about MTD and SPI-NOR.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 Documentation/driver-api/mtd/index.rst                    | 2 +-
 .../driver-api/mtd/{intel-spi.rst => spi-intel.rst}       | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)
 rename Documentation/driver-api/mtd/{intel-spi.rst => spi-intel.rst} (94%)

diff --git a/Documentation/driver-api/mtd/index.rst b/Documentation/driver-api/mtd/index.rst
index 436ba5a851d7..6a4278f409d7 100644
--- a/Documentation/driver-api/mtd/index.rst
+++ b/Documentation/driver-api/mtd/index.rst
@@ -7,6 +7,6 @@ Memory Technology Device (MTD)
 .. toctree::
    :maxdepth: 1
 
-   intel-spi
+   spi-intel
    nand_ecc
    spi-nor
diff --git a/Documentation/driver-api/mtd/intel-spi.rst b/Documentation/driver-api/mtd/spi-intel.rst
similarity index 94%
rename from Documentation/driver-api/mtd/intel-spi.rst
rename to Documentation/driver-api/mtd/spi-intel.rst
index 0465f6879262..df854f20ead1 100644
--- a/Documentation/driver-api/mtd/intel-spi.rst
+++ b/Documentation/driver-api/mtd/spi-intel.rst
@@ -1,5 +1,5 @@
 ==============================
-Upgrading BIOS using intel-spi
+Upgrading BIOS using spi-intel
 ==============================
 
 Many Intel CPUs like Baytrail and Braswell include SPI serial flash host
@@ -11,12 +11,12 @@ avoid accidental (or on purpose) overwrite of the content.
 Not all manufacturers protect the SPI serial flash, mainly because it
 allows upgrading the BIOS image directly from an OS.
 
-The intel-spi driver makes it possible to read and write the SPI serial
+The spi-intel driver makes it possible to read and write the SPI serial
 flash, if certain protection bits are not set and locked. If it finds
 any of them set, the whole MTD device is made read-only to prevent
 partial overwrites. By default the driver exposes SPI serial flash
 contents as read-only but it can be changed from kernel command line,
-passing "intel-spi.writeable=1".
+passing "spi_intel.writeable=1".
 
 Please keep in mind that overwriting the BIOS image on SPI serial flash
 might render the machine unbootable and requires special equipment like
@@ -32,7 +32,7 @@ Linux.
     serial flash. Distros like Debian and Fedora have this prepackaged with
     name "mtd-utils".
 
- 3) Add "intel-spi.writeable=1" to the kernel command line and reboot
+ 3) Add "spi_intel.writeable=1" to the kernel command line and reboot
     the board (you can also reload the driver passing "writeable=1" as
     module parameter to modprobe).
 
-- 
2.33.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/3] Documentation / MTD: Rename the intel-spi driver

[Alexander Sverdlin]

Hi!

On 30/09/2021 12:07, Mika Westerberg wrote:
> Since the driver is renamed (and moved) update the BIOS upgrade guide
> accordingly from intel-spi to intel-spi. Keep the guide under MTD
                   ^^^^^^^^^^^^^^^^^^^^^^
Copy-paste error.

> documentation because this is pretty much still about MTD and SPI-NOR.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  Documentation/driver-api/mtd/index.rst                    | 2 +-
>  .../driver-api/mtd/{intel-spi.rst => spi-intel.rst}       | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>  rename Documentation/driver-api/mtd/{intel-spi.rst => spi-intel.rst} (94%)
> 
> diff --git a/Documentation/driver-api/mtd/index.rst b/Documentation/driver-api/mtd/index.rst
> index 436ba5a851d7..6a4278f409d7 100644
> --- a/Documentation/driver-api/mtd/index.rst
> +++ b/Documentation/driver-api/mtd/index.rst
> @@ -7,6 +7,6 @@ Memory Technology Device (MTD)
>  .. toctree::
>     :maxdepth: 1
>  
> -   intel-spi
> +   spi-intel

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 3/3] Documentation / MTD: Rename the intel-spi driver

[Alexander Sverdlin]

Hi!

On 30/09/2021 12:07, Mika Westerberg wrote:
> Since the driver is renamed (and moved) update the BIOS upgrade guide
> accordingly from intel-spi to intel-spi. Keep the guide under MTD
                   ^^^^^^^^^^^^^^^^^^^^^^
Copy-paste error.

> documentation because this is pretty much still about MTD and SPI-NOR.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  Documentation/driver-api/mtd/index.rst                    | 2 +-
>  .../driver-api/mtd/{intel-spi.rst => spi-intel.rst}       | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>  rename Documentation/driver-api/mtd/{intel-spi.rst => spi-intel.rst} (94%)
> 
> diff --git a/Documentation/driver-api/mtd/index.rst b/Documentation/driver-api/mtd/index.rst
> index 436ba5a851d7..6a4278f409d7 100644
> --- a/Documentation/driver-api/mtd/index.rst
> +++ b/Documentation/driver-api/mtd/index.rst
> @@ -7,6 +7,6 @@ Memory Technology Device (MTD)
>  .. toctree::
>     :maxdepth: 1
>  
> -   intel-spi
> +   spi-intel

-- 
Best regards,
Alexander Sverdlin.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/