[PATCH v2 0/3] Altera ASMI Parallel II IP Core

[PATCH v2 0/3] Altera ASMI Parallel II IP Core

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

This patch set adds a spi-nor flash driver for the Altera ASMI Parallel II 
IP Core.  This driver was created based on feedback from Marek Vasut,
Cyrill Pitchen, and Michal Suchanek regarding Version 2 of the Altera
Quadspi Controller: https://lkml.org/lkml/2017/6/26/518

The overall problem with Version 2 of the Altera Quadspi Controller and 
its driver was the fact that there was much duplication of code and logic
with the spi-nor framework.  This new combination of fpga hardware and 
software "gets out of the way" of the spi-nor framework.  The result is a
much simpler driver with the spi-nor framework performing the bulk of the work.

Patch 1 contains the device tree bindings for the driver for
the Altera ASMI-Parallel II IP Core.

Patch 2 contains the driver code for the Altera ASMI-Parallel II IP Core.
This driver supports being configured via a device tree or with platform
data.  In the later case, the memory for the registers has been remapped.

Patch 3 contains a work around for some non-standard behavior of EPCQ flash.

Matthew Gerlach (3):
  dt-bindings: mtd: Altera ASMI Parallel II IP Core
  mtd: spi-nor: Altera ASMI Parallel II IP Core
  mtd: spi-nor: add flag for reading dummy cycles from nv cfg reg

 .../devicetree/bindings/mtd/altera-asmip2.txt      |  15 +
 MAINTAINERS                                        |   7 +
 drivers/mtd/spi-nor/Kconfig                        |   6 +
 drivers/mtd/spi-nor/Makefile                       |   1 +
 drivers/mtd/spi-nor/altera-asmip2.c                | 478 +++++++++++++++++++++
 include/linux/mtd/altera-asmip2.h                  |  27 ++
 6 files changed, 534 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/altera-asmip2.txt
 create mode 100644 drivers/mtd/spi-nor/altera-asmip2.c
 create mode 100644 include/linux/mtd/altera-asmip2.h

-- 
2.7.4


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

[PATCH v2 1/3] dt-bindings: mtd: Altera ASMI Parallel II IP Core

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Device Tree bindings for Altera ASMI Parallel II IP Core
connected to a flash chip.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v2:
    Made substitutions suggested by Rob Herring.
    Emphasize driver expects controller is connected to spi-nor flash.
---
 Documentation/devicetree/bindings/mtd/altera-asmip2.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/altera-asmip2.txt

diff --git a/Documentation/devicetree/bindings/mtd/altera-asmip2.txt b/Documentation/devicetree/bindings/mtd/altera-asmip2.txt
new file mode 100644
index 0000000..2e58817
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/altera-asmip2.txt
@@ -0,0 +1,15 @@
+* Altera ASMI Parallel II IP Core
+
+Devicetree bindings for an Altera ASMI Parallel II IP core connected to a
+spi-nor flash chip.
+
+Required properties:
+- compatible : Should be "altr,asmi-parallel2-spi-nor".
+- reg : A tuple consisting of a physical address and length.
+
+Example:
+
+qspi: spi@a0001000 {
+	compatible = "altr,asmi-parallel2-spi-nor";
+	reg = <0xa0001000 0x10>;
+};
-- 
2.7.4

Re: [PATCH v2 1/3] dt-bindings: mtd: Altera ASMI Parallel II IP Core

[Rob Herring]

On Wed, Sep 20, 2017 at 11:28:57AM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Device Tree bindings for Altera ASMI Parallel II IP Core
> connected to a flash chip.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v2:
>     Made substitutions suggested by Rob Herring.
>     Emphasize driver expects controller is connected to spi-nor flash.
> ---
>  Documentation/devicetree/bindings/mtd/altera-asmip2.txt | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/altera-asmip2.txt

Acked-by: Rob Herring <robh@kernel.org>

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

[PATCH v2 2/3] mtd: spi-nor: Altera ASMI Parallel II IP Core

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

This patch adds support for a spi-nor, platform driver for the
Altera ASMI Parallel II IP Core.  The intended use case is to be able
to update the flash used to load a FPGA at power up with mtd-utils.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v2:
    minor checkpatch fixing by Wu Hao <hao.wu@intel.com>
    Use read_dummy value as suggested by Cyrille Pitchen.
    Don't assume 4 byte addressing (Cryille Pichecn and Marek Vasut).
    Fixed #define indenting as suggested by Marek Vasut.
    Added units to timer values as suggested by Marek Vasut.
    Use io(read|write)8_rep() as suggested by Marek Vasut.
    Renamed function prefixed with __ as suggested by Marek Vasut.
---
 MAINTAINERS                         |   7 +
 drivers/mtd/spi-nor/Kconfig         |   6 +
 drivers/mtd/spi-nor/Makefile        |   1 +
 drivers/mtd/spi-nor/altera-asmip2.c | 457 ++++++++++++++++++++++++++++++++++++
 include/linux/mtd/altera-asmip2.h   |  24 ++
 5 files changed, 495 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/altera-asmip2.c
 create mode 100644 include/linux/mtd/altera-asmip2.h

diff --git a/MAINTAINERS b/MAINTAINERS
index b87c698..af48255 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -644,6 +644,13 @@ ALPS PS/2 TOUCHPAD DRIVER
 R:	Pali Rohár <pali.rohar@gmail.com>
 F:	drivers/input/mouse/alps.*
 
+ALTERA ASMI Parallel II Driver
+M:      Matthew Gerlach <matthew.gerlach@linux.intel.com>
+L:      linux-mtd@lists.infradead.org
+S:      Maintained
+F:      drivers/mtd/spi-nor/altera-asmip2.c
+F:      inclulde/linux/mtd/altera-asmip2.h
+
 ALTERA I2C CONTROLLER DRIVER
 M:	Thor Thayer <thor.thayer@linux.intel.com>
 S:	Maintained
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 69c638d..f81dbec 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -129,4 +129,10 @@ config SPI_STM32_QUADSPI
 	  This enables support for the STM32 Quad SPI controller.
 	  We only connect the NOR to this controller.
 
+config SPI_ALTERA_ASMIP2
+        tristate "Altera ASMI Parallel II IP"
+        depends on OF && HAS_IOMEM
+        help
+          Enable support for Altera ASMI Parallel II.
+
 endif # MTD_SPI_NOR
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 7d84c51..1c79324 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
+obj-$(CONFIG_SPI_ALTERA_ASMIP2)	+= altera-asmip2.o
 obj-$(CONFIG_SPI_ASPEED_SMC)	+= aspeed-smc.o
 obj-$(CONFIG_SPI_ATMEL_QUADSPI)	+= atmel-quadspi.o
 obj-$(CONFIG_SPI_CADENCE_QUADSPI)	+= cadence-quadspi.o
diff --git a/drivers/mtd/spi-nor/altera-asmip2.c b/drivers/mtd/spi-nor/altera-asmip2.c
new file mode 100644
index 0000000..a977765
--- /dev/null
+++ b/drivers/mtd/spi-nor/altera-asmip2.c
@@ -0,0 +1,457 @@
+/*
+ * Copyright (C) 2017 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/altera-asmip2.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/spi-nor.h>
+#include <linux/of_device.h>
+
+#define QSPI_ACTION_REG			0
+#define QSPI_ACTION_RST			BIT(0)
+#define QSPI_ACTION_EN			BIT(1)
+#define QSPI_ACTION_SC			BIT(2)
+#define QSPI_ACTION_CHIP_SEL_SFT	4
+#define QSPI_ACTION_DUMMY_SFT		8
+#define QSPI_ACTION_READ_BACK_SFT	16
+
+#define QSPI_FIFO_CNT_REG		4
+#define QSPI_FIFO_DEPTH			0x200
+#define QSPI_FIFO_CNT_MSK		0x3ff
+#define QSPI_FIFO_CNT_RX_SFT		0
+#define QSPI_FIFO_CNT_TX_SFT		12
+
+#define QSPI_DATA_REG			0x8
+
+#define QSPI_POLL_TIMEOUT_US		10000000
+#define QSPI_POLL_INTERVAL_US		5
+
+struct altera_asmip2 {
+	void __iomem *csr_base;
+	u32 num_flashes;
+	struct device *dev;
+	struct altera_asmip2_flash *flash[ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP];
+	struct mutex bus_mutex;
+};
+
+struct altera_asmip2_flash {
+	struct spi_nor nor;
+	struct altera_asmip2 *q;
+};
+
+static int altera_asmip2_write_reg(struct spi_nor *nor, u8 opcode, u8 *val,
+				    int len)
+{
+	struct altera_asmip2_flash *flash = nor->priv;
+	struct altera_asmip2 *q = flash->q;
+	u32 reg;
+	int ret;
+
+	if ((len + 1) > QSPI_FIFO_DEPTH) {
+		dev_err(q->dev, "%s bad len %d > %d\n",
+			__func__, len + 1, QSPI_FIFO_DEPTH);
+		return -EINVAL;
+	}
+
+	writeb(opcode, q->csr_base + QSPI_DATA_REG);
+
+	iowrite8_rep(q->csr_base + QSPI_DATA_REG, val, len);
+
+	reg = QSPI_ACTION_EN | QSPI_ACTION_SC;
+
+	writel(reg, q->csr_base + QSPI_ACTION_REG);
+
+	ret = readl_poll_timeout(q->csr_base + QSPI_FIFO_CNT_REG, reg,
+				 (((reg >> QSPI_FIFO_CNT_TX_SFT) &
+				 QSPI_FIFO_CNT_MSK) == 0),
+				 QSPI_POLL_INTERVAL_US, QSPI_POLL_TIMEOUT_US);
+	if (ret)
+		dev_err(q->dev, "%s timed out\n", __func__);
+
+	reg = QSPI_ACTION_EN;
+
+	writel(reg, q->csr_base + QSPI_ACTION_REG);
+
+	return ret;
+}
+
+static int altera_asmip2_read_reg(struct spi_nor *nor, u8 opcode, u8 *val,
+				   int len)
+{
+	struct altera_asmip2_flash *flash = nor->priv;
+	struct altera_asmip2 *q = flash->q;
+	u32 reg;
+	int ret;
+
+	if (len > QSPI_FIFO_DEPTH) {
+		dev_err(q->dev, "%s bad len %d > %d\n",
+			__func__, len, QSPI_FIFO_DEPTH);
+		return -EINVAL;
+	}
+
+	writeb(opcode, q->csr_base + QSPI_DATA_REG);
+
+	reg = QSPI_ACTION_EN | QSPI_ACTION_SC |
+		(len << QSPI_ACTION_READ_BACK_SFT);
+
+	writel(reg, q->csr_base + QSPI_ACTION_REG);
+
+	ret = readl_poll_timeout(q->csr_base + QSPI_FIFO_CNT_REG, reg,
+				 ((reg & QSPI_FIFO_CNT_MSK) == len),
+				 QSPI_POLL_INTERVAL_US, QSPI_POLL_TIMEOUT_US);
+
+	if (!ret)
+		ioread8_rep(q->csr_base + QSPI_DATA_REG, val, len);
+	else
+		dev_err(q->dev, "%s timeout\n", __func__);
+
+	writel(QSPI_ACTION_EN, q->csr_base + QSPI_ACTION_REG);
+
+	return ret;
+}
+
+static inline void altera_asmip2_push_offset(struct altera_asmip2 *q,
+					     struct spi_nor *nor,
+					     loff_t offset)
+{
+	int i;
+	u32 val;
+
+	for (i = (nor->addr_width - 1) * 8; i >= 0; i -= 8) {
+		val = (offset & (0xff << i)) >> i;
+		writeb(val, q->csr_base + QSPI_DATA_REG);
+	}
+}
+
+static ssize_t altera_asmip2_read(struct spi_nor *nor, loff_t from, size_t len,
+				   u_char *buf)
+{
+	struct altera_asmip2_flash *flash = nor->priv;
+	struct altera_asmip2 *q = flash->q;
+	size_t bytes_to_read;
+	u32 reg;
+	int ret;
+
+	bytes_to_read = min_t(size_t, len, QSPI_FIFO_DEPTH);
+
+	writeb(nor->read_opcode, q->csr_base + QSPI_DATA_REG);
+
+	altera_asmip2_push_offset(q, nor, from);
+
+	reg = QSPI_ACTION_EN | QSPI_ACTION_SC |
+		(nor->read_dummy << QSPI_ACTION_DUMMY_SFT) |
+		(bytes_to_read << QSPI_ACTION_READ_BACK_SFT);
+
+	writel(reg, q->csr_base + QSPI_ACTION_REG);
+
+	ret = readl_poll_timeout(q->csr_base + QSPI_FIFO_CNT_REG, reg,
+				 ((reg & QSPI_FIFO_CNT_MSK) ==
+				 bytes_to_read), QSPI_POLL_INTERVAL_US,
+				 QSPI_POLL_TIMEOUT_US);
+	if (ret) {
+		dev_err(q->dev, "%s timed out\n", __func__);
+		bytes_to_read = 0;
+	} else
+		ioread8_rep(q->csr_base + QSPI_DATA_REG, buf, bytes_to_read);
+
+	writel(QSPI_ACTION_EN, q->csr_base + QSPI_ACTION_REG);
+
+	return bytes_to_read;
+}
+
+static ssize_t altera_asmip2_write(struct spi_nor *nor, loff_t to,
+				    size_t len, const u_char *buf)
+{
+	struct altera_asmip2_flash *flash = nor->priv;
+	struct altera_asmip2 *q = flash->q;
+	size_t bytes_to_write;
+	u32 reg;
+	int ret;
+
+	bytes_to_write = min_t(size_t, len,
+			       (QSPI_FIFO_DEPTH - (nor->addr_width + 1)));
+
+	writeb(nor->program_opcode, q->csr_base + QSPI_DATA_REG);
+
+	altera_asmip2_push_offset(q, nor, to);
+
+	iowrite8_rep(q->csr_base + QSPI_DATA_REG, buf, bytes_to_write);
+
+	reg = QSPI_ACTION_EN | QSPI_ACTION_SC;
+
+	writel(reg, q->csr_base + QSPI_ACTION_REG);
+
+	ret = readl_poll_timeout(q->csr_base + QSPI_FIFO_CNT_REG, reg,
+				 (((reg >> QSPI_FIFO_CNT_TX_SFT) &
+				 QSPI_FIFO_CNT_MSK) == 0),
+				 QSPI_POLL_INTERVAL_US, QSPI_POLL_TIMEOUT_US);
+
+	if (ret) {
+		dev_err(q->dev,
+			"%s timed out waiting for fifo to clear\n",
+			__func__);
+		bytes_to_write = 0;
+	}
+
+	writel(QSPI_ACTION_EN, q->csr_base + QSPI_ACTION_REG);
+
+	return bytes_to_write;
+}
+
+static int altera_asmip2_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct altera_asmip2_flash *flash = nor->priv;
+	struct altera_asmip2 *q = flash->q;
+
+	mutex_lock(&q->bus_mutex);
+
+	return 0;
+}
+
+static void altera_asmip2_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct altera_asmip2_flash *flash = nor->priv;
+	struct altera_asmip2 *q = flash->q;
+
+	mutex_unlock(&q->bus_mutex);
+}
+
+static int altera_asmip2_setup_banks(struct device *dev,
+				      u32 bank, struct device_node *np)
+{
+	const struct spi_nor_hwcaps hwcaps = {
+		.mask = SNOR_HWCAPS_READ |
+			SNOR_HWCAPS_READ_FAST |
+			SNOR_HWCAPS_PP,
+	};
+	struct altera_asmip2 *q = dev_get_drvdata(dev);
+	struct altera_asmip2_flash *flash;
+	struct spi_nor *nor;
+	int ret = 0;
+
+	if (bank > q->num_flashes - 1)
+		return -EINVAL;
+
+	flash = devm_kzalloc(q->dev, sizeof(*flash), GFP_KERNEL);
+	if (!flash)
+		return -ENOMEM;
+
+	q->flash[bank] = flash;
+	flash->q = q;
+
+	nor = &flash->nor;
+	nor->dev = dev;
+	nor->priv = flash;
+	nor->mtd.priv = nor;
+	spi_nor_set_flash_node(nor, np);
+
+	/* spi nor framework*/
+	nor->read_reg = altera_asmip2_read_reg;
+	nor->write_reg = altera_asmip2_write_reg;
+	nor->read = altera_asmip2_read;
+	nor->write = altera_asmip2_write;
+	nor->prepare = altera_asmip2_prep;
+	nor->unprepare = altera_asmip2_unprep;
+
+	ret = spi_nor_scan(nor, NULL, &hwcaps);
+	if (ret) {
+		dev_err(nor->dev, "flash not found\n");
+		return ret;
+	}
+
+	ret =  mtd_device_register(&nor->mtd, NULL, 0);
+
+	return ret;
+}
+
+static int altera_asmip2_create(struct device *dev, void __iomem *csr_base)
+{
+	struct altera_asmip2 *q;
+	u32 reg;
+
+	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
+	if (!q)
+		return -ENOMEM;
+
+	q->dev = dev;
+	q->csr_base = csr_base;
+
+	mutex_init(&q->bus_mutex);
+
+	dev_set_drvdata(dev, q);
+
+	reg = readl(q->csr_base + QSPI_ACTION_REG);
+	if (!(reg & QSPI_ACTION_RST)) {
+		writel((reg | QSPI_ACTION_RST), q->csr_base + QSPI_ACTION_REG);
+		dev_info(dev, "%s asserting reset\n", __func__);
+		udelay(10);
+	}
+
+	writel((reg & ~QSPI_ACTION_RST), q->csr_base + QSPI_ACTION_REG);
+	udelay(10);
+
+	return 0;
+}
+
+static int altera_asmip2_add_bank(struct device *dev,
+			 u32 bank, struct device_node *np)
+{
+	struct altera_asmip2 *q = dev_get_drvdata(dev);
+
+	if (q->num_flashes >= ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP)
+		return -ENOMEM;
+
+	q->num_flashes++;
+
+	return altera_asmip2_setup_banks(dev, bank, np);
+}
+
+static int altera_asmip2_remove_banks(struct device *dev)
+{
+	struct altera_asmip2 *q = dev_get_drvdata(dev);
+	struct altera_asmip2_flash *flash;
+	int i;
+	int ret = 0;
+
+	if (!q)
+		return -EINVAL;
+
+	/* clean up for all nor flash */
+	for (i = 0; i < q->num_flashes; i++) {
+		flash = q->flash[i];
+		if (!flash)
+			continue;
+
+		/* clean up mtd stuff */
+		ret = mtd_device_unregister(&flash->nor.mtd);
+		if (ret) {
+			dev_err(dev, "error removing mtd\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int altera_asmip2_probe_with_pdata(struct platform_device *pdev,
+			     struct altera_asmip2_plat_data *qdata)
+{
+	struct device *dev = &pdev->dev;
+	int ret, i;
+
+	ret = altera_asmip2_create(dev, qdata->csr_base);
+
+	if (ret) {
+		dev_err(dev, "failed to create qspi device %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < qdata->num_chip_sel; i++) {
+		ret = altera_asmip2_add_bank(dev, i, NULL);
+		if (ret) {
+			dev_err(dev, "failed to add qspi bank %d\n", ret);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int altera_asmip2_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct altera_asmip2_plat_data *qdata;
+	struct resource *res;
+	void __iomem *csr_base;
+	u32 bank;
+	int ret;
+	struct device_node *pp;
+
+	qdata = dev_get_platdata(dev);
+
+	if (qdata)
+		return altera_asmip2_probe_with_pdata(pdev, qdata);
+
+	if (!np) {
+		dev_err(dev, "no device tree found %p\n", pdev);
+		return -ENODEV;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	csr_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(csr_base)) {
+		dev_err(dev, "%s: ERROR: failed to map csr base\n", __func__);
+		return PTR_ERR(csr_base);
+	}
+
+	ret = altera_asmip2_create(dev, csr_base);
+
+	if (ret) {
+		dev_err(dev, "failed to create qspi device\n");
+		return ret;
+	}
+
+	for_each_available_child_of_node(np, pp) {
+		of_property_read_u32(pp, "reg", &bank);
+		if (bank >= ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP) {
+			dev_err(dev, "bad reg value %u >= %u\n", bank,
+				ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP);
+			goto error;
+		}
+
+		if (altera_asmip2_add_bank(dev, bank, pp)) {
+			dev_err(dev, "failed to add bank %u\n", bank);
+			goto error;
+		}
+	}
+
+	return 0;
+error:
+	altera_asmip2_remove_banks(dev);
+	return -EIO;
+}
+
+static int altera_asmip2_remove(struct platform_device *pdev)
+{
+	struct altera_asmip2 *q = dev_get_drvdata(&pdev->dev);
+
+	mutex_destroy(&q->bus_mutex);
+
+	return altera_asmip2_remove_banks(&pdev->dev);
+}
+
+static const struct of_device_id altera_asmip2_id_table[] = {
+	{ .compatible = "altr,asmi-parallel2-spi-nor",},
+	{}
+};
+MODULE_DEVICE_TABLE(of, altera_asmip2_id_table);
+
+static struct platform_driver altera_asmip2_driver = {
+	.driver = {
+		.name = ALTERA_ASMIP2_DRV_NAME,
+		.of_match_table = altera_asmip2_id_table,
+	},
+	.probe = altera_asmip2_probe,
+	.remove = altera_asmip2_remove,
+};
+module_platform_driver(altera_asmip2_driver);
+
+MODULE_AUTHOR("Matthew Gerlach <matthew.gerlach@linux.intel.com>");
+MODULE_DESCRIPTION("Altera ASMI Parallel II");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" ALTERA_ASMIP2_DRV_NAME);
diff --git a/include/linux/mtd/altera-asmip2.h b/include/linux/mtd/altera-asmip2.h
new file mode 100644
index 0000000..580c43c
--- /dev/null
+++ b/include/linux/mtd/altera-asmip2.h
@@ -0,0 +1,24 @@
+/*
+ *
+ * Copyright 2017 Intel Corporation, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#ifndef __ALTERA_QUADSPI_H
+#define __ALTERA_QUADSPI_H
+
+#include <linux/device.h>
+
+#define ALTERA_ASMIP2_DRV_NAME "altr-asmip2"
+#define ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP 3
+#define ALTERA_ASMIP2_RESOURCE_SIZE 0x10
+
+struct altera_asmip2_plat_data {
+	void __iomem *csr_base;
+	u32 num_chip_sel;
+};
+
+#endif
-- 
2.7.4

Re: [PATCH v2 2/3] mtd: spi-nor: Altera ASMI Parallel II IP Core

[Marek Vasut]

On 09/20/2017 08:28 PM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> This patch adds support for a spi-nor, platform driver for the
> Altera ASMI Parallel II IP Core.  The intended use case is to be able
> to update the flash used to load a FPGA at power up with mtd-utils.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v2:
>     minor checkpatch fixing by Wu Hao <hao.wu@intel.com>
>     Use read_dummy value as suggested by Cyrille Pitchen.
>     Don't assume 4 byte addressing (Cryille Pichecn and Marek Vasut).
>     Fixed #define indenting as suggested by Marek Vasut.
>     Added units to timer values as suggested by Marek Vasut.
>     Use io(read|write)8_rep() as suggested by Marek Vasut.
>     Renamed function prefixed with __ as suggested by Marek Vasut.

[...]

> +#define QSPI_ACTION_REG			0
> +#define QSPI_ACTION_RST			BIT(0)
> +#define QSPI_ACTION_EN			BIT(1)
> +#define QSPI_ACTION_SC			BIT(2)
> +#define QSPI_ACTION_CHIP_SEL_SFT	4
> +#define QSPI_ACTION_DUMMY_SFT		8
> +#define QSPI_ACTION_READ_BACK_SFT	16
> +
> +#define QSPI_FIFO_CNT_REG		4
> +#define QSPI_FIFO_DEPTH			0x200
> +#define QSPI_FIFO_CNT_MSK		0x3ff
> +#define QSPI_FIFO_CNT_RX_SFT		0
> +#define QSPI_FIFO_CNT_TX_SFT		12
> +
> +#define QSPI_DATA_REG			0x8
> +
> +#define QSPI_POLL_TIMEOUT_US		10000000

10 s poll timeout ? :)

> +#define QSPI_POLL_INTERVAL_US		5
> +
> +struct altera_asmip2 {

[...]

Otherwise looks good

-- 
Best regards,
Marek Vasut

Re: [PATCH v2 2/3] mtd: spi-nor: Altera ASMI Parallel II IP Core

[matthew.gerlach]

On Tue, 10 Oct 2017, Marek Vasut wrote:

> On 09/20/2017 08:28 PM, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> This patch adds support for a spi-nor, platform driver for the
>> Altera ASMI Parallel II IP Core.  The intended use case is to be able
>> to update the flash used to load a FPGA at power up with mtd-utils.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v2:
>>     minor checkpatch fixing by Wu Hao <hao.wu@intel.com>
>>     Use read_dummy value as suggested by Cyrille Pitchen.
>>     Don't assume 4 byte addressing (Cryille Pichecn and Marek Vasut).
>>     Fixed #define indenting as suggested by Marek Vasut.
>>     Added units to timer values as suggested by Marek Vasut.
>>     Use io(read|write)8_rep() as suggested by Marek Vasut.
>>     Renamed function prefixed with __ as suggested by Marek Vasut.
>
> [...]
>
>> +#define QSPI_ACTION_REG			0
>> +#define QSPI_ACTION_RST			BIT(0)
>> +#define QSPI_ACTION_EN			BIT(1)
>> +#define QSPI_ACTION_SC			BIT(2)
>> +#define QSPI_ACTION_CHIP_SEL_SFT	4
>> +#define QSPI_ACTION_DUMMY_SFT		8
>> +#define QSPI_ACTION_READ_BACK_SFT	16
>> +
>> +#define QSPI_FIFO_CNT_REG		4
>> +#define QSPI_FIFO_DEPTH			0x200
>> +#define QSPI_FIFO_CNT_MSK		0x3ff
>> +#define QSPI_FIFO_CNT_RX_SFT		0
>> +#define QSPI_FIFO_CNT_TX_SFT		12
>> +
>> +#define QSPI_DATA_REG			0x8
>> +
>> +#define QSPI_POLL_TIMEOUT_US		10000000
>
> 10 s poll timeout ? :)

Hi Marek,

The 10s timeout is fairly arbitrary.  In other words, I pulled it out of 
thin air.  Can you suggest a better timeout?  From a practical standpoint 10s 
seemed to be much better than no timeout when I was debugging bad FPGA 
images.  Without a timeout I was hanging the system when the FPGA image 
failed.  With this timeout, we get a nice message and Linux keeps running 
happily.

Thanks for the feedback,

Matthew Gerlach

>
>> +#define QSPI_POLL_INTERVAL_US		 5 >> + >> +struct 
altera_asmip2 { >
> [...]
>
> Otherwise looks good
>
> -- 
> Best regards,
> Marek Vasut
>

Re: [PATCH v2 2/3] mtd: spi-nor: Altera ASMI Parallel II IP Core

[Marek Vasut]

On 10/11/2017 07:00 PM, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 10 Oct 2017, Marek Vasut wrote:
> 
>> On 09/20/2017 08:28 PM, matthew.gerlach@linux.intel.com wrote:
>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>
>>> This patch adds support for a spi-nor, platform driver for the
>>> Altera ASMI Parallel II IP Core.  The intended use case is to be able
>>> to update the flash used to load a FPGA at power up with mtd-utils.
>>>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>> ---
>>> v2:
>>>     minor checkpatch fixing by Wu Hao <hao.wu@intel.com>
>>>     Use read_dummy value as suggested by Cyrille Pitchen.
>>>     Don't assume 4 byte addressing (Cryille Pichecn and Marek Vasut).
>>>     Fixed #define indenting as suggested by Marek Vasut.
>>>     Added units to timer values as suggested by Marek Vasut.
>>>     Use io(read|write)8_rep() as suggested by Marek Vasut.
>>>     Renamed function prefixed with __ as suggested by Marek Vasut.
>>
>> [...]
>>
>>> +#define QSPI_ACTION_REG            0
>>> +#define QSPI_ACTION_RST            BIT(0)
>>> +#define QSPI_ACTION_EN            BIT(1)
>>> +#define QSPI_ACTION_SC            BIT(2)
>>> +#define QSPI_ACTION_CHIP_SEL_SFT    4
>>> +#define QSPI_ACTION_DUMMY_SFT        8
>>> +#define QSPI_ACTION_READ_BACK_SFT    16
>>> +
>>> +#define QSPI_FIFO_CNT_REG        4
>>> +#define QSPI_FIFO_DEPTH            0x200
>>> +#define QSPI_FIFO_CNT_MSK        0x3ff
>>> +#define QSPI_FIFO_CNT_RX_SFT        0
>>> +#define QSPI_FIFO_CNT_TX_SFT        12
>>> +
>>> +#define QSPI_DATA_REG            0x8
>>> +
>>> +#define QSPI_POLL_TIMEOUT_US        10000000
>>
>> 10 s poll timeout ? :)
> 
> Hi Marek,
> 
> The 10s timeout is fairly arbitrary.  In other words, I pulled it out of
> thin air.  Can you suggest a better timeout?  From a practical
> standpoint 10s seemed to be much better than no timeout when I was
> debugging bad FPGA images.  Without a timeout I was hanging the system
> when the FPGA image failed.  With this timeout, we get a nice message
> and Linux keeps running happily.
AFAIK the SPI subsystem has a timeout which is adaptive to the bus
clock, maybe that's what you want to use here ?

-- 
Best regards,
Marek Vasut

Re: [PATCH v2 2/3] mtd: spi-nor: Altera ASMI Parallel II IP Core

[matthew.gerlach-VuQAYsv1563Yd54FQh9/CA]

[-- Attachment #1: Type: text/plain, Size: 3527 bytes --]



On Wed, 11 Oct 2017, Marek Vasut wrote:

> On 10/11/2017 07:00 PM, matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org wrote:
>>
>>
>> On Tue, 10 Oct 2017, Marek Vasut wrote:
>>
>>> On 09/20/2017 08:28 PM, matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org wrote:
>>>> From: Matthew Gerlach <matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>>
>>>> This patch adds support for a spi-nor, platform driver for the
>>>> Altera ASMI Parallel II IP Core.  The intended use case is to be able
>>>> to update the flash used to load a FPGA at power up with mtd-utils.
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>> ---
>>>> v2:
>>>>     minor checkpatch fixing by Wu Hao <hao.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>     Use read_dummy value as suggested by Cyrille Pitchen.
>>>>     Don't assume 4 byte addressing (Cryille Pichecn and Marek Vasut).
>>>>     Fixed #define indenting as suggested by Marek Vasut.
>>>>     Added units to timer values as suggested by Marek Vasut.
>>>>     Use io(read|write)8_rep() as suggested by Marek Vasut.
>>>>     Renamed function prefixed with __ as suggested by Marek Vasut.
>>>
>>> [...]
>>>
>>>> +#define QSPI_ACTION_REG            0
>>>> +#define QSPI_ACTION_RST            BIT(0)
>>>> +#define QSPI_ACTION_EN            BIT(1)
>>>> +#define QSPI_ACTION_SC            BIT(2)
>>>> +#define QSPI_ACTION_CHIP_SEL_SFT    4
>>>> +#define QSPI_ACTION_DUMMY_SFT        8
>>>> +#define QSPI_ACTION_READ_BACK_SFT    16
>>>> +
>>>> +#define QSPI_FIFO_CNT_REG        4
>>>> +#define QSPI_FIFO_DEPTH            0x200
>>>> +#define QSPI_FIFO_CNT_MSK        0x3ff
>>>> +#define QSPI_FIFO_CNT_RX_SFT        0
>>>> +#define QSPI_FIFO_CNT_TX_SFT        12
>>>> +
>>>> +#define QSPI_DATA_REG            0x8
>>>> +
>>>> +#define QSPI_POLL_TIMEOUT_US        10000000
>>>
>>> 10 s poll timeout ? :)
>>
>> Hi Marek,
>>
>> The 10s timeout is fairly arbitrary.  In other words, I pulled it out of
>> thin air.  Can you suggest a better timeout?  From a practical
>> standpoint 10s seemed to be much better than no timeout when I was
>> debugging bad FPGA images.  Without a timeout I was hanging the system
>> when the FPGA image failed.  With this timeout, we get a nice message
>> and Linux keeps running happily.
> AFAIK the SPI subsystem has a timeout which is adaptive to the bus
> clock, maybe that's what you want to use here ?

Hi Marek,

I looked in spi-nor.c, and I see the following two macros:
#define DEFAULT_READY_WAIT_JIFFIES		(40UL * HZ)
#define CHIP_ERASE_2MB_READY_WAIT_JIFFIES	(40UL * HZ)

These timers values are used at the spi-nor layer for waiting for an
operation to complete.  It is the time spent waiting for Work In Progress
to clear.

The timer value I'm using, QSPI_POLL_TIMEOUT_US, is used at a lower layer.
This timer value is used for the time it takes to send the opcode and tx 
data and receive any bytes from the flash.  While 10 seconds may seem 
long, I am just trying to avoid a system hang when there is a catastrophic
failure with the flash controller in the FPGA or the flash part itself.

I certainly could be missing something with regards to the timeouts, but I 
think 10s for QSPI_POLL_TIMEOUT_US doesn't hurt, and it definately helps 
when something goes wrong.

Thanks for the feedback,
Matthew Gerlach

>
> -- 
> Best regards,
> Marek Vasut
>

Re: [PATCH v2 2/3] mtd: spi-nor: Altera ASMI Parallel II IP Core

[Marek Vasut]

On 10/13/2017 09:24 PM, matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org wrote:

[ culling the ridiculous cc list ]

> On Wed, 11 Oct 2017, Marek Vasut wrote:
> 
>> On 10/11/2017 07:00 PM, matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org wrote:
>>>
>>>
>>> On Tue, 10 Oct 2017, Marek Vasut wrote:
>>>
>>>> On 09/20/2017 08:28 PM, matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org wrote:
>>>>> From: Matthew Gerlach <matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>>>
>>>>> This patch adds support for a spi-nor, platform driver for the
>>>>> Altera ASMI Parallel II IP Core.  The intended use case is to be able
>>>>> to update the flash used to load a FPGA at power up with mtd-utils.
>>>>>
>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>>> ---
>>>>> v2:
>>>>>     minor checkpatch fixing by Wu Hao <hao.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>>     Use read_dummy value as suggested by Cyrille Pitchen.
>>>>>     Don't assume 4 byte addressing (Cryille Pichecn and Marek Vasut).
>>>>>     Fixed #define indenting as suggested by Marek Vasut.
>>>>>     Added units to timer values as suggested by Marek Vasut.
>>>>>     Use io(read|write)8_rep() as suggested by Marek Vasut.
>>>>>     Renamed function prefixed with __ as suggested by Marek Vasut.
>>>>
>>>> [...]
>>>>
>>>>> +#define QSPI_ACTION_REG            0
>>>>> +#define QSPI_ACTION_RST            BIT(0)
>>>>> +#define QSPI_ACTION_EN            BIT(1)
>>>>> +#define QSPI_ACTION_SC            BIT(2)
>>>>> +#define QSPI_ACTION_CHIP_SEL_SFT    4
>>>>> +#define QSPI_ACTION_DUMMY_SFT        8
>>>>> +#define QSPI_ACTION_READ_BACK_SFT    16
>>>>> +
>>>>> +#define QSPI_FIFO_CNT_REG        4
>>>>> +#define QSPI_FIFO_DEPTH            0x200
>>>>> +#define QSPI_FIFO_CNT_MSK        0x3ff
>>>>> +#define QSPI_FIFO_CNT_RX_SFT        0
>>>>> +#define QSPI_FIFO_CNT_TX_SFT        12
>>>>> +
>>>>> +#define QSPI_DATA_REG            0x8
>>>>> +
>>>>> +#define QSPI_POLL_TIMEOUT_US        10000000
>>>>
>>>> 10 s poll timeout ? :)
>>>
>>> Hi Marek,
>>>
>>> The 10s timeout is fairly arbitrary.  In other words, I pulled it out of
>>> thin air.  Can you suggest a better timeout?  From a practical
>>> standpoint 10s seemed to be much better than no timeout when I was
>>> debugging bad FPGA images.  Without a timeout I was hanging the system
>>> when the FPGA image failed.  With this timeout, we get a nice message
>>> and Linux keeps running happily.
>> AFAIK the SPI subsystem has a timeout which is adaptive to the bus
>> clock, maybe that's what you want to use here ?
> 
> Hi Marek,
> 
> I looked in spi-nor.c, and I see the following two macros:
> #define DEFAULT_READY_WAIT_JIFFIES        (40UL * HZ)
> #define CHIP_ERASE_2MB_READY_WAIT_JIFFIES    (40UL * HZ)
> 
> These timers values are used at the spi-nor layer for waiting for an
> operation to complete.  It is the time spent waiting for Work In Progress
> to clear.
> 
> The timer value I'm using, QSPI_POLL_TIMEOUT_US, is used at a lower layer.
> This timer value is used for the time it takes to send the opcode and tx
> data and receive any bytes from the flash.  While 10 seconds may seem
> long, I am just trying to avoid a system hang when there is a catastrophic
> failure with the flash controller in the FPGA or the flash part itself.

It takes 10s to detect catastrophic failure ? I'd expect you can scale
that timeout value by the bus clock speed.

We have ie. this in drivers/spi/spi.c:

9                                 ret = 0;
1040                                 ms = 8LL * 1000LL * xfer->len;
1041                                 do_div(ms, xfer->speed_hz);
1042                                 ms += ms + 200; /* some tolerance */
1043
1044                                 if (ms > UINT_MAX)
1045                                         ms = UINT_MAX;
1046
1047                                 ms =
wait_for_completion_timeout(&master->xfer_completion,
1048
msecs_to_jiffies(ms));

> I certainly could be missing something with regards to the timeouts, but
> I think 10s for QSPI_POLL_TIMEOUT_US doesn't hurt, and it definately
> helps when something goes wrong.
> 
> Thanks for the feedback,
> Matthew Gerlach
> 
>>
>> -- 
>> Best regards,
>> Marek Vasut
>>


-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] mtd: spi-nor: add flag for reading dummy cycles from nv cfg reg

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

This patch is a work around for some non-standard behavior
of EPCQ flash parts:

https://www.altera.com/documentation/wtw1396921531042.html#wtw1396921651224

These flash parts are generally used to configure Intel/Altera FPGAs
on power up.  These parts report a JEDEC id of the Micron part at the core,
but have a different number of dummy cycles than specified in the Micron
data sheet.  The number of required dummy cycles can be read from the
Non-Volatile Configuration register.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/mtd/spi-nor/altera-asmip2.c | 31 ++++++++++++++++++++++++++-----
 include/linux/mtd/altera-asmip2.h   |  3 +++
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi-nor/altera-asmip2.c b/drivers/mtd/spi-nor/altera-asmip2.c
index a977765..d9cd807 100644
--- a/drivers/mtd/spi-nor/altera-asmip2.c
+++ b/drivers/mtd/spi-nor/altera-asmip2.c
@@ -40,6 +40,10 @@
 #define QSPI_POLL_TIMEOUT_US		10000000
 #define QSPI_POLL_INTERVAL_US		5
 
+#define SPINOR_OP_RD_NVCFG		0xb5
+#define NVCFG_DUMMY_SFT			12
+#define NVCFG_DUMMY_MASK		0xf
+
 struct altera_asmip2 {
 	void __iomem *csr_base;
 	u32 num_flashes;
@@ -231,7 +235,8 @@ static void altera_asmip2_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
 }
 
 static int altera_asmip2_setup_banks(struct device *dev,
-				      u32 bank, struct device_node *np)
+				     u32 bank, struct device_node *np,
+				     u32 flags)
 {
 	const struct spi_nor_hwcaps hwcaps = {
 		.mask = SNOR_HWCAPS_READ |
@@ -241,6 +246,7 @@ static int altera_asmip2_setup_banks(struct device *dev,
 	struct altera_asmip2 *q = dev_get_drvdata(dev);
 	struct altera_asmip2_flash *flash;
 	struct spi_nor *nor;
+	u16 nvcfg;
 	int ret = 0;
 
 	if (bank > q->num_flashes - 1)
@@ -273,6 +279,20 @@ static int altera_asmip2_setup_banks(struct device *dev,
 		return ret;
 	}
 
+	if (flags & ALTERA_ASMIP2_FLASH_FLG_RD_NVCFG) {
+		ret = altera_asmip2_read_reg(nor, SPINOR_OP_RD_NVCFG,
+		 			     (u8*)&nvcfg, sizeof(nvcfg));
+
+		if (ret) {
+			dev_err(nor->dev,
+				"failed to read NV Configuration register\n");
+			return ret;
+		}
+
+		nor->read_dummy = (nvcfg >> NVCFG_DUMMY_SFT) & NVCFG_DUMMY_MASK;
+		dev_info(nor->dev, "%s dummy %d\n", __func__, nor->read_dummy);
+	}
+
 	ret =  mtd_device_register(&nor->mtd, NULL, 0);
 
 	return ret;
@@ -308,7 +328,7 @@ static int altera_asmip2_create(struct device *dev, void __iomem *csr_base)
 }
 
 static int altera_asmip2_add_bank(struct device *dev,
-			 u32 bank, struct device_node *np)
+			 u32 bank, struct device_node *np, u32 flags)
 {
 	struct altera_asmip2 *q = dev_get_drvdata(dev);
 
@@ -317,7 +337,7 @@ static int altera_asmip2_add_bank(struct device *dev,
 
 	q->num_flashes++;
 
-	return altera_asmip2_setup_banks(dev, bank, np);
+	return altera_asmip2_setup_banks(dev, bank, np, flags);
 }
 
 static int altera_asmip2_remove_banks(struct device *dev)
@@ -361,7 +381,8 @@ static int altera_asmip2_probe_with_pdata(struct platform_device *pdev,
 	}
 
 	for (i = 0; i < qdata->num_chip_sel; i++) {
-		ret = altera_asmip2_add_bank(dev, i, NULL);
+		ret = altera_asmip2_add_bank(dev, i, NULL,
+					     qdata->flash_flags[i]);
 		if (ret) {
 			dev_err(dev, "failed to add qspi bank %d\n", ret);
 			break;
@@ -414,7 +435,7 @@ static int altera_asmip2_probe(struct platform_device *pdev)
 			goto error;
 		}
 
-		if (altera_asmip2_add_bank(dev, bank, pp)) {
+		if (altera_asmip2_add_bank(dev, bank, pp, 0)) {
 			dev_err(dev, "failed to add bank %u\n", bank);
 			goto error;
 		}
diff --git a/include/linux/mtd/altera-asmip2.h b/include/linux/mtd/altera-asmip2.h
index 580c43c..185a9b2 100644
--- a/include/linux/mtd/altera-asmip2.h
+++ b/include/linux/mtd/altera-asmip2.h
@@ -16,9 +16,12 @@
 #define ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP 3
 #define ALTERA_ASMIP2_RESOURCE_SIZE 0x10
 
+#define ALTERA_ASMIP2_FLASH_FLG_RD_NVCFG	BIT(0)
+
 struct altera_asmip2_plat_data {
 	void __iomem *csr_base;
 	u32 num_chip_sel;
+	u32 flash_flags[ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP];
 };
 
 #endif
-- 
2.7.4

Re: [PATCH v2 3/3] mtd: spi-nor: add flag for reading dummy cycles from nv cfg reg

[Cyrille Pitchen]

Hi Matthew

NAK for this patch

Le 20/09/2017 à 20:28, matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org a écrit :
> From: Matthew Gerlach <matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> This patch is a work around for some non-standard behavior
> of EPCQ flash parts:
> 
> https://www.altera.com/documentation/wtw1396921531042.html#wtw1396921651224
>

>From the above documentation:
"""
Write Non-Volatile Configuration Register Operation

You need to write the non-volatile configuration registers for EPCQ-L
devices for different configuration schemes. If you are using the .jic
file, the Quartus® Prime programmer sets the number of dummy clock
cycles and address bytes. If you are using an external programmer tools
(3rd party programmer tools), you must set the non-volatile
configuration registers.

To set the non-volatile configuration register, follow these steps:

    Execute the write enable operation.
    Execute the write non-volatile configuration register operation.
    Set the 16-bit register value.

Set the 16-bit register value as b'1110 1110 xxxx 1111 where xxxx is the
dummy clock value. When the xxxx value is from 0001 to 1110, the dummy
clock value is from 1 to 14. When xxxx is 0000 or 1111, the dummy clock
value is at the default value, which is 8 for standard fast read (AS x1)
mode and 10 for extended quad input fast read (AS x4) mode.
"""

AFAIU, it is stated that you can set the number of dummy cycle to either
0000b or 1111b, the default value. There is no valid reason to use any
other value, like there is no valid reason to tune the number of dummy
clock cycles. Just keep the default settings, please!

If we start to play changing the number of dummy cycles it would be real
mess to maintain.

First, should we read the value to be used from some register or should
we force this value instead by writing that register ?
Some would prefer reading whereas other would prefer updating...

Moreover, the method to read or write the number of dummy cycles is not
standard and is manufacturer specific:

- Micron uses 2 registers: the Volatile Configuration Register and the
Non-Volatile configuration Register.

- Macronix uses another register but doesn't store the number of dummy
cycles directly: instead this manufacturer uses codes. So we would have
to store tables mapping codes <-> number dummy clock cycles

- Spansion/Cypress also uses code tables and I already know that
depending on the memory part number the code is either 2bit or 4bit and
stored in different registers. Damned!

If I allow you to tune the number of dummy clock cycles for Micron
memory it would be fair that I allow other people to tune this number
for other memory manufacturers. However it would be a real pain to
maintain because there is no standard for that hence manufacturers just
do what they want and change things when they want. Far too
unpredictable, IMHO.

So to avoid a messy situation, the rule is simple: SPI-NOR memory MUST
be configured in their default factory settings when spi_nor_scan() is
called. Your memory has the very same JEDEC ID as some Micron memory,
then it should behave the exact same way: in this case the value for the
number of dummy clock cycles should be set to 0000b or 1111b in the NVCR
(and VCR too).


> These flash parts are generally used to configure Intel/Altera FPGAs
> on power up.  These parts report a JEDEC id of the Micron part at the core,
> but have a different number of dummy cycles than specified in the Micron
> data sheet.  The number of required dummy cycles can be read from the
> Non-Volatile Configuration register.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/mtd/spi-nor/altera-asmip2.c | 31 ++++++++++++++++++++++++++-----
>  include/linux/mtd/altera-asmip2.h   |  3 +++
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/altera-asmip2.c b/drivers/mtd/spi-nor/altera-asmip2.c
> index a977765..d9cd807 100644
> --- a/drivers/mtd/spi-nor/altera-asmip2.c
> +++ b/drivers/mtd/spi-nor/altera-asmip2.c
> @@ -40,6 +40,10 @@
>  #define QSPI_POLL_TIMEOUT_US		10000000
>  #define QSPI_POLL_INTERVAL_US		5
>  
> +#define SPINOR_OP_RD_NVCFG		0xb5
> +#define NVCFG_DUMMY_SFT			12
> +#define NVCFG_DUMMY_MASK		0xf
> +
>  struct altera_asmip2 {
>  	void __iomem *csr_base;
>  	u32 num_flashes;
> @@ -231,7 +235,8 @@ static void altera_asmip2_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>  }
>  
>  static int altera_asmip2_setup_banks(struct device *dev,
> -				      u32 bank, struct device_node *np)
> +				     u32 bank, struct device_node *np,
> +				     u32 flags)
>  {
>  	const struct spi_nor_hwcaps hwcaps = {
>  		.mask = SNOR_HWCAPS_READ |
> @@ -241,6 +246,7 @@ static int altera_asmip2_setup_banks(struct device *dev,
>  	struct altera_asmip2 *q = dev_get_drvdata(dev);
>  	struct altera_asmip2_flash *flash;
>  	struct spi_nor *nor;
> +	u16 nvcfg;
>  	int ret = 0;
>  
>  	if (bank > q->num_flashes - 1)
> @@ -273,6 +279,20 @@ static int altera_asmip2_setup_banks(struct device *dev,
>  		return ret;
>  	}
>  
> +	if (flags & ALTERA_ASMIP2_FLASH_FLG_RD_NVCFG) {
> +		ret = altera_asmip2_read_reg(nor, SPINOR_OP_RD_NVCFG,
> +		 			     (u8*)&nvcfg, sizeof(nvcfg));
> +
> +		if (ret) {
> +			dev_err(nor->dev,
> +				"failed to read NV Configuration register\n");
> +			return ret;
> +		}
> +
> +		nor->read_dummy = (nvcfg >> NVCFG_DUMMY_SFT) & NVCFG_DUMMY_MASK;
> +		dev_info(nor->dev, "%s dummy %d\n", __func__, nor->read_dummy);
> +	}
> +

You have forgotten the special case for values 0000b and 1111b (default
settings). For those 2 values, the actual number of dummy clock cycles is:
- 0 for Read (03h/13h)
- 8 for Fast Read 1-1-1 (0Bh/0Ch)
- 8 for Fast Read 1-1-2 (3Bh/3Ch)
- 8 for Fast Read 1-2-2 (BBh/BCh)
- 8 for Fast Read 1-1-4 (6Bh/6Ch)
- 10 for Fast Read 1-4-4 (EBh/ECh)

Besides, the value read from the Non-Volatile Configuration Register is
the value loaded into the Volatile Configuration Register at power-up.
The actual number of dummy clock cycles to be used by Fast Read commands
should be read from the Volatile Configuration Register.

Anyway, this not the way to go.

Best regards,

Cyrille

>  	ret =  mtd_device_register(&nor->mtd, NULL, 0);
>  
>  	return ret;
> @@ -308,7 +328,7 @@ static int altera_asmip2_create(struct device *dev, void __iomem *csr_base)
>  }
>  
>  static int altera_asmip2_add_bank(struct device *dev,
> -			 u32 bank, struct device_node *np)
> +			 u32 bank, struct device_node *np, u32 flags)
>  {
>  	struct altera_asmip2 *q = dev_get_drvdata(dev);
>  
> @@ -317,7 +337,7 @@ static int altera_asmip2_add_bank(struct device *dev,
>  
>  	q->num_flashes++;
>  
> -	return altera_asmip2_setup_banks(dev, bank, np);
> +	return altera_asmip2_setup_banks(dev, bank, np, flags);
>  }
>  
>  static int altera_asmip2_remove_banks(struct device *dev)
> @@ -361,7 +381,8 @@ static int altera_asmip2_probe_with_pdata(struct platform_device *pdev,
>  	}
>  
>  	for (i = 0; i < qdata->num_chip_sel; i++) {
> -		ret = altera_asmip2_add_bank(dev, i, NULL);
> +		ret = altera_asmip2_add_bank(dev, i, NULL,
> +					     qdata->flash_flags[i]);
>  		if (ret) {
>  			dev_err(dev, "failed to add qspi bank %d\n", ret);
>  			break;
> @@ -414,7 +435,7 @@ static int altera_asmip2_probe(struct platform_device *pdev)
>  			goto error;
>  		}
>  
> -		if (altera_asmip2_add_bank(dev, bank, pp)) {
> +		if (altera_asmip2_add_bank(dev, bank, pp, 0)) {
>  			dev_err(dev, "failed to add bank %u\n", bank);
>  			goto error;
>  		}
> diff --git a/include/linux/mtd/altera-asmip2.h b/include/linux/mtd/altera-asmip2.h
> index 580c43c..185a9b2 100644
> --- a/include/linux/mtd/altera-asmip2.h
> +++ b/include/linux/mtd/altera-asmip2.h
> @@ -16,9 +16,12 @@
>  #define ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP 3
>  #define ALTERA_ASMIP2_RESOURCE_SIZE 0x10
>  
> +#define ALTERA_ASMIP2_FLASH_FLG_RD_NVCFG	BIT(0)
> +
>  struct altera_asmip2_plat_data {
>  	void __iomem *csr_base;
>  	u32 num_chip_sel;
> +	u32 flash_flags[ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP];
>  };
>  
>  #endif
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] mtd: spi-nor: add flag for reading dummy cycles from nv cfg reg

[matthew.gerlach-VuQAYsv1563Yd54FQh9/CA]

[-- Attachment #1: Type: text/plain, Size: 9919 bytes --]


Hi Cyrille,

Thanks for the feedback.  See my comments in line below.

Matthew Gerlach


On Tue, 10 Oct 2017, Cyrille Pitchen wrote:

> Hi Matthew
>
> NAK for this patch
>
> Le 20/09/2017 à 20:28, matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org a écrit :
>> From: Matthew Gerlach <matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>
>> This patch is a work around for some non-standard behavior
>> of EPCQ flash parts:
>>
>> https://www.altera.com/documentation/wtw1396921531042.html#wtw1396921651224
>>
>
> From the above documentation:
> """
> Write Non-Volatile Configuration Register Operation
>
> You need to write the non-volatile configuration registers for EPCQ-L
> devices for different configuration schemes. If you are using the .jic
> file, the Quartus® Prime programmer sets the number of dummy clock
> cycles and address bytes. If you are using an external programmer tools
> (3rd party programmer tools), you must set the non-volatile
> configuration registers.
>
> To set the non-volatile configuration register, follow these steps:
>
>    Execute the write enable operation.
>    Execute the write non-volatile configuration register operation.
>    Set the 16-bit register value.
>

This documentation does not match my experience with the flash.  No where
am I setting the non-volatile configuration register.  After power up, I 
just read the register and observe 10 dummy clock cycles, which is the 
only value that results in successfully reading correct data from the 
flash.  I suspect the FPGA is doing the same at power up.

> Set the 16-bit register value as b'1110 1110 xxxx 1111 where xxxx is the
> dummy clock value. When the xxxx value is from 0001 to 1110, the dummy
> clock value is from 1 to 14. When xxxx is 0000 or 1111, the dummy clock
> value is at the default value, which is 8 for standard fast read (AS x1)
> mode and 10 for extended quad input fast read (AS x4) mode.
> """
>
> AFAIU, it is stated that you can set the number of dummy cycle to either
> 0000b or 1111b, the default value. There is no valid reason to use any
> other value, like there is no valid reason to tune the number of dummy
> clock cycles. Just keep the default settings, please!

The default Micron setting simply does not work.

>
> If we start to play changing the number of dummy cycles it would be real
> mess to maintain.

I completely understand the maintainance issue.

>
> First, should we read the value to be used from some register or should
> we force this value instead by writing that register ?
> Some would prefer reading whereas other would prefer updating...
>
> Moreover, the method to read or write the number of dummy cycles is not
> standard and is manufacturer specific:

Yes I get it that is manufacturer specific, and this would be another 
example of yet a different manufacturer's approach.

>
> - Micron uses 2 registers: the Volatile Configuration Register and the
> Non-Volatile configuration Register.
>
> - Macronix uses another register but doesn't store the number of dummy
> cycles directly: instead this manufacturer uses codes. So we would have
> to store tables mapping codes <-> number dummy clock cycles
>
> - Spansion/Cypress also uses code tables and I already know that
> depending on the memory part number the code is either 2bit or 4bit and
> stored in different registers. Damned!
>
> If I allow you to tune the number of dummy clock cycles for Micron
> memory it would be fair that I allow other people to tune this number
> for other memory manufacturers. However it would be a real pain to
> maintain because there is no standard for that hence manufacturers just
> do what they want and change things when they want. Far too
> unpredictable, IMHO.
>
> So to avoid a messy situation, the rule is simple: SPI-NOR memory MUST
> be configured in their default factory settings when spi_nor_scan() is
> called. Your memory has the very same JEDEC ID as some Micron memory,
> then it should behave the exact same way: in this case the value for the
> number of dummy clock cycles should be set to 0000b or 1111b in the NVCR
> (and VCR too).

I think this is crux of the problem.  The flash chip is essentially lying 
about what kind of chip it is.  It says it is a Micron memory, but it 
certainly is not behaving like a Micron chip.  I believe the purpose of 
these EPCQ chips is somehow make it easier for the FPGA to configure 
itself on power up, but it is doing it in a very non-standard and 
non-maintainable way.  I believe the FPGA is reading dummy cycles from the
NVCR on power up.  How else would it know how many dummy cycles to use?


The good news is that these chips are on path to 
end of life, and moving forward we will be using standard flash parts. 
The bad news is that there are a lot of these chips already in the wild, 
and I suspect more are in the pipeline.

At this point I see no path forward to being able to upstream support for 
these EPCQ chips.

>
>
>> These flash parts are generally used to configure Intel/Altera FPGAs
>> on power up.  These parts report a JEDEC id of the Micron part at the core,
>> but have a different number of dummy cycles than specified in the Micron
>> data sheet.  The number of required dummy cycles can be read from the
>> Non-Volatile Configuration register.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>  drivers/mtd/spi-nor/altera-asmip2.c | 31 ++++++++++++++++++++++++++-----
>>  include/linux/mtd/altera-asmip2.h   |  3 +++
>>  2 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/altera-asmip2.c b/drivers/mtd/spi-nor/altera-asmip2.c
>> index a977765..d9cd807 100644
>> --- a/drivers/mtd/spi-nor/altera-asmip2.c
>> +++ b/drivers/mtd/spi-nor/altera-asmip2.c
>> @@ -40,6 +40,10 @@
>>  #define QSPI_POLL_TIMEOUT_US		10000000
>>  #define QSPI_POLL_INTERVAL_US		5
>>
>> +#define SPINOR_OP_RD_NVCFG		0xb5
>> +#define NVCFG_DUMMY_SFT			12
>> +#define NVCFG_DUMMY_MASK		0xf
>> +
>>  struct altera_asmip2 {
>>  	void __iomem *csr_base;
>>  	u32 num_flashes;
>> @@ -231,7 +235,8 @@ static void altera_asmip2_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>>  }
>>
>>  static int altera_asmip2_setup_banks(struct device *dev,
>> -				      u32 bank, struct device_node *np)
>> +				     u32 bank, struct device_node *np,
>> +				     u32 flags)
>>  {
>>  	const struct spi_nor_hwcaps hwcaps = {
>>  		.mask = SNOR_HWCAPS_READ |
>> @@ -241,6 +246,7 @@ static int altera_asmip2_setup_banks(struct device *dev,
>>  	struct altera_asmip2 *q = dev_get_drvdata(dev);
>>  	struct altera_asmip2_flash *flash;
>>  	struct spi_nor *nor;
>> +	u16 nvcfg;
>>  	int ret = 0;
>>
>>  	if (bank > q->num_flashes - 1)
>> @@ -273,6 +279,20 @@ static int altera_asmip2_setup_banks(struct device *dev,
>>  		return ret;
>>  	}
>>
>> +	if (flags & ALTERA_ASMIP2_FLASH_FLG_RD_NVCFG) {
>> +		ret = altera_asmip2_read_reg(nor, SPINOR_OP_RD_NVCFG,
>> +		 			     (u8*)&nvcfg, sizeof(nvcfg));
>> +
>> +		if (ret) {
>> +			dev_err(nor->dev,
>> +				"failed to read NV Configuration register\n");
>> +			return ret;
>> +		}
>> +
>> +		nor->read_dummy = (nvcfg >> NVCFG_DUMMY_SFT) & NVCFG_DUMMY_MASK;
>> +		dev_info(nor->dev, "%s dummy %d\n", __func__, nor->read_dummy);
>> +	}
>> +
>
> You have forgotten the special case for values 0000b and 1111b (default
> settings). For those 2 values, the actual number of dummy clock cycles is:
> - 0 for Read (03h/13h)
> - 8 for Fast Read 1-1-1 (0Bh/0Ch)
> - 8 for Fast Read 1-1-2 (3Bh/3Ch)
> - 8 for Fast Read 1-2-2 (BBh/BCh)
> - 8 for Fast Read 1-1-4 (6Bh/6Ch)
> - 10 for Fast Read 1-4-4 (EBh/ECh)
>
> Besides, the value read from the Non-Volatile Configuration Register is
> the value loaded into the Volatile Configuration Register at power-up.
> The actual number of dummy clock cycles to be used by Fast Read commands
> should be read from the Volatile Configuration Register.
>
> Anyway, this not the way to go.
>
> Best regards,
>
> Cyrille
>
>>  	ret =  mtd_device_register(&nor->mtd, NULL, 0);
>>
>>  	return ret;
>> @@ -308,7 +328,7 @@ static int altera_asmip2_create(struct device *dev, void __iomem *csr_base)
>>  }
>>
>>  static int altera_asmip2_add_bank(struct device *dev,
>> -			 u32 bank, struct device_node *np)
>> +			 u32 bank, struct device_node *np, u32 flags)
>>  {
>>  	struct altera_asmip2 *q = dev_get_drvdata(dev);
>>
>> @@ -317,7 +337,7 @@ static int altera_asmip2_add_bank(struct device *dev,
>>
>>  	q->num_flashes++;
>>
>> -	return altera_asmip2_setup_banks(dev, bank, np);
>> +	return altera_asmip2_setup_banks(dev, bank, np, flags);
>>  }
>>
>>  static int altera_asmip2_remove_banks(struct device *dev)
>> @@ -361,7 +381,8 @@ static int altera_asmip2_probe_with_pdata(struct platform_device *pdev,
>>  	}
>>
>>  	for (i = 0; i < qdata->num_chip_sel; i++) {
>> -		ret = altera_asmip2_add_bank(dev, i, NULL);
>> +		ret = altera_asmip2_add_bank(dev, i, NULL,
>> +					     qdata->flash_flags[i]);
>>  		if (ret) {
>>  			dev_err(dev, "failed to add qspi bank %d\n", ret);
>>  			break;
>> @@ -414,7 +435,7 @@ static int altera_asmip2_probe(struct platform_device *pdev)
>>  			goto error;
>>  		}
>>
>> -		if (altera_asmip2_add_bank(dev, bank, pp)) {
>> +		if (altera_asmip2_add_bank(dev, bank, pp, 0)) {
>>  			dev_err(dev, "failed to add bank %u\n", bank);
>>  			goto error;
>>  		}
>> diff --git a/include/linux/mtd/altera-asmip2.h b/include/linux/mtd/altera-asmip2.h
>> index 580c43c..185a9b2 100644
>> --- a/include/linux/mtd/altera-asmip2.h
>> +++ b/include/linux/mtd/altera-asmip2.h
>> @@ -16,9 +16,12 @@
>>  #define ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP 3
>>  #define ALTERA_ASMIP2_RESOURCE_SIZE 0x10
>>
>> +#define ALTERA_ASMIP2_FLASH_FLG_RD_NVCFG	BIT(0)
>> +
>>  struct altera_asmip2_plat_data {
>>  	void __iomem *csr_base;
>>  	u32 num_chip_sel;
>> +	u32 flash_flags[ALTERA_ASMIP2_MAX_NUM_FLASH_CHIP];
>>  };
>>
>>  #endif
>>
>
>

Re: [PATCH v2 0/3] Altera ASMI Parallel II IP Core

[matthew.gerlach]

Hi Everyone,

Thanks to Rob Herring for Acking the device tree bindings part of the 
patch.  Does anyone have any feedback for the rest of the patch set?

Thanks,

Matthew Gerlach

On Wed, 20 Sep 2017, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> This patch set adds a spi-nor flash driver for the Altera ASMI Parallel II
> IP Core.  This driver was created based on feedback from Marek Vasut,
> Cyrill Pitchen, and Michal Suchanek regarding Version 2 of the Altera
> Quadspi Controller: https://lkml.org/lkml/2017/6/26/518
>
> The overall problem with Version 2 of the Altera Quadspi Controller and
> its driver was the fact that there was much duplication of code and logic
> with the spi-nor framework.  This new combination of fpga hardware and
> software "gets out of the way" of the spi-nor framework.  The result is a
> much simpler driver with the spi-nor framework performing the bulk of the work.
>
> Patch 1 contains the device tree bindings for the driver for
> the Altera ASMI-Parallel II IP Core.
>
> Patch 2 contains the driver code for the Altera ASMI-Parallel II IP Core.
> This driver supports being configured via a device tree or with platform
> data.  In the later case, the memory for the registers has been remapped.
>
> Patch 3 contains a work around for some non-standard behavior of EPCQ flash.
>
> Matthew Gerlach (3):
>  dt-bindings: mtd: Altera ASMI Parallel II IP Core
>  mtd: spi-nor: Altera ASMI Parallel II IP Core
>  mtd: spi-nor: add flag for reading dummy cycles from nv cfg reg
>
> .../devicetree/bindings/mtd/altera-asmip2.txt      |  15 +
> MAINTAINERS                                        |   7 +
> drivers/mtd/spi-nor/Kconfig                        |   6 +
> drivers/mtd/spi-nor/Makefile                       |   1 +
> drivers/mtd/spi-nor/altera-asmip2.c                | 478 +++++++++++++++++++++
> include/linux/mtd/altera-asmip2.h                  |  27 ++
> 6 files changed, 534 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/altera-asmip2.txt
> create mode 100644 drivers/mtd/spi-nor/altera-asmip2.c
> create mode 100644 include/linux/mtd/altera-asmip2.h
>
> -- 
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>