[PATCHv4 0/3] spi: support for Socionext Synquacer platform

[PATCHv4 0/3] spi: support for Socionext Synquacer platform

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Hello,

 Support for Socionext's FIP controller intended for flash device interfacing.
The controller can operate in 'direct' or 'command' mode. One mode directly
talks and provide a read/write i/f to the flash device. Other works as plain
SPI mode. This driver runs the controller as a SPI controller.

Changes since v3:
	# Convert IS_ERR returned from devm_clk_get(iPCLK) to NULL, that way
	  we can call clk_xxx(IPCLK) without first checking for it being valid.

Changes since v2:
        # Made iHCLK clock property required in DT, and iPCLK an optional extra.
        # Hardcode max number of slaves to 4, as specified in the manual.

Changes since v1:
        # Changed licence header to C++ style comment.
        # Removed redundant lock and transfer_mode backup member.
        # Fixed divisor to allow upto 254.

Jassi Brar (3):
  dt-bindings: spi: Add DT bindings for Synquacer
  spi: Add spi driver for Socionext Synquacer platform
  MAINTAINERS: Add entry for Synquacer SPI driver

 .../devicetree/bindings/spi/spi-synquacer.txt      |  23 +
 MAINTAINERS                                        |   7 +
 drivers/spi/Kconfig                                |  11 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-synquacer.c                        | 663 +++++++++++++++++++++
 5 files changed, 705 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-synquacer.txt
 create mode 100644 drivers/spi/spi-synquacer.c

-- 
2.7.4

[PATCHv4 1/3] dt-bindings: spi: Add DT bindings for Synquacer

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

This patch adds documentation for Device-Tree bindings for the
Socionext Synquacer spi driver.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/spi/spi-synquacer.txt      | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-synquacer.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-synquacer.txt b/Documentation/devicetree/bindings/spi/spi-synquacer.txt
new file mode 100644
index 0000000..d945a4a
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-synquacer.txt
@@ -0,0 +1,23 @@
+* Socionext Synquacer HS-SPI bindings
+
+Required Properties:
+- compatible: should be "socionext,synquacer-spi"
+- reg: physical base address of the controller and length of memory mapped
+       region.
+- clocks: core clock iHCLK. Optional rate clock iPCLK (default is iHCLK)
+- clock-names: Shall be "iHCLK" and "iPCLK" respectively
+
+Optional Properties:
+- socionext,use-rtm: boolean, if required to use "retimed clock" for RX
+- socionext,set-aces: boolean, if same active clock edges field to be set.
+
+Example:
+
+	spi0: spi@ff110000 {
+		compatible = "socionext,synquacer-spi";
+		reg = <0xff110000 0x1000>;
+		clocks = <&clk_fip006_spi>;
+		clock-names = "iHCLK";
+		socionext,use-rtm;
+		socionext,set-aces;
+	};
-- 
2.7.4

[PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

This patch adds support for controller found on synquacer platforms.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/spi/Kconfig         |  11 +
 drivers/spi/Makefile        |   1 +
 drivers/spi/spi-synquacer.c | 663 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 675 insertions(+)
 create mode 100644 drivers/spi/spi-synquacer.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 6037839..9e04bbe 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -659,6 +659,17 @@ config SPI_SUN6I
 	help
 	  This enables using the SPI controller on the Allwinner A31 SoCs.
 
+config SPI_SYNQUACER
+	tristate "Socionext's Synquacer HighSpeed SPI controller"
+	depends on ARCH_SYNQUACER || COMPILE_TEST
+	select SPI_BITBANG
+	help
+	  SPI driver for Socionext's High speed SPI controller which provides
+	  various operating modes for interfacing to serial peripheral devices
+	  that use the de-facto standard SPI protocol.
+
+	  It also supports the new dual-bit and quad-bit SPI protocol.
+
 config SPI_MXS
 	tristate "Freescale MXS SPI controller"
 	depends on ARCH_MXS
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 34c5f28..7c222f2 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_SPI_STM32) 		+= spi-stm32.o
 obj-$(CONFIG_SPI_ST_SSC4)		+= spi-st-ssc4.o
 obj-$(CONFIG_SPI_SUN4I)			+= spi-sun4i.o
 obj-$(CONFIG_SPI_SUN6I)			+= spi-sun6i.o
+obj-$(CONFIG_SPI_SYNQUACER)		+= spi-synquacer.o
 obj-$(CONFIG_SPI_TEGRA114)		+= spi-tegra114.o
 obj-$(CONFIG_SPI_TEGRA20_SFLASH)	+= spi-tegra20-sflash.o
 obj-$(CONFIG_SPI_TEGRA20_SLINK)		+= spi-tegra20-slink.o
diff --git a/drivers/spi/spi-synquacer.c b/drivers/spi/spi-synquacer.c
new file mode 100644
index 0000000..45c6c6c
--- /dev/null
+++ b/drivers/spi/spi-synquacer.c
@@ -0,0 +1,663 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Synquacer HSSPI controller driver
+//
+// Copyright (c) 2015-2018 Socionext Inc.
+// Copyright (c) 2018 Linaro Ltd.
+//
+
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/spinlock.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+
+#define MCTRL		0x0
+#define MEN	BIT(0)
+#define CSEN	BIT(1)
+#define BPCLK	BIT(3)
+#define MES	BIT(4)
+#define SYNCON	BIT(5)
+
+#define PCC0		0x4
+#define PCC(n)		(PCC0 + (n) * 4)
+#define RTM	BIT(3)
+#define ACES	BIT(2)
+#define SAFESYNC	BIT(16)
+#define CPHA	BIT(0)
+#define CPOL	BIT(1)
+#define SSPOL	BIT(4)
+#define SDIR	BIT(7)
+#define SS2CD	5
+#define SENDIAN	BIT(8)
+#define CDRS_SHIFT	9
+#define CDRS_MASK	0x7f
+
+#define TXF		0x14
+#define TXE		0x18
+#define TXC		0x1c
+#define RXF		0x20
+#define RXE		0x24
+#define RXC		0x28
+
+#define FAULTF		0x2c
+#define FAULTC		0x30
+
+#define DMCFG		0x34
+#define SSDC		BIT(1)
+#define MSTARTEN	BIT(2)
+
+#define	DMSTART		0x38
+#define TRIGGER		BIT(0)
+#define DMSTOP		BIT(8)
+#define CS_MASK		3
+#define CS_SHIFT	16
+#define DATA_TXRX	0
+#define DATA_RX		1
+#define DATA_TX		2
+#define DATA_MASK	3
+#define DATA_SHIFT	26
+#define BUS_WIDTH	24
+
+#define	DMBCC		0x3c
+#define DMSTATUS	0x40
+#define RX_DATA_MASK	0x1f
+#define RX_DATA_SHIFT	8
+#define TX_DATA_MASK	0x1f
+#define TX_DATA_SHIFT	16
+
+#define TXBITCNT	0x44
+
+#define FIFOCFG		0x4c
+#define BPW_MASK	0x3
+#define BPW_SHIFT	8
+#define RX_FLUSH	BIT(11)
+#define TX_FLUSH	BIT(12)
+#define RX_TRSHLD_MASK		0xf
+#define RX_TRSHLD_SHIFT		0
+#define TX_TRSHLD_MASK		0xf
+#define TX_TRSHLD_SHIFT		4
+
+#define TXFIFO		0x50
+#define RXFIFO		0x90
+#define MID		0xfc
+
+#define FIFO_DEPTH	16
+#define TX_TRSHLD	4
+#define RX_TRSHLD	(FIFO_DEPTH - TX_TRSHLD)
+
+#define TXBIT	BIT(1)
+#define RXBIT	BIT(2)
+
+#define IHCLK	0
+#define IPCLK	1
+
+struct synquacer_spi {
+	struct device *dev;
+	struct spi_master *master;
+
+	unsigned int cs;
+	unsigned int bpw;
+	unsigned int mode;
+	unsigned int speed;
+	bool aces, rtm;
+	void *rx_buf;
+	const void *tx_buf;
+	struct clk *clk[2];
+	void __iomem *regs;
+	unsigned int tx_words, rx_words;
+	unsigned int bus_width;
+};
+
+static void read_fifo(struct synquacer_spi *sspi)
+{
+	u32 len = readl_relaxed(sspi->regs + DMSTATUS);
+	int i;
+
+	len = (len >> RX_DATA_SHIFT) & RX_DATA_MASK;
+	len = min_t(unsigned int, len, sspi->rx_words);
+
+	switch (sspi->bpw) {
+	case 8:
+		{
+		u8 *buf = sspi->rx_buf;
+
+		for (i = 0; i < len; i++)
+			*buf++ = readb_relaxed(sspi->regs + RXFIFO);
+		sspi->rx_buf = buf;
+		break;
+		}
+	case 16:
+		{
+		u16 *buf = sspi->rx_buf;
+
+		for (i = 0; i < len; i++)
+			*buf++ = readw_relaxed(sspi->regs + RXFIFO);
+		sspi->rx_buf = buf;
+		break;
+		}
+	default:
+		{
+		u32 *buf = sspi->rx_buf;
+
+		for (i = 0; i < len; i++)
+			*buf++ = readl_relaxed(sspi->regs + RXFIFO);
+		sspi->rx_buf = buf;
+		break;
+		}
+	}
+
+	sspi->rx_words -= len;
+}
+
+static void write_fifo(struct synquacer_spi *sspi)
+{
+	u32 len = readl_relaxed(sspi->regs + DMSTATUS);
+	int i;
+
+	len = (len >> TX_DATA_SHIFT) & TX_DATA_MASK;
+	len = min_t(unsigned int, FIFO_DEPTH - len, sspi->tx_words);
+
+	switch (sspi->bpw) {
+	case 8:
+		{
+		const u8 *buf = sspi->tx_buf;
+
+		for (i = 0; i < len; i++)
+			writeb_relaxed(*buf++, sspi->regs + TXFIFO);
+		sspi->tx_buf = buf;
+		break;
+		}
+	case 16:
+		{
+		const u16 *buf = sspi->tx_buf;
+
+		for (i = 0; i < len; i++)
+			writew_relaxed(*buf++, sspi->regs + TXFIFO);
+		sspi->tx_buf = buf;
+		break;
+		}
+	default:
+		{
+		const u32 *buf = sspi->tx_buf;
+
+		for (i = 0; i < len; i++)
+			writel_relaxed(*buf++, sspi->regs + TXFIFO);
+		sspi->tx_buf = buf;
+		break;
+		}
+	}
+	sspi->tx_words -= len;
+}
+
+static int synquacer_spi_config(struct spi_master *master,
+				struct spi_device *spi,
+				struct spi_transfer *xfer)
+{
+	struct synquacer_spi *sspi = spi_master_get_devdata(master);
+	unsigned int speed, mode, bpw, cs, bus_width;
+	unsigned long rate;
+	u32 val, div;
+
+	/* Full Duplex only on 1bit wide bus */
+	if (xfer->rx_buf && xfer->tx_buf &&
+	    (xfer->rx_nbits != 1 || xfer->tx_nbits != 1)) {
+		dev_err(sspi->dev,
+			"RX and TX bus widths must match for Full-Duplex!\n");
+		return -EINVAL;
+	}
+
+	if (xfer->tx_buf)
+		bus_width = xfer->tx_nbits;
+	else
+		bus_width = xfer->rx_nbits;
+
+	mode = spi->mode;
+	cs = spi->chip_select;
+	speed = xfer->speed_hz;
+	bpw = xfer->bits_per_word;
+
+	/* return if nothing to change */
+	if (speed == sspi->speed &&
+	    bus_width == sspi->bus_width && bpw == sspi->bpw &&
+	    mode == sspi->mode && cs == sspi->cs) {
+		return 0;
+	}
+
+	rate = master->max_speed_hz;
+
+	div = DIV_ROUND_UP(rate, speed);
+	if (div > 254) {
+		dev_err(sspi->dev, "Requested rate too low (%u)\n",
+			sspi->speed);
+		return -EINVAL;
+	}
+
+	val = readl_relaxed(sspi->regs + PCC(cs));
+	val &= ~SAFESYNC;
+	if (bpw == 8 &&	(mode & (SPI_TX_DUAL | SPI_RX_DUAL)) && div < 3)
+		val |= SAFESYNC;
+	if (bpw == 8 &&	(mode & (SPI_TX_QUAD | SPI_RX_QUAD)) && div < 6)
+		val |= SAFESYNC;
+	if (bpw == 16 && (mode & (SPI_TX_QUAD | SPI_RX_QUAD)) && div < 3)
+		val |= SAFESYNC;
+
+	if (mode & SPI_CPHA)
+		val |= CPHA;
+	else
+		val &= ~CPHA;
+
+	if (mode & SPI_CPOL)
+		val |= CPOL;
+	else
+		val &= ~CPOL;
+
+	if (mode & SPI_CS_HIGH)
+		val |= SSPOL;
+	else
+		val &= ~SSPOL;
+
+	if (mode & SPI_LSB_FIRST)
+		val |= SDIR;
+	else
+		val &= ~SDIR;
+
+	if (sspi->aces)
+		val |= ACES;
+	else
+		val &= ~ACES;
+
+	if (sspi->rtm)
+		val |= RTM;
+	else
+		val &= ~RTM;
+
+	val |= (3 << SS2CD);
+	val |= SENDIAN;
+
+	val &= ~(CDRS_MASK << CDRS_SHIFT);
+	val |= ((div >> 1) << CDRS_SHIFT);
+
+	writel_relaxed(val, sspi->regs + PCC(cs));
+
+	val = readl_relaxed(sspi->regs + FIFOCFG);
+	val &= ~(BPW_MASK << BPW_SHIFT);
+	val |= ((bpw / 8 - 1) << BPW_SHIFT);
+	writel_relaxed(val, sspi->regs + FIFOCFG);
+
+	val = readl_relaxed(sspi->regs + DMSTART);
+	val &= ~(DATA_MASK << DATA_SHIFT);
+
+	if (xfer->tx_buf && xfer->rx_buf)
+		val |= (DATA_TXRX << DATA_SHIFT);
+	else if (xfer->rx_buf)
+		val |= (DATA_RX << DATA_SHIFT);
+	else
+		val |= (DATA_TX << DATA_SHIFT);
+
+	val &= ~(3 << BUS_WIDTH);
+	val |= ((bus_width >> 1) << BUS_WIDTH);
+	writel_relaxed(val, sspi->regs + DMSTART);
+
+	sspi->bpw = bpw;
+	sspi->mode = mode;
+	sspi->speed = speed;
+	sspi->cs = spi->chip_select;
+	sspi->bus_width = bus_width;
+
+	return 0;
+}
+
+static int synquacer_spi_transfer_one(struct spi_master *master,
+				      struct spi_device *spi,
+				      struct spi_transfer *xfer)
+{
+	struct synquacer_spi *sspi = spi_master_get_devdata(master);
+	int ret, words, busy = 0;
+	unsigned long bpw;
+	u32 val;
+
+	val = readl_relaxed(sspi->regs + FIFOCFG);
+	val |= RX_FLUSH;
+	val |= TX_FLUSH;
+	writel_relaxed(val, sspi->regs + FIFOCFG);
+
+	/* See if we can transfer 4-bytes as 1 word even if not asked */
+	bpw = xfer->bits_per_word;
+	if (bpw == 8 && !(xfer->len % 4) && !(spi->mode & SPI_LSB_FIRST))
+		xfer->bits_per_word = 32;
+
+	ret = synquacer_spi_config(master, spi, xfer);
+
+	/* restore */
+	xfer->bits_per_word = bpw;
+
+	if (ret)
+		return ret;
+
+	sspi->tx_buf = xfer->tx_buf;
+	sspi->rx_buf = xfer->rx_buf;
+
+	switch (sspi->bpw) {
+	case 8:
+		words = xfer->len;
+		break;
+	case 16:
+		words = xfer->len / 2;
+		break;
+	default:
+		words = xfer->len / 4;
+		break;
+	}
+
+	if (xfer->tx_buf) {
+		busy |= TXBIT;
+		sspi->tx_words = words;
+	} else {
+		sspi->tx_words = 0;
+	}
+
+	if (xfer->rx_buf) {
+		busy |= RXBIT;
+		sspi->rx_words = words;
+	} else {
+		sspi->rx_words = 0;
+	}
+
+	if (xfer->tx_buf)
+		write_fifo(sspi);
+
+	if (xfer->rx_buf) {
+		val = readl_relaxed(sspi->regs + FIFOCFG);
+		val &= ~(RX_TRSHLD_MASK << RX_TRSHLD_SHIFT);
+		val |= ((sspi->rx_words > FIFO_DEPTH ?
+			RX_TRSHLD : sspi->rx_words) << RX_TRSHLD_SHIFT);
+		writel_relaxed(val, sspi->regs + FIFOCFG);
+	}
+
+	writel_relaxed(~0, sspi->regs + TXC);
+	writel_relaxed(~0, sspi->regs + RXC);
+
+	/* Trigger */
+	val = readl_relaxed(sspi->regs + DMSTART);
+	val |= TRIGGER;
+	writel_relaxed(val, sspi->regs + DMSTART);
+
+	while (busy & (RXBIT | TXBIT)) {
+		if (sspi->rx_words)
+			read_fifo(sspi);
+		else
+			busy &= ~RXBIT;
+
+		if (sspi->tx_words) {
+			write_fifo(sspi);
+		} else {
+			u32 len;
+
+			do { /* wait for shifter to empty out */
+				cpu_relax();
+				len = readl_relaxed(sspi->regs + DMSTATUS);
+				len = (len >> TX_DATA_SHIFT) & TX_DATA_MASK;
+			} while (xfer->tx_buf && len);
+			busy &= ~TXBIT;
+		}
+	}
+
+	return 0;
+}
+
+static void synquacer_spi_set_cs(struct spi_device *spi, bool enable)
+{
+	struct synquacer_spi *sspi = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl_relaxed(sspi->regs + DMSTART);
+	val &= ~(CS_MASK << CS_SHIFT);
+	val |= spi->chip_select << CS_SHIFT;
+
+	if (!enable) {
+		writel_relaxed(val, sspi->regs + DMSTART);
+
+		val = readl_relaxed(sspi->regs + DMSTART);
+		val &= ~DMSTOP;
+		writel_relaxed(val, sspi->regs + DMSTART);
+	} else {
+		val |= DMSTOP;
+		writel_relaxed(val, sspi->regs + DMSTART);
+
+		if (sspi->rx_buf) {
+			u32 buf[16];
+
+			sspi->rx_buf = buf;
+			sspi->rx_words = 16;
+			read_fifo(sspi);
+		}
+	}
+}
+
+static int synquacer_spi_enable(struct spi_master *master)
+{
+	struct synquacer_spi *sspi = spi_master_get_devdata(master);
+	u32 val;
+
+	/* Disable module */
+	writel_relaxed(0, sspi->regs + MCTRL);
+	val = 0xfffff;
+	while (--val && (readl_relaxed(sspi->regs + MCTRL) & MES))
+		cpu_relax();
+	if (!val)
+		return -EBUSY;
+
+	writel_relaxed(0, sspi->regs + TXE);
+	writel_relaxed(0, sspi->regs + RXE);
+	val = readl_relaxed(sspi->regs + TXF);
+	writel_relaxed(val, sspi->regs + TXC);
+	val = readl_relaxed(sspi->regs + RXF);
+	writel_relaxed(val, sspi->regs + RXC);
+	val = readl_relaxed(sspi->regs + FAULTF);
+	writel_relaxed(val, sspi->regs + FAULTC);
+
+	val = readl_relaxed(sspi->regs + DMCFG);
+	val &= ~SSDC;
+	val &= ~MSTARTEN;
+	writel_relaxed(val, sspi->regs + DMCFG);
+
+	val = readl_relaxed(sspi->regs + MCTRL);
+	if (sspi->clk[IPCLK])
+		val |= BPCLK;
+	else
+		val &= ~BPCLK;
+
+	val &= ~CSEN;
+	val |= MEN;
+	val |= SYNCON;
+	writel_relaxed(val, sspi->regs + MCTRL);
+
+	return 0;
+}
+
+static int synquacer_spi_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct spi_master *master;
+	struct synquacer_spi *sspi;
+	struct resource *res;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*sspi));
+	if (!master)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, master);
+
+	sspi = spi_master_get_devdata(master);
+	sspi->dev = &pdev->dev;
+	sspi->master = master;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sspi->regs = devm_ioremap_resource(sspi->dev, res);
+	if (IS_ERR(sspi->regs)) {
+		ret = PTR_ERR(sspi->regs);
+		goto put_spi;
+	}
+
+	sspi->clk[IHCLK] = devm_clk_get(sspi->dev, "iHCLK");
+	if (IS_ERR(sspi->clk[IHCLK])) {
+		dev_err(&pdev->dev, "iHCLK not found\n");
+		ret = PTR_ERR(sspi->clk[IHCLK]);
+		goto put_spi;
+	}
+
+	sspi->clk[IPCLK] = devm_clk_get(sspi->dev, "iPCLK");
+	if (IS_ERR(sspi->clk[IPCLK]))
+		sspi->clk[IPCLK] = NULL;
+
+	sspi->aces = of_property_read_bool(np, "socionext,set-aces");
+	sspi->rtm = of_property_read_bool(np, "socionext,use-rtm");
+
+	master->num_chipselect = 4; /* max 4 supported */
+
+	clk_prepare_enable(sspi->clk[IPCLK]);
+	ret = clk_prepare_enable(sspi->clk[IHCLK]);
+	if (ret)
+		goto put_spi;
+
+	master->dev.of_node = np;
+	master->auto_runtime_pm = true;
+	master->bus_num = pdev->id;
+
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL |
+				SPI_TX_QUAD | SPI_RX_QUAD;
+	master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(24)
+					 | SPI_BPW_MASK(16) | SPI_BPW_MASK(8);
+
+	if (sspi->clk[IPCLK])
+		master->max_speed_hz = clk_get_rate(sspi->clk[IPCLK]);
+	else
+		master->max_speed_hz = clk_get_rate(sspi->clk[IHCLK]);
+	master->min_speed_hz = master->max_speed_hz / 254;
+
+	master->set_cs = synquacer_spi_set_cs;
+	master->transfer_one = synquacer_spi_transfer_one;
+
+	ret = synquacer_spi_enable(master);
+	if (ret)
+		goto fail_enable;
+
+	pm_runtime_set_active(sspi->dev);
+	pm_runtime_enable(sspi->dev);
+
+	ret = devm_spi_register_master(sspi->dev, master);
+	if (ret)
+		goto disable_pm;
+
+	return 0;
+
+disable_pm:
+	pm_runtime_disable(sspi->dev);
+fail_enable:
+	clk_disable_unprepare(sspi->clk[IHCLK]);
+	clk_disable_unprepare(sspi->clk[IPCLK]);
+put_spi:
+	spi_master_put(master);
+
+	return ret;
+}
+
+static int synquacer_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct synquacer_spi *sspi = spi_master_get_devdata(master);
+
+	pm_runtime_disable(sspi->dev);
+	clk_disable_unprepare(sspi->clk[IHCLK]);
+	clk_disable_unprepare(sspi->clk[IPCLK]);
+	spi_master_put(master);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int synquacer_spi_suspend(struct device *dev)
+{
+	struct spi_master *master = dev_get_drvdata(dev);
+	struct synquacer_spi *sspi = spi_master_get_devdata(master);
+	int ret;
+
+	ret = spi_master_suspend(master);
+	if (ret)
+		return ret;
+
+	if (!pm_runtime_suspended(dev)) {
+		clk_disable_unprepare(sspi->clk[IPCLK]);
+		clk_disable_unprepare(sspi->clk[IHCLK]);
+	}
+
+	return ret;
+}
+
+static int synquacer_spi_resume(struct device *dev)
+{
+	struct spi_master *master = dev_get_drvdata(dev);
+	struct synquacer_spi *sspi = spi_master_get_devdata(master);
+	int ret;
+
+	if (!pm_runtime_suspended(dev)) {
+		/* Ensure reconfigure during next xfer */
+		sspi->speed = 0;
+
+		clk_prepare_enable(sspi->clk[IPCLK]);
+		ret = clk_prepare_enable(sspi->clk[IHCLK]);
+		if (ret < 0) {
+			dev_err(dev, "failed to enable clk (%d)\n", ret);
+			return ret;
+		}
+
+		ret = synquacer_spi_enable(master);
+		if (ret) {
+			dev_err(dev, "failed to enable spi (%d)\n", ret);
+			return ret;
+		}
+	}
+
+	ret = spi_master_resume(master);
+	if (ret < 0) {
+		clk_disable_unprepare(sspi->clk[IHCLK]);
+		clk_disable_unprepare(sspi->clk[IPCLK]);
+	}
+
+	return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops synquacer_spi_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(synquacer_spi_suspend, synquacer_spi_resume)
+};
+
+static const struct of_device_id synquacer_spi_of_match[] = {
+	{.compatible = "socionext,synquacer-spi",},
+	{},
+};
+MODULE_DEVICE_TABLE(of, synquacer_spi_of_match);
+
+static struct platform_driver synquacer_spi_driver = {
+	.driver = {
+		.name = "synquacer-spi",
+		.pm = &synquacer_spi_pm_ops,
+		.of_match_table = of_match_ptr(synquacer_spi_of_match),
+	},
+	.probe = synquacer_spi_probe,
+	.remove = synquacer_spi_remove,
+};
+module_platform_driver(synquacer_spi_driver);
+
+MODULE_DESCRIPTION("Socionext Synquacer HS-SPI controller driver");
+MODULE_AUTHOR("Jassi Brar <jaswinder.singh@linaro.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCHv4 3/3] MAINTAINERS: Add entry for Synquacer SPI driver

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Add entry for the Synquacer spi driver and DT bindings.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 93a12af..866b230 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12851,6 +12851,13 @@ S:	Maintained
 F:	drivers/net/ethernet/socionext/netsec.c
 F:	Documentation/devicetree/bindings/net/socionext-netsec.txt
 
+SOCIONEXT (SNI) Synquacer SPI DRIVER
+M:	Jassi Brar <jaswinder.singh@linaro.org>
+L:	linux-spi@vger.kernel.org
+S:	Maintained
+F:	drivers/spi/spi-synquacer.c
+F:	Documentation/devicetree/bindings/spi/spi-synquacer.txt
+
 SONIC NETWORK DRIVER
 M:	Thomas Bogendoerfer <tsbogend@alpha.franken.de>
 L:	netdev@vger.kernel.org
-- 
2.7.4

Re: [PATCHv4 0/3] spi: support for Socionext Synquacer platform

[Mark Brown]

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

On Tue, Feb 27, 2018 at 06:26:22PM +0530, jassisinghbrar@gmail.com wrote:

>  Support for Socionext's FIP controller intended for flash device interfacing.
> The controller can operate in 'direct' or 'command' mode. One mode directly
> talks and provide a read/write i/f to the flash device. Other works as plain
> SPI mode. This driver runs the controller as a SPI controller.

If any updates are needed to a patch that's already been applied you
should submit incremental patches which make those updates.  This avoids
having to change published git commits which could cause problems for
people working against git.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[Geert Uytterhoeven]

Hi Jassi,

On Tue, Feb 27, 2018 at 1:58 PM,  <jassisinghbrar@gmail.com> wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> This patch adds support for controller found on synquacer platforms.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/spi/spi-synquacer.c

> +static void read_fifo(struct synquacer_spi *sspi)
> +{
> +       u32 len = readl_relaxed(sspi->regs + DMSTATUS);
> +       int i;

unsigned int, as len is unsigned.

> +static void write_fifo(struct synquacer_spi *sspi)
> +{
> +       u32 len = readl_relaxed(sspi->regs + DMSTATUS);
> +       int i;

unsigned int, as len is unsigned.

> +static int synquacer_spi_config(struct spi_master *master,
> +                               struct spi_device *spi,
> +                               struct spi_transfer *xfer)
> +{
> +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
> +       unsigned int speed, mode, bpw, cs, bus_width;
> +       unsigned long rate;

unsigned int, as max_speed_hz is u32, else you'll do a 64/32-bit division later.

> +static int synquacer_spi_transfer_one(struct spi_master *master,
> +                                     struct spi_device *spi,
> +                                     struct spi_transfer *xfer)
> +{
> +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
> +       int ret, words, busy = 0;

unsigned int words, as xfr->len is unsigned.

> +       unsigned long bpw;

unsigned int is plenty (bits_per_word is even u8).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[Trent Piepho]

On Tue, 2018-02-27 at 18:28 +0530, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> This patch adds support for controller found on synquacer platforms.
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  drivers/spi/Kconfig         |  11 +
>  drivers/spi/Makefile        |   1 +
>  drivers/spi/spi-synquacer.c | 663 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 675 insertions(+)
>  create mode 100644 drivers/spi/spi-synquacer.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 6037839..9e04bbe 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -659,6 +659,17 @@ config SPI_SUN6I
>  	help
>  	  This enables using the SPI controller on the Allwinner A31 SoCs.
>  
> +config SPI_SYNQUACER
> +	tristate "Socionext's Synquacer HighSpeed SPI controller"
> +	depends on ARCH_SYNQUACER || COMPILE_TEST
> +	select SPI_BITBANG
> +	help
> +	  SPI driver for Socionext's High speed SPI controller which provides
> +	  various operating modes for interfacing to serial peripheral devices
> +	  that use the de-facto standard SPI protocol.
> +
> +	  It also supports the new dual-bit and quad-bit SPI protocol.
> +

As someone who designs and develops embedded Linux devices, it would be
really useful if there was a bit more documentation about the driver.

The hardware doesn't support DMA or even interrupts, and must busy wait
for the duration of the entire SPI transfers.  That's kind of a Big
Deal.  A design that involves lots of SPI transfers on a slow clock
will work fine on most SPI hardware, but will be utterly unfeasible
here.  It would be nice to say something about that.  I'd probably put
it in driver code.

Re: [PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[Jassi Brar]

On Wed, Feb 28, 2018 at 11:25 PM, Trent Piepho <tpiepho@impinj.com> wrote:
> On Tue, 2018-02-27 at 18:28 +0530, jassisinghbrar@gmail.com wrote:
>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>
>> This patch adds support for controller found on synquacer platforms.
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>> ---
>>  drivers/spi/Kconfig         |  11 +
>>  drivers/spi/Makefile        |   1 +
>>  drivers/spi/spi-synquacer.c | 663 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 675 insertions(+)
>>  create mode 100644 drivers/spi/spi-synquacer.c
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 6037839..9e04bbe 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -659,6 +659,17 @@ config SPI_SUN6I
>>       help
>>         This enables using the SPI controller on the Allwinner A31 SoCs.
>>
>> +config SPI_SYNQUACER
>> +     tristate "Socionext's Synquacer HighSpeed SPI controller"
>> +     depends on ARCH_SYNQUACER || COMPILE_TEST
>> +     select SPI_BITBANG
>> +     help
>> +       SPI driver for Socionext's High speed SPI controller which provides
>> +       various operating modes for interfacing to serial peripheral devices
>> +       that use the de-facto standard SPI protocol.
>> +
>> +       It also supports the new dual-bit and quad-bit SPI protocol.
>> +
>
> As someone who designs and develops embedded Linux devices, it would be
> really useful if there was a bit more documentation about the driver.
>
> The hardware doesn't support DMA or even interrupts, and must busy wait
> for the duration of the entire SPI transfers.  That's kind of a Big
> Deal.  A design that involves lots of SPI transfers on a slow clock
> will work fine on most SPI hardware, but will be utterly unfeasible
> here.  It would be nice to say something about that.  I'd probably put
> it in driver code.
>
>From marketing point of view, why would I want to advertise my limitations? :)

Its not like any random platform might want to use this driver, nor
the enduser could do anything about it (its not a s/w issue). The
decision to integrate the ip in a SoC is already taken before the
driver is written. Obviously the users that need high spi performance
are not its intended target.

BTW, the IP is actually not that bad. It is meant for interfacing
flash devices and can operate in two modes - "direct" mode where it
maps flash memory as i/o region and the other last resort "spi" mode
where we could manually drive the flash. This driver implements that
limited "spi" mode.

Cheers!

Re: [PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[Jassi Brar]

On Wed, Feb 28, 2018 at 4:47 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Jassi,
>
> On Tue, Feb 27, 2018 at 1:58 PM,  <jassisinghbrar@gmail.com> wrote:
>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>
>> This patch adds support for controller found on synquacer platforms.
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>
> Thanks for your patch!
>
>> --- /dev/null
>> +++ b/drivers/spi/spi-synquacer.c
>
>> +static void read_fifo(struct synquacer_spi *sspi)
>> +{
>> +       u32 len = readl_relaxed(sspi->regs + DMSTATUS);
>> +       int i;
>
> unsigned int, as len is unsigned.
>
>> +static void write_fifo(struct synquacer_spi *sspi)
>> +{
>> +       u32 len = readl_relaxed(sspi->regs + DMSTATUS);
>> +       int i;
>
> unsigned int, as len is unsigned.
>
>> +static int synquacer_spi_config(struct spi_master *master,
>> +                               struct spi_device *spi,
>> +                               struct spi_transfer *xfer)
>> +{
>> +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
>> +       unsigned int speed, mode, bpw, cs, bus_width;
>> +       unsigned long rate;
>
> unsigned int, as max_speed_hz is u32, else you'll do a 64/32-bit division later.
>
>> +static int synquacer_spi_transfer_one(struct spi_master *master,
>> +                                     struct spi_device *spi,
>> +                                     struct spi_transfer *xfer)
>> +{
>> +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
>> +       int ret, words, busy = 0;
>
> unsigned int words, as xfr->len is unsigned.
>
>> +       unsigned long bpw;
>
> unsigned int is plenty (bits_per_word is even u8).
>
Will fix all.

Thank you.

Re: [PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[Ard Biesheuvel]

On 28 February 2018 at 18:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Wed, Feb 28, 2018 at 11:25 PM, Trent Piepho <tpiepho@impinj.com> wrote:
>> On Tue, 2018-02-27 at 18:28 +0530, jassisinghbrar@gmail.com wrote:
>>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>>
>>> This patch adds support for controller found on synquacer platforms.
>>>
>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>> ---
>>>  drivers/spi/Kconfig         |  11 +
>>>  drivers/spi/Makefile        |   1 +
>>>  drivers/spi/spi-synquacer.c | 663 ++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 675 insertions(+)
>>>  create mode 100644 drivers/spi/spi-synquacer.c
>>>
>>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>>> index 6037839..9e04bbe 100644
>>> --- a/drivers/spi/Kconfig
>>> +++ b/drivers/spi/Kconfig
>>> @@ -659,6 +659,17 @@ config SPI_SUN6I
>>>       help
>>>         This enables using the SPI controller on the Allwinner A31 SoCs.
>>>
>>> +config SPI_SYNQUACER
>>> +     tristate "Socionext's Synquacer HighSpeed SPI controller"
>>> +     depends on ARCH_SYNQUACER || COMPILE_TEST
>>> +     select SPI_BITBANG
>>> +     help
>>> +       SPI driver for Socionext's High speed SPI controller which provides
>>> +       various operating modes for interfacing to serial peripheral devices
>>> +       that use the de-facto standard SPI protocol.
>>> +
>>> +       It also supports the new dual-bit and quad-bit SPI protocol.
>>> +
>>
>> As someone who designs and develops embedded Linux devices, it would be
>> really useful if there was a bit more documentation about the driver.
>>
>> The hardware doesn't support DMA or even interrupts, and must busy wait
>> for the duration of the entire SPI transfers.  That's kind of a Big
>> Deal.  A design that involves lots of SPI transfers on a slow clock
>> will work fine on most SPI hardware, but will be utterly unfeasible
>> here.  It would be nice to say something about that.  I'd probably put
>> it in driver code.
>>
> From marketing point of view, why would I want to advertise my limitations? :)
>
> Its not like any random platform might want to use this driver, nor
> the enduser could do anything about it (its not a s/w issue). The
> decision to integrate the ip in a SoC is already taken before the
> driver is written. Obviously the users that need high spi performance
> are not its intended target.
>
> BTW, the IP is actually not that bad. It is meant for interfacing
> flash devices and can operate in two modes - "direct" mode where it
> maps flash memory as i/o region and the other last resort "spi" mode
> where we could manually drive the flash. This driver implements that
> limited "spi" mode.
>

My SoC manual does list interrupts for this IP block. Are you sure
SynQuacer uses the same version of the IP as before?

Re: [PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[Trent Piepho]

On Wed, 2018-02-28 at 23:59 +0530, Jassi Brar wrote:
> On Wed, Feb 28, 2018 at 11:25 PM, Trent Piepho <tpiepho@impinj.com> wrote:
> > On Tue, 2018-02-27 at 18:28 +0530, jassisinghbrar@gmail.com wrote:
> > > 
> > > +config SPI_SYNQUACER
> > > +     tristate "Socionext's Synquacer HighSpeed SPI controller"
> > > +     depends on ARCH_SYNQUACER || COMPILE_TEST
> > > +     select SPI_BITBANG
> > > +     help
> > > +       SPI driver for Socionext's High speed SPI controller which provides
> > > +       various operating modes for interfacing to serial peripheral devices
> > > +       that use the de-facto standard SPI protocol.
> > > +
> > > +       It also supports the new dual-bit and quad-bit SPI protocol.
> > > +
> > 
> > As someone who designs and develops embedded Linux devices, it would be
> > really useful if there was a bit more documentation about the driver.
> > 
> > The hardware doesn't support DMA or even interrupts, and must busy wait
> > for the duration of the entire SPI transfers.  That's kind of a Big
> > Deal.  A design that involves lots of SPI transfers on a slow clock
> > will work fine on most SPI hardware, but will be utterly unfeasible
> > here.  It would be nice to say something about that.  I'd probably put
> > it in driver code.
> > 
> 
> From marketing point of view, why would I want to advertise my limitations? :)
> 
> Its not like any random platform might want to use this driver, nor
> the enduser could do anything about it (its not a s/w issue). The
> decision to integrate the ip in a SoC is already taken before the
> driver is written. Obviously the users that need high spi performance
> are not its intended target.

It helps when choosing hardware, designing products, and also with
software design.  Example:

I really need another GPIO pin on this SoC.  Here's this interrupt line
from a spi slave.  I don't need that and can take the pin.  I can poll
the slave at a low rate and it'll be ok as there is no tight latency
requirement.

That might use a lot more CPU that one would think.

> BTW, the IP is actually not that bad. It is meant for interfacing
> flash devices and can operate in two modes - "direct" mode where it
> maps flash memory as i/o region and the other last resort "spi" mode
> where we could manually drive the flash. This driver implements that
> limited "spi" mode.

Here's another example, someone has a "direct" flash on their hardware,
probably just copied from the reference design.  In doing cost
engineering for the next rev, they determine that they don't need much
performance from this flash and should be able to use a cheaper non-vol 
storage part (eeprom..).  The new part has a Linux driver.  No one
realizes the significance of switching from "direct" to "spi" mode.

Re: [PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[Andy Shevchenko]

On Tue, Feb 27, 2018 at 2:58 PM,  <jassisinghbrar@gmail.com> wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> This patch adds support for controller found on synquacer platforms.

Thanks for the patch!

Unfortunately it has a set of typical mistakes.
Perhaps you guys eventually do follow some common style?

See my comments below.

> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/scatterlist.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spinlock.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

> +#define PCC0           0x4
> +#define PCC(n)         (PCC0 + (n) * 4)
> +#define RTM    BIT(3)
> +#define ACES   BIT(2)
> +#define SAFESYNC       BIT(16)
> +#define CPHA   BIT(0)
> +#define CPOL   BIT(1)
> +#define SSPOL  BIT(4)
> +#define SDIR   BIT(7)

> +#define SS2CD  5

What is this?

> +#define SENDIAN        BIT(8)

Why above list of bits is unordered?

> +#define CDRS_SHIFT     9

> +#define CDRS_MASK      0x7f

GENMASK() ?

> +#define        DMSTART         0x38
> +#define TRIGGER                BIT(0)
> +#define DMSTOP         BIT(8)

> +#define CS_MASK                3

GENMASK() ?

> +#define CS_SHIFT       16
> +#define DATA_TXRX      0
> +#define DATA_RX                1
> +#define DATA_TX                2

> +#define DATA_MASK      3

GENMASK() ?

> +#define DATA_SHIFT     26
> +#define BUS_WIDTH      24
> +
> +#define        DMBCC           0x3c
> +#define DMSTATUS       0x40

> +#define RX_DATA_MASK   0x1f

GENMASK() ?

> +#define RX_DATA_SHIFT  8

> +#define TX_DATA_MASK   0x1f

GENMASK() ?

> +#define TX_DATA_SHIFT  16
> +
> +#define TXBITCNT       0x44
> +
> +#define FIFOCFG                0x4c

> +#define BPW_MASK       0x3

GENMASK() ?

> +#define BPW_SHIFT      8
> +#define RX_FLUSH       BIT(11)
> +#define TX_FLUSH       BIT(12)

> +#define RX_TRSHLD_MASK         0xf

GENMASK() ?

> +#define RX_TRSHLD_SHIFT                0

> +#define TX_TRSHLD_MASK         0xf

GENMASK() ?

> +#define TX_TRSHLD_SHIFT                4
> +
> +#define TXFIFO         0x50
> +#define RXFIFO         0x90
> +#define MID            0xfc
> +
> +#define FIFO_DEPTH     16
> +#define TX_TRSHLD      4
> +#define RX_TRSHLD      (FIFO_DEPTH - TX_TRSHLD)
> +
> +#define TXBIT  BIT(1)
> +#define RXBIT  BIT(2)
> +
> +#define IHCLK  0
> +#define IPCLK  1

To all above. Can you do all definitions under a dedicated namespace?

> +struct synquacer_spi {

> +       struct device *dev;
> +       struct spi_master *master;

Isn't one refers to another?

> +
> +       unsigned int cs;
> +       unsigned int bpw;
> +       unsigned int mode;
> +       unsigned int speed;
> +       bool aces, rtm;
> +       void *rx_buf;
> +       const void *tx_buf;

> +       struct clk *clk[2];

Ouch. Please, split with a proper names.

> +       void __iomem *regs;
> +       unsigned int tx_words, rx_words;
> +       unsigned int bus_width;
> +};
> +
> +static void read_fifo(struct synquacer_spi *sspi)
> +{
> +       u32 len = readl_relaxed(sspi->regs + DMSTATUS);
> +       int i;
> +
> +       len = (len >> RX_DATA_SHIFT) & RX_DATA_MASK;
> +       len = min_t(unsigned int, len, sspi->rx_words);
> +

> +       switch (sspi->bpw) {
> +       case 8:
> +               {
> +               u8 *buf = sspi->rx_buf;
> +
> +               for (i = 0; i < len; i++)
> +                       *buf++ = readb_relaxed(sspi->regs + RXFIFO);

readsb() ?

> +               sspi->rx_buf = buf;
> +               break;
> +               }
> +       case 16:
> +               {
> +               u16 *buf = sspi->rx_buf;
> +
> +               for (i = 0; i < len; i++)
> +                       *buf++ = readw_relaxed(sspi->regs + RXFIFO);
> +               sspi->rx_buf = buf;
> +               break;
> +               }

readsw() ?

> +       default:
> +               {
> +               u32 *buf = sspi->rx_buf;
> +
> +               for (i = 0; i < len; i++)
> +                       *buf++ = readl_relaxed(sspi->regs + RXFIFO);
> +               sspi->rx_buf = buf;
> +               break;
> +               }

readsl() ?

> +       }
> +
> +       sspi->rx_words -= len;
> +}
> +
> +static void write_fifo(struct synquacer_spi *sspi)
> +{
> +       u32 len = readl_relaxed(sspi->regs + DMSTATUS);
> +       int i;
> +
> +       len = (len >> TX_DATA_SHIFT) & TX_DATA_MASK;
> +       len = min_t(unsigned int, FIFO_DEPTH - len, sspi->tx_words);
> +
> +       switch (sspi->bpw) {
> +       case 8:
> +               {
> +               const u8 *buf = sspi->tx_buf;
> +
> +               for (i = 0; i < len; i++)
> +                       writeb_relaxed(*buf++, sspi->regs + TXFIFO);
> +               sspi->tx_buf = buf;
> +               break;
> +               }

writesb() ?

> +       case 16:
> +               {
> +               const u16 *buf = sspi->tx_buf;
> +
> +               for (i = 0; i < len; i++)
> +                       writew_relaxed(*buf++, sspi->regs + TXFIFO);
> +               sspi->tx_buf = buf;
> +               break;
> +               }

writesw() ?

> +       default:
> +               {
> +               const u32 *buf = sspi->tx_buf;
> +
> +               for (i = 0; i < len; i++)
> +                       writel_relaxed(*buf++, sspi->regs + TXFIFO);
> +               sspi->tx_buf = buf;
> +               break;
> +               }

writesl() ?

> +       }
> +       sspi->tx_words -= len;
> +}
> +
> +static int synquacer_spi_config(struct spi_master *master,
> +                               struct spi_device *spi,
> +                               struct spi_transfer *xfer)
> +{
> +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
> +       unsigned int speed, mode, bpw, cs, bus_width;
> +       unsigned long rate;
> +       u32 val, div;
> +
> +       /* Full Duplex only on 1bit wide bus */

1-bit

> +}

> +static int synquacer_spi_transfer_one(struct spi_master *master,
> +                                     struct spi_device *spi,
> +                                     struct spi_transfer *xfer)
> +{

> +       /* See if we can transfer 4-bytes as 1 word even if not asked */

Why?

> +       bpw = xfer->bits_per_word;
> +       if (bpw == 8 && !(xfer->len % 4) && !(spi->mode & SPI_LSB_FIRST))
> +               xfer->bits_per_word = 32;

> +}

> +static void synquacer_spi_set_cs(struct spi_device *spi, bool enable)
> +{
> +       struct synquacer_spi *sspi = spi_master_get_devdata(spi->master);
> +       u32 val;

> +       if (!enable) {

Why not to use positive condition?

> +       } else {

> +       }
> +}

> +static int synquacer_spi_enable(struct spi_master *master)
> +{
> +       struct synquacer_spi *sspi = spi_master_get_devdata(master);

> +       u32 val;

unsigned int retries = 65535;

> +
> +       /* Disable module */
> +       writel_relaxed(0, sspi->regs + MCTRL);

> +       val = 0xfffff;
> +       while (--val && (readl_relaxed(sspi->regs + MCTRL) & MES))
> +               cpu_relax();
> +       if (!val)
> +               return -EBUSY;

while (readl...(...) && --retries)
 cpu_relax();
if (!retries)
 return -EBUSY;


> +static int synquacer_spi_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct spi_master *master;
> +       struct synquacer_spi *sspi;
> +       struct resource *res;
> +       int ret;
> +


> +       master = spi_alloc_master(&pdev->dev, sizeof(*sspi));

s/master/controller/ ?

I guess it might make sense to do for entire driver.

> +       if (!master)
> +               return -ENOMEM;

+ empty line ?

> +       platform_set_drvdata(pdev, master);

> +       clk_prepare_enable(sspi->clk[IPCLK]);

Also can fail.

> +       ret = clk_prepare_enable(sspi->clk[IHCLK]);
> +       if (ret)
> +               goto put_spi;

> +#ifdef CONFIG_PM_SLEEP

__maybe_unused

> +static int synquacer_spi_suspend(struct device *dev)

> +static int synquacer_spi_resume(struct device *dev)

> +               clk_prepare_enable(sspi->clk[IPCLK]);

It can fail.

> +               ret = clk_prepare_enable(sspi->clk[IHCLK]);
> +               if (ret < 0) {
> +                       dev_err(dev, "failed to enable clk (%d)\n", ret);
> +                       return ret;
> +               }

> +}
> +#endif /* CONFIG_PM_SLEEP */

> +static const struct dev_pm_ops synquacer_spi_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(synquacer_spi_suspend, synquacer_spi_resume)
> +};

static SIMPLE_DEV_PM_OPS() ?

> +static const struct of_device_id synquacer_spi_of_match[] = {
> +       {.compatible = "socionext,synquacer-spi",},

> +       {},

No comma needed.

> +};
> +MODULE_DEVICE_TABLE(of, synquacer_spi_of_match);

> +static struct platform_driver synquacer_spi_driver = {
> +       .driver = {
> +               .name = "synquacer-spi",
> +               .pm = &synquacer_spi_pm_ops,

> +               .of_match_table = of_match_ptr(synquacer_spi_of_match),

of_match_ptr() should be dropped.

> +       },
> +       .probe = synquacer_spi_probe,
> +       .remove = synquacer_spi_remove,
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 12:02 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/scatterlist.h>
>> +#include <linux/slab.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/spinlock.h>

>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>

Forgot to add.
Is there any clock provider? I perhaps missed it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCHv4 2/3] spi: Add spi driver for Socionext Synquacer platform

[Jassi Brar]

On Thu, Mar 1, 2018 at 12:06 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 28 February 2018 at 18:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Wed, Feb 28, 2018 at 11:25 PM, Trent Piepho <tpiepho@impinj.com> wrote:
>>> On Tue, 2018-02-27 at 18:28 +0530, jassisinghbrar@gmail.com wrote:
>>>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>>>
>>>> This patch adds support for controller found on synquacer platforms.
>>>>
>>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>>> ---
>>>>  drivers/spi/Kconfig         |  11 +
>>>>  drivers/spi/Makefile        |   1 +
>>>>  drivers/spi/spi-synquacer.c | 663 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 675 insertions(+)
>>>>  create mode 100644 drivers/spi/spi-synquacer.c
>>>>
>>>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>>>> index 6037839..9e04bbe 100644
>>>> --- a/drivers/spi/Kconfig
>>>> +++ b/drivers/spi/Kconfig
>>>> @@ -659,6 +659,17 @@ config SPI_SUN6I
>>>>       help
>>>>         This enables using the SPI controller on the Allwinner A31 SoCs.
>>>>
>>>> +config SPI_SYNQUACER
>>>> +     tristate "Socionext's Synquacer HighSpeed SPI controller"
>>>> +     depends on ARCH_SYNQUACER || COMPILE_TEST
>>>> +     select SPI_BITBANG
>>>> +     help
>>>> +       SPI driver for Socionext's High speed SPI controller which provides
>>>> +       various operating modes for interfacing to serial peripheral devices
>>>> +       that use the de-facto standard SPI protocol.
>>>> +
>>>> +       It also supports the new dual-bit and quad-bit SPI protocol.
>>>> +
>>>
>>> As someone who designs and develops embedded Linux devices, it would be
>>> really useful if there was a bit more documentation about the driver.
>>>
>>> The hardware doesn't support DMA or even interrupts, and must busy wait
>>> for the duration of the entire SPI transfers.  That's kind of a Big
>>> Deal.  A design that involves lots of SPI transfers on a slow clock
>>> will work fine on most SPI hardware, but will be utterly unfeasible
>>> here.  It would be nice to say something about that.  I'd probably put
>>> it in driver code.
>>>
>> From marketing point of view, why would I want to advertise my limitations? :)
>>
>> Its not like any random platform might want to use this driver, nor
>> the enduser could do anything about it (its not a s/w issue). The
>> decision to integrate the ip in a SoC is already taken before the
>> driver is written. Obviously the users that need high spi performance
>> are not its intended target.
>>
>> BTW, the IP is actually not that bad. It is meant for interfacing
>> flash devices and can operate in two modes - "direct" mode where it
>> maps flash memory as i/o region and the other last resort "spi" mode
>> where we could manually drive the flash. This driver implements that
>> limited "spi" mode.
>>
>
> My SoC manual does list interrupts for this IP block. Are you sure
> SynQuacer uses the same version of the IP as before?
>
Having got hold of english specs, the newer version does seem to
support IRQ despite being named same "FIP006" as before.
Let me investigate deeper.

Thanks.