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

Message ID	1502043844-3626-3-git-send-email-matthew.gerlach@linux.intel.com (mailing list archive)
State	Superseded, archived
Headers	show Return-Path: <linux-fpga-owner@kernel.org> From: matthew.gerlach@linux.intel.com To: vndao@altera.com, dwmw2@infradead.org, computersforpeace@gmail.com, boris.brezillon@free-electrons.com, marek.vasut@gmail.com, richard@nod.at, cyrille.pitchen@wedev4u.fr, robh+dt@kernel.org, mark.rutland@arm.com, linux-mtd@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org, davem@davemloft.net, mchehab@kernel.org, linux-fpga@vger.kernel.org Cc: Matthew Gerlach <matthew.gerlach@linux.intel.com> Subject: [PATCH 2/2] mtd: spi-nor: Altera ASMI Parallel II IP Core Date: Sun, 6 Aug 2017 11:24:03 -0700 Message-Id: <1502043844-3626-3-git-send-email-matthew.gerlach@linux.intel.com> In-Reply-To: <1502043844-3626-1-git-send-email-matthew.gerlach@linux.intel.com> References: <1502043844-3626-1-git-send-email-matthew.gerlach@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-fpga-owner@vger.kernel.org Precedence: bulk

Matthew Gerlach Aug. 6, 2017, 6:24 p.m. UTC

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

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 MAINTAINERS                         |   7 +
 drivers/mtd/spi-nor/Kconfig         |   6 +
 drivers/mtd/spi-nor/Makefile        |   3 +-
 drivers/mtd/spi-nor/altera-asmip2.c | 474 ++++++++++++++++++++++++++++++++++++
 include/linux/mtd/altera-asmip2.h   |  24 ++
 5 files changed, 513 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/spi-nor/altera-asmip2.c
 create mode 100644 include/linux/mtd/altera-asmip2.h

Marek Vasut Aug. 6, 2017, 7:27 p.m. UTC | #1

On 08/06/2017 08:24 PM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Thanks for the descriptive commit message. Could you explain what this
patch is all about ?

> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>

[...]

> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index c5f171d..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
> @@ -9,4 +10,4 @@ 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
> -obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
> \ No newline at end of file
> +obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o

Drop this hunk

> diff --git a/drivers/mtd/spi-nor/altera-asmip2.c b/drivers/mtd/spi-nor/altera-asmip2.c
> new file mode 100644
> index 0000000..2095f2e
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/altera-asmip2.c
> @@ -0,0 +1,474 @@
> +/*
> + * 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

Indent the value with a tab at least ...

> +#define QSPI_DATA_REG 0x8
> +
> +#define QSPI_POLL_TIMEOUT 10000000

In what units is this ?

> +#define QSPI_POLL_INTERVAL 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;
> +	u32 bank;
> +};
> +
> +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;
> +	int i;
> +
> +	if ((len + 1) > QSPI_FIFO_DEPTH) {
> +		dev_err(q->dev, "%s bad len %d > %d\n",
> +			__func__, len + 1, QSPI_FIFO_DEPTH);
> +		return -EINVAL;
> +	}
> +
> +	writel(opcode, q->csr_base + QSPI_DATA_REG);
> +
> +	for (i = 0; i < len; i++) {
> +		writel((u32)val[i], q->csr_base + QSPI_DATA_REG);

iowrite32_rep() ?

> +	}
> +
> +	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,
> +				 QSPI_POLL_TIMEOUT);
> +	if (ret)
> +		dev_err(q->dev, "%s timed out\n", __func__);

So if the poll fails , you ignore the failure and continue enabling
whatever action you enable here ?

> +	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;
> +	int i;
> +
> +	if (len > QSPI_FIFO_DEPTH) {
> +		dev_err(q->dev, "%s bad len %d > %d\n",
> +			__func__, len, QSPI_FIFO_DEPTH);
> +		return -EINVAL;
> +	}
> +
> +	writel(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, QSPI_POLL_TIMEOUT);
> +
> +	if (!ret)
> +		for (i = 0; i < len; i++) {
> +			reg = readl(q->csr_base + QSPI_DATA_REG);
> +			val[i] = (u8)(reg & 0xff);

ioread32_rep , plus same comments as for the write function

> +		}
> +	else
> +		dev_err(q->dev, "%s timeout\n", __func__);
> +
> +	writel(QSPI_ACTION_EN, q->csr_base + QSPI_ACTION_REG);
> +
> +	return ret;
> +}
> +
> +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, i;
> +	u32 reg;
> +	int ret;
> +
> +	bytes_to_read = min_t(size_t, len, QSPI_FIFO_DEPTH);
> +
> +	writel(nor->read_opcode, q->csr_base + QSPI_DATA_REG);
> +
> +	writel((from & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
> +	writel((from & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
> +	writel((from & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
> +	writel((from & 0x000000ff), q->csr_base + QSPI_DATA_REG);

Use a for() loop ?

> +	reg = QSPI_ACTION_EN | QSPI_ACTION_SC |
> +		(10 << 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,
> +				 QSPI_POLL_TIMEOUT);
> +	if (ret) {
> +		dev_err(q->dev, "%s timed out\n", __func__);
> +		bytes_to_read = 0;
> +	} else
> +		for (i = 0; i < bytes_to_read; i++) {
> +			reg = readl(q->csr_base + QSPI_DATA_REG);
> +			*buf++ = (u8)(reg & 0xff);

ioread8_rep or something ?

> +		}
> +
> +	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, i;
> +	u32 reg;
> +	int ret;
> +
> +	bytes_to_write = min_t(size_t, len, (QSPI_FIFO_DEPTH - 5));
> +
> +	writel(nor->program_opcode, q->csr_base + QSPI_DATA_REG);
> +
> +	writel((to & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
> +	writel((to & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
> +	writel((to & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
> +	writel((to & 0x000000ff), q->csr_base + QSPI_DATA_REG);
> +
> +	for (i = 0; i < bytes_to_write; i++) {
> +		reg = (u32)*buf++;
> +		writel(reg, q->csr_base + QSPI_DATA_REG);
> +	}
> +
> +	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,
> +				 QSPI_POLL_TIMEOUT);
> +
> +	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);

We should factor this mutex stuff into the SPI NOR framework, this
pattern is repeating itself way too often.

> +	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;
> +	char modalias[40] = {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;
> +	flash->bank = bank;
> +
> +	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;
> +
> +	/* scanning flash and checking dev id */
> +#ifdef CONFIG_OF

Why is this here ?

> +	if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
> +		return -EINVAL;
> +#endif
> +
> +	ret = spi_nor_scan(nor, modalias, &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_qspi_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 __probe_with_data(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_qspi_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 __probe_with_data(pdev, qdata);

Avoid function names with __ prefix.

> +	if (!np) {
> +		dev_err(dev, "no device tree found %p\n", pdev);
> +		return -ENODEV;
> +	}

You might as well introduce a function to probe with of to be consistent
... or convert between pdata and ofdata and have one function for both.

> +	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_qspi_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)
> +{
> +	if (!pdev) {
> +		dev_err(&pdev->dev, "%s NULL\n", __func__);
> +		return -EINVAL;

Can this really happen ?

> +	} else {
> +		return altera_asmip2_remove_banks(&pdev->dev);
> +	}
> +}
> +
> +static const struct of_device_id altera_asmip2_id_table[] = {
> +
> +	{ .compatible = "altr,asmi_parallel2",},
> +	{}
> +};
> +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
>

Matthew Gerlach Aug. 7, 2017, 3:21 p.m. UTC | #2

On Sun, 6 Aug 2017, Marek Vasut wrote:


Hi Marek,

Thanks for the feedback.  Please see comments inline.

Matthew Gerlach

> On 08/06/2017 08:24 PM, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Thanks for the descriptive commit message. Could you explain what this
> patch is all about ?

Ok, I will add more of a comment.
>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> [...]
>
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index c5f171d..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
>> @@ -9,4 +10,4 @@ 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
>> -obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
>> \ No newline at end of file
>> +obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
>
> Drop this hunk

I will fix this.

>
>> diff --git a/drivers/mtd/spi-nor/altera-asmip2.c b/drivers/mtd/spi-nor/altera-asmip2.c
>> new file mode 100644
>> index 0000000..2095f2e
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/altera-asmip2.c
>> @@ -0,0 +1,474 @@
>> +/*
>> + * 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
>
> Indent the value with a tab at least ...

Ok, I can indent with tabs.

>
>> +#define QSPI_DATA_REG 0x8
>> +
>> +#define QSPI_POLL_TIMEOUT 10000000
>
> In what units is this ?

Units are in microseconds.  I will add a comment.

>
>> +#define QSPI_POLL_INTERVAL 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;
>> +	u32 bank;
>> +};
>> +
>> +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;
>> +	int i;
>> +
>> +	if ((len + 1) > QSPI_FIFO_DEPTH) {
>> +		dev_err(q->dev, "%s bad len %d > %d\n",
>> +			__func__, len + 1, QSPI_FIFO_DEPTH);
>> +		return -EINVAL;
>> +	}
>> +
>> +	writel(opcode, q->csr_base + QSPI_DATA_REG);
>> +
>> +	for (i = 0; i < len; i++) {
>> +		writel((u32)val[i], q->csr_base + QSPI_DATA_REG);
>
> iowrite32_rep() ?

I don't think I can use iowrite32_rep() here because writes to the
register must be 32 bits, but the data to be written is 8 bits wide.

>
>> +	}
>> +
>> +	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,
>> +				 QSPI_POLL_TIMEOUT);
>> +	if (ret)
>> +		dev_err(q->dev, "%s timed out\n", __func__);
>
> So if the poll fails , you ignore the failure and continue enabling
> whatever action you enable here ?

My intent is to put the controller in the ready state and shut off the 
failed action by clearing the QSPI_ACTION_SC bit.

>
>> +	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;
>> +	int i;
>> +
>> +	if (len > QSPI_FIFO_DEPTH) {
>> +		dev_err(q->dev, "%s bad len %d > %d\n",
>> +			__func__, len, QSPI_FIFO_DEPTH);
>> +		return -EINVAL;
>> +	}
>> +
>> +	writel(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, QSPI_POLL_TIMEOUT);
>> +
>> +	if (!ret)
>> +		for (i = 0; i < len; i++) {
>> +			reg = readl(q->csr_base + QSPI_DATA_REG);
>> +			val[i] = (u8)(reg & 0xff);
>
> ioread32_rep , plus same comments as for the write function

I don't think I can use ioread32_rep here either. The register is 32 bits, 
but the data is 8 bits.

Again, I am clearing the QSPI_ACTION_SC bit which is a one shot which must
be cleared before it can be set again.

>
>> +		}
>> +	else
>> +		dev_err(q->dev, "%s timeout\n", __func__);
>> +
>> +	writel(QSPI_ACTION_EN, q->csr_base + QSPI_ACTION_REG);
>> +
>> +	return ret;
>> +}
>> +
>> +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, i;
>> +	u32 reg;
>> +	int ret;
>> +
>> +	bytes_to_read = min_t(size_t, len, QSPI_FIFO_DEPTH);
>> +
>> +	writel(nor->read_opcode, q->csr_base + QSPI_DATA_REG);
>> +
>> +	writel((from & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
>> +	writel((from & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
>> +	writel((from & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
>> +	writel((from & 0x000000ff), q->csr_base + QSPI_DATA_REG);
>
> Use a for() loop ?
>
>> +	reg = QSPI_ACTION_EN | QSPI_ACTION_SC |
>> +		(10 << 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,
>> +				 QSPI_POLL_TIMEOUT);
>> +	if (ret) {
>> +		dev_err(q->dev, "%s timed out\n", __func__);
>> +		bytes_to_read = 0;
>> +	} else
>> +		for (i = 0; i < bytes_to_read; i++) {
>> +			reg = readl(q->csr_base + QSPI_DATA_REG);
>> +			*buf++ = (u8)(reg & 0xff);
>
> ioread8_rep or something ?
>
>> +		}
>> +
>> +	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, i;
>> +	u32 reg;
>> +	int ret;
>> +
>> +	bytes_to_write = min_t(size_t, len, (QSPI_FIFO_DEPTH - 5));
>> +
>> +	writel(nor->program_opcode, q->csr_base + QSPI_DATA_REG);
>> +
>> +	writel((to & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
>> +	writel((to & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
>> +	writel((to & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
>> +	writel((to & 0x000000ff), q->csr_base + QSPI_DATA_REG);
>> +
>> +	for (i = 0; i < bytes_to_write; i++) {
>> +		reg = (u32)*buf++;
>> +		writel(reg, q->csr_base + QSPI_DATA_REG);
>> +	}
>> +
>> +	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,
>> +				 QSPI_POLL_TIMEOUT);
>> +
>> +	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);
>
> We should factor this mutex stuff into the SPI NOR framework, this
> pattern is repeating itself way too often.

I agree this seems to be a pattern.  Should I keep it for v2 of this patch
set?

>
>> +	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;
>> +	char modalias[40] = {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;
>> +	flash->bank = bank;
>> +
>> +	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;
>> +
>> +	/* scanning flash and checking dev id */
>> +#ifdef CONFIG_OF
>
> Why is this here ?

Looking at this again, I think this can be removed.

>
>> +	if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
>> +		return -EINVAL;
>> +#endif
>> +
>> +	ret = spi_nor_scan(nor, modalias, &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_qspi_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 __probe_with_data(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_qspi_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 __probe_with_data(pdev, qdata);
>
> Avoid function names with __ prefix.

OK, I can rename the function without the __ prefix.

>
>> +	if (!np) {
>> +		dev_err(dev, "no device tree found %p\n", pdev);
>> +		return -ENODEV;
>> +	}
>
> You might as well introduce a function to probe with of to be consistent
> ... or convert between pdata and ofdata and have one function for both.

There is a subtle difference when using platform data versus of data.
The of data requires the memory to be remapped, whereas the platform data
has the memory already mapped by the parent, PCIe driver.

Either way ends up with a single call to altera_asmip2_create plus one or 
more calls to altera_qspi_add_bank, which needs to be renamed for 
consistency.

>
>> +	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_qspi_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)
>> +{
>> +	if (!pdev) {
>> +		dev_err(&pdev->dev, "%s NULL\n", __func__);
>> +		return -EINVAL;
>
> Can this really happen ?

This probably cannot happen, and dereferencing a NULL pointer is bad.
I will get rid of this.

>
>> +	} else {
>> +		return altera_asmip2_remove_banks(&pdev->dev);
>> +	}
>> +}
>> +
>> +static const struct of_device_id altera_asmip2_id_table[] = {
>> +
>> +	{ .compatible = "altr,asmi_parallel2",},
>> +	{}
>> +};
>> +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
>>
>
>
> -- 
> Best regards,
> Marek Vasut
>
--
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

Cyrille Pitchen Aug. 11, 2017, 5:20 p.m. UTC | #3

Hi Matthew,

Le 06/08/2017 à 20:24, matthew.gerlach@linux.intel.com a écrit :
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  MAINTAINERS                         |   7 +
>  drivers/mtd/spi-nor/Kconfig         |   6 +
>  drivers/mtd/spi-nor/Makefile        |   3 +-
>  drivers/mtd/spi-nor/altera-asmip2.c | 474 ++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/altera-asmip2.h   |  24 ++
>  5 files changed, 513 insertions(+), 1 deletion(-)
>  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 672b5d5..9583c1a 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 MAILBOX DRIVER
>  M:	Ley Foon Tan <lftan@altera.com>
>  L:	nios2-dev@lists.rocketboards.org (moderated for non-subscribers)
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 69c638d..eb2c522 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 c5f171d..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
> @@ -9,4 +10,4 @@ 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
> -obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
> \ No newline at end of file
> +obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-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..2095f2e
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/altera-asmip2.c
> @@ -0,0 +1,474 @@
> +/*
> + * 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 10000000
> +#define QSPI_POLL_INTERVAL 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;
> +	u32 bank;
> +};
> +
> +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;
> +	int i;
> +
> +	if ((len + 1) > QSPI_FIFO_DEPTH) {
> +		dev_err(q->dev, "%s bad len %d > %d\n",
> +			__func__, len + 1, QSPI_FIFO_DEPTH);
> +		return -EINVAL;
> +	}
> +
> +	writel(opcode, q->csr_base + QSPI_DATA_REG);
> +
> +	for (i = 0; i < len; i++) {
> +		writel((u32)val[i], q->csr_base + QSPI_DATA_REG);
> +	}
> +
> +	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,
> +				 QSPI_POLL_TIMEOUT);
> +	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;
> +	int i;
> +
> +	if (len > QSPI_FIFO_DEPTH) {
> +		dev_err(q->dev, "%s bad len %d > %d\n",
> +			__func__, len, QSPI_FIFO_DEPTH);
> +		return -EINVAL;
> +	}
> +
> +	writel(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, QSPI_POLL_TIMEOUT);
> +
> +	if (!ret)
> +		for (i = 0; i < len; i++) {
> +			reg = readl(q->csr_base + QSPI_DATA_REG);
> +			val[i] = (u8)(reg & 0xff);
> +		}
> +	else
> +		dev_err(q->dev, "%s timeout\n", __func__);
> +
> +	writel(QSPI_ACTION_EN, q->csr_base + QSPI_ACTION_REG);
> +
> +	return ret;
> +}
> +
> +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, i;
> +	u32 reg;
> +	int ret;
> +
> +	bytes_to_read = min_t(size_t, len, QSPI_FIFO_DEPTH);
> +
> +	writel(nor->read_opcode, q->csr_base + QSPI_DATA_REG);
> +
> +	writel((from & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
> +	writel((from & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
> +	writel((from & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
> +	writel((from & 0x000000ff), q->csr_base + QSPI_DATA_REG);

Here it looks like you assume the address width is 32 bits. This is not
always the case, the address width is often 24 bits (almost always for
memory < 128Mib). Please check nor->addr_width to know the correct
number of address bytes to be used with the nor->read_opcode command.

> +
> +	reg = QSPI_ACTION_EN | QSPI_ACTION_SC |
> +		(10 << QSPI_ACTION_DUMMY_SFT) |

Here DUMMY_SFT suggests that you provide the number of dummy cycles to
be inserted between the address cycles and the data cycles.
If so, please fill this field based on nor->read_dummy: this is a number
of dummy CLOCK cycles. If you need to convert it into the number of byte
cycles you will have to take into account the number of I/O lines used
to transmit the address (+ dummy) clocks cycles:

spi_nor_get_protocol_addr_nbits(nor->read_proto)

This gives the 'y' in SPI x-y-z protocol. Should be 1, 2 or 4.

Also could you tell us which SPI memory did you use to test your driver?
10 is really strange as a number of dummy bytes or clock cycles
especially since this driver seems to support only Read (03h / 13h) and
Fast Read (0Bh / 0Ch) operations (see hwcaps below).

Read doesn't use any dummy clock cycles whereas Fast Read uses 8 dummy
clock cycles for all SPI NOR memories I know.

> +		(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,
> +				 QSPI_POLL_TIMEOUT);
> +	if (ret) {
> +		dev_err(q->dev, "%s timed out\n", __func__);
> +		bytes_to_read = 0;
> +	} else
> +		for (i = 0; i < bytes_to_read; i++) {
> +			reg = readl(q->csr_base + QSPI_DATA_REG);
> +			*buf++ = (u8)(reg & 0xff);
> +		}
> +
> +	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, i;
> +	u32 reg;
> +	int ret;
> +
> +	bytes_to_write = min_t(size_t, len, (QSPI_FIFO_DEPTH - 5));
> +

not 5 but (1 + nor->addr_width)


> +	writel(nor->program_opcode, q->csr_base + QSPI_DATA_REG);
> +
> +	writel((to & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
> +	writel((to & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
> +	writel((to & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
> +	writel((to & 0x000000ff), q->csr_base + QSPI_DATA_REG);

Same as for altera_asmip2_read()
> +
> +	for (i = 0; i < bytes_to_write; i++) {
> +		reg = (u32)*buf++;
> +		writel(reg, q->csr_base + QSPI_DATA_REG);
> +	}
> +
> +	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,
> +				 QSPI_POLL_TIMEOUT);
> +
> +	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;
> +	char modalias[40] = {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;
> +	flash->bank = bank;
> +
> +	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;
> +
> +	/* scanning flash and checking dev id */
> +#ifdef CONFIG_OF
> +	if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
> +		return -EINVAL;
> +#endif
> +
> +	ret = spi_nor_scan(nor, modalias, &hwcaps);

modalias is for legacy non-jedec SPI NOR memory support. Drop it and
pass NULL as the 2nd argument of spi_nor_scan() if possible. Unless you
really plan to use some old non-jedec SPI NOR memory?

All but m25p80.c driver pass NULL as the 2nd argument of spi_nor_scan().

Best regards,

Cyrille

> +	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_qspi_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 __probe_with_data(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_qspi_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 __probe_with_data(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_qspi_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)
> +{
> +	if (!pdev) {
> +		dev_err(&pdev->dev, "%s NULL\n", __func__);
> +		return -EINVAL;
> +	} else {
> +		return altera_asmip2_remove_banks(&pdev->dev);
> +	}
> +}
> +
> +static const struct of_device_id altera_asmip2_id_table[] = {
> +
> +	{ .compatible = "altr,asmi_parallel2",},
> +	{}
> +};
> +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
> 

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

Matthew Gerlach Aug. 15, 2017, 5:20 p.m. UTC | #4

Hi Cyrille,

Thanks for the great feedback.  See my comments inline.

Matthew Gerlach

On Fri, 11 Aug 2017, Cyrille Pitchen wrote:

> Hi Matthew,
>
> Le 06/08/2017 à 20:24, matthew.gerlach@linux.intel.com a écrit :
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  MAINTAINERS                         |   7 +
>>  drivers/mtd/spi-nor/Kconfig         |   6 +
>>  drivers/mtd/spi-nor/Makefile        |   3 +-
>>  drivers/mtd/spi-nor/altera-asmip2.c | 474 ++++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/altera-asmip2.h   |  24 ++
>>  5 files changed, 513 insertions(+), 1 deletion(-)
>>  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 672b5d5..9583c1a 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 MAILBOX DRIVER
>>  M:	Ley Foon Tan <lftan@altera.com>
>>  L:	nios2-dev@lists.rocketboards.org (moderated for non-subscribers)
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 69c638d..eb2c522 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 c5f171d..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
>> @@ -9,4 +10,4 @@ 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
>> -obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
>> \ No newline at end of file
>> +obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-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..2095f2e
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/altera-asmip2.c
>> @@ -0,0 +1,474 @@
>> +/*
>> + * 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 10000000
>> +#define QSPI_POLL_INTERVAL 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;
>> +	u32 bank;
>> +};
>> +
>> +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;
>> +	int i;
>> +
>> +	if ((len + 1) > QSPI_FIFO_DEPTH) {
>> +		dev_err(q->dev, "%s bad len %d > %d\n",
>> +			__func__, len + 1, QSPI_FIFO_DEPTH);
>> +		return -EINVAL;
>> +	}
>> +
>> +	writel(opcode, q->csr_base + QSPI_DATA_REG);
>> +
>> +	for (i = 0; i < len; i++) {
>> +		writel((u32)val[i], q->csr_base + QSPI_DATA_REG);
>> +	}
>> +
>> +	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,
>> +				 QSPI_POLL_TIMEOUT);
>> +	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;
>> +	int i;
>> +
>> +	if (len > QSPI_FIFO_DEPTH) {
>> +		dev_err(q->dev, "%s bad len %d > %d\n",
>> +			__func__, len, QSPI_FIFO_DEPTH);
>> +		return -EINVAL;
>> +	}
>> +
>> +	writel(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, QSPI_POLL_TIMEOUT);
>> +
>> +	if (!ret)
>> +		for (i = 0; i < len; i++) {
>> +			reg = readl(q->csr_base + QSPI_DATA_REG);
>> +			val[i] = (u8)(reg & 0xff);
>> +		}
>> +	else
>> +		dev_err(q->dev, "%s timeout\n", __func__);
>> +
>> +	writel(QSPI_ACTION_EN, q->csr_base + QSPI_ACTION_REG);
>> +
>> +	return ret;
>> +}
>> +
>> +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, i;
>> +	u32 reg;
>> +	int ret;
>> +
>> +	bytes_to_read = min_t(size_t, len, QSPI_FIFO_DEPTH);
>> +
>> +	writel(nor->read_opcode, q->csr_base + QSPI_DATA_REG);
>> +
>> +	writel((from & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
>> +	writel((from & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
>> +	writel((from & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
>> +	writel((from & 0x000000ff), q->csr_base + QSPI_DATA_REG);
>
> Here it looks like you assume the address width is 32 bits. This is not
> always the case, the address width is often 24 bits (almost always for
> memory < 128Mib). Please check nor->addr_width to know the correct
> number of address bytes to be used with the nor->read_opcode command.

I will be combining this suggestion with Marek's suggestion to use a for 
loop.

>
>> +
>> +	reg = QSPI_ACTION_EN | QSPI_ACTION_SC |
>> +		(10 << QSPI_ACTION_DUMMY_SFT) |
>
> Here DUMMY_SFT suggests that you provide the number of dummy cycles to
> be inserted between the address cycles and the data cycles.
> If so, please fill this field based on nor->read_dummy: this is a number
> of dummy CLOCK cycles. If you need to convert it into the number of byte
> cycles you will have to take into account the number of I/O lines used
> to transmit the address (+ dummy) clocks cycles:
>
> spi_nor_get_protocol_addr_nbits(nor->read_proto)
>
> This gives the 'y' in SPI x-y-z protocol. Should be 1, 2 or 4.
>
> Also could you tell us which SPI memory did you use to test your driver?
> 10 is really strange as a number of dummy bytes or clock cycles
> especially since this driver seems to support only Read (03h / 13h) and
> Fast Read (0Bh / 0Ch) operations (see hwcaps below).
>
> Read doesn't use any dummy clock cycles whereas Fast Read uses 8 dummy
> clock cycles for all SPI NOR memories I know.


As it turns out, I tested this code with two Micron parts, "n25q00a" and 
"n25q512a".  To be honest, the formula for calculating dummy cycles is 
not clear to me.  This controller is instantiated in an fpga, and the fpga
documentation states "The number of dummy cycles must be set to 12 for
SPI flash operating in 3-byte addressing.  If the SPI flash operates
in 4-byte addressing, the dummy cycles should be set to 4 for SPIx1 and
10 for SPIx4".

I'm not sure that (nor->read_dummy + spi_nor_get_protocol_addr_nbits()) 
implements what is in the spec.

>
>> +		(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,
>> +				 QSPI_POLL_TIMEOUT);
>> +	if (ret) {
>> +		dev_err(q->dev, "%s timed out\n", __func__);
>> +		bytes_to_read = 0;
>> +	} else
>> +		for (i = 0; i < bytes_to_read; i++) {
>> +			reg = readl(q->csr_base + QSPI_DATA_REG);
>> +			*buf++ = (u8)(reg & 0xff);
>> +		}
>> +
>> +	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, i;
>> +	u32 reg;
>> +	int ret;
>> +
>> +	bytes_to_write = min_t(size_t, len, (QSPI_FIFO_DEPTH - 5));
>> +
>
> not 5 but (1 + nor->addr_width)
>
>
>> +	writel(nor->program_opcode, q->csr_base + QSPI_DATA_REG);
>> +
>> +	writel((to & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
>> +	writel((to & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
>> +	writel((to & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
>> +	writel((to & 0x000000ff), q->csr_base + QSPI_DATA_REG);
>
> Same as for altera_asmip2_read()
>> +
>> +	for (i = 0; i < bytes_to_write; i++) {
>> +		reg = (u32)*buf++;
>> +		writel(reg, q->csr_base + QSPI_DATA_REG);
>> +	}
>> +
>> +	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,
>> +				 QSPI_POLL_TIMEOUT);
>> +
>> +	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;
>> +	char modalias[40] = {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;
>> +	flash->bank = bank;
>> +
>> +	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;
>> +
>> +	/* scanning flash and checking dev id */
>> +#ifdef CONFIG_OF
>> +	if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
>> +		return -EINVAL;
>> +#endif
>> +
>> +	ret = spi_nor_scan(nor, modalias, &hwcaps);
>
> modalias is for legacy non-jedec SPI NOR memory support. Drop it and
> pass NULL as the 2nd argument of spi_nor_scan() if possible. Unless you
> really plan to use some old non-jedec SPI NOR memory?

I am more than happy to get rid of old legacy code.

>
> All but m25p80.c driver pass NULL as the 2nd argument of spi_nor_scan().
>
> Best regards,
>
> Cyrille
>
>> +	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_qspi_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 __probe_with_data(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_qspi_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 __probe_with_data(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_qspi_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)
>> +{
>> +	if (!pdev) {
>> +		dev_err(&pdev->dev, "%s NULL\n", __func__);
>> +		return -EINVAL;
>> +	} else {
>> +		return altera_asmip2_remove_banks(&pdev->dev);
>> +	}
>> +}
>> +
>> +static const struct of_device_id altera_asmip2_id_table[] = {
>> +
>> +	{ .compatible = "altr,asmi_parallel2",},
>> +	{}
>> +};
>> +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
>>
>
> --
> 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
>

Cyrille Pitchen Aug. 15, 2017, 8:08 p.m. UTC | #5

Le 15/08/2017 à 19:20, matthew.gerlach@linux.intel.com a écrit :
> 
> Hi Cyrille,
> 
> Thanks for the great feedback.  See my comments inline.
> 
> Matthew Gerlach
> 
> On Fri, 11 Aug 2017, Cyrille Pitchen wrote:
> 
>> Hi Matthew,
>>
>> Le 06/08/2017 à 20:24, matthew.gerlach@linux.intel.com a écrit :
>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>> ---
>>>  MAINTAINERS                         |   7 +
>>>  drivers/mtd/spi-nor/Kconfig         |   6 +
>>>  drivers/mtd/spi-nor/Makefile        |   3 +-
>>>  drivers/mtd/spi-nor/altera-asmip2.c | 474
>>> ++++++++++++++++++++++++++++++++++++
>>>  include/linux/mtd/altera-asmip2.h   |  24 ++
>>>  5 files changed, 513 insertions(+), 1 deletion(-)
>>>  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 672b5d5..9583c1a 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 MAILBOX DRIVER
>>>  M:    Ley Foon Tan <lftan@altera.com>
>>>  L:    nios2-dev@lists.rocketboards.org (moderated for non-subscribers)
>>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>>> index 69c638d..eb2c522 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 c5f171d..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
>>> @@ -9,4 +10,4 @@ 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
>>> -obj-$(CONFIG_SPI_STM32_QUADSPI)    += stm32-quadspi.o
>>> \ No newline at end of file
>>> +obj-$(CONFIG_SPI_STM32_QUADSPI)    += stm32-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..2095f2e
>>> --- /dev/null
>>> +++ b/drivers/mtd/spi-nor/altera-asmip2.c
>>> @@ -0,0 +1,474 @@
>>> +/*
>>> + * 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 10000000
>>> +#define QSPI_POLL_INTERVAL 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;
>>> +    u32 bank;
>>> +};
>>> +
>>> +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;
>>> +    int i;
>>> +
>>> +    if ((len + 1) > QSPI_FIFO_DEPTH) {
>>> +        dev_err(q->dev, "%s bad len %d > %d\n",
>>> +            __func__, len + 1, QSPI_FIFO_DEPTH);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    writel(opcode, q->csr_base + QSPI_DATA_REG);
>>> +
>>> +    for (i = 0; i < len; i++) {
>>> +        writel((u32)val[i], q->csr_base + QSPI_DATA_REG);
>>> +    }
>>> +
>>> +    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,
>>> +                 QSPI_POLL_TIMEOUT);
>>> +    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;
>>> +    int i;
>>> +
>>> +    if (len > QSPI_FIFO_DEPTH) {
>>> +        dev_err(q->dev, "%s bad len %d > %d\n",
>>> +            __func__, len, QSPI_FIFO_DEPTH);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    writel(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, QSPI_POLL_TIMEOUT);
>>> +
>>> +    if (!ret)
>>> +        for (i = 0; i < len; i++) {
>>> +            reg = readl(q->csr_base + QSPI_DATA_REG);
>>> +            val[i] = (u8)(reg & 0xff);
>>> +        }
>>> +    else
>>> +        dev_err(q->dev, "%s timeout\n", __func__);
>>> +
>>> +    writel(QSPI_ACTION_EN, q->csr_base + QSPI_ACTION_REG);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +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, i;
>>> +    u32 reg;
>>> +    int ret;
>>> +
>>> +    bytes_to_read = min_t(size_t, len, QSPI_FIFO_DEPTH);
>>> +
>>> +    writel(nor->read_opcode, q->csr_base + QSPI_DATA_REG);
>>> +
>>> +    writel((from & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
>>> +    writel((from & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
>>> +    writel((from & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
>>> +    writel((from & 0x000000ff), q->csr_base + QSPI_DATA_REG);
>>
>> Here it looks like you assume the address width is 32 bits. This is not
>> always the case, the address width is often 24 bits (almost always for
>> memory < 128Mib). Please check nor->addr_width to know the correct
>> number of address bytes to be used with the nor->read_opcode command.
> 
> I will be combining this suggestion with Marek's suggestion to use a for
> loop.
> 
>>
>>> +
>>> +    reg = QSPI_ACTION_EN | QSPI_ACTION_SC |
>>> +        (10 << QSPI_ACTION_DUMMY_SFT) |
>>
>> Here DUMMY_SFT suggests that you provide the number of dummy cycles to
>> be inserted between the address cycles and the data cycles.
>> If so, please fill this field based on nor->read_dummy: this is a number
>> of dummy CLOCK cycles. If you need to convert it into the number of byte
>> cycles you will have to take into account the number of I/O lines used
>> to transmit the address (+ dummy) clocks cycles:
>>
>> spi_nor_get_protocol_addr_nbits(nor->read_proto)
>>
>> This gives the 'y' in SPI x-y-z protocol. Should be 1, 2 or 4.
>>
>> Also could you tell us which SPI memory did you use to test your driver?
>> 10 is really strange as a number of dummy bytes or clock cycles
>> especially since this driver seems to support only Read (03h / 13h) and
>> Fast Read (0Bh / 0Ch) operations (see hwcaps below).
>>
>> Read doesn't use any dummy clock cycles whereas Fast Read uses 8 dummy
>> clock cycles for all SPI NOR memories I know.
> 
> 
> As it turns out, I tested this code with two Micron parts, "n25q00a" and
> "n25q512a".  To be honest, the formula for calculating dummy cycles is
> not clear to me.  This controller is instantiated in an fpga, and the fpga
> documentation states "The number of dummy cycles must be set to 12 for
> SPI flash operating in 3-byte addressing.  If the SPI flash operates
> in 4-byte addressing, the dummy cycles should be set to 4 for SPIx1 and
> 10 for SPIx4".
> 
> I'm not sure that (nor->read_dummy + spi_nor_get_protocol_addr_nbits())
> implements what is in the spec.
>

The number of dummy clock cycles to be inserted between address clock
cycles and data clock cycles is totally up to the SPI NOR memory. The
SPI controller has no other choice but to insert the exact number of
dummy clock cycles as expected by the memory.

You can find the number of dummy clock cycles in the *memory* datasheet.
Usually this number depends on the SPI x-y-z protocol used to read the
memory data.

x : number of I/O lines used during instruction op code clock cycles in
{0, 1, 2, 4, 8}. 0 for Continuous Read/XIP, when the instruction op code
is not sent at all: not supported by spi-nor, only relevant for XIP
application out of the scope of the MTD sub-system.

y : number of I/O lines used during address/mode/dummy clock cycles in
{1, 2, 4, 8}

z : number of I/O lines used during data clock cycles in {1, 2, 4, 8}.

Existing SPI x-y-z protocols respect the following patterns:
- (x <= y) && (y <= z)
- (x == y) || (y == z)

These are not formal rules but in practice all existing SPI protocols
match those patterns. Hence valid Quad SPI protocol are SPI 1-1-4, SPI
1-4-4 or SPI 4-4-4 (plus SPI 0-4-4 for Continuous Read/XIP).

The spi-nor.c driver doesn't enable the Micron Quad I/O mode which
forces the SPI 4-4-4 for ALL SPI commands. Micron Dual I/O mode is
neither used (it forces the SPI 2-2-2 protocols for ALL commands).

The spi-nor driver expects the Micron (or any SPI NOR) memory to still
be in its factory settings. For Micron, it means that neither the Quad
I/O nor the Dual I/O modes should be enabled.

Anyway, if one of those 2 modes were enabled before calling
spi_nor_scan(), this later function would fail to detect the Micron
memory since the Read JEDEC ID (9Fh) command is not supported by Micron
memories once in Quad or Dual I/O mode.

Read commands (03h / 13h) expect no dummy clock cycles whereas Fast Read
commands (0Bh / 0Ch) expect 8 dummy clock cycles, not 10.

10 dummy clock cycles are the Micron factory settings when you use some
SPI x-4-4 protocol, ie with address, mode and dummy sent on 4 I/O lines.
This won't be the case with this Altera driver since it claims to
support only Read (SPI 1-1-1) and Fast Read (SPI 1-1-1) operations.

The number of dummy clock cycles doesn't depend on whether we use a
3-byte address or a 4-byte address SPI command.


tl;dr

Your FPGA documentation seems to be all wrong, please just trust the
settings provided by the spi-nor.c driver and refer the *memory*
datasheet if the values chosen by spi-nor.c are not correct.

Also please refer to the m25p80_read() function in
drivers/mtd/device/m25p80.c to get the formula to convert the number of
dummy clock cycles ino the number of dummy bytes:

/* SPI x-y-z protocol */
u8 y = spi_not_get_protocol_addr_nbits(nor->read_proto);

/* nor->read_dummy is the number of dummy clock cycles */
unsigned int dummy_bytes = (nor->read_dummy * y) / 8;


Best regards,

Cyrille

>>
>>> +        (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,
>>> +                 QSPI_POLL_TIMEOUT);
>>> +    if (ret) {
>>> +        dev_err(q->dev, "%s timed out\n", __func__);
>>> +        bytes_to_read = 0;
>>> +    } else
>>> +        for (i = 0; i < bytes_to_read; i++) {
>>> +            reg = readl(q->csr_base + QSPI_DATA_REG);
>>> +            *buf++ = (u8)(reg & 0xff);
>>> +        }
>>> +
>>> +    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, i;
>>> +    u32 reg;
>>> +    int ret;
>>> +
>>> +    bytes_to_write = min_t(size_t, len, (QSPI_FIFO_DEPTH - 5));
>>> +
>>
>> not 5 but (1 + nor->addr_width)
>>
>>
>>> +    writel(nor->program_opcode, q->csr_base + QSPI_DATA_REG);
>>> +
>>> +    writel((to & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
>>> +    writel((to & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
>>> +    writel((to & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
>>> +    writel((to & 0x000000ff), q->csr_base + QSPI_DATA_REG);
>>
>> Same as for altera_asmip2_read()
>>> +
>>> +    for (i = 0; i < bytes_to_write; i++) {
>>> +        reg = (u32)*buf++;
>>> +        writel(reg, q->csr_base + QSPI_DATA_REG);
>>> +    }
>>> +
>>> +    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,
>>> +                 QSPI_POLL_TIMEOUT);
>>> +
>>> +    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;
>>> +    char modalias[40] = {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;
>>> +    flash->bank = bank;
>>> +
>>> +    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;
>>> +
>>> +    /* scanning flash and checking dev id */
>>> +#ifdef CONFIG_OF
>>> +    if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
>>> +        return -EINVAL;
>>> +#endif
>>> +
>>> +    ret = spi_nor_scan(nor, modalias, &hwcaps);
>>
>> modalias is for legacy non-jedec SPI NOR memory support. Drop it and
>> pass NULL as the 2nd argument of spi_nor_scan() if possible. Unless you
>> really plan to use some old non-jedec SPI NOR memory?
> 
> I am more than happy to get rid of old legacy code.
> 
>>
>> All but m25p80.c driver pass NULL as the 2nd argument of spi_nor_scan().
>>
>> Best regards,
>>
>> Cyrille
>>
>>> +    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_qspi_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 __probe_with_data(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_qspi_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 __probe_with_data(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_qspi_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)
>>> +{
>>> +    if (!pdev) {
>>> +        dev_err(&pdev->dev, "%s NULL\n", __func__);
>>> +        return -EINVAL;
>>> +    } else {
>>> +        return altera_asmip2_remove_banks(&pdev->dev);
>>> +    }
>>> +}
>>> +
>>> +static const struct of_device_id altera_asmip2_id_table[] = {
>>> +
>>> +    { .compatible = "altr,asmi_parallel2",},
>>> +    {}
>>> +};
>>> +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
>>>
>>
>> -- 
>> 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
>>

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

Matthew Gerlach Aug. 27, 2017, 3:42 p.m. UTC | #6

Hi Cyrille,

I think I figured out the confusion with regards to dummy cycles.  See my 
comment in line.

Matthew Gerlach

On Tue, 15 Aug 2017, Cyrille Pitchen wrote:

> Le 15/08/2017 à 19:20, matthew.gerlach@linux.intel.com a écrit :
>>
>> Hi Cyrille,
>>
>> Thanks for the great feedback.  See my comments inline.
>>
>> Matthew Gerlach
>>
>> On Fri, 11 Aug 2017, Cyrille Pitchen wrote:
>>
>>> Hi Matthew,
>>>
>>> Le 06/08/2017 à 20:24, matthew.gerlach@linux.intel.com a écrit :
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>> ---
>>>>  MAINTAINERS                         |   7 +
>>>>  drivers/mtd/spi-nor/Kconfig         |   6 +
>>>>  drivers/mtd/spi-nor/Makefile        |   3 +-
>>>>  drivers/mtd/spi-nor/altera-asmip2.c | 474
>>>> ++++++++++++++++++++++++++++++++++++
>>>>  include/linux/mtd/altera-asmip2.h   |  24 ++
>>>>  5 files changed, 513 insertions(+), 1 deletion(-)
>>>>  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 672b5d5..9583c1a 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 MAILBOX DRIVER
>>>>  M:    Ley Foon Tan <lftan@altera.com>
>>>>  L:    nios2-dev@lists.rocketboards.org (moderated for non-subscribers)
>>>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>>>> index 69c638d..eb2c522 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 c5f171d..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
>>>> @@ -9,4 +10,4 @@ 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
>>>> -obj-$(CONFIG_SPI_STM32_QUADSPI)    += stm32-quadspi.o
>>>> \ No newline at end of file
>>>> +obj-$(CONFIG_SPI_STM32_QUADSPI)    += stm32-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..2095f2e
>>>> --- /dev/null
>>>> +++ b/drivers/mtd/spi-nor/altera-asmip2.c
>>>> @@ -0,0 +1,474 @@
>>>> +/*
>>>> + * 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 10000000
>>>> +#define QSPI_POLL_INTERVAL 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;
>>>> +    u32 bank;
>>>> +};
>>>> +
>>>> +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;
>>>> +    int i;
>>>> +
>>>> +    if ((len + 1) > QSPI_FIFO_DEPTH) {
>>>> +        dev_err(q->dev, "%s bad len %d > %d\n",
>>>> +            __func__, len + 1, QSPI_FIFO_DEPTH);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    writel(opcode, q->csr_base + QSPI_DATA_REG);
>>>> +
>>>> +    for (i = 0; i < len; i++) {
>>>> +        writel((u32)val[i], q->csr_base + QSPI_DATA_REG);
>>>> +    }
>>>> +
>>>> +    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,
>>>> +                 QSPI_POLL_TIMEOUT);
>>>> +    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;
>>>> +    int i;
>>>> +
>>>> +    if (len > QSPI_FIFO_DEPTH) {
>>>> +        dev_err(q->dev, "%s bad len %d > %d\n",
>>>> +            __func__, len, QSPI_FIFO_DEPTH);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    writel(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, QSPI_POLL_TIMEOUT);
>>>> +
>>>> +    if (!ret)
>>>> +        for (i = 0; i < len; i++) {
>>>> +            reg = readl(q->csr_base + QSPI_DATA_REG);
>>>> +            val[i] = (u8)(reg & 0xff);
>>>> +        }
>>>> +    else
>>>> +        dev_err(q->dev, "%s timeout\n", __func__);
>>>> +
>>>> +    writel(QSPI_ACTION_EN, q->csr_base + QSPI_ACTION_REG);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +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, i;
>>>> +    u32 reg;
>>>> +    int ret;
>>>> +
>>>> +    bytes_to_read = min_t(size_t, len, QSPI_FIFO_DEPTH);
>>>> +
>>>> +    writel(nor->read_opcode, q->csr_base + QSPI_DATA_REG);
>>>> +
>>>> +    writel((from & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
>>>> +    writel((from & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
>>>> +    writel((from & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
>>>> +    writel((from & 0x000000ff), q->csr_base + QSPI_DATA_REG);
>>>
>>> Here it looks like you assume the address width is 32 bits. This is not
>>> always the case, the address width is often 24 bits (almost always for
>>> memory < 128Mib). Please check nor->addr_width to know the correct
>>> number of address bytes to be used with the nor->read_opcode command.
>>
>> I will be combining this suggestion with Marek's suggestion to use a for
>> loop.
>>
>>>
>>>> +
>>>> +    reg = QSPI_ACTION_EN | QSPI_ACTION_SC |
>>>> +        (10 << QSPI_ACTION_DUMMY_SFT) |
>>>
>>> Here DUMMY_SFT suggests that you provide the number of dummy cycles to
>>> be inserted between the address cycles and the data cycles.
>>> If so, please fill this field based on nor->read_dummy: this is a number
>>> of dummy CLOCK cycles. If you need to convert it into the number of byte
>>> cycles you will have to take into account the number of I/O lines used
>>> to transmit the address (+ dummy) clocks cycles:
>>>
>>> spi_nor_get_protocol_addr_nbits(nor->read_proto)
>>>
>>> This gives the 'y' in SPI x-y-z protocol. Should be 1, 2 or 4.
>>>
>>> Also could you tell us which SPI memory did you use to test your driver?
>>> 10 is really strange as a number of dummy bytes or clock cycles
>>> especially since this driver seems to support only Read (03h / 13h) and
>>> Fast Read (0Bh / 0Ch) operations (see hwcaps below).
>>>
>>> Read doesn't use any dummy clock cycles whereas Fast Read uses 8 dummy
>>> clock cycles for all SPI NOR memories I know.
>>
>>
>> As it turns out, I tested this code with two Micron parts, "n25q00a" and
>> "n25q512a".  To be honest, the formula for calculating dummy cycles is
>> not clear to me.  This controller is instantiated in an fpga, and the fpga
>> documentation states "The number of dummy cycles must be set to 12 for
>> SPI flash operating in 3-byte addressing.  If the SPI flash operates
>> in 4-byte addressing, the dummy cycles should be set to 4 for SPIx1 and
>> 10 for SPIx4".
>>
>> I'm not sure that (nor->read_dummy + spi_nor_get_protocol_addr_nbits())
>> implements what is in the spec.
>>
>
> The number of dummy clock cycles to be inserted between address clock
> cycles and data clock cycles is totally up to the SPI NOR memory. The
> SPI controller has no other choice but to insert the exact number of
> dummy clock cycles as expected by the memory.
>
> You can find the number of dummy clock cycles in the *memory* datasheet.
> Usually this number depends on the SPI x-y-z protocol used to read the
> memory data.


Well my problem is that I was looking at the Micron data sheets when I 
should have been looking at the EPCQ data sheets:

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

The EPCQ data sheet clearly states 10 dummy cycles, but unfortunately when 
one reads the jedec id register, micron is reported back.  I suspect the 
EPCQ flash are actually micron flash parts with some logic to make it 
easier for the FPGA to configure itself on power up.

Would it make sense to add a flag to the platform data/device tree to
tell the driver to get the dummy cycles by reading Non-Volatile 
configuration register, or should such a flag be passed to spi_nor_scan() 
to tell it read the Non-Volatile configuration register for the dummy 
cycles?

>
> x : number of I/O lines used during instruction op code clock cycles in
> {0, 1, 2, 4, 8}. 0 for Continuous Read/XIP, when the instruction op code
> is not sent at all: not supported by spi-nor, only relevant for XIP
> application out of the scope of the MTD sub-system.
>
> y : number of I/O lines used during address/mode/dummy clock cycles in
> {1, 2, 4, 8}
>
> z : number of I/O lines used during data clock cycles in {1, 2, 4, 8}.
>
> Existing SPI x-y-z protocols respect the following patterns:
> - (x <= y) && (y <= z)
> - (x == y) || (y == z)
>
> These are not formal rules but in practice all existing SPI protocols
> match those patterns. Hence valid Quad SPI protocol are SPI 1-1-4, SPI
> 1-4-4 or SPI 4-4-4 (plus SPI 0-4-4 for Continuous Read/XIP).
>
> The spi-nor.c driver doesn't enable the Micron Quad I/O mode which
> forces the SPI 4-4-4 for ALL SPI commands. Micron Dual I/O mode is
> neither used (it forces the SPI 2-2-2 protocols for ALL commands).
>
> The spi-nor driver expects the Micron (or any SPI NOR) memory to still
> be in its factory settings. For Micron, it means that neither the Quad
> I/O nor the Dual I/O modes should be enabled.
>
> Anyway, if one of those 2 modes were enabled before calling
> spi_nor_scan(), this later function would fail to detect the Micron
> memory since the Read JEDEC ID (9Fh) command is not supported by Micron
> memories once in Quad or Dual I/O mode.
>
> Read commands (03h / 13h) expect no dummy clock cycles whereas Fast Read
> commands (0Bh / 0Ch) expect 8 dummy clock cycles, not 10.
>
> 10 dummy clock cycles are the Micron factory settings when you use some
> SPI x-4-4 protocol, ie with address, mode and dummy sent on 4 I/O lines.
> This won't be the case with this Altera driver since it claims to
> support only Read (SPI 1-1-1) and Fast Read (SPI 1-1-1) operations.
>
> The number of dummy clock cycles doesn't depend on whether we use a
> 3-byte address or a 4-byte address SPI command.
>
>
> tl;dr
>
> Your FPGA documentation seems to be all wrong, please just trust the
> settings provided by the spi-nor.c driver and refer the *memory*
> datasheet if the values chosen by spi-nor.c are not correct.
>
> Also please refer to the m25p80_read() function in
> drivers/mtd/device/m25p80.c to get the formula to convert the number of
> dummy clock cycles ino the number of dummy bytes:
>
> /* SPI x-y-z protocol */
> u8 y = spi_not_get_protocol_addr_nbits(nor->read_proto);
>
> /* nor->read_dummy is the number of dummy clock cycles */
> unsigned int dummy_bytes = (nor->read_dummy * y) / 8;
>
>
> Best regards,
>
> Cyrille
>
>>>
>>>> +        (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,
>>>> +                 QSPI_POLL_TIMEOUT);
>>>> +    if (ret) {
>>>> +        dev_err(q->dev, "%s timed out\n", __func__);
>>>> +        bytes_to_read = 0;
>>>> +    } else
>>>> +        for (i = 0; i < bytes_to_read; i++) {
>>>> +            reg = readl(q->csr_base + QSPI_DATA_REG);
>>>> +            *buf++ = (u8)(reg & 0xff);
>>>> +        }
>>>> +
>>>> +    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, i;
>>>> +    u32 reg;
>>>> +    int ret;
>>>> +
>>>> +    bytes_to_write = min_t(size_t, len, (QSPI_FIFO_DEPTH - 5));
>>>> +
>>>
>>> not 5 but (1 + nor->addr_width)
>>>
>>>
>>>> +    writel(nor->program_opcode, q->csr_base + QSPI_DATA_REG);
>>>> +
>>>> +    writel((to & 0xff000000) >> 24, q->csr_base + QSPI_DATA_REG);
>>>> +    writel((to & 0x00ff0000) >> 16, q->csr_base + QSPI_DATA_REG);
>>>> +    writel((to & 0x0000ff00) >> 8, q->csr_base + QSPI_DATA_REG);
>>>> +    writel((to & 0x000000ff), q->csr_base + QSPI_DATA_REG);
>>>
>>> Same as for altera_asmip2_read()
>>>> +
>>>> +    for (i = 0; i < bytes_to_write; i++) {
>>>> +        reg = (u32)*buf++;
>>>> +        writel(reg, q->csr_base + QSPI_DATA_REG);
>>>> +    }
>>>> +
>>>> +    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,
>>>> +                 QSPI_POLL_TIMEOUT);
>>>> +
>>>> +    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;
>>>> +    char modalias[40] = {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;
>>>> +    flash->bank = bank;
>>>> +
>>>> +    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;
>>>> +
>>>> +    /* scanning flash and checking dev id */
>>>> +#ifdef CONFIG_OF
>>>> +    if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
>>>> +        return -EINVAL;
>>>> +#endif
>>>> +
>>>> +    ret = spi_nor_scan(nor, modalias, &hwcaps);
>>>
>>> modalias is for legacy non-jedec SPI NOR memory support. Drop it and
>>> pass NULL as the 2nd argument of spi_nor_scan() if possible. Unless you
>>> really plan to use some old non-jedec SPI NOR memory?
>>
>> I am more than happy to get rid of old legacy code.
>>
>>>
>>> All but m25p80.c driver pass NULL as the 2nd argument of spi_nor_scan().
>>>
>>> Best regards,
>>>
>>> Cyrille
>>>
>>>> +    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_qspi_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 __probe_with_data(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_qspi_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 __probe_with_data(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_qspi_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)
>>>> +{
>>>> +    if (!pdev) {
>>>> +        dev_err(&pdev->dev, "%s NULL\n", __func__);
>>>> +        return -EINVAL;
>>>> +    } else {
>>>> +        return altera_asmip2_remove_banks(&pdev->dev);
>>>> +    }
>>>> +}
>>>> +
>>>> +static const struct of_device_id altera_asmip2_id_table[] = {
>>>> +
>>>> +    { .compatible = "altr,asmi_parallel2",},
>>>> +    {}
>>>> +};
>>>> +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
>>>>
>>>
>>> --
>>> 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
>>>
>
> --
> 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
>

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

Commit Message

Comments

Patch