[4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI controller

Add support for the QSPI controller found in Adaptrum Anarion SOCs.
This controller is designed specifically to handle SPI NOR chips, and
the driver is modeled as such.

Because the system is emulated on an FPGA, we don't have a way to test
all the hardware adjustemts, so only basic features are implemented at
this time.

Signed-off-by: Alexandru Gagniuc <alex.g@adaptrum.com>
---
 .../devicetree/bindings/mtd/anarion-quadspi.txt    |  22 +
 drivers/mtd/spi-nor/Kconfig                        |   7 +
 drivers/mtd/spi-nor/Makefile                       |   1 +
 drivers/mtd/spi-nor/anarion-quadspi.c              | 490 +++++++++++++++++++++
 4 files changed, 520 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/anarion-quadspi.txt
 create mode 100644 drivers/mtd/spi-nor/anarion-quadspi.c

On 07/29/2017 12:07 AM, Alexandru Gagniuc wrote:
> Add support for the QSPI controller found in Adaptrum Anarion SOCs.
> This controller is designed specifically to handle SPI NOR chips, and
> the driver is modeled as such.
> 
> Because the system is emulated on an FPGA, we don't have a way to test
> all the hardware adjustemts, so only basic features are implemented at
> this time.
> 
> Signed-off-by: Alexandru Gagniuc <alex.g@adaptrum.com>
> ---
>  .../devicetree/bindings/mtd/anarion-quadspi.txt    |  22 +
>  drivers/mtd/spi-nor/Kconfig                        |   7 +
>  drivers/mtd/spi-nor/Makefile                       |   1 +
>  drivers/mtd/spi-nor/anarion-quadspi.c              | 490 +++++++++++++++++++++
>  4 files changed, 520 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/anarion-quadspi.txt
>  create mode 100644 drivers/mtd/spi-nor/anarion-quadspi.c
> 
> diff --git a/Documentation/devicetree/bindings/mtd/anarion-quadspi.txt b/Documentation/devicetree/bindings/mtd/anarion-quadspi.txt
> new file mode 100644
> index 0000000..b4971e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/anarion-quadspi.txt
> @@ -0,0 +1,22 @@
> +* Adaptrum Anarion Quad SPI controller
> +
> +Required properties:
> +- compatible : Should be "adaptrum,anarion-qspi".
> +- reg : Contains two entries, each of which is a tuple consisting of a
> +	physical address and length. The first entry is the address and
> +	length of the controller register set. The second entry is the
> +	address and length of the memory-mapped flash. This second region is
> +	the region where the controller responds to XIP requests, and may be
> +	larger than the size of the attached flash.

You want to split the bindings into separate patch and CC Rob to review
them.

> +Example:
> +
> +	qspi: qspi@f200f000 {
> +		compatible = "adaptrum,anarion-qspi";
> +		reg = 	<0xf200f000 0x1000>,
> +			<0x20000000 0x08000000>;
> +
> +		flash0: w25q128fvn@0 {
> +			...
> +		}
> +	};
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 293c8a4..98dc012 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -48,6 +48,13 @@ config SPI_ATMEL_QUADSPI
>  	  This driver does not support generic SPI. The implementation only
>  	  supports SPI NOR.
>  
> +config SPI_ANARION_QUADSPI
> +	tristate "Adaptrum Anarion Quad SPI Controller"
> +	depends on OF && HAS_IOMEM
> +	help
> +	  Enable support for the Adaptrum Anarion Quad SPI controller.
> +	  This driver does not support generic SPI. It only supports SPI NOR.

Keep the list sorted.

>  config SPI_CADENCE_QUADSPI
>  	tristate "Cadence Quad SPI controller"
>  	depends on OF && (ARM || COMPILE_TEST)
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 285aab8..53635f6 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
>  obj-$(CONFIG_SPI_ASPEED_SMC)	+= aspeed-smc.o
>  obj-$(CONFIG_SPI_ATMEL_QUADSPI)	+= atmel-quadspi.o
> +obj-$(CONFIG_SPI_ANARION_QUADSPI)	+= anarion-quadspi.o

DTTO, N is before S and T .

>  obj-$(CONFIG_SPI_CADENCE_QUADSPI)	+= cadence-quadspi.o
>  obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.o
>  obj-$(CONFIG_SPI_HISI_SFC)	+= hisi-sfc.o
> diff --git a/drivers/mtd/spi-nor/anarion-quadspi.c b/drivers/mtd/spi-nor/anarion-quadspi.c
> new file mode 100644
> index 0000000..d981356
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/anarion-quadspi.c
> @@ -0,0 +1,490 @@
> +/*
> + * Adaptrum Anarion Quad SPI controller driver
> + *
> + * Copyright (C) 2017, Adaptrum, Inc.
> + * (Written by Alexandru Gagniuc <alex.g at adaptrum.com> for Adaptrum, Inc.)
> + * Licensed under the GPLv2 or (at your option) any later version.

The GPL boilerplate should be here.

> + */
> +
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mtd/spi-nor.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define ASPI_REG_CLOCK			0x00
> +#define ASPI_REG_GO			0x04
> +#define ASPI_REG_CHAIN			0x08
> +#define ASPI_REG_CMD1			0x0c
> +#define ASPI_REG_CMD2			0x10
> +#define ASPI_REG_ADDR1			0x14
> +#define ASPI_REG_ADDR2			0x18
> +#define ASPI_REG_PERF1			0x1c
> +#define ASPI_REG_PERF2			0x20
> +#define ASPI_REG_HI_Z			0x24
> +#define ASPI_REG_BYTE_COUNT		0x28
> +#define ASPI_REG_DATA1			0x2c
> +#define ASPI_REG_DATA2			0x30
> +#define ASPI_REG_FINISH			0x34
> +#define ASPI_REG_XIP			0x38
> +#define ASPI_REG_FIFO_STATUS		0x3c
> +#define ASPI_REG_LAT			0x40
> +#define ASPI_REG_OUT_DELAY_0		0x44
> +#define ASPI_REG_OUT_DELAY_1		0x48
> +#define ASPI_REG_IN_DELAY_0		0x4c
> +#define ASPI_REG_IN_DELAY_1		0x50
> +#define ASPI_REG_DQS_DELAY		0x54
> +#define ASPI_REG_STATUS			0x58
> +#define ASPI_REG_IRQ_ENABLE		0x5c
> +#define ASPI_REG_IRQ_STATUS		0x60
> +#define ASPI_REG_AXI_BAR		0x64
> +#define ASPI_REG_READ_CFG		0x6c
> +
> +#define ASPI_CLK_SW_RESET		(1 << 0)

BIT(0) , fix globally

> +#define ASPI_CLK_RESET_BUF		(1 << 1)
> +#define ASPI_CLK_RESET_ALL		(ASPI_CLK_SW_RESET | ASPI_CLK_RESET_BUF)
> +#define ASPI_CLK_SPI_MODE3		(1 << 2)
> +#define ASPI_CLOCK_DIV_MASK		(0xff << 8)
> +#define ASPI_CLOCK_DIV(d)		(((d) << 8) & ASPI_CLOCK_DIV_MASK)
> +
> +#define ASPI_TIMEOUT_US		100000
> +
> +#define ASPI_DATA_LEN_MASK		0x3fff
> +#define ASPI_MAX_XFER_LEN		(size_t)(ASPI_DATA_LEN_MASK + 1)
> +
> +#define MODE_IO_X1			(0 << 16)
> +#define MODE_IO_X2			(1 << 16)
> +#define MODE_IO_X4			(2 << 16)
> +#define MODE_IO_SDR_POS_SKEW		(0 << 20)
> +#define MODE_IO_SDR_NEG_SKEW		(1 << 20)
> +#define MODE_IO_DDR_34_SKEW		(2 << 20)
> +#define MODE_IO_DDR_PN_SKEW		(3 << 20)
> +#define MODE_IO_DDR_DQS			(5 << 20)
> +
> +#define ASPI_STATUS_BUSY			(1 << 2)
> +
> +/*
> + * This mask does not match reality. Get over it:

What is this about ?

> + * DATA2:	0x3fff
> + * CMD2:	0x0003
> + * ADDR2:	0x0007
> + * PERF2:	0x0000
> + * HI_Z:	0x003f
> + * BCNT:	0x0007
> + */
> +#define CHAIN_LEN(x)		((x - 1) & ASPI_DATA_LEN_MASK)
> +
> +struct anarion_qspi {
> +	struct		spi_nor nor;
> +	struct		device *dev;
> +	uintptr_t	regbase;

Should be void __iomem * I guess ?

> +	uintptr_t	xipbase;
> +	uint32_t	xfer_mode_cmd;

u32 etc, fix globally, this is not userspace.

> +	uint32_t	xfer_mode_addr;
> +	uint32_t	xfer_mode_data;
> +	uint8_t		num_hi_z_clocks;
> +};
> +
> +struct qspi_io_chain {
> +	uint8_t action;
> +	uint32_t data;
> +	uint16_t data_len;
> +	uint32_t mode;
> +};
> +
> +enum chain_code {
> +	CHAIN_NOP = 0,
> +	CHAIN_CMD = 1,
> +	CHAIN_ADDR = 2,
> +	CHAIN_WTFIUM = 3,
> +	CHAIN_HI_Z = 4,
> +	CHAIN_DATA_OUT = 5,
> +	CHAIN_DATA_IN = 6,
> +	CHAIN_FINISH = 7,
> +};
> +
> +static const struct chain_to_reg {
> +	uint8_t data_reg;
> +	uint8_t ctl_reg;
> +} chain_to_reg_map[] = {
> +	[CHAIN_NOP] =		{0, 0},
> +	[CHAIN_CMD] =		{ASPI_REG_CMD1, ASPI_REG_CMD2},
> +	[CHAIN_ADDR] =		{ASPI_REG_ADDR1, ASPI_REG_ADDR2},
> +	[CHAIN_WTFIUM] =	{0, 0},
> +	[CHAIN_HI_Z] =		{0, ASPI_REG_HI_Z},
> +	[CHAIN_DATA_OUT] =	{0, ASPI_REG_DATA2},
> +	[CHAIN_DATA_IN] =	{0, ASPI_REG_DATA2},
> +	[CHAIN_FINISH] =	{0, ASPI_REG_FINISH},
> +};
> +
> +static uint32_t aspi_read_reg(struct anarion_qspi *spi, uint8_t reg)
> +{
> +	return readl((void *)(spi->regbase + reg));
> +};
> +
> +static void aspi_write_reg(struct anarion_qspi *spi, uint8_t reg, uint32_t val)
> +{
> +	writel(val, (void *)(spi->regbase + reg));
> +};
> +
> +static size_t aspi_get_fifo_level(struct anarion_qspi *spi)
> +{
> +	return aspi_read_reg(spi, ASPI_REG_FIFO_STATUS) & 0xff;
> +}
> +
> +static void aspi_drain_fifo(struct anarion_qspi *aspi, uint8_t *buf, size_t len)
> +{
> +	uint32_t data;

Is this stuff below something like ioread32_rep() ?

> +	aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
> +	while (len >= 4) {
> +		data = aspi_read_reg(aspi, ASPI_REG_DATA1);
> +		memcpy(buf, &data, sizeof(data));
> +		buf += 4;
> +		len -= 4;
> +	}
> +
> +	if (len) {
> +		aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, len);
> +		data = aspi_read_reg(aspi, ASPI_REG_DATA1);
> +		memcpy(buf, &data, len);
> +	}
> +}
> +
> +static void aspi_seed_fifo(struct anarion_qspi *spi,
> +			   const uint8_t *buf, size_t len)
> +{
> +	uint32_t data;
> +
> +	aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
> +	while (len >= 4) {
> +		memcpy(&data, buf, sizeof(data));
> +		aspi_write_reg(spi, ASPI_REG_DATA1, data);

iowrite32_rep ?

> +		buf += 4;
> +		len -= 4;
> +	}
> +
> +	if (len) {
> +		aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
> +		memcpy(&data, buf, len);
> +		aspi_write_reg(spi, ASPI_REG_DATA1, data);
> +	}
> +}
> +
> +static int aspi_wait_idle(struct anarion_qspi *aspi)
> +{
> +	uint32_t status;
> +	void *status_reg = (void *)(aspi->regbase + ASPI_REG_STATUS);
> +
> +	return readl_poll_timeout(status_reg, status,
> +				  !(status & ASPI_STATUS_BUSY),
> +				  1, ASPI_TIMEOUT_US);
> +}
> +
> +static int aspi_poll_and_seed_fifo(struct anarion_qspi *spi,
> +				   const void *src_addr, size_t len)
> +{
> +	size_t wait_us, fifo_space = 0, xfer_len;
> +	const uint8_t *src = src_addr;
> +
> +	while (len > 0) {
> +		wait_us = 0;
> +		while (wait_us++ < ASPI_TIMEOUT_US) {
> +			fifo_space = 64 - aspi_get_fifo_level(spi);
> +			if (fifo_space)
> +				break;
> +			udelay(1);
> +		}
> +
> +		xfer_len = min(len, fifo_space);
> +		aspi_seed_fifo(spi, src, xfer_len);
> +		src += xfer_len;
> +		len -= xfer_len;
> +	}
> +
> +	return 0;
> +}
> +
> +static void aspi_setup_chain(struct anarion_qspi *aspi,
> +			     const struct qspi_io_chain *chain,
> +			     size_t chain_len)
> +{
> +	size_t i;
> +	uint32_t chain_reg = 0;
> +	const struct qspi_io_chain *link;
> +	const struct chain_to_reg *regs;
> +
> +	for (link = chain, i = 0; i < chain_len; i++, link++) {
> +		regs = &chain_to_reg_map[link->action];
> +
> +		if (link->data_len && regs->data_reg)
> +			aspi_write_reg(aspi, regs->data_reg, link->data);
> +
> +		if (regs->ctl_reg)
> +			aspi_write_reg(aspi, regs->ctl_reg,
> +				       CHAIN_LEN(link->data_len) | link->mode);
> +
> +		chain_reg |= link->action << (i * 4);
> +	}
> +
> +	chain_reg |= CHAIN_FINISH << (i * 4);
> +
> +	aspi_write_reg(aspi, ASPI_REG_CHAIN, chain_reg);
> +}
> +
> +static int aspi_execute_chain(struct anarion_qspi *aspi)
> +{
> +	/* Go, johnny go */
> +	aspi_write_reg(aspi, ASPI_REG_GO, 1);
> +	return aspi_wait_idle(aspi);
> +}
> +
> +static int anarion_spi_read_nor_reg(struct spi_nor *nor, uint8_t opcode,
> +				    uint8_t *buf, int len)
> +{
> +	struct anarion_qspi *aspi = nor->priv;
> +	struct qspi_io_chain chain[] =  {
> +		{CHAIN_CMD, opcode, 1, MODE_IO_X1},
> +		{CHAIN_DATA_IN, 0, (uint16_t)len, MODE_IO_X1},
> +	};
> +
> +	if (len >= 8)
> +		return -EMSGSIZE;
> +
> +	aspi_setup_chain(aspi, chain, ARRAY_SIZE(chain));
> +	aspi_execute_chain(aspi);
> +
> +	aspi_drain_fifo(aspi, buf, len);
> +
> +	return 0;
> +}
> +
> +static int anarion_qspi_cmd_addr(struct anarion_qspi *aspi, uint16_t cmd,
> +				 uint32_t addr, int addr_len)
> +{
> +	size_t chain_size;
> +	const struct qspi_io_chain chain[] = {
> +		{CHAIN_CMD, cmd, 1, MODE_IO_X1},
> +		{CHAIN_ADDR, addr, addr_len, MODE_IO_X1},
> +	};
> +
> +	chain_size = addr_len ? ARRAY_SIZE(chain) : (ARRAY_SIZE(chain) - 1);
> +	aspi_setup_chain(aspi, chain, chain_size);
> +	return aspi_execute_chain(aspi);
> +}
> +
> +static int anarion_spi_write_nor_reg(struct spi_nor *nor, uint8_t opcode,
> +				     uint8_t *buf, int len)
> +{
> +	uint32_t addr, i;
> +	struct anarion_qspi *aspi = nor->priv;
> +
> +	if (len > sizeof(uint32_t))
> +		return -ENOTSUPP;
> +
> +	for (i = 0, addr = 0; i < len; i++)
> +		addr |= buf[len - 1 - i] << (i * 8);
> +
> +	return anarion_qspi_cmd_addr(aspi, opcode, addr, len);
> +}
> +
> +/* After every operation, we need to restore the IO chain for XIP to work. */
> +static void aspi_setup_xip_read_chain(struct anarion_qspi *spi,
> +				      struct spi_nor *nor)
> +{
> +	struct qspi_io_chain chain[] =  {
> +		{CHAIN_CMD, nor->read_opcode, 1, spi->xfer_mode_cmd},
> +		{CHAIN_ADDR, 0, nor->addr_width, spi->xfer_mode_addr},
> +		{CHAIN_HI_Z, 0, spi->num_hi_z_clocks, spi->xfer_mode_addr},
> +		{CHAIN_DATA_IN, 0, ASPI_DATA_LEN_MASK, spi->xfer_mode_data},
> +	};
> +
> +	aspi_setup_chain(spi, chain, ARRAY_SIZE(chain));
> +}
> +
> +static int aspi_do_write_xfer(struct anarion_qspi *spi,
> +			      struct spi_nor *nor, uint32_t addr,
> +			      const void *buf, size_t len)
> +{
> +	struct qspi_io_chain chain[] =  {
> +		{CHAIN_CMD, nor->program_opcode, 1, MODE_IO_X1},
> +		{CHAIN_ADDR, addr, nor->addr_width, MODE_IO_X1},
> +		{CHAIN_DATA_OUT, 0, len, MODE_IO_X1},
> +	};
> +
> +	aspi_setup_chain(spi, chain, ARRAY_SIZE(chain));
> +
> +	/* Go, johnny go */
> +	aspi_write_reg(spi, ASPI_REG_GO, 1);
> +
> +	aspi_poll_and_seed_fifo(spi, buf, len);
> +	return aspi_wait_idle(spi);
> +}
> +
> +/* While we could send read commands manually to the flash chip, we'd have to
> + * get data back through the DATA2 register. That is on the AHB bus, whereas
> + * XIP reads go over AXI. Hence, we use the memory-mapped flash space for read.
> + * TODO: Look at using DMA instead of memcpy().
> + */

Multiline comment looks like this,
/*
 * foo
 * bar
 */

> +static ssize_t anarion_spi_nor_read(struct spi_nor *nor, loff_t from,
> +				      size_t len, uint8_t *read_buf)
> +{
> +	struct anarion_qspi *aspi = nor->priv;
> +	void *from_xip = (void *)(aspi->xipbase + from);
> +
> +	aspi_setup_xip_read_chain(aspi, nor);
> +	memcpy(read_buf, from_xip, len);
> +
> +	return len;
> +}
> +
> +static ssize_t anarion_spi_nor_write(struct spi_nor *nor, loff_t to,
> +				     size_t len, const uint8_t *src)
> +{
> +	int ret;
> +	struct anarion_qspi *aspi = nor->priv;
> +
> +	dev_err(aspi->dev, "%s, @0x%llx + %zu\n", __func__, to, len);

Drop this.

> +	if (len > nor->page_size)
> +		return -EINVAL;
> +
> +	ret = aspi_do_write_xfer(aspi, nor, to, src, len);
> +	return (ret < 0) ? ret : len;
> +}
> +
> +/* TODO: Revisit this when we get actual HW. Right now max speed is 6 MHz. */
> +static void aspi_configure_clocks(struct anarion_qspi *aspi)
> +{
> +	uint8_t div = 0;
> +	uint32_t ck_ctl = aspi_read_reg(aspi, ASPI_REG_CLOCK);
> +
> +	ck_ctl &= ~ASPI_CLOCK_DIV_MASK;
> +	ck_ctl |= ASPI_CLOCK_DIV(div);
> +	aspi_write_reg(aspi, ASPI_REG_CLOCK, ck_ctl);
> +}
> +
> +static int anarion_qspi_drv_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	void __iomem *mmiobase;
> +	struct resource *res;
> +	struct anarion_qspi *aspi;
> +	struct device_node *flash_node;
> +	struct spi_nor *nor;
> +
> +	aspi = devm_kzalloc(&pdev->dev, sizeof(*aspi), GFP_KERNEL);
> +	if (!aspi)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, aspi);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mmiobase  = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(mmiobase)) {
> +		dev_err(&pdev->dev, "Cannot get base addresses (%ld)!\n",
> +			PTR_ERR(mmiobase));
> +		return PTR_ERR(mmiobase);
> +	}
> +	aspi->regbase = (uintptr_t)mmiobase;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	mmiobase = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(mmiobase)) {
> +		dev_err(&pdev->dev, "Cannot get XIP addresses (%ld)!\n",
> +			PTR_ERR(mmiobase));
> +		return PTR_ERR(mmiobase);
> +	}
> +	aspi->xipbase = (uintptr_t)mmiobase;
> +
> +	aspi->dev = &pdev->dev;
> +
> +	/* only support one attached flash */
> +	flash_node = of_get_next_available_child(pdev->dev.of_node, NULL);
> +	if (!flash_node) {
> +		dev_err(&pdev->dev, "no SPI flash device to configure\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Reset the controller */
> +	aspi_write_reg(aspi, ASPI_REG_CLOCK, ASPI_CLK_RESET_ALL);
> +	aspi_write_reg(aspi, ASPI_REG_LAT, 0x010);
> +	aspi_configure_clocks(aspi);
> +
> +	nor = &aspi->nor;
> +	nor->priv = aspi;
> +	nor->dev = aspi->dev;
> +	nor->read = anarion_spi_nor_read;
> +	nor->write = anarion_spi_nor_write;
> +	nor->read_reg = anarion_spi_read_nor_reg;
> +	nor->write_reg = anarion_spi_write_nor_reg;
> +
> +	spi_nor_set_flash_node(nor, flash_node);
> +
> +	ret = spi_nor_scan(&aspi->nor, NULL, SPI_NOR_DUAL);
> +	if (ret)
> +		return ret;
> +
> +	switch (nor->flash_read) {
> +	default:		/* Fall through */

This will break once we add OSPI support ...

> +	case SPI_NOR_NORMAL:
> +		aspi->num_hi_z_clocks = nor->read_dummy;
> +		aspi->xfer_mode_cmd = MODE_IO_X1;
> +		aspi->xfer_mode_addr = MODE_IO_X1;
> +		aspi->xfer_mode_data = MODE_IO_X1;
> +		break;
> +	case SPI_NOR_FAST:
> +		aspi->num_hi_z_clocks = nor->read_dummy;
> +		aspi->xfer_mode_cmd = MODE_IO_X1;
> +		aspi->xfer_mode_addr = MODE_IO_X1;
> +		aspi->xfer_mode_data = MODE_IO_X1;
> +		break;
> +	case SPI_NOR_DUAL:
> +		aspi->num_hi_z_clocks = nor->read_dummy;
> +		aspi->xfer_mode_cmd = MODE_IO_X1;
> +		aspi->xfer_mode_addr = MODE_IO_X1;
> +		aspi->xfer_mode_data = MODE_IO_X2;
> +		break;
> +	case SPI_NOR_QUAD:
> +		aspi->num_hi_z_clocks = nor->read_dummy;
> +		aspi->xfer_mode_cmd = MODE_IO_X1;
> +		aspi->xfer_mode_addr = MODE_IO_X1;
> +		aspi->xfer_mode_data = MODE_IO_X4;
> +		break;
> +	}
> +
> +	aspi_setup_xip_read_chain(aspi, nor);
> +
> +	mtd_device_register(&aspi->nor.mtd, NULL, 0);
> +
> +	return 0;
> +}
> +
> +static int anarion_qspi_drv_remove(struct platform_device *pdev)
> +{
> +	struct anarion_qspi *aspi = platform_get_drvdata(pdev);
> +
> +	mtd_device_unregister(&aspi->nor.mtd);
> +	return 0;
> +}
> +
> +static const struct of_device_id anarion_qspi_of_match[] = {
> +	{ .compatible = "adaptrum,anarion-qspi" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, anarion_qspi_of_match);
> +
> +static struct platform_driver anarion_qspi_driver = {
> +	.driver = {
> +		.name	= "anarion-qspi",
> +		.of_match_table = anarion_qspi_of_match,
> +	},
> +	.probe          = anarion_qspi_drv_probe,
> +	.remove		= anarion_qspi_drv_remove,
> +};
> +module_platform_driver(anarion_qspi_driver);
> +
> +MODULE_DESCRIPTION("Adaptrum Anarion Quad SPI Controller Driver");
> +MODULE_AUTHOR("Alexandru Gagniuc <mr.nuke.me@gmail.com>");
> +MODULE_LICENSE("GPL v2");
>

Hi Alexandru,

[auto build test WARNING on arc/for-next]
[also build test WARNING on v4.13-rc2 next-20170728]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Alexandru-Gagniuc/Initial-support-for-Adaptrum-Anarion-SOC/20170730-013701
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vgupta/arc.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/mtd/spi-nor/anarion-quadspi.c: In function 'anarion_spi_nor_read':
>> drivers/mtd/spi-nor/anarion-quadspi.c:335:19: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     void *from_xip = (void *)(aspi->xipbase + from);
                      ^

vim +335 drivers/mtd/spi-nor/anarion-quadspi.c

   325	
   326	/* While we could send read commands manually to the flash chip, we'd have to
   327	 * get data back through the DATA2 register. That is on the AHB bus, whereas
   328	 * XIP reads go over AXI. Hence, we use the memory-mapped flash space for read.
   329	 * TODO: Look at using DMA instead of memcpy().
   330	 */
   331	static ssize_t anarion_spi_nor_read(struct spi_nor *nor, loff_t from,
   332					      size_t len, uint8_t *read_buf)
   333	{
   334		struct anarion_qspi *aspi = nor->priv;
 > 335		void *from_xip = (void *)(aspi->xipbase + from);
   336	
   337		aspi_setup_xip_read_chain(aspi, nor);
   338		memcpy(read_buf, from_xip, len);
   339	
   340		return len;
   341	}
   342	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

On 07/29/2017 02:34 AM, Marek Vasut wrote:
> On 07/29/2017 12:07 AM, Alexandru Gagniuc wrote:
>> Add support for the QSPI controller found in Adaptrum Anarion SOCs.
>> This controller is designed specifically to handle SPI NOR chips, and
>> the driver is modeled as such.
>>
>> Because the system is emulated on an FPGA, we don't have a way to test
>> all the hardware adjustemts, so only basic features are implemented at
>> this time.
>>
Hi Marek,

Thank you very much for the comments. I am going to fix most issues you 
pointed out in v2. I do have some follow-up question on some of your 
comments, so please bear with me. I hope I have responded to all your 
comments, but feel free to nudge me if I missed any.

[snip]

>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/anarion-quadspi.txt
>> @@ -0,0 +1,22 @@
>> +* Adaptrum Anarion Quad SPI controller
>> +
>> +Required properties:
>> +- compatible : Should be "adaptrum,anarion-qspi".
>> +- reg : Contains two entries, each of which is a tuple consisting of a
>> +	physical address and length. The first entry is the address and
>> +	length of the controller register set. The second entry is the
>> +	address and length of the memory-mapped flash. This second region is
>> +	the region where the controller responds to XIP requests, and may be
>> +	larger than the size of the attached flash.
>
> You want to split the bindings into separate patch and CC Rob to review
> them.

Yes, I do want that now!

[snip]

>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 293c8a4..98dc012 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -48,6 +48,13 @@ config SPI_ATMEL_QUADSPI
>>  	  This driver does not support generic SPI. The implementation only
>>  	  supports SPI NOR.
>>
>> +config SPI_ANARION_QUADSPI
>> +	tristate "Adaptrum Anarion Quad SPI Controller"
>> +	depends on OF && HAS_IOMEM
>> +	help
>> +	  Enable support for the Adaptrum Anarion Quad SPI controller.
>> +	  This driver does not support generic SPI. It only supports SPI NOR.
>
> Keep the list sorted.

Staged for [PATCH v2].

>>  config SPI_CADENCE_QUADSPI
>>  	tristate "Cadence Quad SPI controller"
>>  	depends on OF && (ARM || COMPILE_TEST)
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index 285aab8..53635f6 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -1,6 +1,7 @@
>>  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
>>  obj-$(CONFIG_SPI_ASPEED_SMC)	+= aspeed-smc.o
>>  obj-$(CONFIG_SPI_ATMEL_QUADSPI)	+= atmel-quadspi.o
>> +obj-$(CONFIG_SPI_ANARION_QUADSPI)	+= anarion-quadspi.o
>
> DTTO, N is before S and T .

Staged for [PATCH v2].

>>  obj-$(CONFIG_SPI_CADENCE_QUADSPI)	+= cadence-quadspi.o
>>  obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.o
>>  obj-$(CONFIG_SPI_HISI_SFC)	+= hisi-sfc.o
>> diff --git a/drivers/mtd/spi-nor/anarion-quadspi.c b/drivers/mtd/spi-nor/anarion-quadspi.c
>> new file mode 100644
>> index 0000000..d981356
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/anarion-quadspi.c
>> @@ -0,0 +1,490 @@
>> +/*
>> + * Adaptrum Anarion Quad SPI controller driver
>> + *
>> + * Copyright (C) 2017, Adaptrum, Inc.
>> + * (Written by Alexandru Gagniuc <alex.g at adaptrum.com> for Adaptrum, Inc.)
>> + * Licensed under the GPLv2 or (at your option) any later version.
>
> The GPL boilerplate should be here.

I chose this form of the boilerplate because it seems to be quite used 
in other places. I am assuming the fatter boilerplate the requirement 
for drivers/mtd, correct?

[snip]

>> +#define ASPI_CLK_SW_RESET		(1 << 0)
>
> BIT(0) , fix globally

Staged for [PATCH v2].

>> +#define ASPI_CLK_RESET_BUF		(1 << 1)
>> +#define ASPI_CLK_RESET_ALL		(ASPI_CLK_SW_RESET | ASPI_CLK_RESET_BUF)
>> +#define ASPI_CLK_SPI_MODE3		(1 << 2)
>> +#define ASPI_CLOCK_DIV_MASK		(0xff << 8)
>> +#define ASPI_CLOCK_DIV(d)		(((d) << 8) & ASPI_CLOCK_DIV_MASK)
>> +
>> +#define ASPI_TIMEOUT_US		100000
>> +
>> +#define ASPI_DATA_LEN_MASK		0x3fff
>> +#define ASPI_MAX_XFER_LEN		(size_t)(ASPI_DATA_LEN_MASK + 1)
>> +
>> +#define MODE_IO_X1			(0 << 16)
>> +#define MODE_IO_X2			(1 << 16)
>> +#define MODE_IO_X4			(2 << 16)
>> +#define MODE_IO_SDR_POS_SKEW		(0 << 20)
>> +#define MODE_IO_SDR_NEG_SKEW		(1 << 20)
>> +#define MODE_IO_DDR_34_SKEW		(2 << 20)
>> +#define MODE_IO_DDR_PN_SKEW		(3 << 20)
>> +#define MODE_IO_DDR_DQS			(5 << 20)
>> +
>> +#define ASPI_STATUS_BUSY			(1 << 2)
>> +
>> +/*
>> + * This mask does not match reality. Get over it:
>
> What is this about ?

Each stage of the QSPI chain has two registers. The second register has 
a bitfield which takes in the length of the stage. For example, for 
DATA2, we can set the length up to 0x4000, but for ADDR2, we can only 
set a max of 4 bytes. I wrote this comment as a reminder to myself to be 
careful about using this mask. I'll rephrase the comment for [v2]

>> + * DATA2:	0x3fff
>> + * CMD2:	0x0003
>> + * ADDR2:	0x0007
>> + * PERF2:	0x0000
>> + * HI_Z:	0x003f
>> + * BCNT:	0x0007
>> + */
>> +#define CHAIN_LEN(x)		((x - 1) & ASPI_DATA_LEN_MASK)
>> +
>> +struct anarion_qspi {
>> +	struct		spi_nor nor;
>> +	struct		device *dev;
>> +	uintptr_t	regbase;
>
> Should be void __iomem * I guess ?

I chose uintptr_t as opposed to void *, because arithmetic on void * is 
not valid in C. What is the right answer hen, without risking undefined 
behavior?

>
>> +	uintptr_t	xipbase;
>> +	uint32_t	xfer_mode_cmd;
>
> u32 etc, fix globally, this is not userspace.

 From coding-style, section 5.(d), my understanding is that 
"Linux-specific u8/u16/u32/u64 types [...] are not mandatory in new 
code". Most of the code in this driver is shared between Linux, u-boot, 
openocd, ASIC validation tests, and manufacturing tests. Unlike, 
shortint types, stdint types are available in all cases.

Therefore, having to use a different set of primitive types makes code 
sharing much more difficult, and increases the maintenance burden, hence 
the strong preference for standard types. Is this reasonable?

[snip]

>> +static void aspi_drain_fifo(struct anarion_qspi *aspi, uint8_t *buf, size_t len)
>> +{
>> +	uint32_t data;
>
> Is this stuff below something like ioread32_rep() ?
>
>> +	aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>> +	while (len >= 4) {
>> +		data = aspi_read_reg(aspi, ASPI_REG_DATA1);
>> +		memcpy(buf, &data, sizeof(data));
>> +		buf += 4;
>> +		len -= 4;
>> +	}

That is very similar to ioread32_rep, yes. I kept this as for the 
reasons outlined above, but changing this to _rep() seems innocent enough.

>> +	if (len) {
>> +		aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, len);
>> +		data = aspi_read_reg(aspi, ASPI_REG_DATA1);
>> +		memcpy(buf, &data, len);
>> +	}
>> +}
>> +
>> +static void aspi_seed_fifo(struct anarion_qspi *spi,
>> +			   const uint8_t *buf, size_t len)
>> +{
>> +	uint32_t data;
>> +
>> +	aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>> +	while (len >= 4) {
>> +		memcpy(&data, buf, sizeof(data));
>> +		aspi_write_reg(spi, ASPI_REG_DATA1, data);
>
> iowrite32_rep ?
>

[snip]
>> +/* While we could send read commands manually to the flash chip, we'd have to
>> + * get data back through the DATA2 register. That is on the AHB bus, whereas
>> + * XIP reads go over AXI. Hence, we use the memory-mapped flash space for read.
>> + * TODO: Look at using DMA instead of memcpy().
>> + */
>
> Multiline comment looks like this,
> /*
>  * foo
>  * bar
>  */
>

Staged for [PATCH v2].

[snip]
>> +static ssize_t anarion_spi_nor_write(struct spi_nor *nor, loff_t to,
>> +				     size_t len, const uint8_t *src)
>> +{
>> +	int ret;
>> +	struct anarion_qspi *aspi = nor->priv;
>> +
>> +	dev_err(aspi->dev, "%s, @0x%llx + %zu\n", __func__, to, len);
>
> Drop this.
>
OOPS! That wasn't supposed to be there.
Staged for [PATCH v2].

[snip]

>> +	switch (nor->flash_read) {
>> +	default:		/* Fall through */
>
> This will break once we add OSPI support ...

Ooh, I see the API here has changed significantly from the 4.9 LTS 
branch where we originally developed the driver. I will add and test 
normal and FAST_READ support, but I won't have the bandwidth to test 
other modes yet. Those will have to remain as a TODO.

>> +	case SPI_NOR_NORMAL:
>> +		aspi->num_hi_z_clocks = nor->read_dummy;
>> +		aspi->xfer_mode_cmd = MODE_IO_X1;
>> +		aspi->xfer_mode_addr = MODE_IO_X1;
>> +		aspi->xfer_mode_data = MODE_IO_X1;
>> +		break;
>> +	case SPI_NOR_FAST:
>> +		aspi->num_hi_z_clocks = nor->read_dummy;
>> +		aspi->xfer_mode_cmd = MODE_IO_X1;
>> +		aspi->xfer_mode_addr = MODE_IO_X1;
>> +		aspi->xfer_mode_data = MODE_IO_X1;
>> +		break;
>> +	case SPI_NOR_DUAL:
>> +		aspi->num_hi_z_clocks = nor->read_dummy;
>> +		aspi->xfer_mode_cmd = MODE_IO_X1;
>> +		aspi->xfer_mode_addr = MODE_IO_X1;
>> +		aspi->xfer_mode_data = MODE_IO_X2;
>> +		break;
>> +	case SPI_NOR_QUAD:
>> +		aspi->num_hi_z_clocks = nor->read_dummy;
>> +		aspi->xfer_mode_cmd = MODE_IO_X1;
>> +		aspi->xfer_mode_addr = MODE_IO_X1;
>> +		aspi->xfer_mode_data = MODE_IO_X4;
>> +		break;
>> +	}
>> +
>> +	aspi_setup_xip_read_chain(aspi, nor);
>> +
>> +	mtd_device_register(&aspi->nor.mtd, NULL, 0);
>> +
>> +	return 0;
>> +}

[snip]

Hi Marek,

Me again!

On 07/29/2017 02:34 AM, Marek Vasut wrote:
> On 07/29/2017 12:07 AM, Alexandru Gagniuc wrote:
>> +static void aspi_drain_fifo(struct anarion_qspi *aspi, uint8_t *buf, size_t len)
>> +{
>> +	uint32_t data;
>
> Is this stuff below something like ioread32_rep() ?
>
[snip]
>> +	aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>> +	while (len >= 4) {
>> +		memcpy(&data, buf, sizeof(data));
>> +		aspi_write_reg(spi, ASPI_REG_DATA1, data);
>
> iowrite32_rep ?
>
>> +		buf += 4;
>> +		len -= 4;
>> +	}

I looked at using io(read|write)32_rep in these two places, and I've run 
into some issues.

First, I'm seeing unaligned MMIO accesses, which are not supported on 
ARC. Note that 'buf' has an alignment of 1, while the register requires 
an alignment of 4. The memcpy() in-between takes care of that, which was 
the original intent.

Other than that, we still need to break off the tail because we need to 
update ASPI_REG_BYTE_COUNT before writing/reading any more data from the 
FIFO. We have to keep track of the remainder, so we're not really saving 
any SLOC.

I'd like to keep the original version as I find it to be much more 
symmetrical and readable.

Thanks,
Alex

>> +
>> +	if (len) {
>> +		aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
>> +		memcpy(&data, buf, len);
>> +		aspi_write_reg(spi, ASPI_REG_DATA1, data);
>> +	}
>> +}


Exhibit A: aspi_seed_fifo with writesl()

static void aspi_seed_fifo(struct anarion_qspi *spi,
			   const uint8_t *buf, size_t len)
{
	uint32_t data;
	void __iomem *data_reg = (void *)(spi->regbase + ASPI_REG_DATA1);

	aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
	writesl(data_reg, buf, len / 4);
	buf += len & ~0x03;
	len &= 0x03;

	if (len) {
		aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
		memcpy(&data, buf, len);
		aspi_write_reg(spi, ASPI_REG_DATA1, data);
	}
}

On 07/31/2017 07:17 PM, Alexandru Gagniuc wrote:
[...]

>>> +++ b/drivers/mtd/spi-nor/anarion-quadspi.c
>>> @@ -0,0 +1,490 @@
>>> +/*
>>> + * Adaptrum Anarion Quad SPI controller driver
>>> + *
>>> + * Copyright (C) 2017, Adaptrum, Inc.
>>> + * (Written by Alexandru Gagniuc <alex.g at adaptrum.com> for
>>> Adaptrum, Inc.)
>>> + * Licensed under the GPLv2 or (at your option) any later version.
>>
>> The GPL boilerplate should be here.
> 
> I chose this form of the boilerplate because it seems to be quite used
> in other places. I am assuming the fatter boilerplate the requirement
> for drivers/mtd, correct?

AFAIK the regular GPLv2 boilerplate is the standard throughout the kernel.

> [snip]
> 
>>> +#define ASPI_CLK_SW_RESET        (1 << 0)
>>
>> BIT(0) , fix globally
> 
> Staged for [PATCH v2].
> 
>>> +#define ASPI_CLK_RESET_BUF        (1 << 1)
>>> +#define ASPI_CLK_RESET_ALL        (ASPI_CLK_SW_RESET |
>>> ASPI_CLK_RESET_BUF)
>>> +#define ASPI_CLK_SPI_MODE3        (1 << 2)
>>> +#define ASPI_CLOCK_DIV_MASK        (0xff << 8)
>>> +#define ASPI_CLOCK_DIV(d)        (((d) << 8) & ASPI_CLOCK_DIV_MASK)
>>> +
>>> +#define ASPI_TIMEOUT_US        100000
>>> +
>>> +#define ASPI_DATA_LEN_MASK        0x3fff
>>> +#define ASPI_MAX_XFER_LEN        (size_t)(ASPI_DATA_LEN_MASK + 1)
>>> +
>>> +#define MODE_IO_X1            (0 << 16)
>>> +#define MODE_IO_X2            (1 << 16)
>>> +#define MODE_IO_X4            (2 << 16)
>>> +#define MODE_IO_SDR_POS_SKEW        (0 << 20)
>>> +#define MODE_IO_SDR_NEG_SKEW        (1 << 20)
>>> +#define MODE_IO_DDR_34_SKEW        (2 << 20)
>>> +#define MODE_IO_DDR_PN_SKEW        (3 << 20)
>>> +#define MODE_IO_DDR_DQS            (5 << 20)
>>> +
>>> +#define ASPI_STATUS_BUSY            (1 << 2)
>>> +
>>> +/*
>>> + * This mask does not match reality. Get over it:
>>
>> What is this about ?
> 
> Each stage of the QSPI chain has two registers. The second register has
> a bitfield which takes in the length of the stage. For example, for
> DATA2, we can set the length up to 0x4000, but for ADDR2, we can only
> set a max of 4 bytes. I wrote this comment as a reminder to myself to be
> careful about using this mask. I'll rephrase the comment for [v2]

Please do.

>>> + * DATA2:    0x3fff
>>> + * CMD2:    0x0003
>>> + * ADDR2:    0x0007
>>> + * PERF2:    0x0000
>>> + * HI_Z:    0x003f
>>> + * BCNT:    0x0007
>>> + */
>>> +#define CHAIN_LEN(x)        ((x - 1) & ASPI_DATA_LEN_MASK)
>>> +
>>> +struct anarion_qspi {
>>> +    struct        spi_nor nor;
>>> +    struct        device *dev;
>>> +    uintptr_t    regbase;
>>
>> Should be void __iomem * I guess ?
> 
> I chose uintptr_t as opposed to void *, because arithmetic on void * is
> not valid in C. What is the right answer hen, without risking undefined
> behavior?

What sort of arithmetic ? It's perfectly valid in general ...

>>> +    uintptr_t    xipbase;
>>> +    uint32_t    xfer_mode_cmd;
>>
>> u32 etc, fix globally, this is not userspace.
> 
> From coding-style, section 5.(d), my understanding is that
> "Linux-specific u8/u16/u32/u64 types [...] are not mandatory in new
> code". Most of the code in this driver is shared between Linux, u-boot,
> openocd, ASIC validation tests, and manufacturing tests. Unlike,
> shortint types, stdint types are available in all cases.
> 
> Therefore, having to use a different set of primitive types makes code
> sharing much more difficult, and increases the maintenance burden, hence
> the strong preference for standard types. Is this reasonable?

The uXX is still prevalent in drivers/mtd/ according to git grep , so
I'd stick with that. Using uXX in U-Boot is perfectly fine and in fact
recommended.

> [snip]
> 
>>> +static void aspi_drain_fifo(struct anarion_qspi *aspi, uint8_t *buf,
>>> size_t len)
>>> +{
>>> +    uint32_t data;
>>
>> Is this stuff below something like ioread32_rep() ?
>>
>>> +    aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>>> +    while (len >= 4) {
>>> +        data = aspi_read_reg(aspi, ASPI_REG_DATA1);
>>> +        memcpy(buf, &data, sizeof(data));
>>> +        buf += 4;
>>> +        len -= 4;
>>> +    }
> 
> That is very similar to ioread32_rep, yes. I kept this as for the
> reasons outlined above, but changing this to _rep() seems innocent enough.

What reason ?

>>> +    if (len) {
>>> +        aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, len);
>>> +        data = aspi_read_reg(aspi, ASPI_REG_DATA1);
>>> +        memcpy(buf, &data, len);
>>> +    }
>>> +}

[...]

>>> +    switch (nor->flash_read) {
>>> +    default:        /* Fall through */
>>
>> This will break once we add OSPI support ...
> 
> Ooh, I see the API here has changed significantly from the 4.9 LTS
> branch where we originally developed the driver.I will add and test
> normal and FAST_READ support, but I won't have the bandwidth to test
> other modes yet. Those will have to remain as a TODO.

Sigh, please be so kind and use -next for your development next time ...

>>> +    case SPI_NOR_NORMAL:
>>> +        aspi->num_hi_z_clocks = nor->read_dummy;
>>> +        aspi->xfer_mode_cmd = MODE_IO_X1;
>>> +        aspi->xfer_mode_addr = MODE_IO_X1;
>>> +        aspi->xfer_mode_data = MODE_IO_X1;
>>> +        break;
>>> +    case SPI_NOR_FAST:
>>> +        aspi->num_hi_z_clocks = nor->read_dummy;
>>> +        aspi->xfer_mode_cmd = MODE_IO_X1;
>>> +        aspi->xfer_mode_addr = MODE_IO_X1;
>>> +        aspi->xfer_mode_data = MODE_IO_X1;
>>> +        break;
>>> +    case SPI_NOR_DUAL:
>>> +        aspi->num_hi_z_clocks = nor->read_dummy;
>>> +        aspi->xfer_mode_cmd = MODE_IO_X1;
>>> +        aspi->xfer_mode_addr = MODE_IO_X1;
>>> +        aspi->xfer_mode_data = MODE_IO_X2;
>>> +        break;
>>> +    case SPI_NOR_QUAD:
>>> +        aspi->num_hi_z_clocks = nor->read_dummy;
>>> +        aspi->xfer_mode_cmd = MODE_IO_X1;
>>> +        aspi->xfer_mode_addr = MODE_IO_X1;
>>> +        aspi->xfer_mode_data = MODE_IO_X4;
>>> +        break;
>>> +    }
>>> +
>>> +    aspi_setup_xip_read_chain(aspi, nor);
>>> +
>>> +    mtd_device_register(&aspi->nor.mtd, NULL, 0);
>>> +
>>> +    return 0;
>>> +}
> 
> [snip]

On 07/31/2017 10:54 PM, Alexandru Gagniuc wrote:
> Hi Marek,
> 
> Me again!
> 
> On 07/29/2017 02:34 AM, Marek Vasut wrote:
>> On 07/29/2017 12:07 AM, Alexandru Gagniuc wrote:
>>> +static void aspi_drain_fifo(struct anarion_qspi *aspi, uint8_t *buf,
>>> size_t len)
>>> +{
>>> +    uint32_t data;
>>
>> Is this stuff below something like ioread32_rep() ?
>>
> [snip]
>>> +    aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>>> +    while (len >= 4) {
>>> +        memcpy(&data, buf, sizeof(data));
>>> +        aspi_write_reg(spi, ASPI_REG_DATA1, data);
>>
>> iowrite32_rep ?
>>
>>> +        buf += 4;
>>> +        len -= 4;
>>> +    }
> 
> I looked at using io(read|write)32_rep in these two places, and I've run
> into some issues.
> 
> First, I'm seeing unaligned MMIO accesses, which are not supported on
> ARC. Note that 'buf' has an alignment of 1, while the register requires
> an alignment of 4. The memcpy() in-between takes care of that, which was
> the original intent.
> 
> Other than that, we still need to break off the tail because we need to
> update ASPI_REG_BYTE_COUNT before writing/reading any more data from the
> FIFO. We have to keep track of the remainder, so we're not really saving
> any SLOC.
> 
> I'd like to keep the original version as I find it to be much more
> symmetrical and readable.

Look at the other drivers, AFAIR the io*_rep() issue was solved over and
over again, maybe you can factor that code out.

> Thanks,
> Alex
> 
>>> +
>>> +    if (len) {
>>> +        aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
>>> +        memcpy(&data, buf, len);
>>> +        aspi_write_reg(spi, ASPI_REG_DATA1, data);
>>> +    }
>>> +}
> 
> 
> Exhibit A: aspi_seed_fifo with writesl()
> 
> static void aspi_seed_fifo(struct anarion_qspi *spi,
>                const uint8_t *buf, size_t len)
> {
>     uint32_t data;
>     void __iomem *data_reg = (void *)(spi->regbase + ASPI_REG_DATA1);
> 
>     aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>     writesl(data_reg, buf, len / 4);
>     buf += len & ~0x03;
>     len &= 0x03;
> 
>     if (len) {
>         aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
>         memcpy(&data, buf, len);
>         aspi_write_reg(spi, ASPI_REG_DATA1, data);
>     }
> }

On 07/31/2017 02:33 PM, Marek Vasut wrote:
> On 07/31/2017 07:17 PM, Alexandru Gagniuc wrote:

Hi Marek,

Thank you again for your feedback. I've applied a majority of your 
suggestions, and I am very happy with the result. I should have v2 
posted within a day or so.

[snip]
>>>> +/*
>>>> + * This mask does not match reality. Get over it:
>>>
>>> What is this about ?
>>
>> Each stage of the QSPI chain has two registers. The second register has
>> a bitfield which takes in the length of the stage. For example, for
>> DATA2, we can set the length up to 0x4000, but for ADDR2, we can only
>> set a max of 4 bytes. I wrote this comment as a reminder to myself to be
>> careful about using this mask. I'll rephrase the comment for [v2]
>
> Please do.
>
Staged for [PATCH v2]

>>>> + * DATA2:    0x3fff
>>>> + * CMD2:    0x0003
>>>> + * ADDR2:    0x0007
>>>> + * PERF2:    0x0000
>>>> + * HI_Z:    0x003f
>>>> + * BCNT:    0x0007
>>>> + */
>>>> +#define CHAIN_LEN(x)        ((x - 1) & ASPI_DATA_LEN_MASK)
>>>> +
>>>> +struct anarion_qspi {
>>>> +    struct        spi_nor nor;
>>>> +    struct        device *dev;
>>>> +    uintptr_t    regbase;
>>>
>>> Should be void __iomem * I guess ?
>>
>> I chose uintptr_t as opposed to void *, because arithmetic on void * is
>> not valid in C. What is the right answer hen, without risking undefined
>> behavior?
>
> What sort of arithmetic ? It's perfectly valid in general ...

ISO/IEC 9899:201x, Section 6.5.6, constraint(2) is not met when the one 
of the operands to addition is a void pointer.
Section 6.2.5 (19) defines void to be an incomplete type.

[snip]

>>> Is this stuff below something like ioread32_rep() ?
>>>
>>>> +    aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>>>> +    while (len >= 4) {
>>>> +        data = aspi_read_reg(aspi, ASPI_REG_DATA1);
>>>> +        memcpy(buf, &data, sizeof(data));
>>>> +        buf += 4;
>>>> +        len -= 4;
>>>> +    }
>>
>> That is very similar to ioread32_rep, yes. I kept this as for the
>> reasons outlined above, but changing this to _rep() seems innocent enough.
>
> What reason ?

Being able to share the code between the different codebases where it is 
used.

Alex

On 08/01/2017 12:20 AM, Alexandru Gagniuc wrote:
> On 07/31/2017 02:33 PM, Marek Vasut wrote:
>> On 07/31/2017 07:17 PM, Alexandru Gagniuc wrote:
> 
> Hi Marek,
> 
> Thank you again for your feedback. I've applied a majority of your
> suggestions, and I am very happy with the result. I should have v2
> posted within a day or so.

No. You should have v2 out in about a week or so after people have time
to review v1 some more.

> [snip]
>>>>> +/*
>>>>> + * This mask does not match reality. Get over it:
>>>>
>>>> What is this about ?
>>>
>>> Each stage of the QSPI chain has two registers. The second register has
>>> a bitfield which takes in the length of the stage. For example, for
>>> DATA2, we can set the length up to 0x4000, but for ADDR2, we can only
>>> set a max of 4 bytes. I wrote this comment as a reminder to myself to be
>>> careful about using this mask. I'll rephrase the comment for [v2]
>>
>> Please do.
>>
> Staged for [PATCH v2]
> 
>>>>> + * DATA2:    0x3fff
>>>>> + * CMD2:    0x0003
>>>>> + * ADDR2:    0x0007
>>>>> + * PERF2:    0x0000
>>>>> + * HI_Z:    0x003f
>>>>> + * BCNT:    0x0007
>>>>> + */
>>>>> +#define CHAIN_LEN(x)        ((x - 1) & ASPI_DATA_LEN_MASK)

btw parenthesis around (x) missing, although this is like GEN_MASK() or
something here ...

>>>>> +struct anarion_qspi {
>>>>> +    struct        spi_nor nor;
>>>>> +    struct        device *dev;
>>>>> +    uintptr_t    regbase;
>>>>
>>>> Should be void __iomem * I guess ?
>>>
>>> I chose uintptr_t as opposed to void *, because arithmetic on void * is
>>> not valid in C. What is the right answer hen, without risking undefined
>>> behavior?
>>
>> What sort of arithmetic ? It's perfectly valid in general ...
> 
> ISO/IEC 9899:201x, Section 6.5.6, constraint(2) is not met when the one
> of the operands to addition is a void pointer.
> Section 6.2.5 (19) defines void to be an incomplete type.

Is that something new in C 201x draft ? Anyway, this would mean half of
the drivers are broken, so I'm not convinced.

> [snip]
> 
>>>> Is this stuff below something like ioread32_rep() ?
>>>>
>>>>> +    aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>>>>> +    while (len >= 4) {
>>>>> +        data = aspi_read_reg(aspi, ASPI_REG_DATA1);
>>>>> +        memcpy(buf, &data, sizeof(data));
>>>>> +        buf += 4;
>>>>> +        len -= 4;
>>>>> +    }
>>>
>>> That is very similar to ioread32_rep, yes. I kept this as for the
>>> reasons outlined above, but changing this to _rep() seems innocent
>>> enough.
>>
>> What reason ?
> 
> Being able to share the code between the different codebases where it is
> used.

Yes, that argument isn't gonna work, it'd make things impossible to
maintain in the kernel.

On 07/31/2017 03:43 PM, Marek Vasut wrote:
> On 08/01/2017 12:20 AM, Alexandru Gagniuc wrote:
>> On 07/31/2017 02:33 PM, Marek Vasut wrote:
>>> On 07/31/2017 07:17 PM, Alexandru Gagniuc wrote:
>>>>>> +struct anarion_qspi {
>>>>>> +    struct        spi_nor nor;
>>>>>> +    struct        device *dev;
>>>>>> +    uintptr_t    regbase;
>>>>>
>>>>> Should be void __iomem * I guess ?
>>>>
>>>> I chose uintptr_t as opposed to void *, because arithmetic on void * is
>>>> not valid in C. What is the right answer hen, without risking undefined
>>>> behavior?
>>>
>>> What sort of arithmetic ? It's perfectly valid in general ...
>>
>> ISO/IEC 9899:201x, Section 6.5.6, constraint(2) is not met when the one
>> of the operands to addition is a void pointer.
>> Section 6.2.5 (19) defines void to be an incomplete type.
>
> Is that something new in C 201x draft ?

C99 had similar restrictions.

> Anyway, this would mean half of the drivers are broken, so I'm not convinced.

They are. Feel free to send me a private email if you want to discuss 
this further.

Alex