[PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Baolin Wang]

From: Lanqing Liu <lanqing.liu@spreadtrum.com>

This patch adds the binding documentation for Spreadtrum SPI
controller device.

Signed-off-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 Documentation/devicetree/bindings/spi/spi-sprd.txt |   31 ++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sprd.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt
new file mode 100644
index 0000000..06ff746
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt
@@ -0,0 +1,31 @@
+Spreadtrum SPI Controller
+
+Required properties:
+- compatible: Should be "sprd,sc9860-spi".
+- reg: Offset and length of SPI controller register space.
+- interrupts: Should contain SPI interrupt.
+- clock-names: Should contain following entries:
+	"spi" for SPI clock,
+	"source" for SPI source (parent) clock,
+	"enable" for SPI module enable clock.
+- clocks: List of clock input name strings sorted in the same order
+	as the clock-names property.
+- #address-cells: The number of cells required to define a chip select
+	address on the SPI bus. Should be set to 1.
+- #size-cells: Should be set to 0.
+
+Optional properties:
+- sprd,spi-interval: Specify the intervals of two SPI frames, which can be
+	converted to the delay clock cycles = interval number * 4 + 10.
+
+Example:
+spi0: spi@70a00000{
+	compatible = "sprd,sc9860-spi";
+	reg = <0 0x70a00000 0 0x1000>;
+	interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+	clock-names = "spi", "source","enable";
+	clocks = <&clk_spi0>, <&ext_26m>, <&clk_ap_apb_gates 5>;
+	sprd,spi-interval = <9>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+};
-- 
1.7.9.5

[PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860

[Baolin Wang]

From: Lanqing Liu <lanqing.liu@spreadtrum.com>

This patch adds the SPI controller driver for Spreadtrum SC9860 platform.

Signed-off-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/spi/Kconfig    |    6 +
 drivers/spi/Makefile   |    1 +
 drivers/spi/spi-sprd.c |  771 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 778 insertions(+)
 create mode 100644 drivers/spi/spi-sprd.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ad5d68e..c49c40c 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -596,6 +596,12 @@ config SPI_SIRF
 	help
 	  SPI driver for CSR SiRFprimaII SoCs
 
+config SPI_SPRD
+	tristate "Spreadtrum SPI controller"
+	depends on ARCH_SPRD || COMPILE_TEST
+	help
+	  SPI driver for Spreadtrum SoCs.
+
 config SPI_SPRD_ADI
 	tristate "Spreadtrum ADI controller"
 	depends on ARCH_SPRD || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index cb1f437..5bb6b37 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_SPI_SH_HSPI)		+= spi-sh-hspi.o
 obj-$(CONFIG_SPI_SH_MSIOF)		+= spi-sh-msiof.o
 obj-$(CONFIG_SPI_SH_SCI)		+= spi-sh-sci.o
 obj-$(CONFIG_SPI_SIRF)		+= spi-sirf.o
+obj-$(CONFIG_SPI_SPRD)			+= spi-sprd.o
 obj-$(CONFIG_SPI_SPRD_ADI)		+= spi-sprd-adi.o
 obj-$(CONFIG_SPI_STM32) 		+= spi-stm32.o
 obj-$(CONFIG_SPI_ST_SSC4)		+= spi-st-ssc4.o
diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c
new file mode 100644
index 0000000..cd77a5a
--- /dev/null
+++ b/drivers/spi/spi-sprd.c
@@ -0,0 +1,771 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Spreadtrum Communications Inc.
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/spi/spi.h>
+
+#define SPRD_SPI_TXD			0x0
+#define SPRD_SPI_CLKD			0x4
+#define SPRD_SPI_CTL0			0x8
+#define SPRD_SPI_CTL1			0xc
+#define SPRD_SPI_CTL2			0x10
+#define SPRD_SPI_CTL3			0x14
+#define SPRD_SPI_CTL4			0x18
+#define SPRD_SPI_CTL5			0x1c
+#define SPRD_SPI_INT_EN			0x20
+#define SPRD_SPI_INT_CLR		0x24
+#define SPRD_SPI_INT_RAW_STS		0x28
+#define SPRD_SPI_INT_MASK_STS		0x2c
+#define SPRD_SPI_STS1			0x30
+#define SPRD_SPI_STS2			0x34
+#define SPRD_SPI_DSP_WAIT		0x38
+#define SPRD_SPI_STS3			0x3c
+#define SPRD_SPI_CTL6			0x40
+#define SPRD_SPI_STS4			0x44
+#define SPRD_SPI_FIFO_RST		0x48
+#define SPRD_SPI_CTL7			0x4c
+#define SPRD_SPI_STS5			0x50
+#define SPRD_SPI_CTL8			0x54
+#define SPRD_SPI_CTL9			0x58
+#define SPRD_SPI_CTL10			0x5c
+#define SPRD_SPI_CTL11			0x60
+#define SPRD_SPI_CTL12			0x64
+#define SPRD_SPI_STS6			0x68
+#define SPRD_SPI_STS7			0x6c
+#define SPRD_SPI_STS8			0x70
+#define SPRD_SPI_STS9			0x74
+
+/* Bits & mask definition for register CTL0 */
+#define SPRD_SPI_SCK_REV		BIT(13)
+#define SPRD_SPI_NG_TX			BIT(1)
+#define SPRD_SPI_NG_RX			BIT(0)
+#define SPRD_SPI_CHNL_LEN_MASK		GENMASK(4, 0)
+#define SPRD_SPI_CSN_MASK		GENMASK(11, 8)
+#define SPRD_SPI_CS0_VALID		BIT(8)
+
+/* Bits & mask definition for register SPI_INT_EN */
+#define SPRD_SPI_TX_END_INT_EN		BIT(8)
+#define SPRD_SPI_RX_END_INT_EN		BIT(9)
+
+/* Bits & mask definition for register SPI_INT_RAW_STS */
+#define SPRD_SPI_TX_END_RAW		BIT(8)
+#define SPRD_SPI_RX_END_RAW		BIT(9)
+
+/* Bits & mask definition for register SPI_INT_CLR */
+#define SPRD_SPI_TX_END_CLR		BIT(8)
+#define SPRD_SPI_RX_END_CLR		BIT(9)
+
+/* Bits & mask definition for register INT_MASK_STS */
+#define SPRD_SPI_MASK_RX_END		BIT(9)
+#define SPRD_SPI_MASK_TX_END		BIT(8)
+
+/* Bits & mask definition for register STS2 */
+#define SPRD_SPI_TX_BUSY		BIT(8)
+
+/* Bits & mask definition for register CTL1 */
+#define SPRD_SPI_RX_MODE		BIT(12)
+#define SPRD_SPI_TX_MODE		BIT(13)
+#define SPRD_SPI_RTX_MD_MASK		GENMASK(13, 12)
+
+/* Bits & mask definition for register CTL2 */
+#define SPRD_SPI_DMA_EN			BIT(6)
+
+/* Bits & mask definition for register CTL4 */
+#define SPRD_SPI_START_RX		BIT(9)
+#define SPRD_SPI_ONLY_RECV_MASK		GENMASK(8, 0)
+
+/* Bits & mask definition for register SPI_INT_CLR */
+#define SPRD_SPI_RX_END_INT_CLR		BIT(9)
+#define SPRD_SPI_TX_END_INT_CLR		BIT(8)
+
+/* Bits & mask definition for register SPI_INT_RAW */
+#define SPRD_SPI_RX_END_IRQ		BIT(9)
+#define SPRD_SPI_TX_END_IRQ		BIT(8)
+
+/* Bits & mask definition for register CTL12 */
+#define SPRD_SPI_SW_RX_REQ		BIT(0)
+#define SPRD_SPI_SW_TX_REQ		BIT(1)
+
+/* Bits & mask definition for register CTL7 */
+#define SPRD_SPI_DATA_LINE2_EN		BIT(15)
+#define SPRD_SPI_MODE_MASK		GENMASK(5, 3)
+#define SPRD_SPI_MODE_OFFSET		3
+#define SPRD_SPI_3WIRE_MODE		4
+#define SPRD_SPI_4WIRE_MODE		0
+
+/* Bits & mask definition for register CTL8 */
+#define SPRD_SPI_TX_MAX_LEN_MASK	GENMASK(19, 0)
+#define SPRD_SPI_TX_LEN_H_MASK		GENMASK(3, 0)
+#define SPRD_SPI_TX_LEN_H_OFFSET	16
+
+/* Bits & mask definition for register CTL9 */
+#define SPRD_SPI_TX_LEN_L_MASK		GENMASK(15, 0)
+
+/* Bits & mask definition for register CTL10 */
+#define SPRD_SPI_RX_MAX_LEN_MASK	GENMASK(19, 0)
+#define SPRD_SPI_RX_LEN_H_MASK		GENMASK(3, 0)
+#define SPRD_SPI_RX_LEN_H_OFFSET	16
+
+/* Bits & mask definition for register CTL11 */
+#define SPRD_SPI_RX_LEN_L_MASK		GENMASK(15, 0)
+
+/* Default interval value */
+#define SPRD_SPI_ITVL_NUM		0x9
+
+#define SPRD_SPI_FIFO_SIZE		32
+#define SPRD_SPI_CHIP_CS_NUM		0x4
+#define SPRD_SPI_CHNL_LEN		2
+#define SPRD_SPI_DEFAULT_SOURCE		26000000
+#define SPRD_SPI_MAX_SPEED_HZ		48000000
+#define SPRD_SPI_HZ			1000000
+#define SPRD_SPI_AUTOSUSPEND_DELAY	100
+
+struct sprd_spi {
+	void __iomem *base;
+	struct device *dev;
+	struct clk *clk;
+	u32 src_clk;
+	u32 hw_mode;
+	u32 trans_len;
+	u32 trans_mode;
+	u32 interval;
+	u32 len;
+	int status;
+	const void *tx_buf;
+	void *rx_buf;
+	int (*read_bufs)(struct sprd_spi *ss, u32 len);
+	int (*write_bufs)(struct sprd_spi *ss, u32 len);
+};
+
+static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss,
+					 struct spi_transfer *t)
+{
+	/*
+	 * The time spent on transmission of the full FIFO data is the maximum
+	 * SPI transmission time.
+	 */
+	u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE;
+	u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1;
+	u32 total_time_us = size * bit_time_us;
+	/*
+	 * There is an interval between data and the data in our SPI hardware,
+	 * so the total transmission time need add the interval time.
+	 *
+	 * The inteval calculation formula:
+	 * interval time (source clock cycles) = interval * 4 + 10.
+	 */
+	u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) + 10);
+	u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk + 1;
+
+	return total_time_us + interval_time_us;
+}
+
+static int sprd_spi_wait_for_tx_end(struct sprd_spi *ss, struct spi_transfer *t)
+{
+	u32 val, us;
+	int ret;
+
+	us = sprd_spi_transfer_max_timeout(ss, t);
+	ret = readl_relaxed_poll_timeout(ss->base + SPRD_SPI_INT_RAW_STS, val,
+					 val & SPRD_SPI_TX_END_IRQ, 0, us);
+	if (ret) {
+		dev_err(ss->dev, "SPI error, spi send timeout!\n");
+		return ret;
+	}
+
+	ret = readl_relaxed_poll_timeout(ss->base + SPRD_SPI_STS2, val,
+					 !(val & SPRD_SPI_TX_BUSY), 0, us);
+	if (ret) {
+		dev_err(ss->dev, "SPI error, spi busy timeout!\n");
+		return ret;
+	}
+
+	writel_relaxed(SPRD_SPI_TX_END_INT_CLR, ss->base + SPRD_SPI_INT_CLR);
+
+	return 0;
+}
+
+static int sprd_spi_wait_for_rx_end(struct sprd_spi *ss, struct spi_transfer *t)
+{
+	u32 val, us;
+	int ret;
+
+	us = sprd_spi_transfer_max_timeout(ss, t);
+	ret = readl_relaxed_poll_timeout(ss->base + SPRD_SPI_INT_RAW_STS, val,
+					 val & SPRD_SPI_RX_END_IRQ, 0, us);
+	if (ret) {
+		dev_err(ss->dev, "SPI error, spi rx timeout!\n");
+		return ret;
+	}
+
+	writel_relaxed(SPRD_SPI_RX_END_INT_CLR, ss->base + SPRD_SPI_INT_CLR);
+
+	return 0;
+}
+
+static void sprd_spi_tx_req(struct sprd_spi *ss)
+{
+	writel_relaxed(SPRD_SPI_SW_TX_REQ, ss->base + SPRD_SPI_CTL12);
+}
+
+static void sprd_spi_rx_req(struct sprd_spi *ss)
+{
+	writel_relaxed(SPRD_SPI_SW_RX_REQ, ss->base + SPRD_SPI_CTL12);
+}
+
+static void sprd_spi_enter_idle(struct sprd_spi *ss)
+{
+	u32 val = readl_relaxed(ss->base + SPRD_SPI_CTL1);
+
+	val &= ~SPRD_SPI_RTX_MD_MASK;
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL1);
+}
+
+static void sprd_spi_set_transfer_bits(struct sprd_spi *ss, u32 bits)
+{
+	u32 val = readl_relaxed(ss->base + SPRD_SPI_CTL0);
+
+	/* Set the valid bits for every transaction */
+	val &= ~(SPRD_SPI_CHNL_LEN_MASK << SPRD_SPI_CHNL_LEN);
+	val |= bits << SPRD_SPI_CHNL_LEN;
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL0);
+}
+
+static void sprd_spi_set_tx_length(struct sprd_spi *ss, u32 length)
+{
+	u32 val = readl_relaxed(ss->base + SPRD_SPI_CTL8);
+
+	length &= SPRD_SPI_TX_MAX_LEN_MASK;
+	val &= ~SPRD_SPI_TX_LEN_H_MASK;
+	val |= length >> SPRD_SPI_TX_LEN_H_OFFSET;
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL8);
+
+	val = length & SPRD_SPI_TX_LEN_L_MASK;
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL9);
+}
+
+static void sprd_spi_set_rx_length(struct sprd_spi *ss, u32 length)
+{
+	u32 val = readl_relaxed(ss->base + SPRD_SPI_CTL10);
+
+	length &= SPRD_SPI_RX_MAX_LEN_MASK;
+	val &= ~SPRD_SPI_RX_LEN_H_MASK;
+	val |= length >> SPRD_SPI_RX_LEN_H_OFFSET;
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL10);
+
+	val = length & SPRD_SPI_RX_LEN_L_MASK;
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL11);
+}
+
+static void sprd_spi_chipselect(struct spi_device *sdev, bool cs)
+{
+	struct spi_controller *sctlr = sdev->controller;
+	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
+	u32 val;
+	int ret;
+
+	ret = pm_runtime_get_sync(ss->dev);
+	if (ret < 0) {
+		dev_err(ss->dev, "failed to resume SPI controller\n");
+		return;
+	}
+
+	val = readl_relaxed(ss->base + SPRD_SPI_CTL0);
+	/*  The SPI controller will pull down CS pin if cs is 0 */
+	if (!cs) {
+		val &= ~SPRD_SPI_CS0_VALID;
+		writel_relaxed(val, ss->base + SPRD_SPI_CTL0);
+	} else {
+		val |= SPRD_SPI_CSN_MASK;
+		writel_relaxed(val, ss->base + SPRD_SPI_CTL0);
+	}
+
+	pm_runtime_mark_last_busy(ss->dev);
+	pm_runtime_put_autosuspend(ss->dev);
+}
+
+static int sprd_spi_write_only_receive(struct sprd_spi *ss, u32 len)
+{
+	u32 val;
+
+	/* Clear the start receive bit and reset receive data number */
+	val = readl_relaxed(ss->base + SPRD_SPI_CTL4);
+	val &= ~(SPRD_SPI_START_RX | SPRD_SPI_ONLY_RECV_MASK);
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL4);
+
+	/* Set the receive data length */
+	val = readl_relaxed(ss->base + SPRD_SPI_CTL4);
+	val |= len & SPRD_SPI_ONLY_RECV_MASK;
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL4);
+
+	/* Trigger to receive data */
+	val = readl_relaxed(ss->base + SPRD_SPI_CTL4);
+	val |= SPRD_SPI_START_RX;
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL4);
+
+	return len;
+}
+
+static int sprd_spi_write_bufs_u8(struct sprd_spi *ss, u32 len)
+{
+	u8 *tx_p = (u8 *)ss->tx_buf;
+	int i;
+
+	for (i = 0; i < len; i++)
+		writeb_relaxed(tx_p[i], ss->base + SPRD_SPI_TXD);
+
+	ss->tx_buf += i;
+	return i;
+}
+
+static int sprd_spi_write_bufs_u16(struct sprd_spi *ss, u32 len)
+{
+	u16 *tx_p = (u16 *)ss->tx_buf;
+	int i;
+
+	for (i = 0; i < len; i++)
+		writew_relaxed(tx_p[i], ss->base + SPRD_SPI_TXD);
+
+	ss->tx_buf += i << 1;
+	return i << 1;
+}
+
+static int sprd_spi_write_bufs_u32(struct sprd_spi *ss, u32 len)
+{
+	u32 *tx_p = (u32 *)ss->tx_buf;
+	int i;
+
+	for (i = 0; i < len; i++)
+		writel_relaxed(tx_p[i], ss->base + SPRD_SPI_TXD);
+
+	ss->tx_buf += i << 2;
+	return i << 2;
+}
+
+static int sprd_spi_read_bufs_u8(struct sprd_spi *ss, u32 len)
+{
+	u8 *rx_p = (u8 *)ss->rx_buf;
+	int i;
+
+	for (i = 0; i < len; i++)
+		rx_p[i] = readb_relaxed(ss->base + SPRD_SPI_TXD);
+
+	ss->rx_buf += i;
+	return i;
+}
+
+static int sprd_spi_read_bufs_u16(struct sprd_spi *ss, u32 len)
+{
+	u16 *rx_p = (u16 *)ss->rx_buf;
+	int i;
+
+	for (i = 0; i < len; i++)
+		rx_p[i] = readw_relaxed(ss->base + SPRD_SPI_TXD);
+
+	ss->rx_buf += i << 1;
+	return i << 1;
+}
+
+static int sprd_spi_read_bufs_u32(struct sprd_spi *ss, u32 len)
+{
+	u32 *rx_p = (u32 *)ss->rx_buf;
+	int i;
+
+	for (i = 0; i < len; i++)
+		rx_p[i] = readl_relaxed(ss->base + SPRD_SPI_TXD);
+
+	ss->rx_buf += i << 2;
+	return i << 2;
+}
+
+static int sprd_spi_txrx_bufs(struct spi_device *sdev, struct spi_transfer *t)
+{
+	struct sprd_spi *ss = spi_controller_get_devdata(sdev->controller);
+	u32 trans_len = ss->trans_len, len;
+	int ret, write_size = 0;
+
+	while (trans_len) {
+		len = trans_len > SPRD_SPI_FIFO_SIZE ? SPRD_SPI_FIFO_SIZE :
+			trans_len;
+		if (ss->trans_mode & SPRD_SPI_TX_MODE) {
+			sprd_spi_set_tx_length(ss, len);
+			write_size += ss->write_bufs(ss, len);
+
+			/*
+			 * For our 3 wires mode or dual TX line mode, we need
+			 * to request the controller to transfer.
+			 */
+			if (ss->hw_mode & SPI_3WIRE || ss->hw_mode & SPI_TX_DUAL)
+				sprd_spi_tx_req(ss);
+
+			ret = sprd_spi_wait_for_tx_end(ss, t);
+		} else {
+			sprd_spi_set_rx_length(ss, len);
+
+			/*
+			 * For our 3 wires mode or dual TX line mode, we need
+			 * to request the controller to read.
+			 */
+			if (ss->hw_mode & SPI_3WIRE || ss->hw_mode & SPI_TX_DUAL)
+				sprd_spi_rx_req(ss);
+			else
+				write_size += ss->write_bufs(ss, len);
+
+			ret = sprd_spi_wait_for_rx_end(ss, t);
+		}
+
+		if (ret)
+			goto complete;
+
+		if (ss->trans_mode & SPRD_SPI_RX_MODE)
+			ss->read_bufs(ss, len);
+
+		trans_len -= len;
+	}
+
+	ret = write_size;
+
+complete:
+	sprd_spi_enter_idle(ss);
+
+	return ret;
+}
+
+static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz)
+{
+	/*
+	 * From SPI datasheet, the prescale calculation formula:
+	 * prescale = SPI source clock / (2 * SPI_freq) - 1;
+	 */
+	u32 clk_div = ss->src_clk / (speed_hz << 1) - 1;
+
+	writel_relaxed(clk_div, ss->base + SPRD_SPI_CLKD);
+}
+
+static int sprd_spi_setup_transfer(struct spi_device *sdev,
+				   struct spi_transfer *t)
+{
+	struct sprd_spi *ss = spi_controller_get_devdata(sdev->controller);
+	u8 bits_per_word = t->bits_per_word;
+	u32 val, mode = 0;
+
+	ss->len = t->len;
+	ss->tx_buf = t->tx_buf;
+	ss->rx_buf = t->rx_buf;
+
+	/* Set tansfer speed and valid bits */
+	sprd_spi_set_speed(ss, t->speed_hz);
+	sprd_spi_set_transfer_bits(ss, bits_per_word);
+
+	bits_per_word = bits_per_word > 16 ? round_up(bits_per_word, 16) :
+		round_up(bits_per_word, 8);
+	switch (bits_per_word) {
+	case 8:
+		ss->trans_len = t->len;
+		ss->read_bufs = sprd_spi_read_bufs_u8;
+		ss->write_bufs = sprd_spi_write_bufs_u8;
+		break;
+	case 16:
+		ss->trans_len = t->len >> 1;
+		ss->read_bufs = sprd_spi_read_bufs_u16;
+		ss->write_bufs = sprd_spi_write_bufs_u16;
+		break;
+	case 32:
+		ss->trans_len = t->len >> 2;
+		ss->read_bufs = sprd_spi_read_bufs_u32;
+		ss->write_bufs = sprd_spi_write_bufs_u32;
+		break;
+	}
+
+	/* Set transfer read or write mode */
+	val = readl_relaxed(ss->base + SPRD_SPI_CTL1);
+	val &= ~SPRD_SPI_RTX_MD_MASK;
+	if (t->tx_buf)
+		mode |= SPRD_SPI_TX_MODE;
+	if (t->rx_buf)
+		mode |= SPRD_SPI_RX_MODE;
+
+	writel_relaxed(val | mode, ss->base + SPRD_SPI_CTL1);
+
+	ss->trans_mode = mode;
+
+	/*
+	 * If in only receive mode, we need to trigger the SPI controller to
+	 * receive data automatically.
+	 */
+	if (ss->trans_mode == SPRD_SPI_RX_MODE)
+		ss->write_bufs = sprd_spi_write_only_receive;
+
+	return 0;
+}
+
+static int sprd_spi_transfer_one(struct spi_controller *sctlr,
+				 struct spi_device *sdev,
+				 struct spi_transfer *t)
+{
+	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
+	int ret;
+
+	ret = pm_runtime_get_sync(ss->dev);
+	if (ret < 0) {
+		dev_err(ss->dev, "failed to resume SPI controller\n");
+		goto rpm_err;
+	}
+
+	ret = sprd_spi_setup_transfer(sdev, t);
+	if (ret)
+		goto setup_err;
+
+	ret = sprd_spi_txrx_bufs(sdev, t);
+	if (ret == t->len)
+		ret = 0;
+	else if (ret >= 0)
+		ret = -EREMOTEIO;
+
+setup_err:
+	pm_runtime_mark_last_busy(ss->dev);
+	pm_runtime_put_autosuspend(ss->dev);
+rpm_err:
+	spi_finalize_current_transfer(sctlr);
+
+	return ret;
+}
+
+static void sprd_spi_init_hw(struct sprd_spi *ss)
+{
+	u32 val = SPRD_SPI_CSN_MASK;
+
+	/* Set default chip selection, clock phase and clock polarity */
+	val |= ss->hw_mode & SPI_CPHA ? SPRD_SPI_NG_RX : SPRD_SPI_NG_TX;
+	val |= ss->hw_mode & SPI_CPOL ? SPRD_SPI_SCK_REV : 0;
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL0);
+
+	/* Set the intervals of two SPI frames */
+	writel_relaxed(ss->interval, ss->base + SPRD_SPI_CTL5);
+
+	/* Reset SPI fifo */
+	writel_relaxed(1, ss->base + SPRD_SPI_FIFO_RST);
+	writel_relaxed(0, ss->base + SPRD_SPI_FIFO_RST);
+
+	/* Set SPI work mode */
+	val = readl_relaxed(ss->base + SPRD_SPI_CTL7);
+	val &= ~SPRD_SPI_MODE_MASK;
+
+	if (ss->hw_mode & SPI_3WIRE)
+		val |= SPRD_SPI_3WIRE_MODE << SPRD_SPI_MODE_OFFSET;
+	else
+		val |= SPRD_SPI_4WIRE_MODE << SPRD_SPI_MODE_OFFSET;
+
+	if (ss->hw_mode & SPI_TX_DUAL)
+		val |= SPRD_SPI_DATA_LINE2_EN;
+	else
+		val &= ~SPRD_SPI_DATA_LINE2_EN;
+
+	writel_relaxed(val, ss->base + SPRD_SPI_CTL7);
+}
+
+static int sprd_spi_setup(struct spi_device *spi_dev)
+{
+	struct spi_controller *sctlr = spi_dev->controller;
+	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
+	int ret;
+
+	ret = pm_runtime_get_sync(ss->dev);
+	if (ret < 0) {
+		dev_err(ss->dev, "failed to resume SPI controller\n");
+		return ret;
+	}
+
+	ss->hw_mode = spi_dev->mode;
+	sprd_spi_init_hw(ss);
+
+	pm_runtime_mark_last_busy(ss->dev);
+	pm_runtime_put_autosuspend(ss->dev);
+
+	return 0;
+}
+
+static int sprd_spi_clk_init(struct platform_device *pdev, struct sprd_spi *ss)
+{
+	struct clk *clk_spi, *clk_parent;
+
+	clk_spi = devm_clk_get(&pdev->dev, "spi");
+	if (IS_ERR(clk_spi)) {
+		dev_warn(&pdev->dev, "can't get the spi clock\n");
+		clk_spi = NULL;
+	}
+
+	clk_parent = devm_clk_get(&pdev->dev, "source");
+	if (IS_ERR(clk_parent)) {
+		dev_warn(&pdev->dev, "can't get the source clock\n");
+		clk_parent = NULL;
+	}
+
+	ss->clk = devm_clk_get(&pdev->dev, "enable");
+	if (IS_ERR(ss->clk)) {
+		dev_err(&pdev->dev, "can't get the enable clock\n");
+		return PTR_ERR(ss->clk);
+	}
+
+	if (!clk_set_parent(clk_spi, clk_parent))
+		ss->src_clk = clk_get_rate(clk_spi);
+	else
+		ss->src_clk = SPRD_SPI_DEFAULT_SOURCE;
+
+	return 0;
+}
+
+static int sprd_spi_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct spi_controller *sctlr;
+	struct resource *res;
+	struct sprd_spi *ss;
+	int ret;
+
+	pdev->id = of_alias_get_id(pdev->dev.of_node, "spi");
+	sctlr = spi_alloc_master(&pdev->dev, sizeof(*ss));
+	if (!sctlr)
+		return -ENOMEM;
+
+	ss = spi_controller_get_devdata(sctlr);
+	if (of_property_read_u32(np, "sprd,spi-interval", &ss->interval))
+		ss->interval = SPRD_SPI_ITVL_NUM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ss->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ss->base)) {
+		ret = PTR_ERR(ss->base);
+		goto free_controller;
+	}
+
+	ss->dev = &pdev->dev;
+	sctlr->dev.of_node = pdev->dev.of_node;
+	sctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | SPI_TX_DUAL;
+	sctlr->bus_num = pdev->id;
+	sctlr->setup = sprd_spi_setup;
+	sctlr->set_cs = sprd_spi_chipselect;
+	sctlr->transfer_one = sprd_spi_transfer_one;
+	sctlr->max_speed_hz = (ss->src_clk / 2) < SPRD_SPI_MAX_SPEED_HZ ?
+		ss->src_clk / 2 : SPRD_SPI_MAX_SPEED_HZ;
+
+	platform_set_drvdata(pdev, sctlr);
+	ret = sprd_spi_clk_init(pdev, ss);
+	if (ret)
+		goto free_controller;
+
+	ret = clk_prepare_enable(ss->clk);
+	if (ret)
+		goto free_controller;
+
+	ret = pm_runtime_set_active(&pdev->dev);
+	if (ret < 0)
+		goto disable_clk;
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev,
+					 SPRD_SPI_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to resume SPI controller\n");
+		goto err_rpm_put;
+	}
+
+	ret = devm_spi_register_controller(&pdev->dev, sctlr);
+	if (ret)
+		goto err_rpm_put;
+
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
+
+	return 0;
+
+err_rpm_put:
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+disable_clk:
+	clk_disable_unprepare(ss->clk);
+free_controller:
+	spi_controller_put(sctlr);
+
+	return ret;
+}
+
+static int __exit sprd_spi_remove(struct platform_device *pdev)
+{
+	struct spi_controller *sctlr = platform_get_drvdata(pdev);
+	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
+	int ret;
+
+	ret = pm_runtime_get_sync(ss->dev);
+	if (ret < 0) {
+		dev_err(ss->dev, "failed to resume SPI controller\n");
+		return ret;
+	}
+
+	clk_disable_unprepare(ss->clk);
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static int __maybe_unused sprd_spi_runtime_suspend(struct device *dev)
+{
+	struct spi_controller *sctlr = dev_get_drvdata(dev);
+	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
+
+	clk_disable_unprepare(ss->clk);
+
+	return 0;
+}
+
+static int __maybe_unused sprd_spi_runtime_resume(struct device *dev)
+{
+	struct spi_controller *sctlr = dev_get_drvdata(dev);
+	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
+	int ret;
+
+	ret = clk_prepare_enable(ss->clk);
+	if (ret)
+		return ret;
+
+	sprd_spi_init_hw(ss);
+	return 0;
+}
+
+static const struct dev_pm_ops sprd_spi_pm_ops = {
+	SET_RUNTIME_PM_OPS(sprd_spi_runtime_suspend,
+			   sprd_spi_runtime_resume, NULL)
+};
+
+static const struct of_device_id sprd_spi_of_match[] = {
+	{ .compatible = "sprd,sc9860-spi", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver sprd_spi_driver = {
+	.driver = {
+		.name = "sprd-spi",
+		.of_match_table = sprd_spi_of_match,
+		.pm = &sprd_spi_pm_ops,
+	},
+	.probe = sprd_spi_probe,
+	.remove  = sprd_spi_remove,
+};
+
+module_platform_driver(sprd_spi_driver);
+
+MODULE_DESCRIPTION("Spreadtrum SPI Controller driver");
+MODULE_AUTHOR("Lanqing Liu <lanqing.liu@spreadtrum.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Mark Brown]

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

On Tue, Aug 07, 2018 at 06:43:37PM +0800, Baolin Wang wrote:

> +Optional properties:
> +- sprd,spi-interval: Specify the intervals of two SPI frames, which can be
> +	converted to the delay clock cycles = interval number * 4 + 10.

What's a frame here, and does it overlap with any of the existing delay
configuration we have?  In general it's better for this stuff to be
configured at runtime by the device rather than at DT time by the
controller since that way if the device needs the delays we always do
them if we can and if they are only needed some of the time (eg, for
only one device on the bus or for only some operations) then we don't
take the performance hit when we don't need to.

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

Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860

[Mark Brown]

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

On Tue, Aug 07, 2018 at 06:43:38PM +0800, Baolin Wang wrote:
> From: Lanqing Liu <lanqing.liu@spreadtrum.com>
> 
> This patch adds the SPI controller driver for Spreadtrum SC9860 platform.

This all looks pretty clean, a few comments below but nothing too major:

> +static void sprd_spi_chipselect(struct spi_device *sdev, bool cs)
> +{
> +	struct spi_controller *sctlr = sdev->controller;
> +	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
> +	u32 val;
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(ss->dev);
> +	if (ret < 0) {
> +		dev_err(ss->dev, "failed to resume SPI controller\n");
> +		return;
> +	}

Something else further up the stack should probably have runtime PM
enabled already - we should only be changing chip select as part of a
bigger operation.  If you use the core auto_runtime_pm feature this will
definitely happen.

> +	bits_per_word = bits_per_word > 16 ? round_up(bits_per_word, 16) :
> +		round_up(bits_per_word, 8);

Please

> +	switch (bits_per_word) {
> +	case 8:
> +	case 16:
> +	case 32:

It'd be nice to have a default case, the core should check for you but
it's nice to have defensive programming here.

> +static int sprd_spi_transfer_one(struct spi_controller *sctlr,
> +				 struct spi_device *sdev,
> +				 struct spi_transfer *t)
> +{
> +	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(ss->dev);
> +	if (ret < 0) {
> +		dev_err(ss->dev, "failed to resume SPI controller\n");
> +		goto rpm_err;
> +	}

Same thing with runtime PM here - the core can do this for you.

> +static int sprd_spi_setup(struct spi_device *spi_dev)
> +{
> +	struct spi_controller *sctlr = spi_dev->controller;
> +	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(ss->dev);
> +	if (ret < 0) {
> +		dev_err(ss->dev, "failed to resume SPI controller\n");
> +		return ret;
> +	}
> +
> +	ss->hw_mode = spi_dev->mode;
> +	sprd_spi_init_hw(ss);

This can be called for one chip select while another is in progress so
it's not good to actually configure the hardware here unless the
configuration is in a chip select specific set of registers.  Instead
you should defer to when the transfer is being set up.

> +static int sprd_spi_clk_init(struct platform_device *pdev, struct sprd_spi *ss)
> +{
> +	struct clk *clk_spi, *clk_parent;
> +
> +	clk_spi = devm_clk_get(&pdev->dev, "spi");
> +	if (IS_ERR(clk_spi)) {
> +		dev_warn(&pdev->dev, "can't get the spi clock\n");
> +		clk_spi = NULL;
> +	}

I suspect some of these clocks are essential and you should probably
return an error if you can't get them (especially if probe deferral
becomes a factor).

> +	if (!clk_set_parent(clk_spi, clk_parent))
> +		ss->src_clk = clk_get_rate(clk_spi);
> +	else
> +		ss->src_clk = SPRD_SPI_DEFAULT_SOURCE;

Are upstream DTs going to be missing the clock setup needed here?

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

Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860

[Trent Piepho]

On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote:
> 
> +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss,
> +					 struct spi_transfer *t)
> +{
> +	/*
> +	 * The time spent on transmission of the full FIFO data is the maximum
> +	 * SPI transmission time.
> +	 */
> +	u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE;
> +	u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1;
> +	u32 total_time_us = size * bit_time_us;
> +	/*
> +	 * There is an interval between data and the data in our SPI hardware,
> +	 * so the total transmission time need add the interval time.
> +	 *
> +	 * The inteval calculation formula:
> +	 * interval time (source clock cycles) = interval * 4 + 10.
> +	 */
> +	u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) + 10);
> +	u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk + 1;

If interval is greater than 31, this will overflow.

Also SPRD_SPI_HZ is not the speed anything runs at, as one might think
from the name.  It's the number of microseconds in a second.  The is
already a Linux macro for that, USEC_PER_SEC, that you should use
instead.

> +
> +	return total_time_us + interval_time_us;
> +}
> +


> +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz)
> +{
> +	/*
> +	 * From SPI datasheet, the prescale calculation formula:
> +	 * prescale = SPI source clock / (2 * SPI_freq) - 1;
> +	 */
> +	u32 clk_div = ss->src_clk / (speed_hz << 1) - 1;

You should probably round up here.  The convention is to use the
closest speed less than equal to the requested speed, but since this is
a divider, rounding it down will select the next highest speed.

> +
> +	writel_relaxed(clk_div, ss->base + SPRD_SPI_CLKD);
> +}
> +

> +
> +static int sprd_spi_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct spi_controller *sctlr;
> +	struct resource *res;
> +	struct sprd_spi *ss;
> +	int ret;
> +
> +	pdev->id = of_alias_get_id(pdev->dev.of_node, "spi");
> +	sctlr = spi_alloc_master(&pdev->dev, sizeof(*ss));
> +	if (!sctlr)
> +		return -ENOMEM;
> +
> +	ss = spi_controller_get_devdata(sctlr);
> +	if (of_property_read_u32(np, "sprd,spi-interval", &ss->interval))
> +		ss->interval = SPRD_SPI_ITVL_NUM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ss->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ss->base)) {
> +		ret = PTR_ERR(ss->base);
> +		goto free_controller;
> +	}
> +
> +	ss->dev = &pdev->dev;
> +	sctlr->dev.of_node = pdev->dev.of_node;
> +	sctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | SPI_TX_DUAL;
> +	sctlr->bus_num = pdev->id;
> +	sctlr->setup = sprd_spi_setup;
> +	sctlr->set_cs = sprd_spi_chipselect;
> +	sctlr->transfer_one = sprd_spi_transfer_one;
> +	sctlr->max_speed_hz = (ss->src_clk / 2) < SPRD_SPI_MAX_SPEED_HZ ?
> +		ss->src_clk / 2 : SPRD_SPI_MAX_SPEED_HZ;

You can write this as:
        sctlr->max_speed_hz = min(ss->src_clk / 2, SPRD_SPI_MAX_SPEED_HZ);

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Baolin Wang]

Hi Mark,

On 7 August 2018 at 21:41, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Aug 07, 2018 at 06:43:37PM +0800, Baolin Wang wrote:
>
>> +Optional properties:
>> +- sprd,spi-interval: Specify the intervals of two SPI frames, which can be
>> +     converted to the delay clock cycles = interval number * 4 + 10.
>
> What's a frame here, and does it overlap with any of the existing delay
> configuration we have?  In general it's better for this stuff to be
> configured at runtime by the device rather than at DT time by the
> controller since that way if the device needs the delays we always do
> them if we can and if they are only needed some of the time (eg, for
> only one device on the bus or for only some operations) then we don't
> take the performance hit when we don't need to.

Sorry for confusing. Let me try to explain it explicitly.
We can set the word size (bits_per_word) for each transmission, for
our SPI controller,  after every word size transmission, we need one
interval time (hardware automatically) to make sure the slave has
enough time to receive the whole data.

Yes, I agree we should configure it at runtime by the device, but we
did not find one member to use in 'struct spi_transfer', we just find
one similar 'delay_usecs' member in 'struct spi_transfer' but not
same. We can use  'delay_usecs' to set our hardware interval value,
but we should clean it when transfer is done, since we do not need to
delay after the transfer in spi_transfer_one _message(). Or can we add
one new member maybe named 'word_interval' to indicate the interval
time between word size transmission?

What do you prefer or other better solution? Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860

[Baolin Wang]

Hi Mark,

On 7 August 2018 at 22:24, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Aug 07, 2018 at 06:43:38PM +0800, Baolin Wang wrote:
>> From: Lanqing Liu <lanqing.liu@spreadtrum.com>
>>
>> This patch adds the SPI controller driver for Spreadtrum SC9860 platform.
>
> This all looks pretty clean, a few comments below but nothing too major:
>
>> +static void sprd_spi_chipselect(struct spi_device *sdev, bool cs)
>> +{
>> +     struct spi_controller *sctlr = sdev->controller;
>> +     struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
>> +     u32 val;
>> +     int ret;
>> +
>> +     ret = pm_runtime_get_sync(ss->dev);
>> +     if (ret < 0) {
>> +             dev_err(ss->dev, "failed to resume SPI controller\n");
>> +             return;
>> +     }
>
> Something else further up the stack should probably have runtime PM
> enabled already - we should only be changing chip select as part of a
> bigger operation.  If you use the core auto_runtime_pm feature this will
> definitely happen.

Indeed, will use auto_runtime_pm.

>
>> +     bits_per_word = bits_per_word > 16 ? round_up(bits_per_word, 16) :
>> +             round_up(bits_per_word, 8);
>
> Please

Sorry I did not get your points here, could you elaborate on?

>
>> +     switch (bits_per_word) {
>> +     case 8:
>> +     case 16:
>> +     case 32:
>
> It'd be nice to have a default case, the core should check for you but
> it's nice to have defensive programming here.

Sure.

>
>> +static int sprd_spi_transfer_one(struct spi_controller *sctlr,
>> +                              struct spi_device *sdev,
>> +                              struct spi_transfer *t)
>> +{
>> +     struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
>> +     int ret;
>> +
>> +     ret = pm_runtime_get_sync(ss->dev);
>> +     if (ret < 0) {
>> +             dev_err(ss->dev, "failed to resume SPI controller\n");
>> +             goto rpm_err;
>> +     }
>
> Same thing with runtime PM here - the core can do this for you.

Yes.

>
>> +static int sprd_spi_setup(struct spi_device *spi_dev)
>> +{
>> +     struct spi_controller *sctlr = spi_dev->controller;
>> +     struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
>> +     int ret;
>> +
>> +     ret = pm_runtime_get_sync(ss->dev);
>> +     if (ret < 0) {
>> +             dev_err(ss->dev, "failed to resume SPI controller\n");
>> +             return ret;
>> +     }
>> +
>> +     ss->hw_mode = spi_dev->mode;
>> +     sprd_spi_init_hw(ss);
>
> This can be called for one chip select while another is in progress so
> it's not good to actually configure the hardware here unless the
> configuration is in a chip select specific set of registers.  Instead
> you should defer to when the transfer is being set up.

You are right, will move these into transfer setup.

>
>> +static int sprd_spi_clk_init(struct platform_device *pdev, struct sprd_spi *ss)
>> +{
>> +     struct clk *clk_spi, *clk_parent;
>> +
>> +     clk_spi = devm_clk_get(&pdev->dev, "spi");
>> +     if (IS_ERR(clk_spi)) {
>> +             dev_warn(&pdev->dev, "can't get the spi clock\n");
>> +             clk_spi = NULL;
>> +     }
>
> I suspect some of these clocks are essential and you should probably
> return an error if you can't get them (especially if probe deferral
> becomes a factor).

The 'spi' and 'source' clock can be optional, and the SPI can work at
default source clock without setting 'spi' and 'source' clock. This is
used on our FPGA board which has not set the spi source clock, but
still make the SPI can work. So here we just give a warning.

>> +     if (!clk_set_parent(clk_spi, clk_parent))
>> +             ss->src_clk = clk_get_rate(clk_spi);
>> +     else
>> +             ss->src_clk = SPRD_SPI_DEFAULT_SOURCE;
>
> Are upstream DTs going to be missing the clock setup needed here?

Right. DTs can not set 'spi' and 'source' clock to make SPI work at
default clock. Thanks for your useful comments.

-- 
Baolin Wang
Best Regards

Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860

[Baolin Wang]

Hi Trent,

On 8 August 2018 at 01:10, Trent Piepho <tpiepho@impinj.com> wrote:
> On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote:
>>
>> +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss,
>> +                                      struct spi_transfer *t)
>> +{
>> +     /*
>> +      * The time spent on transmission of the full FIFO data is the maximum
>> +      * SPI transmission time.
>> +      */
>> +     u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE;
>> +     u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1;
>> +     u32 total_time_us = size * bit_time_us;
>> +     /*
>> +      * There is an interval between data and the data in our SPI hardware,
>> +      * so the total transmission time need add the interval time.
>> +      *
>> +      * The inteval calculation formula:
>> +      * interval time (source clock cycles) = interval * 4 + 10.
>> +      */
>> +     u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) + 10);
>> +     u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk + 1;
>
> If interval is greater than 31, this will overflow.

Usually we will not set such a big value for interval,  but this is a
risk like you said. So we will check and limit the interval value to
make sure the formula will not overflow.

>
> Also SPRD_SPI_HZ is not the speed anything runs at, as one might think
> from the name.  It's the number of microseconds in a second.  The is
> already a Linux macro for that, USEC_PER_SEC, that you should use
> instead.

Right, will use USEC_PER_SEC instead.

>
>> +
>> +     return total_time_us + interval_time_us;
>> +}
>> +
>
>
>> +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz)
>> +{
>> +     /*
>> +      * From SPI datasheet, the prescale calculation formula:
>> +      * prescale = SPI source clock / (2 * SPI_freq) - 1;
>> +      */
>> +     u32 clk_div = ss->src_clk / (speed_hz << 1) - 1;
>
> You should probably round up here.  The convention is to use the
> closest speed less than equal to the requested speed, but since this is
> a divider, rounding it down will select the next highest speed.

Per the datasheet, we do not need round up/down the speed. Since our
hardware can handle the clock calculated by the above formula if the
requested speed is in the normal region (less than ->max_speed_hz).

>> +
>> +     writel_relaxed(clk_div, ss->base + SPRD_SPI_CLKD);
>> +}
>> +
>
>> +
>> +static int sprd_spi_probe(struct platform_device *pdev)
>> +{
>> +     struct device_node *np = pdev->dev.of_node;
>> +     struct spi_controller *sctlr;
>> +     struct resource *res;
>> +     struct sprd_spi *ss;
>> +     int ret;
>> +
>> +     pdev->id = of_alias_get_id(pdev->dev.of_node, "spi");
>> +     sctlr = spi_alloc_master(&pdev->dev, sizeof(*ss));
>> +     if (!sctlr)
>> +             return -ENOMEM;
>> +
>> +     ss = spi_controller_get_devdata(sctlr);
>> +     if (of_property_read_u32(np, "sprd,spi-interval", &ss->interval))
>> +             ss->interval = SPRD_SPI_ITVL_NUM;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     ss->base = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(ss->base)) {
>> +             ret = PTR_ERR(ss->base);
>> +             goto free_controller;
>> +     }
>> +
>> +     ss->dev = &pdev->dev;
>> +     sctlr->dev.of_node = pdev->dev.of_node;
>> +     sctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | SPI_TX_DUAL;
>> +     sctlr->bus_num = pdev->id;
>> +     sctlr->setup = sprd_spi_setup;
>> +     sctlr->set_cs = sprd_spi_chipselect;
>> +     sctlr->transfer_one = sprd_spi_transfer_one;
>> +     sctlr->max_speed_hz = (ss->src_clk / 2) < SPRD_SPI_MAX_SPEED_HZ ?
>> +             ss->src_clk / 2 : SPRD_SPI_MAX_SPEED_HZ;
>
> You can write this as:
>         sctlr->max_speed_hz = min(ss->src_clk / 2, SPRD_SPI_MAX_SPEED_HZ);

Right. Thanks for your comments.

-- 
Baolin Wang
Best Regards

Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860

[Mark Brown]

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

On Wed, Aug 08, 2018 at 10:45:33AM +0800, Baolin Wang wrote:
> On 7 August 2018 at 22:24, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Aug 07, 2018 at 06:43:38PM +0800, Baolin Wang wrote:

> >> +     bits_per_word = bits_per_word > 16 ? round_up(bits_per_word, 16) :
> >> +             round_up(bits_per_word, 8);

> > Please

> Sorry I did not get your points here, could you elaborate on?

Sorry, missed the actual comment there - use normal if statements rather
than the ternery operator, it's easier to read.

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

Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860

[Baolin Wang]

On 8 August 2018 at 17:31, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 08, 2018 at 10:45:33AM +0800, Baolin Wang wrote:
>> On 7 August 2018 at 22:24, Mark Brown <broonie@kernel.org> wrote:
>> > On Tue, Aug 07, 2018 at 06:43:38PM +0800, Baolin Wang wrote:
>
>> >> +     bits_per_word = bits_per_word > 16 ? round_up(bits_per_word, 16) :
>> >> +             round_up(bits_per_word, 8);
>
>> > Please
>
>> Sorry I did not get your points here, could you elaborate on?
>
> Sorry, missed the actual comment there - use normal if statements rather
> than the ternery operator, it's easier to read.

Got it. Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Mark Brown]

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

On Wed, Aug 08, 2018 at 10:26:42AM +0800, Baolin Wang wrote:

> Sorry for confusing. Let me try to explain it explicitly.
> We can set the word size (bits_per_word) for each transmission, for
> our SPI controller,  after every word size transmission, we need one
> interval time (hardware automatically) to make sure the slave has
> enough time to receive the whole data.

OK, so it's an inter word delay.  Some other controllers definitely have
the same feature.

> Yes, I agree we should configure it at runtime by the device, but we
> did not find one member to use in 'struct spi_transfer', we just find
> one similar 'delay_usecs' member in 'struct spi_transfer' but not
> same. We can use  'delay_usecs' to set our hardware interval value,
> but we should clean it when transfer is done, since we do not need to
> delay after the transfer in spi_transfer_one _message(). Or can we add
> one new member maybe named 'word_interval' to indicate the interval
> time between word size transmission?

Right, I don't think we added this yet (if we did I can't see it).  I'd
add a new field to spi_transfer for this, then other controllers with
the same support can implement it as well and drivers can start using
it too.

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

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Baolin Wang]

On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 08, 2018 at 10:26:42AM +0800, Baolin Wang wrote:
>
>> Sorry for confusing. Let me try to explain it explicitly.
>> We can set the word size (bits_per_word) for each transmission, for
>> our SPI controller,  after every word size transmission, we need one
>> interval time (hardware automatically) to make sure the slave has
>> enough time to receive the whole data.
>
> OK, so it's an inter word delay.  Some other controllers definitely have
> the same feature.
>
>> Yes, I agree we should configure it at runtime by the device, but we
>> did not find one member to use in 'struct spi_transfer', we just find
>> one similar 'delay_usecs' member in 'struct spi_transfer' but not
>> same. We can use  'delay_usecs' to set our hardware interval value,
>> but we should clean it when transfer is done, since we do not need to
>> delay after the transfer in spi_transfer_one _message(). Or can we add
>> one new member maybe named 'word_interval' to indicate the interval
>> time between word size transmission?
>
> Right, I don't think we added this yet (if we did I can't see it).  I'd
> add a new field to spi_transfer for this, then other controllers with
> the same support can implement it as well and drivers can start using
> it too.

OK. So I will name the new filed as 'word_delay', is it OK for you?

-- 
Baolin Wang
Best Regards

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Mark Brown]

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

On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote:
> On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote:

> > Right, I don't think we added this yet (if we did I can't see it).  I'd
> > add a new field to spi_transfer for this, then other controllers with
> > the same support can implement it as well and drivers can start using
> > it too.

> OK. So I will name the new filed as 'word_delay', is it OK for you?

Sounds good, yes.

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

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Baolin Wang]

On 8 August 2018 at 18:54, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote:
>> On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote:
>
>> > Right, I don't think we added this yet (if we did I can't see it).  I'd
>> > add a new field to spi_transfer for this, then other controllers with
>> > the same support can implement it as well and drivers can start using
>> > it too.
>
>> OK. So I will name the new filed as 'word_delay', is it OK for you?
>
> Sounds good, yes.

OK. Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Trent Piepho]

On Wed, 2018-08-08 at 11:54 +0100, Mark Brown wrote:
> On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote:
> > On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote:
> > > Right, I don't think we added this yet (if we did I can't see
> > > it).  I'd
> > > add a new field to spi_transfer for this, then other controllers
> > > with
> > > the same support can implement it as well and drivers can start
> > > using
> > > it too.
> > OK. So I will name the new filed as 'word_delay', is it OK for you?
> 
> Sounds good, yes.

Should it be in µs like the existing iter-transfer delay?  I think
perhaps units of cycles of the SPI clock make more sense?

Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860

[Trent Piepho]

On Wed, 2018-08-08 at 11:19 +0800, Baolin Wang wrote:
> On 8 August 2018 at 01:10, Trent Piepho <tpiepho@impinj.com> wrote:
> > On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote:
> > > 
> > > +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss,
> > > +                                      struct spi_transfer *t)
> > > +{
> > > +     /*
> > > +      * The time spent on transmission of the full FIFO data is the maximum
> > > +      * SPI transmission time.
> > > +      */
> > > +     u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE;
> > > +     u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1;

There's another flaw here in that the transfer speed, t->speed_hz,
might not be exactly what is used, due to limitations of the clock
divider.  It would be better to find the actual SPI clock used, then
use that in the calculations.

> > > +     u32 total_time_us = size * bit_time_us;
> > > +     /*
> > > +      * There is an interval between data and the data in our SPI hardware,
> > > +      * so the total transmission time need add the interval time.
> > > +      *
> > > +      * The inteval calculation formula:
> > > +      * interval time (source clock cycles) = interval * 4 + 10.
> > > +      */
> > > +     u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) + 10);
> > > +     u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk + 1;
> > 
> > If interval is greater than 31, this will overflow.
> 
> Usually we will not set such a big value for interval,  but this is a
> risk like you said. So we will check and limit the interval value to
> make sure the formula will not overflow.
> 

Better would be to limit the inter word delay to whatever maximum value
your hardware supports, and then write code that can calculate that
without error.

> > > +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz)
> > > +{
> > > +     /*
> > > +      * From SPI datasheet, the prescale calculation formula:
> > > +      * prescale = SPI source clock / (2 * SPI_freq) - 1;
> > > +      */
> > > +     u32 clk_div = ss->src_clk / (speed_hz << 1) - 1;
> > 
> > You should probably round up here.  The convention is to use the
> > closest speed less than equal to the requested speed, but since this is
> > a divider, rounding it down will select the next highest speed.
> 
> Per the datasheet, we do not need round up/down the speed. Since our
> hardware can handle the clock calculated by the above formula if the
> requested speed is in the normal region (less than ->max_speed_hz).

That is not what I mean.  Let me explain differently.

An integer divider like this can not produce any frequency exactly. 
Consider if src_clk is 10 MHz.  A clk_div value of 0 produces a 5 MHz
SPI clock.  A clk_div value of 1 produces a 2.5 MHz SPI clock.

What if the transfer requests a SPI clock of 3 MHz?

Your math above will produce a SPI clock of 5 MHz, faster than
requested.  This is not the convention in Linux SPI masters.  You
should instead of have chosen a clk_div value of 1 to get a SPI clock
of 2.5 MHz, the closest clock possible that is not greater than the
requested value.

To do this, round up.

clk_div = DIV_ROUND_UP(ss->src_clk, speed_hz * 2) - 1;

> 

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Baolin Wang]

Hi Trent,

On 9 August 2018 at 02:57, Trent Piepho <tpiepho@impinj.com> wrote:
> On Wed, 2018-08-08 at 11:54 +0100, Mark Brown wrote:
>> On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote:
>> > On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote:
>> > > Right, I don't think we added this yet (if we did I can't see
>> > > it).  I'd
>> > > add a new field to spi_transfer for this, then other controllers
>> > > with
>> > > the same support can implement it as well and drivers can start
>> > > using
>> > > it too.
>> > OK. So I will name the new filed as 'word_delay', is it OK for you?
>>
>> Sounds good, yes.
>
> Should it be in µs like the existing iter-transfer delay?  I think
> perhaps units of cycles of the SPI clock make more sense?

Since some SPI controllers just want some interval values (neither µs
unit nor cycles unit ) set into hardware, and the hardware will
convert to the correct delay time automatically. So I did not force
'word_delay' unit as µs or cycle, and just let the slave devices
decide the unit which depends on the SPI hardware requirement.

-- 
Baolin Wang
Best Regards

Re: [PATCH 2/2] spi: sprd: Add SPI driver for Spreadtrum SC9860

[Baolin Wang]

Hi Trent,

On 9 August 2018 at 03:08, Trent Piepho <tpiepho@impinj.com> wrote:
> On Wed, 2018-08-08 at 11:19 +0800, Baolin Wang wrote:
>> On 8 August 2018 at 01:10, Trent Piepho <tpiepho@impinj.com> wrote:
>> > On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote:
>> > >
>> > > +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss,
>> > > +                                      struct spi_transfer *t)
>> > > +{
>> > > +     /*
>> > > +      * The time spent on transmission of the full FIFO data is the maximum
>> > > +      * SPI transmission time.
>> > > +      */
>> > > +     u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE;
>> > > +     u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1;
>
> There's another flaw here in that the transfer speed, t->speed_hz,
> might not be exactly what is used, due to limitations of the clock
> divider.  It would be better to find the actual SPI clock used, then
> use that in the calculations.

Right, will use the real speed to calculate the transfer time.

>
>> > > +     u32 total_time_us = size * bit_time_us;
>> > > +     /*
>> > > +      * There is an interval between data and the data in our SPI hardware,
>> > > +      * so the total transmission time need add the interval time.
>> > > +      *
>> > > +      * The inteval calculation formula:
>> > > +      * interval time (source clock cycles) = interval * 4 + 10.
>> > > +      */
>> > > +     u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) + 10);
>> > > +     u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk + 1;
>> >
>> > If interval is greater than 31, this will overflow.
>>
>> Usually we will not set such a big value for interval,  but this is a
>> risk like you said. So we will check and limit the interval value to
>> make sure the formula will not overflow.
>>
>
> Better would be to limit the inter word delay to whatever maximum value
> your hardware supports, and then write code that can calculate that
> without error.

Yes, will define the maximum word delay values to avoid overflow.

>
>> > > +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz)
>> > > +{
>> > > +     /*
>> > > +      * From SPI datasheet, the prescale calculation formula:
>> > > +      * prescale = SPI source clock / (2 * SPI_freq) - 1;
>> > > +      */
>> > > +     u32 clk_div = ss->src_clk / (speed_hz << 1) - 1;
>> >
>> > You should probably round up here.  The convention is to use the
>> > closest speed less than equal to the requested speed, but since this is
>> > a divider, rounding it down will select the next highest speed.
>>
>> Per the datasheet, we do not need round up/down the speed. Since our
>> hardware can handle the clock calculated by the above formula if the
>> requested speed is in the normal region (less than ->max_speed_hz).
>
> That is not what I mean.  Let me explain differently.
>
> An integer divider like this can not produce any frequency exactly.
> Consider if src_clk is 10 MHz.  A clk_div value of 0 produces a 5 MHz
> SPI clock.  A clk_div value of 1 produces a 2.5 MHz SPI clock.
>
> What if the transfer requests a SPI clock of 3 MHz?
>
> Your math above will produce a SPI clock of 5 MHz, faster than
> requested.  This is not the convention in Linux SPI masters.  You
> should instead of have chosen a clk_div value of 1 to get a SPI clock
> of 2.5 MHz, the closest clock possible that is not greater than the
> requested value.
>
> To do this, round up.
>
> clk_div = DIV_ROUND_UP(ss->src_clk, speed_hz * 2) - 1;

Thanks for your patient explanation. After talking with Lanqing who is
the author of the SPI driver, we think you are definitely correct and
will fix in next version according to your suggestion. Thanks a lot.

-- 
Baolin Wang
Best Regards

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Rob Herring]

On Tue, Aug 07, 2018 at 06:43:37PM +0800, Baolin Wang wrote:
> From: Lanqing Liu <lanqing.liu@spreadtrum.com>
> 
> This patch adds the binding documentation for Spreadtrum SPI
> controller device.
> 
> Signed-off-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-sprd.txt |   31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-sprd.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> new file mode 100644
> index 0000000..06ff746
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> @@ -0,0 +1,31 @@
> +Spreadtrum SPI Controller
> +
> +Required properties:
> +- compatible: Should be "sprd,sc9860-spi".
> +- reg: Offset and length of SPI controller register space.
> +- interrupts: Should contain SPI interrupt.
> +- clock-names: Should contain following entries:
> +	"spi" for SPI clock,
> +	"source" for SPI source (parent) clock,

Do the h/w block actually get this clock or the driver needs it? In the 
latter case, it should not be in DT.

> +	"enable" for SPI module enable clock.
> +- clocks: List of clock input name strings sorted in the same order
> +	as the clock-names property.
> +- #address-cells: The number of cells required to define a chip select
> +	address on the SPI bus. Should be set to 1.
> +- #size-cells: Should be set to 0.
> +
> +Optional properties:
> +- sprd,spi-interval: Specify the intervals of two SPI frames, which can be
> +	converted to the delay clock cycles = interval number * 4 + 10.

There are read and write delay properties you can use.

> +
> +Example:
> +spi0: spi@70a00000{
> +	compatible = "sprd,sc9860-spi";
> +	reg = <0 0x70a00000 0 0x1000>;
> +	interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +	clock-names = "spi", "source","enable";
> +	clocks = <&clk_spi0>, <&ext_26m>, <&clk_ap_apb_gates 5>;
> +	sprd,spi-interval = <9>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +};
> -- 
> 1.7.9.5
> 

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Rob Herring]

On Thu, Aug 09, 2018 at 11:03:11AM +0800, Baolin Wang wrote:
> Hi Trent,
> 
> On 9 August 2018 at 02:57, Trent Piepho <tpiepho@impinj.com> wrote:
> > On Wed, 2018-08-08 at 11:54 +0100, Mark Brown wrote:
> >> On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote:
> >> > On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote:
> >> > > Right, I don't think we added this yet (if we did I can't see
> >> > > it).  I'd
> >> > > add a new field to spi_transfer for this, then other controllers
> >> > > with
> >> > > the same support can implement it as well and drivers can start
> >> > > using
> >> > > it too.
> >> > OK. So I will name the new filed as 'word_delay', is it OK for you?
> >>
> >> Sounds good, yes.
> >
> > Should it be in µs like the existing iter-transfer delay?  I think
> > perhaps units of cycles of the SPI clock make more sense?
> 
> Since some SPI controllers just want some interval values (neither µs
> unit nor cycles unit ) set into hardware, and the hardware will
> convert to the correct delay time automatically. So I did not force
> 'word_delay' unit as µs or cycle, and just let the slave devices
> decide the unit which depends on the SPI hardware requirement.

This needs to be defined units in DT, not decided by each controller.

The controller capabilities just affect what are valid values (or what 
the resolution is).

Rob

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Baolin Wang]

Hi Rob,

On 15 August 2018 at 04:21, Rob Herring <robh@kernel.org> wrote:
> On Tue, Aug 07, 2018 at 06:43:37PM +0800, Baolin Wang wrote:
>> From: Lanqing Liu <lanqing.liu@spreadtrum.com>
>>
>> This patch adds the binding documentation for Spreadtrum SPI
>> controller device.
>>
>> Signed-off-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/spi/spi-sprd.txt |   31 ++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-sprd.txt
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt
>> new file mode 100644
>> index 0000000..06ff746
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt
>> @@ -0,0 +1,31 @@
>> +Spreadtrum SPI Controller
>> +
>> +Required properties:
>> +- compatible: Should be "sprd,sc9860-spi".
>> +- reg: Offset and length of SPI controller register space.
>> +- interrupts: Should contain SPI interrupt.
>> +- clock-names: Should contain following entries:
>> +     "spi" for SPI clock,
>> +     "source" for SPI source (parent) clock,
>
> Do the h/w block actually get this clock or the driver needs it? In the
> latter case, it should not be in DT.

Yes, we will get the actual SPI source clock form the "source" clock property.

>
>> +     "enable" for SPI module enable clock.
>> +- clocks: List of clock input name strings sorted in the same order
>> +     as the clock-names property.
>> +- #address-cells: The number of cells required to define a chip select
>> +     address on the SPI bus. Should be set to 1.
>> +- #size-cells: Should be set to 0.
>> +
>> +Optional properties:
>> +- sprd,spi-interval: Specify the intervals of two SPI frames, which can be
>> +     converted to the delay clock cycles = interval number * 4 + 10.
>
> There are read and write delay properties you can use.

Right. We've agreed introducing another field for spi_transfer to
represent this and will remove 'sprd,spi-interval' from DT.

-- 
Baolin Wang
Best Regards

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum SPI controller documentation

[Baolin Wang]

Hi Rob,

On 15 August 2018 at 04:27, Rob Herring <robh@kernel.org> wrote:
> On Thu, Aug 09, 2018 at 11:03:11AM +0800, Baolin Wang wrote:
>> Hi Trent,
>>
>> On 9 August 2018 at 02:57, Trent Piepho <tpiepho@impinj.com> wrote:
>> > On Wed, 2018-08-08 at 11:54 +0100, Mark Brown wrote:
>> >> On Wed, Aug 08, 2018 at 06:35:28PM +0800, Baolin Wang wrote:
>> >> > On 8 August 2018 at 17:50, Mark Brown <broonie@kernel.org> wrote:
>> >> > > Right, I don't think we added this yet (if we did I can't see
>> >> > > it).  I'd
>> >> > > add a new field to spi_transfer for this, then other controllers
>> >> > > with
>> >> > > the same support can implement it as well and drivers can start
>> >> > > using
>> >> > > it too.
>> >> > OK. So I will name the new filed as 'word_delay', is it OK for you?
>> >>
>> >> Sounds good, yes.
>> >
>> > Should it be in µs like the existing iter-transfer delay?  I think
>> > perhaps units of cycles of the SPI clock make more sense?
>>
>> Since some SPI controllers just want some interval values (neither µs
>> unit nor cycles unit ) set into hardware, and the hardware will
>> convert to the correct delay time automatically. So I did not force
>> 'word_delay' unit as µs or cycle, and just let the slave devices
>> decide the unit which depends on the SPI hardware requirement.
>
> This needs to be defined units in DT, not decided by each controller.

Do you mean we should introduce one standard property (maybe named as
'word_delay_unit') to define the word_delay unit?
If we really need to specify the unit of word_delay, I think we can
add comments for spi_tansfer  to specify the unit, which will be
better.

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index a64235e..7a72c0a 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -711,6 +711,8 @@ extern void spi_res_release(struct spi_controller *ctlr,
  * @delay_usecs: microseconds to delay after this transfer before
  * (optionally) changing the chipselect status, then starting
  * the next transfer or completing this @spi_message.
+ * @word_delay: clock cycles to inter word delay after each word size
(set by bits_per_word)
+ * transmission.
  * @transfer_list: transfers are sequenced through @spi_message.transfers
  * @tx_sg: Scatterlist for transmit, currently not for client use
  * @rx_sg: Scatterlist for receive, currently not for client use
@@ -793,6 +795,7 @@ struct spi_transfer {
  u8 bits_per_word;
  u16 delay_usecs;
  u32 speed_hz;
+ u16 word_delay;

  struct list_head transfer_list;
 };


-- 
Baolin Wang
Best Regards