[PATCH 0/2] add SPI controller driver for UniPhier SoCs

[PATCH 0/2] add SPI controller driver for UniPhier SoCs

[Keiji Hayashibara]

This series adds support for SPI controller driver implemented on UniPhier SoCs.

Keiji Hayashibara (1):
  spi: add SPI controller driver for UniPhier SoC

Kunihiko Hayashi (1):
  dt-bindings: spi: add DT bindings for UniPhier SPI controller

 .../devicetree/bindings/spi/spi-uniphier.txt       |  26 +
 drivers/spi/Kconfig                                |   7 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-uniphier.c                         | 532 +++++++++++++++++++++
 4 files changed, 566 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt
 create mode 100644 drivers/spi/spi-uniphier.c

-- 
2.7.4

[PATCH 0/2] add SPI controller driver for UniPhier SoCs

[Keiji Hayashibara]

This series adds support for SPI controller driver implemented on UniPhier SoCs.

Keiji Hayashibara (1):
  spi: add SPI controller driver for UniPhier SoC

Kunihiko Hayashi (1):
  dt-bindings: spi: add DT bindings for UniPhier SPI controller

 .../devicetree/bindings/spi/spi-uniphier.txt       |  26 +
 drivers/spi/Kconfig                                |   7 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-uniphier.c                         | 532 +++++++++++++++++++++
 4 files changed, 566 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt
 create mode 100644 drivers/spi/spi-uniphier.c

-- 
2.7.4

[PATCH 1/2] dt-bindings: spi: add DT bindings for UniPhier SPI controller

[Keiji Hayashibara]

From: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

Add DT bindings for SPI controller implemented in UniPhier SoCs.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Signed-off-by: Keiji Hayashibara <hayashibara.keiji@socionext.com>
---
 .../devicetree/bindings/spi/spi-uniphier.txt       | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-uniphier.txt b/Documentation/devicetree/bindings/spi/spi-uniphier.txt
new file mode 100644
index 0000000..9c8c4a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-uniphier.txt
@@ -0,0 +1,26 @@
+Socionext UniPhier SPI controller driver
+
+UniPhier SoCs have two types of SPI controllers; SCSSI supports a
+single channel, and MCSSI supports multiple channels.
+Both of them support the SPI master mode only.
+
+Required properties:
+ - compatible: should be
+	"socionext,uniphier-scssi" - for SCSSI device
+	"socionext,uniphier-mcssi" - for MCSSI device
+ - reg: address and length of the spi master registers
+ - #address-cells: must be <1>, see spi-bus.txt
+ - #size-cells: must be <0>, see spi-bus.txt
+ - clocks: A phandle to the clock for the device.
+ - resets: A phandle to the reset control for the device.
+
+Example:
+
+spi0: spi@54006000 {
+	compatible = "socionext,uniphier-scssi";
+	reg = <0x54006000 0x100>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	clocks = <&peri_clk 11>;
+	resets = <&peri_rst 11>;
+};
-- 
2.7.4

[PATCH 1/2] dt-bindings: spi: add DT bindings for UniPhier SPI controller

[Keiji Hayashibara]

From: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

Add DT bindings for SPI controller implemented in UniPhier SoCs.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Signed-off-by: Keiji Hayashibara <hayashibara.keiji@socionext.com>
---
 .../devicetree/bindings/spi/spi-uniphier.txt       | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-uniphier.txt b/Documentation/devicetree/bindings/spi/spi-uniphier.txt
new file mode 100644
index 0000000..9c8c4a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-uniphier.txt
@@ -0,0 +1,26 @@
+Socionext UniPhier SPI controller driver
+
+UniPhier SoCs have two types of SPI controllers; SCSSI supports a
+single channel, and MCSSI supports multiple channels.
+Both of them support the SPI master mode only.
+
+Required properties:
+ - compatible: should be
+	"socionext,uniphier-scssi" - for SCSSI device
+	"socionext,uniphier-mcssi" - for MCSSI device
+ - reg: address and length of the spi master registers
+ - #address-cells: must be <1>, see spi-bus.txt
+ - #size-cells: must be <0>, see spi-bus.txt
+ - clocks: A phandle to the clock for the device.
+ - resets: A phandle to the reset control for the device.
+
+Example:
+
+spi0: spi at 54006000 {
+	compatible = "socionext,uniphier-scssi";
+	reg = <0x54006000 0x100>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	clocks = <&peri_clk 11>;
+	resets = <&peri_rst 11>;
+};
-- 
2.7.4

[PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Keiji Hayashibara]

Add SPI controller driver implemented in Socionext UniPhier SoCs.

UniPhier SoCs have two types SPI controllers; SCSSI supports a
single channel, and MCSSI supports multiple channels.
Currently this driver supports SCSSI only.

This controller has 32bit TX/RX FIFO with depth of eight entry,
and supports the SPI master mode only.

This commit is implemented in PIO transfer mode, not DMA transfer.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Signed-off-by: Keiji Hayashibara <hayashibara.keiji@socionext.com>
---
 drivers/spi/Kconfig        |   7 +
 drivers/spi/Makefile       |   1 +
 drivers/spi/spi-uniphier.c | 532 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 540 insertions(+)
 create mode 100644 drivers/spi/spi-uniphier.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ad5d68e..a3cb0b4 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -688,6 +688,13 @@ config SPI_TXX9
 	help
 	  SPI driver for Toshiba TXx9 MIPS SoCs
 
+config SPI_UNIPHIER
+	tristate "Socionext UniPhier SPI Controller"
+	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
+	help
+	  This driver supports the SPI controller on Socionext
+	  UniPhier SoCs.
+
 config SPI_XCOMM
 	tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
 	depends on I2C
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index cb1f437..a90d559 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -101,6 +101,7 @@ spi-thunderx-objs			:= spi-cavium.o spi-cavium-thunderx.o
 obj-$(CONFIG_SPI_THUNDERX)		+= spi-thunderx.o
 obj-$(CONFIG_SPI_TOPCLIFF_PCH)		+= spi-topcliff-pch.o
 obj-$(CONFIG_SPI_TXX9)			+= spi-txx9.o
+obj-$(CONFIG_SPI_UNIPHIER)		+= spi-uniphier.o
 obj-$(CONFIG_SPI_XCOMM)		+= spi-xcomm.o
 obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
 obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
diff --git a/drivers/spi/spi-uniphier.c b/drivers/spi/spi-uniphier.c
new file mode 100644
index 0000000..92718ee
--- /dev/null
+++ b/drivers/spi/spi-uniphier.c
@@ -0,0 +1,532 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * spi-uniphier.c - Socionext UniPhier SPI controller driver
+ *
+ * Copyright 2012      Panasonic Corporation
+ * Copyright 2016-2018 Socionext Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+
+#define SSI_TIMEOUT		2000	/* ms */
+#define SSI_MAX_CLK_DIVIDER	254
+#define SSI_MIN_CLK_DIVIDER	4
+
+struct uniphier_spi_priv {
+	void __iomem *base;
+	int irq;
+	struct clk *clk;
+	struct spi_master *master;
+	struct completion xfer_done;
+
+	int error;
+	unsigned int tx_bytes;
+	unsigned int rx_bytes;
+	const u8 *tx_buf;
+	u8 *rx_buf;
+
+	u8 bits_per_word;
+	u16 mode;
+	u32 speed_hz;
+};
+
+#define SSI_CTL			0x0
+#define   SSI_CTL_EN		BIT(0)
+
+#define SSI_CKS			0x4
+#define   SSI_CKS_CKRAT_MASK	GENMASK(7, 0)
+#define   SSI_CKS_CKPHS		BIT(14)
+#define   SSI_CKS_CKINIT	BIT(13)
+#define   SSI_CKS_CKDLY		BIT(12)
+
+#define SSI_TXWDS		0x8
+#define   SSI_TXWDS_WDLEN_MASK	GENMASK(13, 8)
+#define   SSI_TXWDS_TDTF_MASK	GENMASK(7, 6)
+#define   SSI_TXWDS_DTLEN_MASK	GENMASK(5, 0)
+
+#define SSI_RXWDS		0xc
+#define   SSI_RXWDS_DTLEN_MASK	GENMASK(5, 0)
+
+#define SSI_FPS			0x10
+#define   SSI_FPS_FSPOL		BIT(15)
+#define   SSI_FPS_FSTRT		BIT(14)
+
+#define SSI_SR			0x14
+#define   SSI_SR_RNE		BIT(0)
+
+#define SSI_IE			0x18
+#define   SSI_IE_RCIE		BIT(3)
+#define   SSI_IE_RORIE		BIT(0)
+
+#define SSI_IS			0x1c
+#define   SSI_IS_RXRS		BIT(9)
+#define   SSI_IS_RCID		BIT(3)
+#define   SSI_IS_RORID		BIT(0)
+
+#define SSI_IC			0x1c
+#define   SSI_IC_TCIC		BIT(4)
+#define   SSI_IC_RCIC		BIT(3)
+#define   SSI_IC_RORIC		BIT(0)
+
+#define SSI_FC			0x20
+#define   SSI_FC_TXFFL		BIT(12)
+#define   SSI_FC_TXFTH_MASK	GENMASK(11, 8)
+#define   SSI_FC_RXFFL		BIT(4)
+#define   SSI_FC_RXFTH_MASK	GENMASK(3, 0)
+
+#define SSI_TXDR		0x24
+#define SSI_RXDR		0x24
+
+#define SSI_FIFO_DEPTH		8
+
+#define BYTES_PER_WORD(x)			\
+({						\
+	int __x;				\
+	__x = (x <= 8)  ? 1 :			\
+	      (x <= 16) ? 2 : 4;		\
+	__x;					\
+})
+
+static inline void uniphier_spi_irq_enable(struct spi_device *spi, u32 mask)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_IE);
+	val |= mask;
+	writel(val, priv->base + SSI_IE);
+}
+
+static inline void uniphier_spi_irq_disable(struct spi_device *spi, u32 mask)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_IE);
+	val &= ~mask;
+	writel(val, priv->base + SSI_IE);
+}
+
+static void uniphier_spi_set_transfer_size(struct spi_device *spi, int size)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_TXWDS);
+	val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
+	val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
+	val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
+	writel(val, priv->base + SSI_TXWDS);
+
+	val = readl(priv->base + SSI_RXWDS);
+	val &= ~SSI_RXWDS_DTLEN_MASK;
+	val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
+	writel(val, priv->base + SSI_RXWDS);
+}
+
+static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val, ckrat;
+
+	/*
+	 * the supported rates are even numbers from 4 to 254. (4,6,8...254)
+	 * round up as we look for equal or less speed
+	 */
+	ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
+	ckrat = roundup(ckrat, 2);
+
+	/* check if requested speed is too small */
+	if (ckrat > SSI_MAX_CLK_DIVIDER)
+		return -EINVAL;
+
+	if (ckrat < SSI_MIN_CLK_DIVIDER)
+		ckrat = SSI_MIN_CLK_DIVIDER;
+
+	val = readl(priv->base + SSI_CKS);
+	val &= ~SSI_CKS_CKRAT_MASK;
+	val |= ckrat & SSI_CKS_CKRAT_MASK;
+	writel(val, priv->base + SSI_CKS);
+
+	return 0;
+}
+
+static int uniphier_spi_setup_transfer(struct spi_device *spi,
+				       struct spi_transfer *t)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+	int ret;
+
+	priv->error = 0;
+	priv->tx_buf = t->tx_buf;
+	priv->rx_buf = t->rx_buf;
+	priv->tx_bytes = priv->rx_bytes = t->len;
+
+	if (priv->bits_per_word != t->bits_per_word) {
+		uniphier_spi_set_transfer_size(spi, t->bits_per_word);
+		priv->bits_per_word = t->bits_per_word;
+	}
+
+	if (priv->speed_hz != t->speed_hz) {
+		ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
+		if (ret)
+			return ret;
+		priv->speed_hz = t->speed_hz;
+	}
+
+	/* reset FIFOs */
+	val = SSI_FC_TXFFL | SSI_FC_RXFFL;
+	writel(val, priv->base + SSI_FC);
+
+	return 0;
+}
+
+static void uniphier_spi_send(struct uniphier_spi_priv *priv)
+{
+	int i, loop;
+	u32 val = 0;
+
+	loop = BYTES_PER_WORD(priv->bits_per_word);
+	if (priv->tx_bytes < loop)
+		loop = priv->tx_bytes;
+
+	priv->tx_bytes -= loop;
+
+	if (priv->tx_buf)
+		for (i = 0; i < loop; i++) {
+			val |= (*(const u8 *)priv->tx_buf)
+						<< (BITS_PER_BYTE * i);
+			(const u8 *)priv->tx_buf++;
+		}
+
+	writel(val, priv->base + SSI_TXDR);
+}
+
+static void uniphier_spi_recv(struct uniphier_spi_priv *priv)
+{
+	int i, loop;
+	u32 val;
+
+	loop = BYTES_PER_WORD(priv->bits_per_word);
+	if (priv->rx_bytes < loop)
+		loop = priv->rx_bytes;
+
+	priv->rx_bytes -= loop;
+
+	val = readl(priv->base + SSI_RXDR);
+
+	if (priv->rx_buf)
+		for (i = 0; i < loop; i++) {
+			val = val >> (BITS_PER_BYTE * i);
+			*(u8 *)priv->rx_buf = val & GENMASK(7, 0);
+			(u8 *)priv->rx_buf++;
+		}
+}
+
+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
+{
+	unsigned int tx_count;
+	int bytes_per_word = BYTES_PER_WORD(priv->bits_per_word);
+	u32 val;
+
+	tx_count = priv->tx_bytes / bytes_per_word;
+	if (tx_count > SSI_FIFO_DEPTH)
+		tx_count = SSI_FIFO_DEPTH;
+
+	/* set fifo threthold */
+	val = readl(priv->base + SSI_FC);
+	val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
+	val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
+	val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
+	writel(val, priv->base + SSI_FC);
+
+	while (tx_count--)
+		uniphier_spi_send(priv);
+}
+
+static void uniphier_spi_set_cs(struct spi_device *spi, bool enable)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_FPS);
+
+	if (enable)
+		val |= SSI_FPS_FSPOL;
+	else
+		val &= ~SSI_FPS_FSPOL;
+
+	writel(val, priv->base + SSI_FPS);
+}
+
+static int uniphier_spi_transfer_one(struct spi_master *master,
+				     struct spi_device *spi,
+				     struct spi_transfer *t)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+	int status;
+
+	status = uniphier_spi_setup_transfer(spi, t);
+	if (status < 0)
+		return status;
+
+	reinit_completion(&priv->xfer_done);
+
+	uniphier_spi_fill_tx_fifo(priv);
+
+	uniphier_spi_irq_enable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
+
+	status = wait_for_completion_timeout(&priv->xfer_done,
+					     msecs_to_jiffies(SSI_TIMEOUT));
+
+	uniphier_spi_irq_disable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
+
+	if (status < 0)
+		return status;
+
+	return priv->error;
+}
+
+static int uniphier_spi_prepare_transfer_hardware(struct spi_master *master)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+
+	writel(SSI_CTL_EN, priv->base + SSI_CTL);
+
+	return 0;
+}
+
+static int uniphier_spi_unprepare_transfer_hardware(struct spi_master *master)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+
+	writel(0, priv->base + SSI_CTL);
+
+	return 0;
+}
+
+static int uniphier_spi_setup(struct spi_device *spi)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val1, val2;
+
+	if (priv->mode == spi->mode)
+		return 0;
+
+	priv->mode = spi->mode;
+
+	/* clock setting
+	 * CKPHS    capture timing. 0:rising edge, 1:falling edge
+	 * CKINIT   clock initial level. 0:low, 1:high
+	 * CKDLY    clock delay. 0:no delay, 1:delay depending on FSTRT
+	 *          (FSTRT=0: 1 clock, FSTRT=1: 0.5 clock)
+	 *
+	 * frame setting
+	 * FSPOL    frame signal porarity. 0: low, 1: high
+	 * FSTRT    start frame timing
+	 *          0: rising edge of clock, 1: falling edge of clock
+	 */
+	switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
+	case SPI_MODE_0:
+		/* CKPHS=1, CKINIT=0, CKDLY=1, FSTRT=0 */
+		val1 = SSI_CKS_CKPHS | SSI_CKS_CKDLY;
+		val2 = 0;
+		break;
+	case SPI_MODE_1:
+		/* CKPHS=0, CKINIT=0, CKDLY=0, FSTRT=1 */
+		val1 = 0;
+		val2 = SSI_FPS_FSTRT;
+		break;
+	case SPI_MODE_2:
+		/* CKPHS=0, CKINIT=1, CKDLY=1, FSTRT=1 */
+		val1 = SSI_CKS_CKINIT | SSI_CKS_CKDLY;
+		val2 = SSI_FPS_FSTRT;
+		break;
+	case SPI_MODE_3:
+		/* CKPHS=1, CKINIT=1, CKDLY=0, FSTRT=0 */
+		val1 = SSI_CKS_CKPHS | SSI_CKS_CKINIT;
+		val2 = 0;
+		break;
+	}
+
+	if (!(spi->mode & SPI_CS_HIGH))
+		val2 |= SSI_FPS_FSPOL;
+
+	writel(val1, priv->base + SSI_CKS);
+	writel(val2, priv->base + SSI_FPS);
+
+	val1 = 0;
+	if (spi->mode & SPI_LSB_FIRST)
+		val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
+	writel(val1, priv->base + SSI_TXWDS);
+	writel(val1, priv->base + SSI_RXWDS);
+
+	return 0;
+}
+
+static irqreturn_t uniphier_spi_handler(int irq, void *dev_id)
+{
+	struct uniphier_spi_priv *priv = dev_id;
+	u32 val, stat;
+
+	stat = readl(priv->base + SSI_IS);
+	val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
+	writel(val, priv->base + SSI_IC);
+
+	/* rx fifo overrun */
+	if (stat & SSI_IS_RORID) {
+		priv->error = -EIO;
+		goto done;
+	}
+
+	/* rx complete */
+	if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
+		while ((readl(priv->base + SSI_SR) & SSI_SR_RNE) &&
+				(priv->rx_bytes - priv->tx_bytes) > 0)
+			uniphier_spi_recv(priv);
+
+		if ((readl(priv->base + SSI_SR) & SSI_SR_RNE) ||
+				(priv->rx_bytes != priv->tx_bytes)) {
+			priv->error = -EIO;
+			goto done;
+		} else if (priv->rx_bytes == 0)
+			goto done;
+
+		/* next tx transfer */
+		uniphier_spi_fill_tx_fifo(priv);
+	}
+
+	return IRQ_HANDLED;
+
+done:
+	complete(&priv->xfer_done);
+	return IRQ_HANDLED;
+}
+
+static int uniphier_spi_probe(struct platform_device *pdev)
+{
+	struct uniphier_spi_priv *priv;
+	struct spi_master *master;
+	struct resource *res;
+	unsigned long clksrc;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*priv));
+	if (!master)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, master);
+
+	priv = spi_master_get_devdata(master);
+	priv->master = master;
+	priv->bits_per_word = 0;
+	priv->mode = 0;
+	priv->speed_hz = 0;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base)) {
+		ret = PTR_ERR(priv->base);
+		goto out_master_put;
+	}
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err(&pdev->dev, "failed to get clock\n");
+		ret = PTR_ERR(priv->clk);
+		goto out_master_put;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		goto out_master_put;
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ\n");
+		ret = -ENXIO;
+		goto out_disable_clk;
+	}
+
+	ret = devm_request_irq(&pdev->dev, priv->irq, uniphier_spi_handler,
+			       0, "uniphier-spi", priv);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request IRQ\n");
+		goto out_disable_clk;
+	}
+
+	init_completion(&priv->xfer_done);
+
+	clksrc = clk_get_rate(priv->clk);
+
+	master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
+	master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
+	master->dev.of_node = pdev->dev.of_node;
+	master->bus_num = pdev->id;
+	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
+
+	master->setup = uniphier_spi_setup;
+	master->set_cs = uniphier_spi_set_cs;
+	master->transfer_one = uniphier_spi_transfer_one;
+	master->prepare_transfer_hardware
+				= uniphier_spi_prepare_transfer_hardware;
+	master->unprepare_transfer_hardware
+				= uniphier_spi_unprepare_transfer_hardware;
+	master->num_chipselect = 1;
+
+	ret = devm_spi_register_master(&pdev->dev, master);
+	if (ret)
+		goto out_disable_clk;
+
+	return ret;
+
+out_disable_clk:
+	clk_disable_unprepare(priv->clk);
+
+out_master_put:
+	spi_master_put(master);
+	return ret;
+}
+
+static int uniphier_spi_remove(struct platform_device *pdev)
+{
+	struct uniphier_spi_priv *priv = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static const struct of_device_id uniphier_spi_match[] = {
+	{ .compatible = "socionext,uniphier-scssi", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, uniphier_spi_match);
+
+static struct platform_driver uniphier_spi_driver = {
+	.probe = uniphier_spi_probe,
+	.remove = uniphier_spi_remove,
+	.driver = {
+		.name = "uniphier-spi",
+		.of_match_table = uniphier_spi_match,
+	},
+};
+module_platform_driver(uniphier_spi_driver);
+
+MODULE_AUTHOR("Kunihiko Hayashi <hayashi.kunihiko@socionext.com>");
+MODULE_AUTHOR("Keiji Hayashibara <hayashibara.keiji@socionext.com>");
+MODULE_DESCRIPTION("Socionext UniPhier SPI controller driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Keiji Hayashibara]

Add SPI controller driver implemented in Socionext UniPhier SoCs.

UniPhier SoCs have two types SPI controllers; SCSSI supports a
single channel, and MCSSI supports multiple channels.
Currently this driver supports SCSSI only.

This controller has 32bit TX/RX FIFO with depth of eight entry,
and supports the SPI master mode only.

This commit is implemented in PIO transfer mode, not DMA transfer.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Signed-off-by: Keiji Hayashibara <hayashibara.keiji@socionext.com>
---
 drivers/spi/Kconfig        |   7 +
 drivers/spi/Makefile       |   1 +
 drivers/spi/spi-uniphier.c | 532 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 540 insertions(+)
 create mode 100644 drivers/spi/spi-uniphier.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ad5d68e..a3cb0b4 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -688,6 +688,13 @@ config SPI_TXX9
 	help
 	  SPI driver for Toshiba TXx9 MIPS SoCs
 
+config SPI_UNIPHIER
+	tristate "Socionext UniPhier SPI Controller"
+	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
+	help
+	  This driver supports the SPI controller on Socionext
+	  UniPhier SoCs.
+
 config SPI_XCOMM
 	tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
 	depends on I2C
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index cb1f437..a90d559 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -101,6 +101,7 @@ spi-thunderx-objs			:= spi-cavium.o spi-cavium-thunderx.o
 obj-$(CONFIG_SPI_THUNDERX)		+= spi-thunderx.o
 obj-$(CONFIG_SPI_TOPCLIFF_PCH)		+= spi-topcliff-pch.o
 obj-$(CONFIG_SPI_TXX9)			+= spi-txx9.o
+obj-$(CONFIG_SPI_UNIPHIER)		+= spi-uniphier.o
 obj-$(CONFIG_SPI_XCOMM)		+= spi-xcomm.o
 obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
 obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
diff --git a/drivers/spi/spi-uniphier.c b/drivers/spi/spi-uniphier.c
new file mode 100644
index 0000000..92718ee
--- /dev/null
+++ b/drivers/spi/spi-uniphier.c
@@ -0,0 +1,532 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * spi-uniphier.c - Socionext UniPhier SPI controller driver
+ *
+ * Copyright 2012      Panasonic Corporation
+ * Copyright 2016-2018 Socionext Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+
+#define SSI_TIMEOUT		2000	/* ms */
+#define SSI_MAX_CLK_DIVIDER	254
+#define SSI_MIN_CLK_DIVIDER	4
+
+struct uniphier_spi_priv {
+	void __iomem *base;
+	int irq;
+	struct clk *clk;
+	struct spi_master *master;
+	struct completion xfer_done;
+
+	int error;
+	unsigned int tx_bytes;
+	unsigned int rx_bytes;
+	const u8 *tx_buf;
+	u8 *rx_buf;
+
+	u8 bits_per_word;
+	u16 mode;
+	u32 speed_hz;
+};
+
+#define SSI_CTL			0x0
+#define   SSI_CTL_EN		BIT(0)
+
+#define SSI_CKS			0x4
+#define   SSI_CKS_CKRAT_MASK	GENMASK(7, 0)
+#define   SSI_CKS_CKPHS		BIT(14)
+#define   SSI_CKS_CKINIT	BIT(13)
+#define   SSI_CKS_CKDLY		BIT(12)
+
+#define SSI_TXWDS		0x8
+#define   SSI_TXWDS_WDLEN_MASK	GENMASK(13, 8)
+#define   SSI_TXWDS_TDTF_MASK	GENMASK(7, 6)
+#define   SSI_TXWDS_DTLEN_MASK	GENMASK(5, 0)
+
+#define SSI_RXWDS		0xc
+#define   SSI_RXWDS_DTLEN_MASK	GENMASK(5, 0)
+
+#define SSI_FPS			0x10
+#define   SSI_FPS_FSPOL		BIT(15)
+#define   SSI_FPS_FSTRT		BIT(14)
+
+#define SSI_SR			0x14
+#define   SSI_SR_RNE		BIT(0)
+
+#define SSI_IE			0x18
+#define   SSI_IE_RCIE		BIT(3)
+#define   SSI_IE_RORIE		BIT(0)
+
+#define SSI_IS			0x1c
+#define   SSI_IS_RXRS		BIT(9)
+#define   SSI_IS_RCID		BIT(3)
+#define   SSI_IS_RORID		BIT(0)
+
+#define SSI_IC			0x1c
+#define   SSI_IC_TCIC		BIT(4)
+#define   SSI_IC_RCIC		BIT(3)
+#define   SSI_IC_RORIC		BIT(0)
+
+#define SSI_FC			0x20
+#define   SSI_FC_TXFFL		BIT(12)
+#define   SSI_FC_TXFTH_MASK	GENMASK(11, 8)
+#define   SSI_FC_RXFFL		BIT(4)
+#define   SSI_FC_RXFTH_MASK	GENMASK(3, 0)
+
+#define SSI_TXDR		0x24
+#define SSI_RXDR		0x24
+
+#define SSI_FIFO_DEPTH		8
+
+#define BYTES_PER_WORD(x)			\
+({						\
+	int __x;				\
+	__x = (x <= 8)  ? 1 :			\
+	      (x <= 16) ? 2 : 4;		\
+	__x;					\
+})
+
+static inline void uniphier_spi_irq_enable(struct spi_device *spi, u32 mask)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_IE);
+	val |= mask;
+	writel(val, priv->base + SSI_IE);
+}
+
+static inline void uniphier_spi_irq_disable(struct spi_device *spi, u32 mask)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_IE);
+	val &= ~mask;
+	writel(val, priv->base + SSI_IE);
+}
+
+static void uniphier_spi_set_transfer_size(struct spi_device *spi, int size)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_TXWDS);
+	val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
+	val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
+	val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
+	writel(val, priv->base + SSI_TXWDS);
+
+	val = readl(priv->base + SSI_RXWDS);
+	val &= ~SSI_RXWDS_DTLEN_MASK;
+	val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
+	writel(val, priv->base + SSI_RXWDS);
+}
+
+static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val, ckrat;
+
+	/*
+	 * the supported rates are even numbers from 4 to 254. (4,6,8...254)
+	 * round up as we look for equal or less speed
+	 */
+	ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
+	ckrat = roundup(ckrat, 2);
+
+	/* check if requested speed is too small */
+	if (ckrat > SSI_MAX_CLK_DIVIDER)
+		return -EINVAL;
+
+	if (ckrat < SSI_MIN_CLK_DIVIDER)
+		ckrat = SSI_MIN_CLK_DIVIDER;
+
+	val = readl(priv->base + SSI_CKS);
+	val &= ~SSI_CKS_CKRAT_MASK;
+	val |= ckrat & SSI_CKS_CKRAT_MASK;
+	writel(val, priv->base + SSI_CKS);
+
+	return 0;
+}
+
+static int uniphier_spi_setup_transfer(struct spi_device *spi,
+				       struct spi_transfer *t)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+	int ret;
+
+	priv->error = 0;
+	priv->tx_buf = t->tx_buf;
+	priv->rx_buf = t->rx_buf;
+	priv->tx_bytes = priv->rx_bytes = t->len;
+
+	if (priv->bits_per_word != t->bits_per_word) {
+		uniphier_spi_set_transfer_size(spi, t->bits_per_word);
+		priv->bits_per_word = t->bits_per_word;
+	}
+
+	if (priv->speed_hz != t->speed_hz) {
+		ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
+		if (ret)
+			return ret;
+		priv->speed_hz = t->speed_hz;
+	}
+
+	/* reset FIFOs */
+	val = SSI_FC_TXFFL | SSI_FC_RXFFL;
+	writel(val, priv->base + SSI_FC);
+
+	return 0;
+}
+
+static void uniphier_spi_send(struct uniphier_spi_priv *priv)
+{
+	int i, loop;
+	u32 val = 0;
+
+	loop = BYTES_PER_WORD(priv->bits_per_word);
+	if (priv->tx_bytes < loop)
+		loop = priv->tx_bytes;
+
+	priv->tx_bytes -= loop;
+
+	if (priv->tx_buf)
+		for (i = 0; i < loop; i++) {
+			val |= (*(const u8 *)priv->tx_buf)
+						<< (BITS_PER_BYTE * i);
+			(const u8 *)priv->tx_buf++;
+		}
+
+	writel(val, priv->base + SSI_TXDR);
+}
+
+static void uniphier_spi_recv(struct uniphier_spi_priv *priv)
+{
+	int i, loop;
+	u32 val;
+
+	loop = BYTES_PER_WORD(priv->bits_per_word);
+	if (priv->rx_bytes < loop)
+		loop = priv->rx_bytes;
+
+	priv->rx_bytes -= loop;
+
+	val = readl(priv->base + SSI_RXDR);
+
+	if (priv->rx_buf)
+		for (i = 0; i < loop; i++) {
+			val = val >> (BITS_PER_BYTE * i);
+			*(u8 *)priv->rx_buf = val & GENMASK(7, 0);
+			(u8 *)priv->rx_buf++;
+		}
+}
+
+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
+{
+	unsigned int tx_count;
+	int bytes_per_word = BYTES_PER_WORD(priv->bits_per_word);
+	u32 val;
+
+	tx_count = priv->tx_bytes / bytes_per_word;
+	if (tx_count > SSI_FIFO_DEPTH)
+		tx_count = SSI_FIFO_DEPTH;
+
+	/* set fifo threthold */
+	val = readl(priv->base + SSI_FC);
+	val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
+	val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
+	val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
+	writel(val, priv->base + SSI_FC);
+
+	while (tx_count--)
+		uniphier_spi_send(priv);
+}
+
+static void uniphier_spi_set_cs(struct spi_device *spi, bool enable)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	val = readl(priv->base + SSI_FPS);
+
+	if (enable)
+		val |= SSI_FPS_FSPOL;
+	else
+		val &= ~SSI_FPS_FSPOL;
+
+	writel(val, priv->base + SSI_FPS);
+}
+
+static int uniphier_spi_transfer_one(struct spi_master *master,
+				     struct spi_device *spi,
+				     struct spi_transfer *t)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+	int status;
+
+	status = uniphier_spi_setup_transfer(spi, t);
+	if (status < 0)
+		return status;
+
+	reinit_completion(&priv->xfer_done);
+
+	uniphier_spi_fill_tx_fifo(priv);
+
+	uniphier_spi_irq_enable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
+
+	status = wait_for_completion_timeout(&priv->xfer_done,
+					     msecs_to_jiffies(SSI_TIMEOUT));
+
+	uniphier_spi_irq_disable(spi, SSI_IE_RCIE | SSI_IE_RORIE);
+
+	if (status < 0)
+		return status;
+
+	return priv->error;
+}
+
+static int uniphier_spi_prepare_transfer_hardware(struct spi_master *master)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+
+	writel(SSI_CTL_EN, priv->base + SSI_CTL);
+
+	return 0;
+}
+
+static int uniphier_spi_unprepare_transfer_hardware(struct spi_master *master)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(master);
+
+	writel(0, priv->base + SSI_CTL);
+
+	return 0;
+}
+
+static int uniphier_spi_setup(struct spi_device *spi)
+{
+	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
+	u32 val1, val2;
+
+	if (priv->mode == spi->mode)
+		return 0;
+
+	priv->mode = spi->mode;
+
+	/* clock setting
+	 * CKPHS    capture timing. 0:rising edge, 1:falling edge
+	 * CKINIT   clock initial level. 0:low, 1:high
+	 * CKDLY    clock delay. 0:no delay, 1:delay depending on FSTRT
+	 *          (FSTRT=0: 1 clock, FSTRT=1: 0.5 clock)
+	 *
+	 * frame setting
+	 * FSPOL    frame signal porarity. 0: low, 1: high
+	 * FSTRT    start frame timing
+	 *          0: rising edge of clock, 1: falling edge of clock
+	 */
+	switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
+	case SPI_MODE_0:
+		/* CKPHS=1, CKINIT=0, CKDLY=1, FSTRT=0 */
+		val1 = SSI_CKS_CKPHS | SSI_CKS_CKDLY;
+		val2 = 0;
+		break;
+	case SPI_MODE_1:
+		/* CKPHS=0, CKINIT=0, CKDLY=0, FSTRT=1 */
+		val1 = 0;
+		val2 = SSI_FPS_FSTRT;
+		break;
+	case SPI_MODE_2:
+		/* CKPHS=0, CKINIT=1, CKDLY=1, FSTRT=1 */
+		val1 = SSI_CKS_CKINIT | SSI_CKS_CKDLY;
+		val2 = SSI_FPS_FSTRT;
+		break;
+	case SPI_MODE_3:
+		/* CKPHS=1, CKINIT=1, CKDLY=0, FSTRT=0 */
+		val1 = SSI_CKS_CKPHS | SSI_CKS_CKINIT;
+		val2 = 0;
+		break;
+	}
+
+	if (!(spi->mode & SPI_CS_HIGH))
+		val2 |= SSI_FPS_FSPOL;
+
+	writel(val1, priv->base + SSI_CKS);
+	writel(val2, priv->base + SSI_FPS);
+
+	val1 = 0;
+	if (spi->mode & SPI_LSB_FIRST)
+		val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
+	writel(val1, priv->base + SSI_TXWDS);
+	writel(val1, priv->base + SSI_RXWDS);
+
+	return 0;
+}
+
+static irqreturn_t uniphier_spi_handler(int irq, void *dev_id)
+{
+	struct uniphier_spi_priv *priv = dev_id;
+	u32 val, stat;
+
+	stat = readl(priv->base + SSI_IS);
+	val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
+	writel(val, priv->base + SSI_IC);
+
+	/* rx fifo overrun */
+	if (stat & SSI_IS_RORID) {
+		priv->error = -EIO;
+		goto done;
+	}
+
+	/* rx complete */
+	if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
+		while ((readl(priv->base + SSI_SR) & SSI_SR_RNE) &&
+				(priv->rx_bytes - priv->tx_bytes) > 0)
+			uniphier_spi_recv(priv);
+
+		if ((readl(priv->base + SSI_SR) & SSI_SR_RNE) ||
+				(priv->rx_bytes != priv->tx_bytes)) {
+			priv->error = -EIO;
+			goto done;
+		} else if (priv->rx_bytes == 0)
+			goto done;
+
+		/* next tx transfer */
+		uniphier_spi_fill_tx_fifo(priv);
+	}
+
+	return IRQ_HANDLED;
+
+done:
+	complete(&priv->xfer_done);
+	return IRQ_HANDLED;
+}
+
+static int uniphier_spi_probe(struct platform_device *pdev)
+{
+	struct uniphier_spi_priv *priv;
+	struct spi_master *master;
+	struct resource *res;
+	unsigned long clksrc;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*priv));
+	if (!master)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, master);
+
+	priv = spi_master_get_devdata(master);
+	priv->master = master;
+	priv->bits_per_word = 0;
+	priv->mode = 0;
+	priv->speed_hz = 0;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base)) {
+		ret = PTR_ERR(priv->base);
+		goto out_master_put;
+	}
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err(&pdev->dev, "failed to get clock\n");
+		ret = PTR_ERR(priv->clk);
+		goto out_master_put;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		goto out_master_put;
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ\n");
+		ret = -ENXIO;
+		goto out_disable_clk;
+	}
+
+	ret = devm_request_irq(&pdev->dev, priv->irq, uniphier_spi_handler,
+			       0, "uniphier-spi", priv);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request IRQ\n");
+		goto out_disable_clk;
+	}
+
+	init_completion(&priv->xfer_done);
+
+	clksrc = clk_get_rate(priv->clk);
+
+	master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
+	master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
+	master->dev.of_node = pdev->dev.of_node;
+	master->bus_num = pdev->id;
+	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
+
+	master->setup = uniphier_spi_setup;
+	master->set_cs = uniphier_spi_set_cs;
+	master->transfer_one = uniphier_spi_transfer_one;
+	master->prepare_transfer_hardware
+				= uniphier_spi_prepare_transfer_hardware;
+	master->unprepare_transfer_hardware
+				= uniphier_spi_unprepare_transfer_hardware;
+	master->num_chipselect = 1;
+
+	ret = devm_spi_register_master(&pdev->dev, master);
+	if (ret)
+		goto out_disable_clk;
+
+	return ret;
+
+out_disable_clk:
+	clk_disable_unprepare(priv->clk);
+
+out_master_put:
+	spi_master_put(master);
+	return ret;
+}
+
+static int uniphier_spi_remove(struct platform_device *pdev)
+{
+	struct uniphier_spi_priv *priv = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static const struct of_device_id uniphier_spi_match[] = {
+	{ .compatible = "socionext,uniphier-scssi", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, uniphier_spi_match);
+
+static struct platform_driver uniphier_spi_driver = {
+	.probe = uniphier_spi_probe,
+	.remove = uniphier_spi_remove,
+	.driver = {
+		.name = "uniphier-spi",
+		.of_match_table = uniphier_spi_match,
+	},
+};
+module_platform_driver(uniphier_spi_driver);
+
+MODULE_AUTHOR("Kunihiko Hayashi <hayashi.kunihiko@socionext.com>");
+MODULE_AUTHOR("Keiji Hayashibara <hayashibara.keiji@socionext.com>");
+MODULE_DESCRIPTION("Socionext UniPhier SPI controller driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Mark Brown]

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

On Thu, Jul 19, 2018 at 03:51:57PM +0900, Keiji Hayashibara wrote:

This all looks good, just a small number of fairly minor things - mostly
style points.

> @@ -0,0 +1,532 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * spi-uniphier.c - Socionext UniPhier SPI controller driver
> + *
> + * Copyright 2012      Panasonic Corporation
> + * Copyright 2016-2018 Socionext Inc.
> + */

Please make the entire comment a C++ one, it makes things look a bit
more joined up/intentional.

> +#define BYTES_PER_WORD(x)			\
> +({						\
> +	int __x;				\
> +	__x = (x <= 8)  ? 1 :			\
> +	      (x <= 16) ? 2 : 4;		\
> +	__x;					\
> +})

Could this be replaced with an inline function?  The usage seems fine
but it's a bit big for a macro.  The end result should be similar.

> +static irqreturn_t uniphier_spi_handler(int irq, void *dev_id)
> +{
> +	struct uniphier_spi_priv *priv = dev_id;
> +	u32 val, stat;
> +
> +	stat = readl(priv->base + SSI_IS);
> +	val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
> +	writel(val, priv->base + SSI_IC);
> +
> +	/* rx fifo overrun */
> +	if (stat & SSI_IS_RORID) {
> +		priv->error = -EIO;
> +		goto done;
> +	}
> +
> +	/* rx complete */
> +	if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {

> +	}
> +
> +	return IRQ_HANDLED;
> +
> +done:
> +	complete(&priv->xfer_done);
> +	return IRQ_HANDLED;

This will unconditionally report IRQ_HANDLED even if none of the flags
were set by the hardware which will cause problems if something goes
wrong - the interrupt will continually be serviced and the interrupt
framework won't be able to mitigate or provide diagnostics.  It's better
to return IRQ_NONE if nothing is detected from the hardware.

> +static const struct of_device_id uniphier_spi_match[] = {
> +	{ .compatible = "socionext,uniphier-scssi", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);

The binding document also listed socionext,uniphier-mcssi as a
compatible but this driver doesn't match that.

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

[PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Mark Brown]

On Thu, Jul 19, 2018 at 03:51:57PM +0900, Keiji Hayashibara wrote:

This all looks good, just a small number of fairly minor things - mostly
style points.

> @@ -0,0 +1,532 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * spi-uniphier.c - Socionext UniPhier SPI controller driver
> + *
> + * Copyright 2012      Panasonic Corporation
> + * Copyright 2016-2018 Socionext Inc.
> + */

Please make the entire comment a C++ one, it makes things look a bit
more joined up/intentional.

> +#define BYTES_PER_WORD(x)			\
> +({						\
> +	int __x;				\
> +	__x = (x <= 8)  ? 1 :			\
> +	      (x <= 16) ? 2 : 4;		\
> +	__x;					\
> +})

Could this be replaced with an inline function?  The usage seems fine
but it's a bit big for a macro.  The end result should be similar.

> +static irqreturn_t uniphier_spi_handler(int irq, void *dev_id)
> +{
> +	struct uniphier_spi_priv *priv = dev_id;
> +	u32 val, stat;
> +
> +	stat = readl(priv->base + SSI_IS);
> +	val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
> +	writel(val, priv->base + SSI_IC);
> +
> +	/* rx fifo overrun */
> +	if (stat & SSI_IS_RORID) {
> +		priv->error = -EIO;
> +		goto done;
> +	}
> +
> +	/* rx complete */
> +	if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {

> +	}
> +
> +	return IRQ_HANDLED;
> +
> +done:
> +	complete(&priv->xfer_done);
> +	return IRQ_HANDLED;

This will unconditionally report IRQ_HANDLED even if none of the flags
were set by the hardware which will cause problems if something goes
wrong - the interrupt will continually be serviced and the interrupt
framework won't be able to mitigate or provide diagnostics.  It's better
to return IRQ_NONE if nothing is detected from the hardware.

> +static const struct of_device_id uniphier_spi_match[] = {
> +	{ .compatible = "socionext,uniphier-scssi", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);

The binding document also listed socionext,uniphier-mcssi as a
compatible but this driver doesn't match that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180719/6db264ca/attachment.sig>

Re: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Trent Piepho]

On Thu, 2018-07-19 at 15:51 +0900, Keiji Hayashibara wrote:
> 
> +config SPI_UNIPHIER
> +	tristate "Socionext UniPhier SPI Controller"
> +	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> +	help
> +	  This driver supports the SPI controller on Socionext
> +	  UniPhier SoCs.

Perhaps add the bit that this is for the SCSSI and not MCSSI here?

> 
> +
> +#define BYTES_PER_WORD(x)			\
> +({						\
> +	int __x;				\
> +	__x = (x <= 8)  ? 1 :			\
> +	      (x <= 16) ? 2 : 4;		\
> +	__x;					\
> +})

Or:

static inline bytes_per_word(unsigned int bits) {
   return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4);
}



> +
> +static inline void uniphier_spi_irq_enable(struct spi_device *spi, u32 mask)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl(priv->base + SSI_IE);
> +	val |= mask;
> +	writel(val, priv->base + SSI_IE);
> +}
> +
> +static inline void uniphier_spi_irq_disable(struct spi_device *spi, u32 mask)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl(priv->base + SSI_IE);
> +	val &= ~mask;
> +	writel(val, priv->base + SSI_IE);
> +}
> +
> +static void uniphier_spi_set_transfer_size(struct spi_device *spi, int size)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl(priv->base + SSI_TXWDS);
> +	val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> +	val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> +	val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> +	writel(val, priv->base + SSI_TXWDS);
> +
> +	val = readl(priv->base + SSI_RXWDS);
> +	val &= ~SSI_RXWDS_DTLEN_MASK;
> +	val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> +	writel(val, priv->base + SSI_RXWDS);
> +}
> +
> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val, ckrat;
> +
> +	/*
> +	 * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> +	 * round up as we look for equal or less speed
> +	 */
> +	ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> +	ckrat = roundup(ckrat, 2);
> +
> +	/* check if requested speed is too small */
> +	if (ckrat > SSI_MAX_CLK_DIVIDER)
> +		return -EINVAL;
> +
> +	if (ckrat < SSI_MIN_CLK_DIVIDER)
> +		ckrat = SSI_MIN_CLK_DIVIDER;
> +
> +	val = readl(priv->base + SSI_CKS);
> +	val &= ~SSI_CKS_CKRAT_MASK;
> +	val |= ckrat & SSI_CKS_CKRAT_MASK;
> +	writel(val, priv->base + SSI_CKS);
> +
> +	return 0;
> +}
> +
> +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> +				       struct spi_transfer *t)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +	int ret;
> +
> +	priv->error = 0;
> +	priv->tx_buf = t->tx_buf;
> +	priv->rx_buf = t->rx_buf;
> +	priv->tx_bytes = priv->rx_bytes = t->len;
> +
> +	if (priv->bits_per_word != t->bits_per_word) {
> +		uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> +		priv->bits_per_word = t->bits_per_word;
> +	}
> +
> +	if (priv->speed_hz != t->speed_hz) {
> +		ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> +		if (ret)
> +			return ret;
> +		priv->speed_hz = t->speed_hz;
> +	}
> +
> +	/* reset FIFOs */
> +	val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> +	writel(val, priv->base + SSI_FC);
> +
> +	return 0;
> +}
> +
> +static void uniphier_spi_send(struct uniphier_spi_priv *priv)
> +{
> +	int i, loop;
> +	u32 val = 0;
> +
> +	loop = BYTES_PER_WORD(priv->bits_per_word);
> +	if (priv->tx_bytes < loop)
> +		loop = priv->tx_bytes;
> +
> +	priv->tx_bytes -= loop;
> +
> +	if (priv->tx_buf)
> +		for (i = 0; i < loop; i++) {
> +			val |= (*(const u8 *)priv->tx_buf)
> +						<< (BITS_PER_BYTE * i);

priv->tx_buf is already a const u8*, no need to cast it.  Also in recv,
no need to cast the pointer.  It'll just hide errors if someone changes
the type of the field.

> +			(const u8 *)priv->tx_buf++;
> +		}
> +
> +	writel(val, priv->base + SSI_TXDR);
> +}

The loop to read the data will likely be somewhat slow.  It might be
faster to use:

    val = get_unaligned_le32(priv->tx_buf);

To support different sizes a switch can be used:

    switch (MIN(BYTES_PER_WORD(priv->bits_per_word), priv->tx_bytes)) {
    case 1:
         val = *priv->tx_buf; break;
    case 2:
         val = get_unaligned_le16(priv->tx_buf); break;
    case 4:
         val = get_unaligned_le32(priv->tx_buf); break;
    }

However, I don't think either the existing code or this code is
correctly handling word sizes that are not an even number of bytes.  I
think it needs to left shift the data, but of course it also depends on
what the uniphier hardware expected in the TXDR register.


> +static void uniphier_spi_recv(struct uniphier_spi_priv *priv)
> +{
> +	int i, loop;
> +	u32 val;
> +
> +	loop = BYTES_PER_WORD(priv->bits_per_word);
> +	if (priv->rx_bytes < loop)
> +		loop = priv->rx_bytes;
> +
> +	priv->rx_bytes -= loop;
> +
> +	val = readl(priv->base + SSI_RXDR);
> +
> +	if (priv->rx_buf)
> +		for (i = 0; i < loop; i++) {
> +			val = val >> (BITS_PER_BYTE * i);
> +			*(u8 *)priv->rx_buf = val & GENMASK(7, 0);
> +			(u8 *)priv->rx_buf++;
> +		}



> +}+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
> +{
> +	unsigned int tx_count;
> +	int bytes_per_word = BYTES_PER_WORD(priv->bits_per_word);
> +	u32 val;
> +
> +	tx_count = priv->tx_bytes / bytes_per_word;
> +	if (tx_count > SSI_FIFO_DEPTH)
> +		tx_count = SSI_FIFO_DEPTH;
> +
> +	/* set fifo threthold */
> +	val = readl(priv->base + SSI_FC);
> +	val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> +	val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> +	val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> +	writel(val, priv->base + SSI_FC);
> +
> +	while (tx_count--)
> +		uniphier_spi_send(priv);
> +}

If you have 24 bits per word, 3 words, that's 9 bytes. 
BYTES_PER_WORD(24) is 4.  tx_count = 9/4 = 2.  Looks like your tx_count
rounds incorrectly, as it will only send 8 of the 9 bytes.

> +static int uniphier_spi_setup(struct spi_device *spi)
> +{

> +
> +	writel(val1, priv->base + SSI_CKS);
> +	writel(val2, priv->base + SSI_FPS);
> +
> +	val1 = 0;
> +	if (spi->mode & SPI_LSB_FIRST)
> +		val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> +	writel(val1, priv->base + SSI_TXWDS);
> +	writel(val1, priv->base + SSI_RXWDS);

Did you see this in the spi docs?

        Unless each SPI slave has its own configuration registers, don't
        change them right away ... otherwise drivers could corrupt I/O
        that's in progress for other SPI devices.

                ** BUG ALERT:  for some reason the first version of
                ** many spi_master drivers seems to get this wrong.
                ** When you code setup(), ASSUME that the controller
                ** is actively processing transfers for another device.

You have one chipselect, so maybe this is ok.  Until you want to
support more than one chipselect.

With gpio lines as chip selects, there's really no reason any spi
master can't support multiple slaves.

Re: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Trent Piepho]

On Thu, 2018-07-19 at 15:51 +0900, Keiji Hayashibara wrote:
> 
> +config SPI_UNIPHIER
> +	tristate "Socionext UniPhier SPI Controller"
> +	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> +	help
> +	  This driver supports the SPI controller on Socionext
> +	  UniPhier SoCs.

Perhaps add the bit that this is for the SCSSI and not MCSSI here?

> 
> +
> +#define BYTES_PER_WORD(x)			\
> +({						\
> +	int __x;				\
> +	__x = (x <= 8)  ? 1 :			\
> +	      (x <= 16) ? 2 : 4;		\
> +	__x;					\
> +})

Or:

static inline bytes_per_word(unsigned int bits) {
   return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4);
}



> +
> +static inline void uniphier_spi_irq_enable(struct spi_device *spi, u32 mask)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl(priv->base + SSI_IE);
> +	val |= mask;
> +	writel(val, priv->base + SSI_IE);
> +}
> +
> +static inline void uniphier_spi_irq_disable(struct spi_device *spi, u32 mask)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl(priv->base + SSI_IE);
> +	val &= ~mask;
> +	writel(val, priv->base + SSI_IE);
> +}
> +
> +static void uniphier_spi_set_transfer_size(struct spi_device *spi, int size)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl(priv->base + SSI_TXWDS);
> +	val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> +	val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> +	val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> +	writel(val, priv->base + SSI_TXWDS);
> +
> +	val = readl(priv->base + SSI_RXWDS);
> +	val &= ~SSI_RXWDS_DTLEN_MASK;
> +	val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> +	writel(val, priv->base + SSI_RXWDS);
> +}
> +
> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val, ckrat;
> +
> +	/*
> +	 * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> +	 * round up as we look for equal or less speed
> +	 */
> +	ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> +	ckrat = roundup(ckrat, 2);
> +
> +	/* check if requested speed is too small */
> +	if (ckrat > SSI_MAX_CLK_DIVIDER)
> +		return -EINVAL;
> +
> +	if (ckrat < SSI_MIN_CLK_DIVIDER)
> +		ckrat = SSI_MIN_CLK_DIVIDER;
> +
> +	val = readl(priv->base + SSI_CKS);
> +	val &= ~SSI_CKS_CKRAT_MASK;
> +	val |= ckrat & SSI_CKS_CKRAT_MASK;
> +	writel(val, priv->base + SSI_CKS);
> +
> +	return 0;
> +}
> +
> +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> +				       struct spi_transfer *t)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +	int ret;
> +
> +	priv->error = 0;
> +	priv->tx_buf = t->tx_buf;
> +	priv->rx_buf = t->rx_buf;
> +	priv->tx_bytes = priv->rx_bytes = t->len;
> +
> +	if (priv->bits_per_word != t->bits_per_word) {
> +		uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> +		priv->bits_per_word = t->bits_per_word;
> +	}
> +
> +	if (priv->speed_hz != t->speed_hz) {
> +		ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> +		if (ret)
> +			return ret;
> +		priv->speed_hz = t->speed_hz;
> +	}
> +
> +	/* reset FIFOs */
> +	val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> +	writel(val, priv->base + SSI_FC);
> +
> +	return 0;
> +}
> +
> +static void uniphier_spi_send(struct uniphier_spi_priv *priv)
> +{
> +	int i, loop;
> +	u32 val = 0;
> +
> +	loop = BYTES_PER_WORD(priv->bits_per_word);
> +	if (priv->tx_bytes < loop)
> +		loop = priv->tx_bytes;
> +
> +	priv->tx_bytes -= loop;
> +
> +	if (priv->tx_buf)
> +		for (i = 0; i < loop; i++) {
> +			val |= (*(const u8 *)priv->tx_buf)
> +						<< (BITS_PER_BYTE * i);

priv->tx_buf is already a const u8*, no need to cast it.  Also in recv,
no need to cast the pointer.  It'll just hide errors if someone changes
the type of the field.

> +			(const u8 *)priv->tx_buf++;
> +		}
> +
> +	writel(val, priv->base + SSI_TXDR);
> +}

The loop to read the data will likely be somewhat slow.  It might be
faster to use:

    val = get_unaligned_le32(priv->tx_buf);

To support different sizes a switch can be used:

    switch (MIN(BYTES_PER_WORD(priv->bits_per_word), priv->tx_bytes)) {
    case 1:
         val = *priv->tx_buf; break;
    case 2:
         val = get_unaligned_le16(priv->tx_buf); break;
    case 4:
         val = get_unaligned_le32(priv->tx_buf); break;
    }

However, I don't think either the existing code or this code is
correctly handling word sizes that are not an even number of bytes.  I
think it needs to left shift the data, but of course it also depends on
what the uniphier hardware expected in the TXDR register.


> +static void uniphier_spi_recv(struct uniphier_spi_priv *priv)
> +{
> +	int i, loop;
> +	u32 val;
> +
> +	loop = BYTES_PER_WORD(priv->bits_per_word);
> +	if (priv->rx_bytes < loop)
> +		loop = priv->rx_bytes;
> +
> +	priv->rx_bytes -= loop;
> +
> +	val = readl(priv->base + SSI_RXDR);
> +
> +	if (priv->rx_buf)
> +		for (i = 0; i < loop; i++) {
> +			val = val >> (BITS_PER_BYTE * i);
> +			*(u8 *)priv->rx_buf = val & GENMASK(7, 0);
> +			(u8 *)priv->rx_buf++;
> +		}



> +}+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
> +{
> +	unsigned int tx_count;
> +	int bytes_per_word = BYTES_PER_WORD(priv->bits_per_word);
> +	u32 val;
> +
> +	tx_count = priv->tx_bytes / bytes_per_word;
> +	if (tx_count > SSI_FIFO_DEPTH)
> +		tx_count = SSI_FIFO_DEPTH;
> +
> +	/* set fifo threthold */
> +	val = readl(priv->base + SSI_FC);
> +	val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> +	val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> +	val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> +	writel(val, priv->base + SSI_FC);
> +
> +	while (tx_count--)
> +		uniphier_spi_send(priv);
> +}

If you have 24 bits per word, 3 words, that's 9 bytes. 
BYTES_PER_WORD(24) is 4.  tx_count = 9/4 = 2.  Looks like your tx_count
rounds incorrectly, as it will only send 8 of the 9 bytes.

> +static int uniphier_spi_setup(struct spi_device *spi)
> +{

> +
> +	writel(val1, priv->base + SSI_CKS);
> +	writel(val2, priv->base + SSI_FPS);
> +
> +	val1 = 0;
> +	if (spi->mode & SPI_LSB_FIRST)
> +		val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> +	writel(val1, priv->base + SSI_TXWDS);
> +	writel(val1, priv->base + SSI_RXWDS);

Did you see this in the spi docs?

        Unless each SPI slave has its own configuration registers, don't
        change them right away ... otherwise drivers could corrupt I/O
        that's in progress for other SPI devices.

                ** BUG ALERT:  for some reason the first version of
                ** many spi_master drivers seems to get this wrong.
                ** When you code setup(), ASSUME that the controller
                ** is actively processing transfers for another device.

You have one chipselect, so maybe this is ok.  Until you want to
support more than one chipselect.

With gpio lines as chip selects, there's really no reason any spi
master can't support multiple slaves.

[PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Trent Piepho]

On Thu, 2018-07-19 at 15:51 +0900, Keiji Hayashibara wrote:
> 
> +config SPI_UNIPHIER
> +	tristate "Socionext UniPhier SPI Controller"
> +	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> +	help
> +	  This driver supports the SPI controller on Socionext
> +	  UniPhier SoCs.

Perhaps add the bit that this is for the SCSSI and not MCSSI here?

> 
> +
> +#define BYTES_PER_WORD(x)			\
> +({						\
> +	int __x;				\
> +	__x = (x <= 8)  ? 1 :			\
> +	      (x <= 16) ? 2 : 4;		\
> +	__x;					\
> +})

Or:

static inline bytes_per_word(unsigned int bits) {
   return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4);
}



> +
> +static inline void uniphier_spi_irq_enable(struct spi_device *spi, u32 mask)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl(priv->base + SSI_IE);
> +	val |= mask;
> +	writel(val, priv->base + SSI_IE);
> +}
> +
> +static inline void uniphier_spi_irq_disable(struct spi_device *spi, u32 mask)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl(priv->base + SSI_IE);
> +	val &= ~mask;
> +	writel(val, priv->base + SSI_IE);
> +}
> +
> +static void uniphier_spi_set_transfer_size(struct spi_device *spi, int size)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl(priv->base + SSI_TXWDS);
> +	val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> +	val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> +	val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> +	writel(val, priv->base + SSI_TXWDS);
> +
> +	val = readl(priv->base + SSI_RXWDS);
> +	val &= ~SSI_RXWDS_DTLEN_MASK;
> +	val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> +	writel(val, priv->base + SSI_RXWDS);
> +}
> +
> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned int speed)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val, ckrat;
> +
> +	/*
> +	 * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> +	 * round up as we look for equal or less speed
> +	 */
> +	ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> +	ckrat = roundup(ckrat, 2);
> +
> +	/* check if requested speed is too small */
> +	if (ckrat > SSI_MAX_CLK_DIVIDER)
> +		return -EINVAL;
> +
> +	if (ckrat < SSI_MIN_CLK_DIVIDER)
> +		ckrat = SSI_MIN_CLK_DIVIDER;
> +
> +	val = readl(priv->base + SSI_CKS);
> +	val &= ~SSI_CKS_CKRAT_MASK;
> +	val |= ckrat & SSI_CKS_CKRAT_MASK;
> +	writel(val, priv->base + SSI_CKS);
> +
> +	return 0;
> +}
> +
> +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> +				       struct spi_transfer *t)
> +{
> +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> +	u32 val;
> +	int ret;
> +
> +	priv->error = 0;
> +	priv->tx_buf = t->tx_buf;
> +	priv->rx_buf = t->rx_buf;
> +	priv->tx_bytes = priv->rx_bytes = t->len;
> +
> +	if (priv->bits_per_word != t->bits_per_word) {
> +		uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> +		priv->bits_per_word = t->bits_per_word;
> +	}
> +
> +	if (priv->speed_hz != t->speed_hz) {
> +		ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> +		if (ret)
> +			return ret;
> +		priv->speed_hz = t->speed_hz;
> +	}
> +
> +	/* reset FIFOs */
> +	val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> +	writel(val, priv->base + SSI_FC);
> +
> +	return 0;
> +}
> +
> +static void uniphier_spi_send(struct uniphier_spi_priv *priv)
> +{
> +	int i, loop;
> +	u32 val = 0;
> +
> +	loop = BYTES_PER_WORD(priv->bits_per_word);
> +	if (priv->tx_bytes < loop)
> +		loop = priv->tx_bytes;
> +
> +	priv->tx_bytes -= loop;
> +
> +	if (priv->tx_buf)
> +		for (i = 0; i < loop; i++) {
> +			val |= (*(const u8 *)priv->tx_buf)
> +						<< (BITS_PER_BYTE * i);

priv->tx_buf is already a const u8*, no need to cast it.  Also in recv,
no need to cast the pointer.  It'll just hide errors if someone changes
the type of the field.

> +			(const u8 *)priv->tx_buf++;
> +		}
> +
> +	writel(val, priv->base + SSI_TXDR);
> +}

The loop to read the data will likely be somewhat slow.  It might be
faster to use:

    val = get_unaligned_le32(priv->tx_buf);

To support different sizes a switch can be used:

    switch (MIN(BYTES_PER_WORD(priv->bits_per_word), priv->tx_bytes)) {
    case 1:
         val = *priv->tx_buf; break;
    case 2:
         val = get_unaligned_le16(priv->tx_buf); break;
    case 4:
         val = get_unaligned_le32(priv->tx_buf); break;
    }

However, I don't think either the existing code or this code is
correctly handling word sizes that are not an even number of bytes.  I
think it needs to left shift the data, but of course it also depends on
what the uniphier hardware expected in the TXDR register.


> +static void uniphier_spi_recv(struct uniphier_spi_priv *priv)
> +{
> +	int i, loop;
> +	u32 val;
> +
> +	loop = BYTES_PER_WORD(priv->bits_per_word);
> +	if (priv->rx_bytes < loop)
> +		loop = priv->rx_bytes;
> +
> +	priv->rx_bytes -= loop;
> +
> +	val = readl(priv->base + SSI_RXDR);
> +
> +	if (priv->rx_buf)
> +		for (i = 0; i < loop; i++) {
> +			val = val >> (BITS_PER_BYTE * i);
> +			*(u8 *)priv->rx_buf = val & GENMASK(7, 0);
> +			(u8 *)priv->rx_buf++;
> +		}



> +}+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv *priv)
> +{
> +	unsigned int tx_count;
> +	int bytes_per_word = BYTES_PER_WORD(priv->bits_per_word);
> +	u32 val;
> +
> +	tx_count = priv->tx_bytes / bytes_per_word;
> +	if (tx_count > SSI_FIFO_DEPTH)
> +		tx_count = SSI_FIFO_DEPTH;
> +
> +	/* set fifo threthold */
> +	val = readl(priv->base + SSI_FC);
> +	val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> +	val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> +	val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> +	writel(val, priv->base + SSI_FC);
> +
> +	while (tx_count--)
> +		uniphier_spi_send(priv);
> +}

If you have 24 bits per word, 3 words, that's 9 bytes. 
BYTES_PER_WORD(24) is 4.  tx_count = 9/4 = 2.  Looks like your tx_count
rounds incorrectly, as it will only send 8 of the 9 bytes.

> +static int uniphier_spi_setup(struct spi_device *spi)
> +{

> +
> +	writel(val1, priv->base + SSI_CKS);
> +	writel(val2, priv->base + SSI_FPS);
> +
> +	val1 = 0;
> +	if (spi->mode & SPI_LSB_FIRST)
> +		val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> +	writel(val1, priv->base + SSI_TXWDS);
> +	writel(val1, priv->base + SSI_RXWDS);

Did you see this in the spi docs?

        Unless each SPI slave has its own configuration registers, don't
        change them right away ... otherwise drivers could corrupt I/O
        that's in progress for other SPI devices.

                ** BUG ALERT:  for some reason the first version of
                ** many spi_master drivers seems to get this wrong.
                ** When you code setup(), ASSUME that the controller
                ** is actively processing transfers for another device.

You have one chipselect, so maybe this is ok.  Until you want to
support more than one chipselect.

With gpio lines as chip selects, there's really no reason any spi
master can't support multiple slaves.

RE: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Keiji Hayashibara]

Hi Mark,

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Friday, July 20, 2018 1:52 AM
> Subject: Re: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> On Thu, Jul 19, 2018 at 03:51:57PM +0900, Keiji Hayashibara wrote:
> 
> This all looks good, just a small number of fairly minor things - mostly style points.
> 
> > @@ -0,0 +1,532 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * spi-uniphier.c - Socionext UniPhier SPI controller driver
> > + *
> > + * Copyright 2012      Panasonic Corporation
> > + * Copyright 2016-2018 Socionext Inc.
> > + */
> 
> Please make the entire comment a C++ one, it makes things look a bit more joined up/intentional.

OK. I will modify to C++ style.


> > +#define BYTES_PER_WORD(x)			\
> > +({						\
> > +	int __x;				\
> > +	__x = (x <= 8)  ? 1 :			\
> > +	      (x <= 16) ? 2 : 4;		\
> > +	__x;					\
> > +})
> 
> Could this be replaced with an inline function?  The usage seems fine but it's a bit big for a macro.  The end
> result should be similar.

OK. I will replace with an inline function.

> 
> > +static irqreturn_t uniphier_spi_handler(int irq, void *dev_id) {
> > +	struct uniphier_spi_priv *priv = dev_id;
> > +	u32 val, stat;
> > +
> > +	stat = readl(priv->base + SSI_IS);
> > +	val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
> > +	writel(val, priv->base + SSI_IC);
> > +
> > +	/* rx fifo overrun */
> > +	if (stat & SSI_IS_RORID) {
> > +		priv->error = -EIO;
> > +		goto done;
> > +	}
> > +
> > +	/* rx complete */
> > +	if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
> 
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +
> > +done:
> > +	complete(&priv->xfer_done);
> > +	return IRQ_HANDLED;
> 
> This will unconditionally report IRQ_HANDLED even if none of the flags were set by the hardware which will cause
> problems if something goes wrong - the interrupt will continually be serviced and the interrupt framework won't
> be able to mitigate or provide diagnostics.  It's better to return IRQ_NONE if nothing is detected from the hardware.

I agree. I will modify it.

> > +static const struct of_device_id uniphier_spi_match[] = {
> > +	{ .compatible = "socionext,uniphier-scssi", },
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
> 
> The binding document also listed socionext,uniphier-mcssi as a compatible but this driver doesn't match that.

This driver doesn't support uniphier-mcssi, and support uniphier-scssi only.
I described in the commit comment,
but I will also describe it in the binding document.

Thank you.

-----------------
Best Regards,
Keiji Hayashibara

[PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Keiji Hayashibara]

Hi Mark,

> -----Original Message-----
> From: Mark Brown [mailto:broonie at kernel.org]
> Sent: Friday, July 20, 2018 1:52 AM
> Subject: Re: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> On Thu, Jul 19, 2018 at 03:51:57PM +0900, Keiji Hayashibara wrote:
> 
> This all looks good, just a small number of fairly minor things - mostly style points.
> 
> > @@ -0,0 +1,532 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * spi-uniphier.c - Socionext UniPhier SPI controller driver
> > + *
> > + * Copyright 2012      Panasonic Corporation
> > + * Copyright 2016-2018 Socionext Inc.
> > + */
> 
> Please make the entire comment a C++ one, it makes things look a bit more joined up/intentional.

OK. I will modify to C++ style.


> > +#define BYTES_PER_WORD(x)			\
> > +({						\
> > +	int __x;				\
> > +	__x = (x <= 8)  ? 1 :			\
> > +	      (x <= 16) ? 2 : 4;		\
> > +	__x;					\
> > +})
> 
> Could this be replaced with an inline function?  The usage seems fine but it's a bit big for a macro.  The end
> result should be similar.

OK. I will replace with an inline function.

> 
> > +static irqreturn_t uniphier_spi_handler(int irq, void *dev_id) {
> > +	struct uniphier_spi_priv *priv = dev_id;
> > +	u32 val, stat;
> > +
> > +	stat = readl(priv->base + SSI_IS);
> > +	val = SSI_IC_TCIC | SSI_IC_RCIC | SSI_IC_RORIC;
> > +	writel(val, priv->base + SSI_IC);
> > +
> > +	/* rx fifo overrun */
> > +	if (stat & SSI_IS_RORID) {
> > +		priv->error = -EIO;
> > +		goto done;
> > +	}
> > +
> > +	/* rx complete */
> > +	if ((stat & SSI_IS_RCID) && (stat & SSI_IS_RXRS)) {
> 
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +
> > +done:
> > +	complete(&priv->xfer_done);
> > +	return IRQ_HANDLED;
> 
> This will unconditionally report IRQ_HANDLED even if none of the flags were set by the hardware which will cause
> problems if something goes wrong - the interrupt will continually be serviced and the interrupt framework won't
> be able to mitigate or provide diagnostics.  It's better to return IRQ_NONE if nothing is detected from the hardware.

I agree. I will modify it.

> > +static const struct of_device_id uniphier_spi_match[] = {
> > +	{ .compatible = "socionext,uniphier-scssi", },
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
> 
> The binding document also listed socionext,uniphier-mcssi as a compatible but this driver doesn't match that.

This driver doesn't support uniphier-mcssi, and support uniphier-scssi only.
I described in the commit comment,
but I will also describe it in the binding document.

Thank you.

-----------------
Best Regards,
Keiji Hayashibara

RE: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Keiji Hayashibara]

Hi Trent,

> -----Original Message-----
> From: Trent Piepho [mailto:tpiepho@impinj.com]
> Sent: Friday, July 20, 2018 4:46 AM
> Subject: Re: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> On Thu, 2018-07-19 at 15:51 +0900, Keiji Hayashibara wrote:
> >
> > +config SPI_UNIPHIER
> > +	tristate "Socionext UniPhier SPI Controller"
> > +	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> > +	help
> > +	  This driver supports the SPI controller on Socionext
> > +	  UniPhier SoCs.
> 
> Perhaps add the bit that this is for the SCSSI and not MCSSI here?

OK. I will add it.

> >
> > +
> > +#define BYTES_PER_WORD(x)			\
> > +({						\
> > +	int __x;				\
> > +	__x = (x <= 8)  ? 1 :			\
> > +	      (x <= 16) ? 2 : 4;		\
> > +	__x;					\
> > +})
> 
> Or:
> 
> static inline bytes_per_word(unsigned int bits) {
>    return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4); }

I will modify.


> 
> > +
> > +static inline void uniphier_spi_irq_enable(struct spi_device *spi,
> > +u32 mask) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +
> > +	val = readl(priv->base + SSI_IE);
> > +	val |= mask;
> > +	writel(val, priv->base + SSI_IE);
> > +}
> > +
> > +static inline void uniphier_spi_irq_disable(struct spi_device *spi,
> > +u32 mask) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +
> > +	val = readl(priv->base + SSI_IE);
> > +	val &= ~mask;
> > +	writel(val, priv->base + SSI_IE);
> > +}
> > +
> > +static void uniphier_spi_set_transfer_size(struct spi_device *spi,
> > +int size) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +
> > +	val = readl(priv->base + SSI_TXWDS);
> > +	val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> > +	val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> > +	val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> > +	writel(val, priv->base + SSI_TXWDS);
> > +
> > +	val = readl(priv->base + SSI_RXWDS);
> > +	val &= ~SSI_RXWDS_DTLEN_MASK;
> > +	val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> > +	writel(val, priv->base + SSI_RXWDS); }
> > +
> > +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
> > +int speed) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val, ckrat;
> > +
> > +	/*
> > +	 * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > +	 * round up as we look for equal or less speed
> > +	 */
> > +	ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> > +	ckrat = roundup(ckrat, 2);
> > +
> > +	/* check if requested speed is too small */
> > +	if (ckrat > SSI_MAX_CLK_DIVIDER)
> > +		return -EINVAL;
> > +
> > +	if (ckrat < SSI_MIN_CLK_DIVIDER)
> > +		ckrat = SSI_MIN_CLK_DIVIDER;
> > +
> > +	val = readl(priv->base + SSI_CKS);
> > +	val &= ~SSI_CKS_CKRAT_MASK;
> > +	val |= ckrat & SSI_CKS_CKRAT_MASK;
> > +	writel(val, priv->base + SSI_CKS);
> > +
> > +	return 0;
> > +}
> > +
> > +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> > +				       struct spi_transfer *t)
> > +{
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +	int ret;
> > +
> > +	priv->error = 0;
> > +	priv->tx_buf = t->tx_buf;
> > +	priv->rx_buf = t->rx_buf;
> > +	priv->tx_bytes = priv->rx_bytes = t->len;
> > +
> > +	if (priv->bits_per_word != t->bits_per_word) {
> > +		uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> > +		priv->bits_per_word = t->bits_per_word;
> > +	}
> > +
> > +	if (priv->speed_hz != t->speed_hz) {
> > +		ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> > +		if (ret)
> > +			return ret;
> > +		priv->speed_hz = t->speed_hz;
> > +	}
> > +
> > +	/* reset FIFOs */
> > +	val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> > +	writel(val, priv->base + SSI_FC);
> > +
> > +	return 0;
> > +}
> > +
> > +static void uniphier_spi_send(struct uniphier_spi_priv *priv) {
> > +	int i, loop;
> > +	u32 val = 0;
> > +
> > +	loop = BYTES_PER_WORD(priv->bits_per_word);
> > +	if (priv->tx_bytes < loop)
> > +		loop = priv->tx_bytes;
> > +
> > +	priv->tx_bytes -= loop;
> > +
> > +	if (priv->tx_buf)
> > +		for (i = 0; i < loop; i++) {
> > +			val |= (*(const u8 *)priv->tx_buf)
> > +						<< (BITS_PER_BYTE * i);
> 
> priv->tx_buf is already a const u8*, no need to cast it.  Also in recv,
> no need to cast the pointer.  It'll just hide errors if someone changes the type of the field.

I agree.

> > +			(const u8 *)priv->tx_buf++;
> > +		}
> > +
> > +	writel(val, priv->base + SSI_TXDR);
> > +}
> 
> The loop to read the data will likely be somewhat slow.  It might be faster to use:
> 
>     val = get_unaligned_le32(priv->tx_buf);
> 
> To support different sizes a switch can be used:
> 
>     switch (MIN(BYTES_PER_WORD(priv->bits_per_word), priv->tx_bytes)) {
>     case 1:
>          val = *priv->tx_buf; break;
>     case 2:
>          val = get_unaligned_le16(priv->tx_buf); break;
>     case 4:
>          val = get_unaligned_le32(priv->tx_buf); break;
>     }
> 
> However, I don't think either the existing code or this code is correctly handling word sizes that are not an
> even number of bytes.  I think it needs to left shift the data, but of course it also depends on what the uniphier
> hardware expected in the TXDR register.

I agree. This code is simple for no loop.
I will modify to this code.

> 
> > +static void uniphier_spi_recv(struct uniphier_spi_priv *priv) {
> > +	int i, loop;
> > +	u32 val;
> > +
> > +	loop = BYTES_PER_WORD(priv->bits_per_word);
> > +	if (priv->rx_bytes < loop)
> > +		loop = priv->rx_bytes;
> > +
> > +	priv->rx_bytes -= loop;
> > +
> > +	val = readl(priv->base + SSI_RXDR);
> > +
> > +	if (priv->rx_buf)
> > +		for (i = 0; i < loop; i++) {
> > +			val = val >> (BITS_PER_BYTE * i);
> > +			*(u8 *)priv->rx_buf = val & GENMASK(7, 0);
> > +			(u8 *)priv->rx_buf++;
> > +		}
> 
> 
> 
> > +}+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv
> > +*priv) {
> > +	unsigned int tx_count;
> > +	int bytes_per_word = BYTES_PER_WORD(priv->bits_per_word);
> > +	u32 val;
> > +
> > +	tx_count = priv->tx_bytes / bytes_per_word;
> > +	if (tx_count > SSI_FIFO_DEPTH)
> > +		tx_count = SSI_FIFO_DEPTH;
> > +
> > +	/* set fifo threthold */
> > +	val = readl(priv->base + SSI_FC);
> > +	val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> > +	val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> > +	val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> > +	writel(val, priv->base + SSI_FC);
> > +
> > +	while (tx_count--)
> > +		uniphier_spi_send(priv);
> > +}
> 
> If you have 24 bits per word, 3 words, that's 9 bytes.
> BYTES_PER_WORD(24) is 4.  tx_count = 9/4 = 2.  Looks like your tx_count rounds incorrectly, as it will only send
> 8 of the 9 bytes.

Oh, I will fix this bug.

> 
> > +static int uniphier_spi_setup(struct spi_device *spi) {
> 
> > +
> > +	writel(val1, priv->base + SSI_CKS);
> > +	writel(val2, priv->base + SSI_FPS);
> > +
> > +	val1 = 0;
> > +	if (spi->mode & SPI_LSB_FIRST)
> > +		val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> > +	writel(val1, priv->base + SSI_TXWDS);
> > +	writel(val1, priv->base + SSI_RXWDS);
> 
> Did you see this in the spi docs?
> 
>         Unless each SPI slave has its own configuration registers, don't
>         change them right away ... otherwise drivers could corrupt I/O
>         that's in progress for other SPI devices.
> 
>                 ** BUG ALERT:  for some reason the first version of
>                 ** many spi_master drivers seems to get this wrong.
>                 ** When you code setup(), ASSUME that the controller
>                 ** is actively processing transfers for another device.
> 
> You have one chipselect, so maybe this is ok.  Until you want to support more than one chipselect.
> 
> With gpio lines as chip selects, there's really no reason any spi master can't support multiple slaves.

I agree. I will re-think about this.

Thank you for your advice.

-----------------
Best Regards,
Keiji Hayashibara

RE: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Keiji Hayashibara]

Hi Trent,

> -----Original Message-----
> From: Trent Piepho [mailto:tpiepho@impinj.com]
> Sent: Friday, July 20, 2018 4:46 AM
> Subject: Re: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> On Thu, 2018-07-19 at 15:51 +0900, Keiji Hayashibara wrote:
> >
> > +config SPI_UNIPHIER
> > +	tristate "Socionext UniPhier SPI Controller"
> > +	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> > +	help
> > +	  This driver supports the SPI controller on Socionext
> > +	  UniPhier SoCs.
> 
> Perhaps add the bit that this is for the SCSSI and not MCSSI here?

OK. I will add it.

> >
> > +
> > +#define BYTES_PER_WORD(x)			\
> > +({						\
> > +	int __x;				\
> > +	__x = (x <= 8)  ? 1 :			\
> > +	      (x <= 16) ? 2 : 4;		\
> > +	__x;					\
> > +})
> 
> Or:
> 
> static inline bytes_per_word(unsigned int bits) {
>    return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4); }

I will modify.


> 
> > +
> > +static inline void uniphier_spi_irq_enable(struct spi_device *spi,
> > +u32 mask) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +
> > +	val = readl(priv->base + SSI_IE);
> > +	val |= mask;
> > +	writel(val, priv->base + SSI_IE);
> > +}
> > +
> > +static inline void uniphier_spi_irq_disable(struct spi_device *spi,
> > +u32 mask) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +
> > +	val = readl(priv->base + SSI_IE);
> > +	val &= ~mask;
> > +	writel(val, priv->base + SSI_IE);
> > +}
> > +
> > +static void uniphier_spi_set_transfer_size(struct spi_device *spi,
> > +int size) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +
> > +	val = readl(priv->base + SSI_TXWDS);
> > +	val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> > +	val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> > +	val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> > +	writel(val, priv->base + SSI_TXWDS);
> > +
> > +	val = readl(priv->base + SSI_RXWDS);
> > +	val &= ~SSI_RXWDS_DTLEN_MASK;
> > +	val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> > +	writel(val, priv->base + SSI_RXWDS); }
> > +
> > +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
> > +int speed) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val, ckrat;
> > +
> > +	/*
> > +	 * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > +	 * round up as we look for equal or less speed
> > +	 */
> > +	ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> > +	ckrat = roundup(ckrat, 2);
> > +
> > +	/* check if requested speed is too small */
> > +	if (ckrat > SSI_MAX_CLK_DIVIDER)
> > +		return -EINVAL;
> > +
> > +	if (ckrat < SSI_MIN_CLK_DIVIDER)
> > +		ckrat = SSI_MIN_CLK_DIVIDER;
> > +
> > +	val = readl(priv->base + SSI_CKS);
> > +	val &= ~SSI_CKS_CKRAT_MASK;
> > +	val |= ckrat & SSI_CKS_CKRAT_MASK;
> > +	writel(val, priv->base + SSI_CKS);
> > +
> > +	return 0;
> > +}
> > +
> > +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> > +				       struct spi_transfer *t)
> > +{
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +	int ret;
> > +
> > +	priv->error = 0;
> > +	priv->tx_buf = t->tx_buf;
> > +	priv->rx_buf = t->rx_buf;
> > +	priv->tx_bytes = priv->rx_bytes = t->len;
> > +
> > +	if (priv->bits_per_word != t->bits_per_word) {
> > +		uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> > +		priv->bits_per_word = t->bits_per_word;
> > +	}
> > +
> > +	if (priv->speed_hz != t->speed_hz) {
> > +		ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> > +		if (ret)
> > +			return ret;
> > +		priv->speed_hz = t->speed_hz;
> > +	}
> > +
> > +	/* reset FIFOs */
> > +	val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> > +	writel(val, priv->base + SSI_FC);
> > +
> > +	return 0;
> > +}
> > +
> > +static void uniphier_spi_send(struct uniphier_spi_priv *priv) {
> > +	int i, loop;
> > +	u32 val = 0;
> > +
> > +	loop = BYTES_PER_WORD(priv->bits_per_word);
> > +	if (priv->tx_bytes < loop)
> > +		loop = priv->tx_bytes;
> > +
> > +	priv->tx_bytes -= loop;
> > +
> > +	if (priv->tx_buf)
> > +		for (i = 0; i < loop; i++) {
> > +			val |= (*(const u8 *)priv->tx_buf)
> > +						<< (BITS_PER_BYTE * i);
> 
> priv->tx_buf is already a const u8*, no need to cast it.  Also in recv,
> no need to cast the pointer.  It'll just hide errors if someone changes the type of the field.

I agree.

> > +			(const u8 *)priv->tx_buf++;
> > +		}
> > +
> > +	writel(val, priv->base + SSI_TXDR);
> > +}
> 
> The loop to read the data will likely be somewhat slow.  It might be faster to use:
> 
>     val = get_unaligned_le32(priv->tx_buf);
> 
> To support different sizes a switch can be used:
> 
>     switch (MIN(BYTES_PER_WORD(priv->bits_per_word), priv->tx_bytes)) {
>     case 1:
>          val = *priv->tx_buf; break;
>     case 2:
>          val = get_unaligned_le16(priv->tx_buf); break;
>     case 4:
>          val = get_unaligned_le32(priv->tx_buf); break;
>     }
> 
> However, I don't think either the existing code or this code is correctly handling word sizes that are not an
> even number of bytes.  I think it needs to left shift the data, but of course it also depends on what the uniphier
> hardware expected in the TXDR register.

I agree. This code is simple for no loop.
I will modify to this code.

> 
> > +static void uniphier_spi_recv(struct uniphier_spi_priv *priv) {
> > +	int i, loop;
> > +	u32 val;
> > +
> > +	loop = BYTES_PER_WORD(priv->bits_per_word);
> > +	if (priv->rx_bytes < loop)
> > +		loop = priv->rx_bytes;
> > +
> > +	priv->rx_bytes -= loop;
> > +
> > +	val = readl(priv->base + SSI_RXDR);
> > +
> > +	if (priv->rx_buf)
> > +		for (i = 0; i < loop; i++) {
> > +			val = val >> (BITS_PER_BYTE * i);
> > +			*(u8 *)priv->rx_buf = val & GENMASK(7, 0);
> > +			(u8 *)priv->rx_buf++;
> > +		}
> 
> 
> 
> > +}+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv
> > +*priv) {
> > +	unsigned int tx_count;
> > +	int bytes_per_word = BYTES_PER_WORD(priv->bits_per_word);
> > +	u32 val;
> > +
> > +	tx_count = priv->tx_bytes / bytes_per_word;
> > +	if (tx_count > SSI_FIFO_DEPTH)
> > +		tx_count = SSI_FIFO_DEPTH;
> > +
> > +	/* set fifo threthold */
> > +	val = readl(priv->base + SSI_FC);
> > +	val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> > +	val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> > +	val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> > +	writel(val, priv->base + SSI_FC);
> > +
> > +	while (tx_count--)
> > +		uniphier_spi_send(priv);
> > +}
> 
> If you have 24 bits per word, 3 words, that's 9 bytes.
> BYTES_PER_WORD(24) is 4.  tx_count = 9/4 = 2.  Looks like your tx_count rounds incorrectly, as it will only send
> 8 of the 9 bytes.

Oh, I will fix this bug.

> 
> > +static int uniphier_spi_setup(struct spi_device *spi) {
> 
> > +
> > +	writel(val1, priv->base + SSI_CKS);
> > +	writel(val2, priv->base + SSI_FPS);
> > +
> > +	val1 = 0;
> > +	if (spi->mode & SPI_LSB_FIRST)
> > +		val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> > +	writel(val1, priv->base + SSI_TXWDS);
> > +	writel(val1, priv->base + SSI_RXWDS);
> 
> Did you see this in the spi docs?
> 
>         Unless each SPI slave has its own configuration registers, don't
>         change them right away ... otherwise drivers could corrupt I/O
>         that's in progress for other SPI devices.
> 
>                 ** BUG ALERT:  for some reason the first version of
>                 ** many spi_master drivers seems to get this wrong.
>                 ** When you code setup(), ASSUME that the controller
>                 ** is actively processing transfers for another device.
> 
> You have one chipselect, so maybe this is ok.  Until you want to support more than one chipselect.
> 
> With gpio lines as chip selects, there's really no reason any spi master can't support multiple slaves.

I agree. I will re-think about this.

Thank you for your advice.

-----------------
Best Regards,
Keiji Hayashibara

[PATCH 2/2] spi: add SPI controller driver for UniPhier SoC

[Keiji Hayashibara]

Hi Trent,

> -----Original Message-----
> From: Trent Piepho [mailto:tpiepho at impinj.com]
> Sent: Friday, July 20, 2018 4:46 AM
> Subject: Re: [PATCH 2/2] spi: add SPI controller driver for UniPhier SoC
> 
> On Thu, 2018-07-19 at 15:51 +0900, Keiji Hayashibara wrote:
> >
> > +config SPI_UNIPHIER
> > +	tristate "Socionext UniPhier SPI Controller"
> > +	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> > +	help
> > +	  This driver supports the SPI controller on Socionext
> > +	  UniPhier SoCs.
> 
> Perhaps add the bit that this is for the SCSSI and not MCSSI here?

OK. I will add it.

> >
> > +
> > +#define BYTES_PER_WORD(x)			\
> > +({						\
> > +	int __x;				\
> > +	__x = (x <= 8)  ? 1 :			\
> > +	      (x <= 16) ? 2 : 4;		\
> > +	__x;					\
> > +})
> 
> Or:
> 
> static inline bytes_per_word(unsigned int bits) {
>    return bits <= 8 ? 1 : (bits <= 16 ? 2 : 4); }

I will modify.


> 
> > +
> > +static inline void uniphier_spi_irq_enable(struct spi_device *spi,
> > +u32 mask) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +
> > +	val = readl(priv->base + SSI_IE);
> > +	val |= mask;
> > +	writel(val, priv->base + SSI_IE);
> > +}
> > +
> > +static inline void uniphier_spi_irq_disable(struct spi_device *spi,
> > +u32 mask) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +
> > +	val = readl(priv->base + SSI_IE);
> > +	val &= ~mask;
> > +	writel(val, priv->base + SSI_IE);
> > +}
> > +
> > +static void uniphier_spi_set_transfer_size(struct spi_device *spi,
> > +int size) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +
> > +	val = readl(priv->base + SSI_TXWDS);
> > +	val &= ~(SSI_TXWDS_WDLEN_MASK | SSI_TXWDS_DTLEN_MASK);
> > +	val |= FIELD_PREP(SSI_TXWDS_WDLEN_MASK, size);
> > +	val |= FIELD_PREP(SSI_TXWDS_DTLEN_MASK, size);
> > +	writel(val, priv->base + SSI_TXWDS);
> > +
> > +	val = readl(priv->base + SSI_RXWDS);
> > +	val &= ~SSI_RXWDS_DTLEN_MASK;
> > +	val |= FIELD_PREP(SSI_RXWDS_DTLEN_MASK, size);
> > +	writel(val, priv->base + SSI_RXWDS); }
> > +
> > +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
> > +int speed) {
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val, ckrat;
> > +
> > +	/*
> > +	 * the supported rates are even numbers from 4 to 254. (4,6,8...254)
> > +	 * round up as we look for equal or less speed
> > +	 */
> > +	ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
> > +	ckrat = roundup(ckrat, 2);
> > +
> > +	/* check if requested speed is too small */
> > +	if (ckrat > SSI_MAX_CLK_DIVIDER)
> > +		return -EINVAL;
> > +
> > +	if (ckrat < SSI_MIN_CLK_DIVIDER)
> > +		ckrat = SSI_MIN_CLK_DIVIDER;
> > +
> > +	val = readl(priv->base + SSI_CKS);
> > +	val &= ~SSI_CKS_CKRAT_MASK;
> > +	val |= ckrat & SSI_CKS_CKRAT_MASK;
> > +	writel(val, priv->base + SSI_CKS);
> > +
> > +	return 0;
> > +}
> > +
> > +static int uniphier_spi_setup_transfer(struct spi_device *spi,
> > +				       struct spi_transfer *t)
> > +{
> > +	struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
> > +	u32 val;
> > +	int ret;
> > +
> > +	priv->error = 0;
> > +	priv->tx_buf = t->tx_buf;
> > +	priv->rx_buf = t->rx_buf;
> > +	priv->tx_bytes = priv->rx_bytes = t->len;
> > +
> > +	if (priv->bits_per_word != t->bits_per_word) {
> > +		uniphier_spi_set_transfer_size(spi, t->bits_per_word);
> > +		priv->bits_per_word = t->bits_per_word;
> > +	}
> > +
> > +	if (priv->speed_hz != t->speed_hz) {
> > +		ret = uniphier_spi_set_baudrate(spi, t->speed_hz);
> > +		if (ret)
> > +			return ret;
> > +		priv->speed_hz = t->speed_hz;
> > +	}
> > +
> > +	/* reset FIFOs */
> > +	val = SSI_FC_TXFFL | SSI_FC_RXFFL;
> > +	writel(val, priv->base + SSI_FC);
> > +
> > +	return 0;
> > +}
> > +
> > +static void uniphier_spi_send(struct uniphier_spi_priv *priv) {
> > +	int i, loop;
> > +	u32 val = 0;
> > +
> > +	loop = BYTES_PER_WORD(priv->bits_per_word);
> > +	if (priv->tx_bytes < loop)
> > +		loop = priv->tx_bytes;
> > +
> > +	priv->tx_bytes -= loop;
> > +
> > +	if (priv->tx_buf)
> > +		for (i = 0; i < loop; i++) {
> > +			val |= (*(const u8 *)priv->tx_buf)
> > +						<< (BITS_PER_BYTE * i);
> 
> priv->tx_buf is already a const u8*, no need to cast it.  Also in recv,
> no need to cast the pointer.  It'll just hide errors if someone changes the type of the field.

I agree.

> > +			(const u8 *)priv->tx_buf++;
> > +		}
> > +
> > +	writel(val, priv->base + SSI_TXDR);
> > +}
> 
> The loop to read the data will likely be somewhat slow.  It might be faster to use:
> 
>     val = get_unaligned_le32(priv->tx_buf);
> 
> To support different sizes a switch can be used:
> 
>     switch (MIN(BYTES_PER_WORD(priv->bits_per_word), priv->tx_bytes)) {
>     case 1:
>          val = *priv->tx_buf; break;
>     case 2:
>          val = get_unaligned_le16(priv->tx_buf); break;
>     case 4:
>          val = get_unaligned_le32(priv->tx_buf); break;
>     }
> 
> However, I don't think either the existing code or this code is correctly handling word sizes that are not an
> even number of bytes.  I think it needs to left shift the data, but of course it also depends on what the uniphier
> hardware expected in the TXDR register.

I agree. This code is simple for no loop.
I will modify to this code.

> 
> > +static void uniphier_spi_recv(struct uniphier_spi_priv *priv) {
> > +	int i, loop;
> > +	u32 val;
> > +
> > +	loop = BYTES_PER_WORD(priv->bits_per_word);
> > +	if (priv->rx_bytes < loop)
> > +		loop = priv->rx_bytes;
> > +
> > +	priv->rx_bytes -= loop;
> > +
> > +	val = readl(priv->base + SSI_RXDR);
> > +
> > +	if (priv->rx_buf)
> > +		for (i = 0; i < loop; i++) {
> > +			val = val >> (BITS_PER_BYTE * i);
> > +			*(u8 *)priv->rx_buf = val & GENMASK(7, 0);
> > +			(u8 *)priv->rx_buf++;
> > +		}
> 
> 
> 
> > +}+static void uniphier_spi_fill_tx_fifo(struct uniphier_spi_priv
> > +*priv) {
> > +	unsigned int tx_count;
> > +	int bytes_per_word = BYTES_PER_WORD(priv->bits_per_word);
> > +	u32 val;
> > +
> > +	tx_count = priv->tx_bytes / bytes_per_word;
> > +	if (tx_count > SSI_FIFO_DEPTH)
> > +		tx_count = SSI_FIFO_DEPTH;
> > +
> > +	/* set fifo threthold */
> > +	val = readl(priv->base + SSI_FC);
> > +	val &= ~(SSI_FC_TXFTH_MASK | SSI_FC_RXFTH_MASK);
> > +	val |= FIELD_PREP(SSI_FC_TXFTH_MASK, tx_count);
> > +	val |= FIELD_PREP(SSI_FC_RXFTH_MASK, tx_count);
> > +	writel(val, priv->base + SSI_FC);
> > +
> > +	while (tx_count--)
> > +		uniphier_spi_send(priv);
> > +}
> 
> If you have 24 bits per word, 3 words, that's 9 bytes.
> BYTES_PER_WORD(24) is 4.  tx_count = 9/4 = 2.  Looks like your tx_count rounds incorrectly, as it will only send
> 8 of the 9 bytes.

Oh, I will fix this bug.

> 
> > +static int uniphier_spi_setup(struct spi_device *spi) {
> 
> > +
> > +	writel(val1, priv->base + SSI_CKS);
> > +	writel(val2, priv->base + SSI_FPS);
> > +
> > +	val1 = 0;
> > +	if (spi->mode & SPI_LSB_FIRST)
> > +		val1 |= FIELD_PREP(SSI_TXWDS_TDTF_MASK, 1);
> > +	writel(val1, priv->base + SSI_TXWDS);
> > +	writel(val1, priv->base + SSI_RXWDS);
> 
> Did you see this in the spi docs?
> 
>         Unless each SPI slave has its own configuration registers, don't
>         change them right away ... otherwise drivers could corrupt I/O
>         that's in progress for other SPI devices.
> 
>                 ** BUG ALERT:  for some reason the first version of
>                 ** many spi_master drivers seems to get this wrong.
>                 ** When you code setup(), ASSUME that the controller
>                 ** is actively processing transfers for another device.
> 
> You have one chipselect, so maybe this is ok.  Until you want to support more than one chipselect.
> 
> With gpio lines as chip selects, there's really no reason any spi master can't support multiple slaves.

I agree. I will re-think about this.

Thank you for your advice.

-----------------
Best Regards,
Keiji Hayashibara

Re: [PATCH 1/2] dt-bindings: spi: add DT bindings for UniPhier SPI controller

[Rob Herring]

On Thu, Jul 19, 2018 at 03:51:56PM +0900, Keiji Hayashibara wrote:
> From: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> 
> Add DT bindings for SPI controller implemented in UniPhier SoCs.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Signed-off-by: Keiji Hayashibara <hayashibara.keiji@socionext.com>
> ---
>  .../devicetree/bindings/spi/spi-uniphier.txt       | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt

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

[PATCH 1/2] dt-bindings: spi: add DT bindings for UniPhier SPI controller

[Rob Herring]

On Thu, Jul 19, 2018 at 03:51:56PM +0900, Keiji Hayashibara wrote:
> From: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> 
> Add DT bindings for SPI controller implemented in UniPhier SoCs.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Signed-off-by: Keiji Hayashibara <hayashibara.keiji@socionext.com>
> ---
>  .../devicetree/bindings/spi/spi-uniphier.txt       | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-uniphier.txt

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