[PATCH 0/2] spi: Add Qualcomm QUP SPI controller support

[PATCH 0/2] spi: Add Qualcomm QUP SPI controller support

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

Hi, 

Following two patches are adding initial support for SPI controller
available in Qualcomm SoC's.

Controller initialization is based on spi_qsd driver available in
CAF repository. 

Controller supports SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP modes, 
up to 3 CS's and from 4 to 32 bits per word. SPI_LOOP mode is limited 
to input FIFO buffer size.

Currently driver support only PIO mode, I am hopping to add also DMA
mode support with dmaengine patches developed by Andy.

Regards,
Ivan

Ivan T. Ivanov (2):
  spi: qup: Add device tree bindings information
  spi: Add Qualcomm QUP SPI controller support

 .../devicetree/bindings/spi/qcom,spi-qup.txt       |   86 ++
 drivers/spi/Kconfig                                |   14 +
 drivers/spi/Makefile                               |    1 +
 drivers/spi/spi-qup.c                              |  898 ++++++++++++++++++++
 4 files changed, 999 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
 create mode 100644 drivers/spi/spi-qup.c

-- 
1.7.9.5

[PATCH 1/2] spi: qup: Add device tree bindings information

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

The Qualcomm Universal Peripheral (QUP) core is an
AHB slave that provides a common data path (an output
FIFO and an input FIFO) for serial peripheral interface
(SPI) mini-core.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 .../devicetree/bindings/spi/qcom,spi-qup.txt       |   86 ++++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt

diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
new file mode 100644
index 0000000..74565f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
@@ -0,0 +1,86 @@
+Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
+
+The QUP core is an AHB slave that provides a common data path (an output FIFO
+and an input FIFO) for serial peripheral interface (SPI) mini-core.
+
+SPI in master mode support up to 50MHz, up to four chip selects, and a
+programmable data path from 4 bits to 32 bits; supports numerous protocol
+variants.
+
+Required properties:
+- compatible: 		Should contain "qcom,spi-qup-v2".
+- reg: 				Should contain base register location and length
+- interrupts: 		Interrupt number used by this controller
+
+- clocks: 			Should contain the core clock and the AHB clock.
+- clock-names: 		Should be "core" for the core clock and "iface" for the
+                	AHB clock.
+
+- #address-cells: 	Number of cells required to define a chip select
+					address on the SPI bus. Should be set to 1.
+- #size-cells: 		Should be zero.
+
+Optional properties:
+- spi-max-frequency: Specifies maximum SPI clock frequency, Units - Hz. Definition
+                    as per Documentation/devicetree/bindings/spi/spi-bus.txt
+
+SPI slave nodes must be children of the SPI master node and can
+contain properties described in Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Example:
+
+	spi_8: spi@f9964000 { /* BLSP2 QUP2 */
+
+		compatible = "qcom,spi-qup-v2";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0xf9964000 0x1000>;
+		interrupts = <0 102 0>;
+		spi-max-frequency = <19200000>;
+
+		clocks = <&gcc GCC_BLSP2_QUP2_SPI_APPS_CLK>, <&gcc GCC_BLSP2_AHB_CLK>;
+		clock-names = "core", "iface";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&spi8_default>;
+
+		device@0 {
+			compatible = "arm,pl022-dummy";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0>; /* Chip select 0 */
+			spi-max-frequency = <19200000>;
+			spi-cpol;
+		};
+
+		device@1 {
+			compatible = "arm,pl022-dummy";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <1>; /* Chip select 1 */
+			spi-max-frequency = <9600000>;
+			spi-cpha;
+		};
+
+		device@2 {
+			compatible = "arm,pl022-dummy";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <2>; /* Chip select 2 */
+			spi-max-frequency = <19200000>;
+			spi-cpol;
+			spi-cpha;
+		};
+
+		device@3 {
+			compatible = "arm,pl022-dummy";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <3>; /* Chip select 3 */
+			spi-max-frequency = <19200000>;
+			spi-cpol;
+			spi-cpha;
+			spi-cs-high;
+		};
+	};
+
-- 
1.7.9.5

[PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

Qualcomm Universal Peripheral (QUP) core is an AHB slave that
provides a common data path (an output FIFO and an input FIFO)
for serial peripheral interface (SPI) mini-core. SPI in master mode
support up to 50MHz, up to four chip selects, and a programmable
data path from 4 bits to 32 bits; MODE0..3 protocols

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
Cc: Alok Chauhan <alokc@codeaurora.org>
Cc: Gilad Avidov <gavidov@codeaurora.org>
Cc: Kiran Gunda <kgunda@codeaurora.org>
Cc: Sagar Dharia <sdharia@codeaurora.org>
---
 drivers/spi/Kconfig   |   14 +
 drivers/spi/Makefile  |    1 +
 drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 913 insertions(+)
 create mode 100644 drivers/spi/spi-qup.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ba9310b..bf8ce6b 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -381,6 +381,20 @@ config SPI_RSPI
 	help
 	  SPI driver for Renesas RSPI blocks.
 
+config SPI_QUP
+	tristate "Qualcomm SPI Support with QUP interface"
+	depends on ARCH_MSM
+	help
+	  Qualcomm Universal Peripheral (QUP) core is an AHB slave that
+	  provides a common data path (an output FIFO and an input FIFO)
+	  for serial peripheral interface (SPI) mini-core. SPI in master
+	  mode support up to 50MHz, up to four chip selects, and a
+	  programmable data path from 4 bits to 32 bits; supports numerous
+	  protocol variants.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called spi_qup.
+
 config SPI_S3C24XX
 	tristate "Samsung S3C24XX series SPI"
 	depends on ARCH_S3C24XX
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 95af48d..e598147 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -59,6 +59,7 @@ spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA)	+= spi-pxa2xx-pxadma.o
 spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA)	+= spi-pxa2xx-dma.o
 obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
 obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
+obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
 obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
 spi-s3c24xx-hw-y			:= spi-s3c24xx.o
diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
new file mode 100644
index 0000000..5eb5e8f
--- /dev/null
+++ b/drivers/spi/spi-qup.c
@@ -0,0 +1,898 @@
+/*
+ * Copyright (c) 2008-2014, The Linux foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License rev 2 and
+ * only rev 2 as published by the free Software foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/spi/spi.h>
+
+#define QUP_CONFIG			0x0000
+#define QUP_STATE			0x0004
+#define QUP_IO_M_MODES			0x0008
+#define QUP_SW_RESET			0x000c
+#define QUP_OPERATIONAL			0x0018
+#define QUP_ERROR_FLAGS			0x001c
+#define QUP_ERROR_FLAGS_EN		0x0020
+#define QUP_OPERATIONAL_MASK		0x0028
+#define QUP_HW_VERSION			0x0030
+#define QUP_MX_OUTPUT_CNT		0x0100
+#define QUP_OUTPUT_FIFO			0x0110
+#define QUP_MX_WRITE_CNT		0x0150
+#define QUP_MX_INPUT_CNT		0x0200
+#define QUP_MX_READ_CNT			0x0208
+#define QUP_INPUT_FIFO			0x0218
+
+#define SPI_CONFIG			0x0300
+#define SPI_IO_CONTROL			0x0304
+#define SPI_ERROR_FLAGS			0x0308
+#define SPI_ERROR_FLAGS_EN		0x030c
+
+/* QUP_CONFIG fields */
+#define QUP_CONFIG_SPI_MODE		(1 << 8)
+#define QUP_CONFIG_NO_INPUT		BIT(7)
+#define QUP_CONFIG_NO_OUTPUT		BIT(6)
+#define QUP_CONFIG_N			0x001f
+
+/* QUP_STATE fields */
+#define QUP_STATE_VALID			BIT(2)
+#define QUP_STATE_RESET			0
+#define QUP_STATE_RUN			1
+#define QUP_STATE_PAUSE			3
+#define QUP_STATE_MASK			3
+#define QUP_STATE_CLEAR			2
+
+#define QUP_HW_VERSION_2_1_1		0x20010001
+
+/* QUP_IO_M_MODES fields */
+#define QUP_IO_M_PACK_EN		BIT(15)
+#define QUP_IO_M_UNPACK_EN		BIT(14)
+#define QUP_IO_M_INPUT_MODE_MASK_SHIFT	12
+#define QUP_IO_M_OUTPUT_MODE_MASK_SHIFT	10
+#define QUP_IO_M_INPUT_MODE_MASK	(3 << QUP_IO_M_INPUT_MODE_MASK_SHIFT)
+#define QUP_IO_M_OUTPUT_MODE_MASK	(3 << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT)
+
+#define QUP_IO_M_OUTPUT_BLOCK_SIZE(x)	(((x) & (0x03 << 0)) >> 0)
+#define QUP_IO_M_OUTPUT_FIFO_SIZE(x)	(((x) & (0x07 << 2)) >> 2)
+#define QUP_IO_M_INPUT_BLOCK_SIZE(x)	(((x) & (0x03 << 5)) >> 5)
+#define QUP_IO_M_INPUT_FIFO_SIZE(x)	(((x) & (0x07 << 7)) >> 7)
+
+#define QUP_IO_M_MODE_FIFO		0
+#define QUP_IO_M_MODE_BLOCK		1
+#define QUP_IO_M_MODE_DMOV		2
+#define QUP_IO_M_MODE_BAM		3
+
+/* QUP_OPERATIONAL fields */
+#define QUP_OP_MAX_INPUT_DONE_FLAG	BIT(11)
+#define QUP_OP_MAX_OUTPUT_DONE_FLAG	BIT(10)
+#define QUP_OP_IN_SERVICE_FLAG		BIT(9)
+#define QUP_OP_OUT_SERVICE_FLAG		BIT(8)
+#define QUP_OP_IN_FIFO_FULL		BIT(7)
+#define QUP_OP_OUT_FIFO_FULL		BIT(6)
+#define QUP_OP_IN_FIFO_NOT_EMPTY	BIT(5)
+#define QUP_OP_OUT_FIFO_NOT_EMPTY	BIT(4)
+
+/* QUP_ERROR_FLAGS and QUP_ERROR_FLAGS_EN fields */
+#define QUP_ERROR_OUTPUT_OVER_RUN	BIT(5)
+#define QUP_ERROR_INPUT_UNDER_RUN	BIT(4)
+#define QUP_ERROR_OUTPUT_UNDER_RUN	BIT(3)
+#define QUP_ERROR_INPUT_OVER_RUN	BIT(2)
+
+/* SPI_CONFIG fields */
+#define SPI_CONFIG_HS_MODE		BIT(10)
+#define SPI_CONFIG_INPUT_FIRST		BIT(9)
+#define SPI_CONFIG_LOOPBACK		BIT(8)
+
+/* SPI_IO_CONTROL fields */
+#define SPI_IO_C_FORCE_CS		BIT(11)
+#define SPI_IO_C_CLK_IDLE_HIGH		BIT(10)
+#define SPI_IO_C_MX_CS_MODE		BIT(8)
+#define SPI_IO_C_CS_N_POLARITY_0	BIT(4)
+#define SPI_IO_C_CS_SELECT(x)		(((x) & 3) << 2)
+#define SPI_IO_C_CS_SELECT_MASK		0x000c
+#define SPI_IO_C_TRISTATE_CS		BIT(1)
+#define SPI_IO_C_NO_TRI_STATE		BIT(0)
+
+/* SPI_ERROR_FLAGS and SPI_ERROR_FLAGS_EN fields */
+#define SPI_ERROR_CLK_OVER_RUN		BIT(1)
+#define SPI_ERROR_CLK_UNDER_RUN		BIT(0)
+
+#define SPI_NUM_CHIPSELECTS		4
+
+/* high speed mode is when bus rate is greater then 26MHz */
+#define SPI_HS_MIN_RATE			26000000
+
+#define SPI_DELAY_THRESHOLD		1
+#define SPI_DELAY_RETRY			10
+
+struct spi_qup_device {
+	int bits_per_word;
+	int chip_select;
+	int speed_hz;
+	u16 mode;
+};
+
+struct spi_qup {
+	void __iomem		*base;
+	struct device		*dev;
+	struct clk		*cclk;	/* core clock */
+	struct clk		*iclk;	/* interface clock */
+	int			irq;
+	u32			max_speed_hz;
+	u32			speed_hz;
+
+	int			in_fifo_sz;
+	int			out_fifo_sz;
+	int			in_blk_sz;
+	int			out_blk_sz;
+
+	struct spi_transfer	*xfer;
+	struct completion	done;
+	int			error;
+	int			bytes_per_word;
+	int			tx_bytes;
+	int			rx_bytes;
+};
+
+
+static inline bool spi_qup_is_valid_state(struct spi_qup *controller)
+{
+	u32 opstate = readl_relaxed(controller->base + QUP_STATE);
+
+	return opstate & QUP_STATE_VALID;
+}
+
+static int spi_qup_set_state(struct spi_qup *controller, u32 state)
+{
+	unsigned long loop = 0;
+	u32 cur_state;
+
+	cur_state = readl_relaxed(controller->base + QUP_STATE);
+	/*
+	 * Per spec: for PAUSE_STATE to RESET_STATE, two writes
+	 * of (b10) are required
+	 */
+	if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
+	    (state == QUP_STATE_RESET)) {
+		writel_relaxed(QUP_STATE_CLEAR, controller->base + QUP_STATE);
+		writel_relaxed(QUP_STATE_CLEAR, controller->base + QUP_STATE);
+	} else {
+		cur_state &= ~QUP_STATE_MASK;
+		cur_state |= state;
+		writel_relaxed(cur_state, controller->base + QUP_STATE);
+	}
+
+	while (!spi_qup_is_valid_state(controller)) {
+
+		usleep_range(SPI_DELAY_THRESHOLD, SPI_DELAY_THRESHOLD * 2);
+
+		if (++loop > SPI_DELAY_RETRY)
+			return -EIO;
+	}
+
+	return 0;
+}
+
+static void spi_qup_deassert_cs(struct spi_qup *controller,
+				struct spi_qup_device *chip)
+{
+	u32 iocontol, mask;
+
+	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
+
+	/* Disable auto CS toggle and use manual */
+	iocontol &= ~SPI_IO_C_MX_CS_MODE;
+	iocontol |= SPI_IO_C_FORCE_CS;
+
+	iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
+	iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
+
+	mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
+
+	if (chip->mode & SPI_CS_HIGH)
+		iocontol &= ~mask;
+	else
+		iocontol |= mask;
+
+	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
+}
+
+static void spi_qup_assert_cs(struct spi_qup *controller,
+			      struct spi_qup_device *chip)
+{
+	u32 iocontol, mask;
+
+	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
+
+	/* Disable auto CS toggle and use manual */
+	iocontol &= ~SPI_IO_C_MX_CS_MODE;
+	iocontol |= SPI_IO_C_FORCE_CS;
+
+	iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
+	iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
+
+	mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
+
+	if (chip->mode & SPI_CS_HIGH)
+		iocontol |= mask;
+	else
+		iocontol &= ~mask;
+
+	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
+}
+
+static void spi_qup_fifo_read(struct spi_qup *controller,
+			      struct spi_transfer *xfer)
+{
+	u8 *rx_buf = xfer->rx_buf;
+	u32 word, state;
+	int idx, shift;
+
+	while (controller->rx_bytes < xfer->len) {
+
+		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
+		if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
+			break;
+
+		word = readl_relaxed(controller->base + QUP_INPUT_FIFO);
+
+		for (idx = 0; idx < controller->bytes_per_word &&
+		     controller->rx_bytes < xfer->len; idx++,
+		     controller->rx_bytes++) {
+
+			if (!rx_buf)
+				continue;
+			/*
+			 * The data format depends on bytes_per_word:
+			 *  4 bytes: 0x12345678
+			 *  2 bytes: 0x00001234
+			 *  1 byte : 0x00000012
+			 */
+			shift = BITS_PER_BYTE;
+			shift *= (controller->bytes_per_word - idx - 1);
+			rx_buf[controller->rx_bytes] = word >> shift;
+		}
+	}
+}
+
+static void spi_qup_fifo_write(struct spi_qup *controller,
+			       struct spi_transfer *xfer)
+{
+	const u8 *tx_buf = xfer->tx_buf;
+	u32 word, state, data;
+	int idx;
+
+	while (controller->tx_bytes < xfer->len) {
+
+		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
+		if (state & QUP_OP_OUT_FIFO_FULL)
+			break;
+
+		word = 0;
+		for (idx = 0; idx < controller->bytes_per_word &&
+		     controller->tx_bytes < xfer->len; idx++,
+		     controller->tx_bytes++) {
+
+			if (!tx_buf)
+				continue;
+
+			data = tx_buf[controller->tx_bytes];
+			word |= data << (BITS_PER_BYTE * (3 - idx));
+		}
+
+		writel_relaxed(word, controller->base + QUP_OUTPUT_FIFO);
+	}
+}
+
+static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
+{
+	struct spi_qup *controller = dev_id;
+	struct spi_transfer *xfer;
+	u32 opflags, qup_err, spi_err;
+
+	xfer = controller->xfer;
+
+	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
+	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
+	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
+
+	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
+	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
+	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
+
+	if (!xfer)
+		return IRQ_HANDLED;
+
+	if (qup_err) {
+		if (qup_err & QUP_ERROR_OUTPUT_OVER_RUN)
+			dev_warn(controller->dev, "OUTPUT_OVER_RUN\n");
+		if (qup_err & QUP_ERROR_INPUT_UNDER_RUN)
+			dev_warn(controller->dev, "INPUT_UNDER_RUN\n");
+		if (qup_err & QUP_ERROR_OUTPUT_UNDER_RUN)
+			dev_warn(controller->dev, "OUTPUT_UNDER_RUN\n");
+		if (qup_err & QUP_ERROR_INPUT_OVER_RUN)
+			dev_warn(controller->dev, "INPUT_OVER_RUN\n");
+
+		controller->error = -EIO;
+	}
+
+	if (spi_err) {
+		if (spi_err & SPI_ERROR_CLK_OVER_RUN)
+			dev_warn(controller->dev, "CLK_OVER_RUN\n");
+		if (spi_err & SPI_ERROR_CLK_UNDER_RUN)
+			dev_warn(controller->dev, "CLK_UNDER_RUN\n");
+
+		controller->error = -EIO;
+	}
+
+	if (opflags & QUP_OP_IN_SERVICE_FLAG) {
+		writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
+		       controller->base + QUP_OPERATIONAL);
+		spi_qup_fifo_read(controller, xfer);
+	}
+
+	if (opflags & QUP_OP_OUT_SERVICE_FLAG) {
+		writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
+		       controller->base + QUP_OPERATIONAL);
+		spi_qup_fifo_write(controller, xfer);
+	}
+
+	if (controller->rx_bytes == xfer->len ||
+	    controller->error)
+		complete(&controller->done);
+
+	return IRQ_HANDLED;
+}
+
+static int spi_qup_transfer_do(struct spi_qup *controller,
+			       struct spi_qup_device *chip,
+			       struct spi_transfer *xfer)
+{
+	unsigned long timeout;
+	int ret = -EIO;
+
+	reinit_completion(&controller->done);
+
+	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
+	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
+	timeout = 100 * msecs_to_jiffies(timeout);
+
+	controller->rx_bytes = 0;
+	controller->tx_bytes = 0;
+	controller->error = 0;
+	controller->xfer = xfer;
+
+	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
+		dev_warn(controller->dev, "cannot set RUN state\n");
+		goto exit;
+	}
+
+	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
+		dev_warn(controller->dev, "cannot set PAUSE state\n");
+		goto exit;
+	}
+
+	spi_qup_fifo_write(controller, xfer);
+
+	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
+		dev_warn(controller->dev, "cannot set EXECUTE state\n");
+		goto exit;
+	}
+
+	if (!wait_for_completion_timeout(&controller->done, timeout))
+		ret = -ETIMEDOUT;
+	else
+		ret = controller->error;
+exit:
+	controller->xfer = NULL;
+	controller->error = 0;
+	controller->rx_bytes = 0;
+	controller->tx_bytes = 0;
+	spi_qup_set_state(controller, QUP_STATE_RESET);
+	return ret;
+}
+
+static int spi_qup_setup(struct spi_device *spi)
+{
+	struct spi_qup *controller = spi_master_get_devdata(spi->master);
+	struct spi_qup_device *chip = spi_get_ctldata(spi);
+
+	if (spi->chip_select >= spi->master->num_chipselect) {
+		dev_err(controller->dev, "invalid chip_select %d\n",
+			spi->chip_select);
+		return -EINVAL;
+	}
+
+	if (spi->max_speed_hz > controller->max_speed_hz) {
+		dev_err(controller->dev, "invalid max_speed_hz %d\n",
+			spi->max_speed_hz);
+		return -EINVAL;
+	}
+
+	if (!chip) {
+		/* First setup */
+		chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+		if (!chip) {
+			dev_err(controller->dev, "no memory for chip data\n");
+			return -ENOMEM;
+		}
+
+		spi_set_ctldata(spi, chip);
+	}
+
+	return 0;
+}
+
+static void spi_qup_cleanup(struct spi_device *spi)
+{
+	struct spi_qup_device *chip = spi_get_ctldata(spi);
+
+	if (!chip)
+		return;
+
+	spi_set_ctldata(spi, NULL);
+	kfree(chip);
+}
+
+/* set clock freq, clock ramp, bits per work */
+static int spi_qup_io_setup(struct spi_device *spi,
+			  struct spi_transfer *xfer)
+{
+	struct spi_qup *controller = spi_master_get_devdata(spi->master);
+	struct spi_qup_device *chip = spi_get_ctldata(spi);
+	u32 iocontol, config, iomode, mode;
+	int ret, n_words;
+
+	if (spi->mode & SPI_LOOP && xfer->len > controller->in_fifo_sz) {
+		dev_err(controller->dev, "too big size for loopback %d > %d\n",
+			xfer->len, controller->in_fifo_sz);
+		return -EIO;
+	}
+
+	chip->mode = spi->mode;
+	chip->speed_hz = spi->max_speed_hz;
+	if (xfer->speed_hz)
+		chip->speed_hz = xfer->speed_hz;
+
+	if (controller->speed_hz != chip->speed_hz) {
+		ret = clk_set_rate(controller->cclk, chip->speed_hz);
+		if (ret) {
+			dev_err(controller->dev, "fail to set frequency %d",
+				chip->speed_hz);
+			return -EIO;
+		}
+	}
+
+	controller->speed_hz = chip->speed_hz;
+
+	chip->bits_per_word = spi->bits_per_word;
+	if (xfer->bits_per_word)
+		chip->bits_per_word = xfer->bits_per_word;
+
+	if (chip->bits_per_word <= 8)
+		controller->bytes_per_word = 1;
+	else if (chip->bits_per_word <= 16)
+		controller->bytes_per_word = 2;
+	else
+		controller->bytes_per_word = 4;
+
+	if (controller->bytes_per_word > xfer->len ||
+	    xfer->len % controller->bytes_per_word != 0){
+		/* No partial transfers */
+		dev_err(controller->dev, "invalid len %d for %d bits\n",
+			xfer->len, chip->bits_per_word);
+		return -EIO;
+	}
+
+	n_words = xfer->len / controller->bytes_per_word;
+
+	if (spi_qup_set_state(controller, QUP_STATE_RESET)) {
+		dev_err(controller->dev, "cannot set RESET state\n");
+		return -EIO;
+	}
+
+	if (n_words <= controller->in_fifo_sz) {
+		mode = QUP_IO_M_MODE_FIFO;
+		writel_relaxed(n_words, controller->base + QUP_MX_READ_CNT);
+		writel_relaxed(n_words, controller->base + QUP_MX_WRITE_CNT);
+		/* must be zero for FIFO */
+		writel_relaxed(0, controller->base + QUP_MX_INPUT_CNT);
+		writel_relaxed(0, controller->base + QUP_MX_OUTPUT_CNT);
+	} else {
+		mode = QUP_IO_M_MODE_BLOCK;
+		writel_relaxed(n_words, controller->base + QUP_MX_INPUT_CNT);
+		writel_relaxed(n_words, controller->base + QUP_MX_OUTPUT_CNT);
+		/* must be zero for BLOCK and BAM */
+		writel_relaxed(0, controller->base + QUP_MX_READ_CNT);
+		writel_relaxed(0, controller->base + QUP_MX_WRITE_CNT);
+	}
+
+	iomode = readl_relaxed(controller->base + QUP_IO_M_MODES);
+	/* Set input and output transfer mode */
+	iomode &= ~(QUP_IO_M_INPUT_MODE_MASK | QUP_IO_M_OUTPUT_MODE_MASK);
+	iomode &= ~(QUP_IO_M_PACK_EN | QUP_IO_M_UNPACK_EN);
+	iomode |= (mode << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT);
+	iomode |= (mode << QUP_IO_M_INPUT_MODE_MASK_SHIFT);
+
+	writel_relaxed(iomode, controller->base + QUP_IO_M_MODES);
+
+	config = readl_relaxed(controller->base + SPI_CONFIG);
+
+	if (chip->mode & SPI_LOOP)
+		config |= SPI_CONFIG_LOOPBACK;
+	else
+		config &= ~SPI_CONFIG_LOOPBACK;
+
+	if (chip->mode & SPI_CPHA)
+		config &= ~SPI_CONFIG_INPUT_FIRST;
+	else
+		config |= SPI_CONFIG_INPUT_FIRST;
+
+	/*
+	 * HS_MODE improves signal stability for spi-clk high rates
+	 * but is invalid in loop back mode.
+	 */
+	if ((controller->speed_hz >= SPI_HS_MIN_RATE) &&
+	    !(chip->mode & SPI_LOOP))
+		config |= SPI_CONFIG_HS_MODE;
+	else
+		config &= ~SPI_CONFIG_HS_MODE;
+
+	writel_relaxed(config, controller->base + SPI_CONFIG);
+
+	config = readl_relaxed(controller->base + QUP_CONFIG);
+	config &= ~(QUP_CONFIG_NO_INPUT | QUP_CONFIG_NO_OUTPUT | QUP_CONFIG_N);
+	config |= chip->bits_per_word - 1;
+	config |= QUP_CONFIG_SPI_MODE;
+	writel_relaxed(config, controller->base + QUP_CONFIG);
+
+	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
+
+	/* Disable auto CS toggle */
+	iocontol &= ~SPI_IO_C_MX_CS_MODE;
+
+	if (chip->mode & SPI_CPOL)
+		iocontol |= SPI_IO_C_CLK_IDLE_HIGH;
+	else
+		iocontol &= ~SPI_IO_C_CLK_IDLE_HIGH;
+
+	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
+
+	/*
+	 * TODO: In BAM mode mask INPUT and OUTPUT service flags in
+	 * to prevent IRQs on FIFO status change.
+	 */
+	writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK);
+
+	return 0;
+}
+
+static int spi_qup_transfer_one(struct spi_master *master,
+				struct spi_message *msg)
+{
+	struct spi_qup *controller = spi_master_get_devdata(master);
+	struct spi_qup_device *chip = spi_get_ctldata(msg->spi);
+	struct spi_transfer *xfer;
+	struct spi_device *spi;
+	unsigned cs_change;
+	int status;
+
+	spi = msg->spi;
+	cs_change = 1;
+	status = 0;
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+
+		status = spi_qup_io_setup(spi, xfer);
+		if (status)
+			break;
+
+		if (cs_change)
+			spi_qup_assert_cs(controller, chip);
+
+		cs_change = xfer->cs_change;
+
+		/* Do actual transfer */
+		status = spi_qup_transfer_do(controller, chip, xfer);
+		if (status)
+			break;
+
+		msg->actual_length += xfer->len;
+
+		if (xfer->delay_usecs)
+			udelay(xfer->delay_usecs);
+
+		if (cs_change)
+			spi_qup_deassert_cs(controller, chip);
+	}
+
+	if (status || !cs_change)
+		spi_qup_deassert_cs(controller, chip);
+
+	msg->status = status;
+	spi_finalize_current_message(master);
+	return status;
+}
+
+static int spi_qup_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct clk *iclk, *cclk;
+	struct spi_qup *controller;
+	struct resource *res;
+	struct device *dev;
+	void __iomem *base;
+	u32 data, max_freq, iomode;
+	int ret, irq, size;
+
+	dev = &pdev->dev;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	irq = platform_get_irq(pdev, 0);
+
+	if (irq < 0)
+		return irq;
+
+	cclk = devm_clk_get(dev, "core");
+	if (IS_ERR(cclk)) {
+		dev_err(dev, "cannot get core clock\n");
+		return PTR_ERR(cclk);
+	}
+
+	iclk = devm_clk_get(dev, "iface");
+	if (IS_ERR(iclk)) {
+		dev_err(dev, "cannot get iface clock\n");
+		return PTR_ERR(iclk);
+	}
+
+	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
+		max_freq = 19200000;
+
+	if (!max_freq) {
+		dev_err(dev, "invalid clock frequency %d\n", max_freq);
+		return -ENXIO;
+	}
+
+	ret = clk_set_rate(cclk, max_freq);
+	if (ret)
+		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
+
+	ret = clk_prepare_enable(cclk);
+	if (ret) {
+		dev_err(dev, "cannot enable core clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(iclk);
+	if (ret) {
+		clk_disable_unprepare(cclk);
+		dev_err(dev, "cannot enable iface clock\n");
+		return ret;
+	}
+
+	data = readl_relaxed(base + QUP_HW_VERSION);
+
+	if (data < QUP_HW_VERSION_2_1_1) {
+		clk_disable_unprepare(cclk);
+		clk_disable_unprepare(iclk);
+		dev_err(dev, "v.%08x is not supported\n", data);
+		return -ENXIO;
+	}
+
+	master = spi_alloc_master(dev, sizeof(struct spi_qup));
+	if (!master) {
+		clk_disable_unprepare(cclk);
+		clk_disable_unprepare(iclk);
+		dev_err(dev, "cannot allocate master\n");
+		return -ENOMEM;
+	}
+
+	master->bus_num = pdev->id;
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
+	master->num_chipselect = SPI_NUM_CHIPSELECTS;
+	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
+	master->setup = spi_qup_setup;
+	master->cleanup = spi_qup_cleanup;
+	master->transfer_one_message = spi_qup_transfer_one;
+	master->dev.of_node = pdev->dev.of_node;
+	master->auto_runtime_pm = true;
+
+	platform_set_drvdata(pdev, master);
+
+	controller = spi_master_get_devdata(master);
+
+	controller->dev  = dev;
+	controller->base = base;
+	controller->iclk = iclk;
+	controller->cclk = cclk;
+	controller->irq  = irq;
+	controller->max_speed_hz = clk_get_rate(cclk);
+	controller->speed_hz = controller->max_speed_hz;
+
+	init_completion(&controller->done);
+
+	iomode = readl_relaxed(base + QUP_IO_M_MODES);
+
+	size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode);
+	if (size)
+		controller->out_blk_sz = size * 16;
+	else
+		controller->out_blk_sz = 4;
+
+	size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode);
+	if (size)
+		controller->in_blk_sz = size * 16;
+	else
+		controller->in_blk_sz = 4;
+
+	size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode);
+	controller->out_fifo_sz = controller->out_blk_sz * (2 << size);
+
+	size = QUP_IO_M_INPUT_FIFO_SIZE(iomode);
+	controller->in_fifo_sz = controller->in_blk_sz * (2 << size);
+
+	dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
+		 data, controller->in_blk_sz, controller->in_fifo_sz,
+		 controller->out_blk_sz, controller->out_fifo_sz);
+
+	writel_relaxed(1, base + QUP_SW_RESET);
+
+	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
+	if (ret) {
+		dev_err(dev, "cannot set RESET state\n");
+		goto error;
+	}
+
+	writel_relaxed(0, base + QUP_OPERATIONAL);
+	writel_relaxed(0, base + QUP_IO_M_MODES);
+	writel_relaxed(0, base + QUP_OPERATIONAL_MASK);
+	writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN,
+		       base + SPI_ERROR_FLAGS_EN);
+
+	writel_relaxed(0, base + SPI_CONFIG);
+	writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL);
+
+	ret = devm_request_irq(dev, irq, spi_qup_qup_irq,
+			       IRQF_TRIGGER_HIGH, pdev->name, controller);
+	if (ret) {
+		dev_err(dev, "cannot request IRQ %d\n", irq);
+		goto error;
+	}
+
+	ret = devm_spi_register_master(dev, master);
+	if (!ret) {
+		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
+		pm_runtime_use_autosuspend(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+		return ret;
+	}
+error:
+	clk_disable_unprepare(cclk);
+	clk_disable_unprepare(iclk);
+	spi_master_put(master);
+	return ret;
+}
+
+#ifdef CONFIG_PM_RUNTIME
+static int spi_qup_pm_suspend_runtime(struct device *device)
+{
+	struct spi_master *master = dev_get_drvdata(device);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+
+	disable_irq(controller->irq);
+	clk_disable_unprepare(controller->cclk);
+	clk_disable_unprepare(controller->iclk);
+	dev_dbg(device, "suspend runtime\n");
+	return 0;
+}
+
+static int spi_qup_pm_resume_runtime(struct device *device)
+{
+	struct spi_master *master = dev_get_drvdata(device);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+
+	clk_prepare_enable(controller->cclk);
+	clk_prepare_enable(controller->iclk);
+	enable_irq(controller->irq);
+	dev_dbg(device, "resume runtime\n");
+	return 0;
+}
+#endif /* CONFIG_PM_RUNTIME */
+
+#ifdef CONFIG_PM_SLEEP
+static int spi_qup_suspend(struct device *device)
+{
+	struct spi_master *master = dev_get_drvdata(device);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+	int status;
+
+	status = spi_master_suspend(master);
+	if (!status) {
+		disable_irq(controller->irq);
+		clk_disable_unprepare(controller->cclk);
+		clk_disable_unprepare(controller->iclk);
+	}
+
+	dev_dbg(device, "system suspend %d\n", status);
+	return status;
+}
+
+static int spi_qup_resume(struct device *device)
+{
+	struct spi_master *master = dev_get_drvdata(device);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+	int status;
+
+	clk_prepare_enable(controller->cclk);
+	clk_prepare_enable(controller->iclk);
+
+	status = spi_master_resume(master);
+
+	dev_dbg(device, "system resume %d\n", status);
+	return status;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static int spi_qup_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = dev_get_drvdata(&pdev->dev);
+	struct spi_qup *controller = spi_master_get_devdata(master);
+
+	pm_runtime_get_sync(&pdev->dev);
+
+	clk_disable_unprepare(controller->cclk);
+	clk_disable_unprepare(controller->iclk);
+
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
+static struct of_device_id spi_qup_dt_match[] = {
+	{ .compatible = "qcom,spi-qup-v2", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, spi_qup_dt_match);
+
+static const struct dev_pm_ops spi_qup_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(spi_qup_suspend, spi_qup_resume)
+	SET_RUNTIME_PM_OPS(spi_qup_pm_suspend_runtime,
+			   spi_qup_pm_resume_runtime,
+			   NULL)
+};
+
+static struct platform_driver spi_qup_driver = {
+	.driver = {
+		.name		= "spi_qup",
+		.owner		= THIS_MODULE,
+		.pm		= &spi_qup_dev_pm_ops,
+		.of_match_table = spi_qup_dt_match,
+	},
+	.probe = spi_qup_probe,
+	.remove = spi_qup_remove,
+};
+module_platform_driver(spi_qup_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("0.4");
+MODULE_ALIAS("platform:spi_qup");
-- 
1.7.9.5

Re: [PATCH 0/2] spi: Add Qualcomm QUP SPI controller support

[Mark Brown]

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

On Thu, Feb 06, 2014 at 06:57:46PM +0200, Ivan T. Ivanov wrote:

> Ivan T. Ivanov (2):
>   spi: qup: Add device tree bindings information
>   spi: Add Qualcomm QUP SPI controller support

I only seem to have the second patch here?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Andy Gross]

On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> 
> Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> provides a common data path (an output FIFO and an input FIFO)
> for serial peripheral interface (SPI) mini-core. SPI in master mode
> support up to 50MHz, up to four chip selects, and a programmable
> data path from 4 bits to 32 bits; MODE0..3 protocols
> 
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> Cc: Alok Chauhan <alokc@codeaurora.org>
> Cc: Gilad Avidov <gavidov@codeaurora.org>
> Cc: Kiran Gunda <kgunda@codeaurora.org>
> Cc: Sagar Dharia <sdharia@codeaurora.org>
> ---
>  drivers/spi/Kconfig   |   14 +
>  drivers/spi/Makefile  |    1 +
>  drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 913 insertions(+)
>  create mode 100644 drivers/spi/spi-qup.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ba9310b..bf8ce6b 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -381,6 +381,20 @@ config SPI_RSPI
>  	help
>  	  SPI driver for Renesas RSPI blocks.
>  
> +config SPI_QUP
> +	tristate "Qualcomm SPI Support with QUP interface"
> +	depends on ARCH_MSM

I'd change to ARCH_MSM_DT.  This ensures the OF component is there.

> +	help
> +	  Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> +	  provides a common data path (an output FIFO and an input FIFO)
> +	  for serial peripheral interface (SPI) mini-core. SPI in master
> +	  mode support up to 50MHz, up to four chip selects, and a
> +	  programmable data path from 4 bits to 32 bits; supports numerous
> +	  protocol variants.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called spi_qup.
> +
>  config SPI_S3C24XX
>  	tristate "Samsung S3C24XX series SPI"
>  	depends on ARCH_S3C24XX
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 95af48d..e598147 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -59,6 +59,7 @@ spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA)	+= spi-pxa2xx-pxadma.o
>  spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA)	+= spi-pxa2xx-dma.o
>  obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
>  obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
> +obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
>  obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
>  obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
>  spi-s3c24xx-hw-y			:= spi-s3c24xx.o
> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> new file mode 100644
> index 0000000..5eb5e8f
> --- /dev/null
> +++ b/drivers/spi/spi-qup.c
> @@ -0,0 +1,898 @@
> +/*
> + * Copyright (c) 2008-2014, The Linux foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License rev 2 and
> + * only rev 2 as published by the free Software foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>

Remove this for now.  No runtime support.

> +#include <linux/spi/spi.h>
> +
> +#define QUP_CONFIG			0x0000
> +#define QUP_STATE			0x0004
> +#define QUP_IO_M_MODES			0x0008
> +#define QUP_SW_RESET			0x000c
> +#define QUP_OPERATIONAL			0x0018
> +#define QUP_ERROR_FLAGS			0x001c
> +#define QUP_ERROR_FLAGS_EN		0x0020
> +#define QUP_OPERATIONAL_MASK		0x0028
> +#define QUP_HW_VERSION			0x0030
> +#define QUP_MX_OUTPUT_CNT		0x0100
> +#define QUP_OUTPUT_FIFO			0x0110
> +#define QUP_MX_WRITE_CNT		0x0150
> +#define QUP_MX_INPUT_CNT		0x0200
> +#define QUP_MX_READ_CNT			0x0208
> +#define QUP_INPUT_FIFO			0x0218
> +
> +#define SPI_CONFIG			0x0300
> +#define SPI_IO_CONTROL			0x0304
> +#define SPI_ERROR_FLAGS			0x0308
> +#define SPI_ERROR_FLAGS_EN		0x030c
> +
> +/* QUP_CONFIG fields */
> +#define QUP_CONFIG_SPI_MODE		(1 << 8)
> +#define QUP_CONFIG_NO_INPUT		BIT(7)
> +#define QUP_CONFIG_NO_OUTPUT		BIT(6)
> +#define QUP_CONFIG_N			0x001f
> +
> +/* QUP_STATE fields */
> +#define QUP_STATE_VALID			BIT(2)
> +#define QUP_STATE_RESET			0
> +#define QUP_STATE_RUN			1
> +#define QUP_STATE_PAUSE			3
> +#define QUP_STATE_MASK			3
> +#define QUP_STATE_CLEAR			2
> +
> +#define QUP_HW_VERSION_2_1_1		0x20010001
> +
> +/* QUP_IO_M_MODES fields */
> +#define QUP_IO_M_PACK_EN		BIT(15)
> +#define QUP_IO_M_UNPACK_EN		BIT(14)
> +#define QUP_IO_M_INPUT_MODE_MASK_SHIFT	12
> +#define QUP_IO_M_OUTPUT_MODE_MASK_SHIFT	10
> +#define QUP_IO_M_INPUT_MODE_MASK	(3 << QUP_IO_M_INPUT_MODE_MASK_SHIFT)
> +#define QUP_IO_M_OUTPUT_MODE_MASK	(3 << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT)
> +
> +#define QUP_IO_M_OUTPUT_BLOCK_SIZE(x)	(((x) & (0x03 << 0)) >> 0)
> +#define QUP_IO_M_OUTPUT_FIFO_SIZE(x)	(((x) & (0x07 << 2)) >> 2)
> +#define QUP_IO_M_INPUT_BLOCK_SIZE(x)	(((x) & (0x03 << 5)) >> 5)
> +#define QUP_IO_M_INPUT_FIFO_SIZE(x)	(((x) & (0x07 << 7)) >> 7)
> +
> +#define QUP_IO_M_MODE_FIFO		0
> +#define QUP_IO_M_MODE_BLOCK		1
> +#define QUP_IO_M_MODE_DMOV		2
> +#define QUP_IO_M_MODE_BAM		3
> +
> +/* QUP_OPERATIONAL fields */
> +#define QUP_OP_MAX_INPUT_DONE_FLAG	BIT(11)
> +#define QUP_OP_MAX_OUTPUT_DONE_FLAG	BIT(10)
> +#define QUP_OP_IN_SERVICE_FLAG		BIT(9)
> +#define QUP_OP_OUT_SERVICE_FLAG		BIT(8)
> +#define QUP_OP_IN_FIFO_FULL		BIT(7)
> +#define QUP_OP_OUT_FIFO_FULL		BIT(6)
> +#define QUP_OP_IN_FIFO_NOT_EMPTY	BIT(5)
> +#define QUP_OP_OUT_FIFO_NOT_EMPTY	BIT(4)
> +
> +/* QUP_ERROR_FLAGS and QUP_ERROR_FLAGS_EN fields */
> +#define QUP_ERROR_OUTPUT_OVER_RUN	BIT(5)
> +#define QUP_ERROR_INPUT_UNDER_RUN	BIT(4)
> +#define QUP_ERROR_OUTPUT_UNDER_RUN	BIT(3)
> +#define QUP_ERROR_INPUT_OVER_RUN	BIT(2)
> +
> +/* SPI_CONFIG fields */
> +#define SPI_CONFIG_HS_MODE		BIT(10)
> +#define SPI_CONFIG_INPUT_FIRST		BIT(9)
> +#define SPI_CONFIG_LOOPBACK		BIT(8)
> +
> +/* SPI_IO_CONTROL fields */
> +#define SPI_IO_C_FORCE_CS		BIT(11)
> +#define SPI_IO_C_CLK_IDLE_HIGH		BIT(10)
> +#define SPI_IO_C_MX_CS_MODE		BIT(8)
> +#define SPI_IO_C_CS_N_POLARITY_0	BIT(4)
> +#define SPI_IO_C_CS_SELECT(x)		(((x) & 3) << 2)
> +#define SPI_IO_C_CS_SELECT_MASK		0x000c
> +#define SPI_IO_C_TRISTATE_CS		BIT(1)
> +#define SPI_IO_C_NO_TRI_STATE		BIT(0)
> +
> +/* SPI_ERROR_FLAGS and SPI_ERROR_FLAGS_EN fields */
> +#define SPI_ERROR_CLK_OVER_RUN		BIT(1)
> +#define SPI_ERROR_CLK_UNDER_RUN		BIT(0)
> +
> +#define SPI_NUM_CHIPSELECTS		4
> +
> +/* high speed mode is when bus rate is greater then 26MHz */
> +#define SPI_HS_MIN_RATE			26000000
> +
> +#define SPI_DELAY_THRESHOLD		1
> +#define SPI_DELAY_RETRY			10
> +
> +struct spi_qup_device {
> +	int bits_per_word;
> +	int chip_select;
> +	int speed_hz;
> +	u16 mode;
> +};
> +
> +struct spi_qup {
> +	void __iomem		*base;
> +	struct device		*dev;
> +	struct clk		*cclk;	/* core clock */
> +	struct clk		*iclk;	/* interface clock */
> +	int			irq;
> +	u32			max_speed_hz;
> +	u32			speed_hz;
> +
> +	int			in_fifo_sz;
> +	int			out_fifo_sz;
> +	int			in_blk_sz;
> +	int			out_blk_sz;
> +
> +	struct spi_transfer	*xfer;
> +	struct completion	done;
> +	int			error;
> +	int			bytes_per_word;
> +	int			tx_bytes;
> +	int			rx_bytes;
> +};
> +
> +
> +static inline bool spi_qup_is_valid_state(struct spi_qup *controller)
> +{
> +	u32 opstate = readl_relaxed(controller->base + QUP_STATE);
> +
> +	return opstate & QUP_STATE_VALID;
> +}
> +
> +static int spi_qup_set_state(struct spi_qup *controller, u32 state)
> +{
> +	unsigned long loop = 0;
> +	u32 cur_state;
> +
> +	cur_state = readl_relaxed(controller->base + QUP_STATE);
> +	/*
> +	 * Per spec: for PAUSE_STATE to RESET_STATE, two writes
> +	 * of (b10) are required
> +	 */
> +	if (((cur_state & QUP_STATE_MASK) == QUP_STATE_PAUSE) &&
> +	    (state == QUP_STATE_RESET)) {
> +		writel_relaxed(QUP_STATE_CLEAR, controller->base + QUP_STATE);
> +		writel_relaxed(QUP_STATE_CLEAR, controller->base + QUP_STATE);
> +	} else {
> +		cur_state &= ~QUP_STATE_MASK;
> +		cur_state |= state;
> +		writel_relaxed(cur_state, controller->base + QUP_STATE);
> +	}
> +
> +	while (!spi_qup_is_valid_state(controller)) {
> +
> +		usleep_range(SPI_DELAY_THRESHOLD, SPI_DELAY_THRESHOLD * 2);
> +
> +		if (++loop > SPI_DELAY_RETRY)
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void spi_qup_deassert_cs(struct spi_qup *controller,
> +				struct spi_qup_device *chip)
> +{
> +	u32 iocontol, mask;
> +
> +	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
> +
> +	/* Disable auto CS toggle and use manual */
> +	iocontol &= ~SPI_IO_C_MX_CS_MODE;
> +	iocontol |= SPI_IO_C_FORCE_CS;
> +
> +	iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
> +	iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
> +
> +	mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
> +
> +	if (chip->mode & SPI_CS_HIGH)
> +		iocontol &= ~mask;
> +	else
> +		iocontol |= mask;
> +
> +	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
> +}
> +
> +static void spi_qup_assert_cs(struct spi_qup *controller,
> +			      struct spi_qup_device *chip)
> +{
> +	u32 iocontol, mask;
> +
> +	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
> +
> +	/* Disable auto CS toggle and use manual */
> +	iocontol &= ~SPI_IO_C_MX_CS_MODE;
> +	iocontol |= SPI_IO_C_FORCE_CS;
> +
> +	iocontol &= ~SPI_IO_C_CS_SELECT_MASK;
> +	iocontol |= SPI_IO_C_CS_SELECT(chip->chip_select);
> +
> +	mask = SPI_IO_C_CS_N_POLARITY_0 << chip->chip_select;
> +
> +	if (chip->mode & SPI_CS_HIGH)
> +		iocontol |= mask;
> +	else
> +		iocontol &= ~mask;
> +
> +	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
> +}
> +
> +static void spi_qup_fifo_read(struct spi_qup *controller,
> +			      struct spi_transfer *xfer)
> +{
> +	u8 *rx_buf = xfer->rx_buf;
> +	u32 word, state;
> +	int idx, shift;
> +
> +	while (controller->rx_bytes < xfer->len) {
> +
> +		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +		if (0 == (state & QUP_OP_IN_FIFO_NOT_EMPTY))
> +			break;
> +
> +		word = readl_relaxed(controller->base + QUP_INPUT_FIFO);
> +
> +		for (idx = 0; idx < controller->bytes_per_word &&
> +		     controller->rx_bytes < xfer->len; idx++,
> +		     controller->rx_bytes++) {
> +
> +			if (!rx_buf)
> +				continue;
> +			/*
> +			 * The data format depends on bytes_per_word:
> +			 *  4 bytes: 0x12345678
> +			 *  2 bytes: 0x00001234
> +			 *  1 byte : 0x00000012
> +			 */
> +			shift = BITS_PER_BYTE;
> +			shift *= (controller->bytes_per_word - idx - 1);
> +			rx_buf[controller->rx_bytes] = word >> shift;
> +		}
> +	}
> +}
> +
> +static void spi_qup_fifo_write(struct spi_qup *controller,
> +			       struct spi_transfer *xfer)
> +{
> +	const u8 *tx_buf = xfer->tx_buf;
> +	u32 word, state, data;
> +	int idx;
> +
> +	while (controller->tx_bytes < xfer->len) {
> +
> +		state = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +		if (state & QUP_OP_OUT_FIFO_FULL)
> +			break;
> +
> +		word = 0;
> +		for (idx = 0; idx < controller->bytes_per_word &&
> +		     controller->tx_bytes < xfer->len; idx++,
> +		     controller->tx_bytes++) {
> +
> +			if (!tx_buf)
> +				continue;
> +
> +			data = tx_buf[controller->tx_bytes];
> +			word |= data << (BITS_PER_BYTE * (3 - idx));
> +		}
> +
> +		writel_relaxed(word, controller->base + QUP_OUTPUT_FIFO);
> +	}
> +}
> +
> +static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
> +{
> +	struct spi_qup *controller = dev_id;
> +	struct spi_transfer *xfer;
> +	u32 opflags, qup_err, spi_err;
> +
> +	xfer = controller->xfer;
> +
> +	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> +	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> +	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +
> +	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> +	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> +	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> +
> +	if (!xfer)
> +		return IRQ_HANDLED;
> +
> +	if (qup_err) {
> +		if (qup_err & QUP_ERROR_OUTPUT_OVER_RUN)
> +			dev_warn(controller->dev, "OUTPUT_OVER_RUN\n");
> +		if (qup_err & QUP_ERROR_INPUT_UNDER_RUN)
> +			dev_warn(controller->dev, "INPUT_UNDER_RUN\n");
> +		if (qup_err & QUP_ERROR_OUTPUT_UNDER_RUN)
> +			dev_warn(controller->dev, "OUTPUT_UNDER_RUN\n");
> +		if (qup_err & QUP_ERROR_INPUT_OVER_RUN)
> +			dev_warn(controller->dev, "INPUT_OVER_RUN\n");
> +
> +		controller->error = -EIO;
> +	}
> +
> +	if (spi_err) {
> +		if (spi_err & SPI_ERROR_CLK_OVER_RUN)
> +			dev_warn(controller->dev, "CLK_OVER_RUN\n");
> +		if (spi_err & SPI_ERROR_CLK_UNDER_RUN)
> +			dev_warn(controller->dev, "CLK_UNDER_RUN\n");
> +
> +		controller->error = -EIO;
> +	}
> +
> +	if (opflags & QUP_OP_IN_SERVICE_FLAG) {
> +		writel_relaxed(QUP_OP_IN_SERVICE_FLAG,
> +		       controller->base + QUP_OPERATIONAL);
> +		spi_qup_fifo_read(controller, xfer);
> +	}
> +
> +	if (opflags & QUP_OP_OUT_SERVICE_FLAG) {
> +		writel_relaxed(QUP_OP_OUT_SERVICE_FLAG,
> +		       controller->base + QUP_OPERATIONAL);
> +		spi_qup_fifo_write(controller, xfer);
> +	}
> +
> +	if (controller->rx_bytes == xfer->len ||
> +	    controller->error)
> +		complete(&controller->done);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int spi_qup_transfer_do(struct spi_qup *controller,
> +			       struct spi_qup_device *chip,
> +			       struct spi_transfer *xfer)
> +{
> +	unsigned long timeout;
> +	int ret = -EIO;
> +
> +	reinit_completion(&controller->done);
> +
> +	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> +	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> +	timeout = 100 * msecs_to_jiffies(timeout);
> +
> +	controller->rx_bytes = 0;
> +	controller->tx_bytes = 0;
> +	controller->error = 0;
> +	controller->xfer = xfer;
> +
> +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> +		dev_warn(controller->dev, "cannot set RUN state\n");
> +		goto exit;
> +	}
> +
> +	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> +		dev_warn(controller->dev, "cannot set PAUSE state\n");
> +		goto exit;
> +	}
> +
> +	spi_qup_fifo_write(controller, xfer);
> +
> +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> +		dev_warn(controller->dev, "cannot set EXECUTE state\n");
> +		goto exit;
> +	}
> +
> +	if (!wait_for_completion_timeout(&controller->done, timeout))
> +		ret = -ETIMEDOUT;
> +	else
> +		ret = controller->error;
> +exit:
> +	controller->xfer = NULL;

Should the manipulation of controller->xfer be protected by spinlock?

> +	controller->error = 0;
> +	controller->rx_bytes = 0;
> +	controller->tx_bytes = 0;
> +	spi_qup_set_state(controller, QUP_STATE_RESET);
> +	return ret;
> +}
> +
> +static int spi_qup_setup(struct spi_device *spi)
> +{
> +	struct spi_qup *controller = spi_master_get_devdata(spi->master);
> +	struct spi_qup_device *chip = spi_get_ctldata(spi);
> +
> +	if (spi->chip_select >= spi->master->num_chipselect) {
> +		dev_err(controller->dev, "invalid chip_select %d\n",
> +			spi->chip_select);
> +		return -EINVAL;
> +	}
> +
> +	if (spi->max_speed_hz > controller->max_speed_hz) {
> +		dev_err(controller->dev, "invalid max_speed_hz %d\n",
> +			spi->max_speed_hz);
> +		return -EINVAL;
> +	}
> +
> +	if (!chip) {
> +		/* First setup */
> +		chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +		if (!chip) {
> +			dev_err(controller->dev, "no memory for chip data\n");
> +			return -ENOMEM;
> +		}
> +
> +		spi_set_ctldata(spi, chip);
> +	}
> +
> +	return 0;
> +}
> +
> +static void spi_qup_cleanup(struct spi_device *spi)
> +{
> +	struct spi_qup_device *chip = spi_get_ctldata(spi);
> +
> +	if (!chip)
> +		return;
> +
> +	spi_set_ctldata(spi, NULL);
> +	kfree(chip);
> +}
> +
> +/* set clock freq, clock ramp, bits per work */
> +static int spi_qup_io_setup(struct spi_device *spi,
> +			  struct spi_transfer *xfer)
> +{
> +	struct spi_qup *controller = spi_master_get_devdata(spi->master);
> +	struct spi_qup_device *chip = spi_get_ctldata(spi);
> +	u32 iocontol, config, iomode, mode;
> +	int ret, n_words;
> +
> +	if (spi->mode & SPI_LOOP && xfer->len > controller->in_fifo_sz) {
> +		dev_err(controller->dev, "too big size for loopback %d > %d\n",
> +			xfer->len, controller->in_fifo_sz);
> +		return -EIO;
> +	}
> +
> +	chip->mode = spi->mode;
> +	chip->speed_hz = spi->max_speed_hz;
> +	if (xfer->speed_hz)
> +		chip->speed_hz = xfer->speed_hz;
> +
> +	if (controller->speed_hz != chip->speed_hz) {
> +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> +		if (ret) {
> +			dev_err(controller->dev, "fail to set frequency %d",
> +				chip->speed_hz);
> +			return -EIO;
> +		}
> +	}
> +
> +	controller->speed_hz = chip->speed_hz;
> +
> +	chip->bits_per_word = spi->bits_per_word;
> +	if (xfer->bits_per_word)
> +		chip->bits_per_word = xfer->bits_per_word;
> +
> +	if (chip->bits_per_word <= 8)
> +		controller->bytes_per_word = 1;
> +	else if (chip->bits_per_word <= 16)
> +		controller->bytes_per_word = 2;
> +	else
> +		controller->bytes_per_word = 4;
> +
> +	if (controller->bytes_per_word > xfer->len ||
> +	    xfer->len % controller->bytes_per_word != 0){
> +		/* No partial transfers */
> +		dev_err(controller->dev, "invalid len %d for %d bits\n",
> +			xfer->len, chip->bits_per_word);
> +		return -EIO;
> +	}
> +
> +	n_words = xfer->len / controller->bytes_per_word;
> +
> +	if (spi_qup_set_state(controller, QUP_STATE_RESET)) {
> +		dev_err(controller->dev, "cannot set RESET state\n");
> +		return -EIO;
> +	}
> +
> +	if (n_words <= controller->in_fifo_sz) {
> +		mode = QUP_IO_M_MODE_FIFO;
> +		writel_relaxed(n_words, controller->base + QUP_MX_READ_CNT);
> +		writel_relaxed(n_words, controller->base + QUP_MX_WRITE_CNT);
> +		/* must be zero for FIFO */
> +		writel_relaxed(0, controller->base + QUP_MX_INPUT_CNT);
> +		writel_relaxed(0, controller->base + QUP_MX_OUTPUT_CNT);
> +	} else {
> +		mode = QUP_IO_M_MODE_BLOCK;
> +		writel_relaxed(n_words, controller->base + QUP_MX_INPUT_CNT);
> +		writel_relaxed(n_words, controller->base + QUP_MX_OUTPUT_CNT);
> +		/* must be zero for BLOCK and BAM */
> +		writel_relaxed(0, controller->base + QUP_MX_READ_CNT);
> +		writel_relaxed(0, controller->base + QUP_MX_WRITE_CNT);
> +	}
> +
> +	iomode = readl_relaxed(controller->base + QUP_IO_M_MODES);
> +	/* Set input and output transfer mode */
> +	iomode &= ~(QUP_IO_M_INPUT_MODE_MASK | QUP_IO_M_OUTPUT_MODE_MASK);
> +	iomode &= ~(QUP_IO_M_PACK_EN | QUP_IO_M_UNPACK_EN);
> +	iomode |= (mode << QUP_IO_M_OUTPUT_MODE_MASK_SHIFT);
> +	iomode |= (mode << QUP_IO_M_INPUT_MODE_MASK_SHIFT);
> +
> +	writel_relaxed(iomode, controller->base + QUP_IO_M_MODES);
> +
> +	config = readl_relaxed(controller->base + SPI_CONFIG);
> +
> +	if (chip->mode & SPI_LOOP)
> +		config |= SPI_CONFIG_LOOPBACK;
> +	else
> +		config &= ~SPI_CONFIG_LOOPBACK;
> +
> +	if (chip->mode & SPI_CPHA)
> +		config &= ~SPI_CONFIG_INPUT_FIRST;
> +	else
> +		config |= SPI_CONFIG_INPUT_FIRST;
> +
> +	/*
> +	 * HS_MODE improves signal stability for spi-clk high rates
> +	 * but is invalid in loop back mode.
> +	 */
> +	if ((controller->speed_hz >= SPI_HS_MIN_RATE) &&
> +	    !(chip->mode & SPI_LOOP))
> +		config |= SPI_CONFIG_HS_MODE;
> +	else
> +		config &= ~SPI_CONFIG_HS_MODE;
> +
> +	writel_relaxed(config, controller->base + SPI_CONFIG);
> +
> +	config = readl_relaxed(controller->base + QUP_CONFIG);
> +	config &= ~(QUP_CONFIG_NO_INPUT | QUP_CONFIG_NO_OUTPUT | QUP_CONFIG_N);
> +	config |= chip->bits_per_word - 1;
> +	config |= QUP_CONFIG_SPI_MODE;
> +	writel_relaxed(config, controller->base + QUP_CONFIG);
> +
> +	iocontol = readl_relaxed(controller->base + SPI_IO_CONTROL);
> +
> +	/* Disable auto CS toggle */
> +	iocontol &= ~SPI_IO_C_MX_CS_MODE;
> +
> +	if (chip->mode & SPI_CPOL)
> +		iocontol |= SPI_IO_C_CLK_IDLE_HIGH;
> +	else
> +		iocontol &= ~SPI_IO_C_CLK_IDLE_HIGH;
> +
> +	writel_relaxed(iocontol, controller->base + SPI_IO_CONTROL);
> +
> +	/*
> +	 * TODO: In BAM mode mask INPUT and OUTPUT service flags in
> +	 * to prevent IRQs on FIFO status change.
> +	 */

Remove the TODO.  Not necessary.  This stuff can be added when it becomes BAM
enabled.

> +	writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK);
> +
> +	return 0;
> +}
> +
> +static int spi_qup_transfer_one(struct spi_master *master,
> +				struct spi_message *msg)
> +{
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +	struct spi_qup_device *chip = spi_get_ctldata(msg->spi);
> +	struct spi_transfer *xfer;
> +	struct spi_device *spi;
> +	unsigned cs_change;
> +	int status;
> +
> +	spi = msg->spi;
> +	cs_change = 1;
> +	status = 0;
> +
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +
> +		status = spi_qup_io_setup(spi, xfer);
> +		if (status)
> +			break;
> +

no locking?  This whole code block needs to have some type of mutex_lock to keep
others from trouncing the hardware while you are doing this transfer.

> +		if (cs_change)
> +			spi_qup_assert_cs(controller, chip);

Should the CS be done outside the loop?  I'd expect the following sequence to
happen:
- change CS
- Loop and do some transfers
- deassert CS

In this code, you reinitialize and assert/deassert CS for every transaction.

> +
> +		cs_change = xfer->cs_change;
> +
> +		/* Do actual transfer */
> +		status = spi_qup_transfer_do(controller, chip, xfer);
> +		if (status)
> +			break;
> +
> +		msg->actual_length += xfer->len;
> +
> +		if (xfer->delay_usecs)
> +			udelay(xfer->delay_usecs);
> +
> +		if (cs_change)
> +			spi_qup_deassert_cs(controller, chip);
> +	}
> +
> +	if (status || !cs_change)
> +		spi_qup_deassert_cs(controller, chip);
> +
> +	msg->status = status;
> +	spi_finalize_current_message(master);
> +	return status;
> +}
> +
> +static int spi_qup_probe(struct platform_device *pdev)
> +{
> +	struct spi_master *master;
> +	struct clk *iclk, *cclk;
> +	struct spi_qup *controller;
> +	struct resource *res;
> +	struct device *dev;
> +	void __iomem *base;
> +	u32 data, max_freq, iomode;
> +	int ret, irq, size;
> +
> +	dev = &pdev->dev;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +
> +	if (irq < 0)
> +		return irq;
> +
> +	cclk = devm_clk_get(dev, "core");
> +	if (IS_ERR(cclk)) {
> +		dev_err(dev, "cannot get core clock\n");
No need to error print.  devm_clk_get already outputs something
> +		return PTR_ERR(cclk);
> +	}
> +
> +	iclk = devm_clk_get(dev, "iface");
> +	if (IS_ERR(iclk)) {
> +		dev_err(dev, "cannot get iface clock\n");

No need to error print.  devm_clk_get already outputs something

> +		return PTR_ERR(iclk);
> +	}
> +
> +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> +		max_freq = 19200000;

I'd set the default to 50MHz as that is the max supported by hardware.  I'd just
set max_freq declaration to 50MHz and then check the value if it is changed via
DT.

> +
> +	if (!max_freq) {
> +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> +		return -ENXIO;
> +	}

This is buggy.  Remove this and collapse into the of_property_read_u32 if
statement.  On non-zero, check the range for validity.

> +
> +	ret = clk_set_rate(cclk, max_freq);
> +	if (ret)
> +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);

Bail here?

> +
> +	ret = clk_prepare_enable(cclk);
> +	if (ret) {
> +		dev_err(dev, "cannot enable core clock\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(iclk);
> +	if (ret) {
> +		clk_disable_unprepare(cclk);
> +		dev_err(dev, "cannot enable iface clock\n");
> +		return ret;
> +	}
> +
> +	data = readl_relaxed(base + QUP_HW_VERSION);
> +
> +	if (data < QUP_HW_VERSION_2_1_1) {
> +		clk_disable_unprepare(cclk);
> +		clk_disable_unprepare(iclk);
> +		dev_err(dev, "v.%08x is not supported\n", data);
> +		return -ENXIO;
> +	}
> +
> +	master = spi_alloc_master(dev, sizeof(struct spi_qup));
> +	if (!master) {
> +		clk_disable_unprepare(cclk);
> +		clk_disable_unprepare(iclk);
> +		dev_err(dev, "cannot allocate master\n");
> +		return -ENOMEM;
> +	}
> +
> +	master->bus_num = pdev->id;
> +	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
> +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> +	master->setup = spi_qup_setup;
> +	master->cleanup = spi_qup_cleanup;
> +	master->transfer_one_message = spi_qup_transfer_one;
> +	master->dev.of_node = pdev->dev.of_node;
> +	master->auto_runtime_pm = true;

Remove this.  No runtime support

> +
> +	platform_set_drvdata(pdev, master);
> +
> +	controller = spi_master_get_devdata(master);
> +
> +	controller->dev  = dev;
> +	controller->base = base;
> +	controller->iclk = iclk;
> +	controller->cclk = cclk;
> +	controller->irq  = irq;
> +	controller->max_speed_hz = clk_get_rate(cclk);
> +	controller->speed_hz = controller->max_speed_hz;
> +
> +	init_completion(&controller->done);
> +
> +	iomode = readl_relaxed(base + QUP_IO_M_MODES);
> +
> +	size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode);
> +	if (size)
> +		controller->out_blk_sz = size * 16;
> +	else
> +		controller->out_blk_sz = 4;
> +
> +	size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode);
> +	if (size)
> +		controller->in_blk_sz = size * 16;
> +	else
> +		controller->in_blk_sz = 4;
> +
> +	size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode);
> +	controller->out_fifo_sz = controller->out_blk_sz * (2 << size);
> +
> +	size = QUP_IO_M_INPUT_FIFO_SIZE(iomode);
> +	controller->in_fifo_sz = controller->in_blk_sz * (2 << size);
> +
> +	dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
> +		 data, controller->in_blk_sz, controller->in_fifo_sz,
> +		 controller->out_blk_sz, controller->out_fifo_sz);
> +
> +	writel_relaxed(1, base + QUP_SW_RESET);
> +
> +	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
> +	if (ret) {
> +		dev_err(dev, "cannot set RESET state\n");
> +		goto error;
> +	}
> +
> +	writel_relaxed(0, base + QUP_OPERATIONAL);
> +	writel_relaxed(0, base + QUP_IO_M_MODES);
> +	writel_relaxed(0, base + QUP_OPERATIONAL_MASK);
> +	writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN,
> +		       base + SPI_ERROR_FLAGS_EN);
> +
> +	writel_relaxed(0, base + SPI_CONFIG);
> +	writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL);
> +
> +	ret = devm_request_irq(dev, irq, spi_qup_qup_irq,
> +			       IRQF_TRIGGER_HIGH, pdev->name, controller);
> +	if (ret) {
> +		dev_err(dev, "cannot request IRQ %d\n", irq);

unnecessary print

> +		goto error;
> +	}
> +
> +	ret = devm_spi_register_master(dev, master);
> +	if (!ret) {
> +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> +		pm_runtime_use_autosuspend(dev);
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);

Remove all the runtime stuff.  not supported right now.

> +		return ret;
> +	}
> +error:
> +	clk_disable_unprepare(cclk);
> +	clk_disable_unprepare(iclk);
> +	spi_master_put(master);
> +	return ret;
> +}
> +
> +#ifdef CONFIG_PM_RUNTIME

Remove all the runtime stuff.  not supported right now.

> +static int spi_qup_pm_suspend_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	disable_irq(controller->irq);
> +	clk_disable_unprepare(controller->cclk);
> +	clk_disable_unprepare(controller->iclk);
> +	dev_dbg(device, "suspend runtime\n");
> +	return 0;
> +}
> +
> +static int spi_qup_pm_resume_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	clk_prepare_enable(controller->cclk);
> +	clk_prepare_enable(controller->iclk);
> +	enable_irq(controller->irq);
> +	dev_dbg(device, "resume runtime\n");
> +	return 0;
> +}
> +#endif /* CONFIG_PM_RUNTIME */
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int spi_qup_suspend(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +	int status;
> +
> +	status = spi_master_suspend(master);
> +	if (!status) {
> +		disable_irq(controller->irq);
> +		clk_disable_unprepare(controller->cclk);
> +		clk_disable_unprepare(controller->iclk);
> +	}
> +
> +	dev_dbg(device, "system suspend %d\n", status);
> +	return status;
> +}

Remove all the suspend/resume stuff.  not supported right now.

> +
> +static int spi_qup_resume(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +	int status;
> +
> +	clk_prepare_enable(controller->cclk);
> +	clk_prepare_enable(controller->iclk);
> +
> +	status = spi_master_resume(master);
> +
> +	dev_dbg(device, "system resume %d\n", status);
> +	return status;
> +}
> +#endif /* CONFIG_PM_SLEEP */

Remove all the suspend/resume stuff.  not supported right now.

> +
> +static int spi_qup_remove(struct platform_device *pdev)
> +{
> +	struct spi_master *master = dev_get_drvdata(&pdev->dev);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	pm_runtime_get_sync(&pdev->dev);
> +

Do we need to wait for any current transactions to complete
and do a devm_free_irq()?

> +	clk_disable_unprepare(controller->cclk);
> +	clk_disable_unprepare(controller->iclk);
> +
> +	pm_runtime_put_noidle(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	return 0;
> +}
> +
> +static struct of_device_id spi_qup_dt_match[] = {
> +	{ .compatible = "qcom,spi-qup-v2", },

Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1
(msm8974 v2)

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, spi_qup_dt_match);
> +
> +static const struct dev_pm_ops spi_qup_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(spi_qup_suspend, spi_qup_resume)
> +	SET_RUNTIME_PM_OPS(spi_qup_pm_suspend_runtime,
> +			   spi_qup_pm_resume_runtime,
> +			   NULL)
> +};

Remove this for now.

> +
> +static struct platform_driver spi_qup_driver = {
> +	.driver = {
> +		.name		= "spi_qup",
> +		.owner		= THIS_MODULE,
> +		.pm		= &spi_qup_dev_pm_ops,
> +		.of_match_table = spi_qup_dt_match,
> +	},
> +	.probe = spi_qup_probe,
> +	.remove = spi_qup_remove,
> +};
> +module_platform_driver(spi_qup_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("0.4");
> +MODULE_ALIAS("platform:spi_qup");
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/2] spi: qup: Add device tree bindings information

[Andy Gross]

On Thu, Feb 06, 2014 at 06:57:47PM +0200, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> 
> The Qualcomm Universal Peripheral (QUP) core is an
> AHB slave that provides a common data path (an output
> FIFO and an input FIFO) for serial peripheral interface
> (SPI) mini-core.
> 
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
>  .../devicetree/bindings/spi/qcom,spi-qup.txt       |   86 ++++++++++++++++++++
>  1 file changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
> new file mode 100644
> index 0000000..74565f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
> @@ -0,0 +1,86 @@
> +Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
> +
> +The QUP core is an AHB slave that provides a common data path (an output FIFO
> +and an input FIFO) for serial peripheral interface (SPI) mini-core.
> +
> +SPI in master mode support up to 50MHz, up to four chip selects, and a
> +programmable data path from 4 bits to 32 bits; supports numerous protocol
> +variants.
> +
> +Required properties:
> +- compatible: 		Should contain "qcom,spi-qup-v2".

Could be more descriptive.  qcom,spi-qup-v2.1.1 for MSM8974 v1 and
qcom,spi-qup-v2.2.1 for MSM8974 v2.

> +- reg: 				Should contain base register location and length
> +- interrupts: 		Interrupt number used by this controller
> +
> +- clocks: 			Should contain the core clock and the AHB clock.
> +- clock-names: 		Should be "core" for the core clock and "iface" for the
> +                	AHB clock.
> +
> +- #address-cells: 	Number of cells required to define a chip select
> +					address on the SPI bus. Should be set to 1.
> +- #size-cells: 		Should be zero.
> +
> +Optional properties:
> +- spi-max-frequency: Specifies maximum SPI clock frequency, Units - Hz. Definition
> +                    as per Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +SPI slave nodes must be children of the SPI master node and can
> +contain properties described in Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Example:
> +
> +	spi_8: spi@f9964000 { /* BLSP2 QUP2 */
> +
> +		compatible = "qcom,spi-qup-v2";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <0xf9964000 0x1000>;
> +		interrupts = <0 102 0>;
> +		spi-max-frequency = <19200000>;
> +
> +		clocks = <&gcc GCC_BLSP2_QUP2_SPI_APPS_CLK>, <&gcc GCC_BLSP2_AHB_CLK>;
> +		clock-names = "core", "iface";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&spi8_default>;
> +
> +		device@0 {
> +			compatible = "arm,pl022-dummy";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0>; /* Chip select 0 */
> +			spi-max-frequency = <19200000>;
> +			spi-cpol;
> +		};
> +
> +		device@1 {
> +			compatible = "arm,pl022-dummy";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <1>; /* Chip select 1 */
> +			spi-max-frequency = <9600000>;
> +			spi-cpha;
> +		};
> +
> +		device@2 {
> +			compatible = "arm,pl022-dummy";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <2>; /* Chip select 2 */
> +			spi-max-frequency = <19200000>;
> +			spi-cpol;
> +			spi-cpha;
> +		};
> +
> +		device@3 {
> +			compatible = "arm,pl022-dummy";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <3>; /* Chip select 3 */
> +			spi-max-frequency = <19200000>;
> +			spi-cpol;
> +			spi-cpha;
> +			spi-cs-high;
> +		};
> +	};
> +
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH RESEND 1/2] spi: qup: Add device tree bindings information

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

The Qualcomm Universal Peripheral (QUP) core is an
AHB slave that provides a common data path (an output
FIFO and an input FIFO) for serial peripheral interface
(SPI) mini-core.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 .../devicetree/bindings/spi/qcom,spi-qup.txt       |   86 ++++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt

diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
new file mode 100644
index 0000000..74565f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
@@ -0,0 +1,86 @@
+Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
+
+The QUP core is an AHB slave that provides a common data path (an output FIFO
+and an input FIFO) for serial peripheral interface (SPI) mini-core.
+
+SPI in master mode support up to 50MHz, up to four chip selects, and a
+programmable data path from 4 bits to 32 bits; supports numerous protocol
+variants.
+
+Required properties:
+- compatible: 		Should contain "qcom,spi-qup-v2".
+- reg: 				Should contain base register location and length
+- interrupts: 		Interrupt number used by this controller
+
+- clocks: 			Should contain the core clock and the AHB clock.
+- clock-names: 		Should be "core" for the core clock and "iface" for the
+                	AHB clock.
+
+- #address-cells: 	Number of cells required to define a chip select
+					address on the SPI bus. Should be set to 1.
+- #size-cells: 		Should be zero.
+
+Optional properties:
+- spi-max-frequency: Specifies maximum SPI clock frequency, Units - Hz. Definition
+                    as per Documentation/devicetree/bindings/spi/spi-bus.txt
+
+SPI slave nodes must be children of the SPI master node and can
+contain properties described in Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Example:
+
+	spi_8: spi@f9964000 { /* BLSP2 QUP2 */
+
+		compatible = "qcom,spi-qup-v2";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0xf9964000 0x1000>;
+		interrupts = <0 102 0>;
+		spi-max-frequency = <19200000>;
+
+		clocks = <&gcc GCC_BLSP2_QUP2_SPI_APPS_CLK>, <&gcc GCC_BLSP2_AHB_CLK>;
+		clock-names = "core", "iface";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&spi8_default>;
+
+		device@0 {
+			compatible = "arm,pl022-dummy";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0>; /* Chip select 0 */
+			spi-max-frequency = <19200000>;
+			spi-cpol;
+		};
+
+		device@1 {
+			compatible = "arm,pl022-dummy";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <1>; /* Chip select 1 */
+			spi-max-frequency = <9600000>;
+			spi-cpha;
+		};
+
+		device@2 {
+			compatible = "arm,pl022-dummy";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <2>; /* Chip select 2 */
+			spi-max-frequency = <19200000>;
+			spi-cpol;
+			spi-cpha;
+		};
+
+		device@3 {
+			compatible = "arm,pl022-dummy";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <3>; /* Chip select 3 */
+			spi-max-frequency = <19200000>;
+			spi-cpol;
+			spi-cpha;
+			spi-cs-high;
+		};
+	};
+
-- 
1.7.9.5

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Ivan T. Ivanov]

Hi Andy,

On Fri, 2014-02-07 at 01:39 -0600, Andy Gross wrote: 
> On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> > 
> > Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> > provides a common data path (an output FIFO and an input FIFO)
> > for serial peripheral interface (SPI) mini-core. SPI in master mode
> > support up to 50MHz, up to four chip selects, and a programmable
> > data path from 4 bits to 32 bits; MODE0..3 protocols
> > 
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > Cc: Alok Chauhan <alokc@codeaurora.org>
> > Cc: Gilad Avidov <gavidov@codeaurora.org>
> > Cc: Kiran Gunda <kgunda@codeaurora.org>
> > Cc: Sagar Dharia <sdharia@codeaurora.org>
> > ---
> >  drivers/spi/Kconfig   |   14 +
> >  drivers/spi/Makefile  |    1 +
> >  drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 913 insertions(+)
> >  create mode 100644 drivers/spi/spi-qup.c
> > 
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index ba9310b..bf8ce6b 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -381,6 +381,20 @@ config SPI_RSPI
> >  	help
> >  	  SPI driver for Renesas RSPI blocks.
> >  
> > +config SPI_QUP
> > +	tristate "Qualcomm SPI Support with QUP interface"
> > +	depends on ARCH_MSM
> 
> I'd change to ARCH_MSM_DT.  This ensures the OF component is there.

Ok. will change.

> 
> > +	help
> > +	  Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> > +	  provides a common data path (an output FIFO and an input FIFO)
> > +	  for serial peripheral interface (SPI) mini-core. SPI in master
> > +	  mode support up to 50MHz, up to four chip selects, and a
> > +	  programmable data path from 4 bits to 32 bits; supports numerous
> > +	  protocol variants.
> > +
> > +	  This driver can also be built as a module.  If so, the module
> > +	  will be called spi_qup.
> > +
> >  config SPI_S3C24XX
> >  	tristate "Samsung S3C24XX series SPI"
> >  	depends on ARCH_S3C24XX
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 95af48d..e598147 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -59,6 +59,7 @@ spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA)	+= spi-pxa2xx-pxadma.o
> >  spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA)	+= spi-pxa2xx-dma.o
> >  obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
> >  obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
> > +obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
> >  obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
> >  obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
> >  spi-s3c24xx-hw-y			:= spi-s3c24xx.o
> > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> > new file mode 100644
> > index 0000000..5eb5e8f
> > --- /dev/null
> > +++ b/drivers/spi/spi-qup.c
> > @@ -0,0 +1,898 @@
> > +/*
> > + * Copyright (c) 2008-2014, The Linux foundation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License rev 2 and
> > + * only rev 2 as published by the free Software foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> 
> Remove this for now.  No runtime support.

Did you see any particular issue with the implementation 
or this is just because this platform didn't have support 
for power management?  

> 
> > +#include <linux/spi/spi.h>
> > +

<snip>

> > +
> > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > +			       struct spi_qup_device *chip,
> > +			       struct spi_transfer *xfer)
> > +{
> > +	unsigned long timeout;
> > +	int ret = -EIO;
> > +
> > +	reinit_completion(&controller->done);
> > +
> > +	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> > +	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> > +	timeout = 100 * msecs_to_jiffies(timeout);
> > +
> > +	controller->rx_bytes = 0;
> > +	controller->tx_bytes = 0;
> > +	controller->error = 0;
> > +	controller->xfer = xfer;
> > +
> > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > +		dev_warn(controller->dev, "cannot set RUN state\n");
> > +		goto exit;
> > +	}
> > +
> > +	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> > +		dev_warn(controller->dev, "cannot set PAUSE state\n");
> > +		goto exit;
> > +	}
> > +
> > +	spi_qup_fifo_write(controller, xfer);
> > +
> > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > +		dev_warn(controller->dev, "cannot set EXECUTE state\n");
> > +		goto exit;
> > +	}
> > +
> > +	if (!wait_for_completion_timeout(&controller->done, timeout))
> > +		ret = -ETIMEDOUT;
> > +	else
> > +		ret = controller->error;
> > +exit:
> > +	controller->xfer = NULL;
> 
> Should the manipulation of controller->xfer be protected by spinlock?

:-). Probably. I am wondering, could I avoid locking if firstly place
QUP into RESET state and then access these field. This should stop
all activities in it, right?
> 
> > +	controller->error = 0;
> > +	controller->rx_bytes = 0;
> > +	controller->tx_bytes = 0;
> > +	spi_qup_set_state(controller, QUP_STATE_RESET);
> > +	return ret;
> > +}
> > +

<snip>

> > +
> > +/* set clock freq, clock ramp, bits per work */
> > +static int spi_qup_io_setup(struct spi_device *spi,
> > +			  struct spi_transfer *xfer)
> > +{

<snip>

> > +
> > +	/*
> > +	 * TODO: In BAM mode mask INPUT and OUTPUT service flags in
> > +	 * to prevent IRQs on FIFO status change.
> > +	 */
> 
> Remove the TODO.  Not necessary.  This stuff can be added when it becomes BAM
> enabled.

Ok. 

> 
> > +	writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK);
> > +
> > +	return 0;
> > +}
> > +
> > +static int spi_qup_transfer_one(struct spi_master *master,
> > +				struct spi_message *msg)
> > +{
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +	struct spi_qup_device *chip = spi_get_ctldata(msg->spi);
> > +	struct spi_transfer *xfer;
> > +	struct spi_device *spi;
> > +	unsigned cs_change;
> > +	int status;
> > +
> > +	spi = msg->spi;
> > +	cs_change = 1;
> > +	status = 0;
> > +
> > +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> > +
> > +		status = spi_qup_io_setup(spi, xfer);
> > +		if (status)
> > +			break;
> > +
> 
> no locking?  This whole code block needs to have some type of mutex_lock to keep
> others from trouncing the hardware while you are doing this transfer.

This is handled by SPI framework.

> 
> > +		if (cs_change)
> > +			spi_qup_assert_cs(controller, chip);
> 
> Should the CS be done outside the loop?  I'd expect the following sequence to
> happen:
> - change CS
> - Loop and do some transfers
> - deassert CS
> 
> In this code, you reinitialize and assert/deassert CS for every transaction.
> 
> > +
> > +		cs_change = xfer->cs_change;


Not exactly. It is allowed that CS goes inactive after every
transaction. This is how I read struct spi_transfer description.

> > +
> > +		/* Do actual transfer */
> > +		status = spi_qup_transfer_do(controller, chip, xfer);
> > +		if (status)
> > +			break;
> > +
> > +		msg->actual_length += xfer->len;
> > +
> > +		if (xfer->delay_usecs)
> > +			udelay(xfer->delay_usecs);
> > +
> > +		if (cs_change)
> > +			spi_qup_deassert_cs(controller, chip);
> > +	}
> > +
> > +	if (status || !cs_change)
> > +		spi_qup_deassert_cs(controller, chip);
> > +
> > +	msg->status = status;
> > +	spi_finalize_current_message(master);
> > +	return status;
> > +}
> > +
> > +static int spi_qup_probe(struct platform_device *pdev)
> > +{
> > +	struct spi_master *master;
> > +	struct clk *iclk, *cclk;
> > +	struct spi_qup *controller;
> > +	struct resource *res;
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	u32 data, max_freq, iomode;
> > +	int ret, irq, size;
> > +
> > +	dev = &pdev->dev;
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	cclk = devm_clk_get(dev, "core");
> > +	if (IS_ERR(cclk)) {
> > +		dev_err(dev, "cannot get core clock\n");
> No need to error print.  devm_clk_get already outputs something

Ok.

> > +		return PTR_ERR(cclk);
> > +	}
> > +
> > +	iclk = devm_clk_get(dev, "iface");
> > +	if (IS_ERR(iclk)) {
> > +		dev_err(dev, "cannot get iface clock\n");
> 
> No need to error print.  devm_clk_get already outputs something

Ok.

> 
> > +		return PTR_ERR(iclk);
> > +	}
> > +
> > +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > +		max_freq = 19200000;
> 
> I'd set the default to 50MHz as that is the max supported by hardware.  I'd just
> set max_freq declaration to 50MHz and then check the value if it is changed via
> DT.

50MHz doesn't seems to be supported on all chip sets. Currently common
denominator on all chip sets, that I can see, is 19.2MHz. I have tried 
to test it with more than 19.2MHz on APQ8074 and it fails.

> 
> > +
> > +	if (!max_freq) {
> > +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> > +		return -ENXIO;
> > +	}
> 
> This is buggy.  Remove this and collapse into the of_property_read_u32 if
> statement.  On non-zero, check the range for validity.

True. Will fix.

> 
> > +
> > +	ret = clk_set_rate(cclk, max_freq);
> > +	if (ret)
> > +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> 
> Bail here?

I don't know. What will be the consequences if controller continue to
operate on its default rate?

> 
> > +
> > +	ret = clk_prepare_enable(cclk);
> > +	if (ret) {
> > +		dev_err(dev, "cannot enable core clock\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(iclk);
> > +	if (ret) {
> > +		clk_disable_unprepare(cclk);
> > +		dev_err(dev, "cannot enable iface clock\n");
> > +		return ret;
> > +	}
> > +
> > +	data = readl_relaxed(base + QUP_HW_VERSION);
> > +
> > +	if (data < QUP_HW_VERSION_2_1_1) {
> > +		clk_disable_unprepare(cclk);
> > +		clk_disable_unprepare(iclk);
> > +		dev_err(dev, "v.%08x is not supported\n", data);
> > +		return -ENXIO;
> > +	}
> > +
> > +	master = spi_alloc_master(dev, sizeof(struct spi_qup));
> > +	if (!master) {
> > +		clk_disable_unprepare(cclk);
> > +		clk_disable_unprepare(iclk);
> > +		dev_err(dev, "cannot allocate master\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	master->bus_num = pdev->id;
> > +	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
> > +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> > +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> > +	master->setup = spi_qup_setup;
> > +	master->cleanup = spi_qup_cleanup;
> > +	master->transfer_one_message = spi_qup_transfer_one;
> > +	master->dev.of_node = pdev->dev.of_node;
> > +	master->auto_runtime_pm = true;
> 
> Remove this.  No runtime support
> 
> > +
> > +	platform_set_drvdata(pdev, master);
> > +
> > +	controller = spi_master_get_devdata(master);
> > +
> > +	controller->dev  = dev;
> > +	controller->base = base;
> > +	controller->iclk = iclk;
> > +	controller->cclk = cclk;
> > +	controller->irq  = irq;
> > +	controller->max_speed_hz = clk_get_rate(cclk);
> > +	controller->speed_hz = controller->max_speed_hz;
> > +
> > +	init_completion(&controller->done);
> > +
> > +	iomode = readl_relaxed(base + QUP_IO_M_MODES);
> > +
> > +	size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode);
> > +	if (size)
> > +		controller->out_blk_sz = size * 16;
> > +	else
> > +		controller->out_blk_sz = 4;
> > +
> > +	size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode);
> > +	if (size)
> > +		controller->in_blk_sz = size * 16;
> > +	else
> > +		controller->in_blk_sz = 4;
> > +
> > +	size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode);
> > +	controller->out_fifo_sz = controller->out_blk_sz * (2 << size);
> > +
> > +	size = QUP_IO_M_INPUT_FIFO_SIZE(iomode);
> > +	controller->in_fifo_sz = controller->in_blk_sz * (2 << size);
> > +
> > +	dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
> > +		 data, controller->in_blk_sz, controller->in_fifo_sz,
> > +		 controller->out_blk_sz, controller->out_fifo_sz);
> > +
> > +	writel_relaxed(1, base + QUP_SW_RESET);
> > +
> > +	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
> > +	if (ret) {
> > +		dev_err(dev, "cannot set RESET state\n");
> > +		goto error;
> > +	}
> > +
> > +	writel_relaxed(0, base + QUP_OPERATIONAL);
> > +	writel_relaxed(0, base + QUP_IO_M_MODES);
> > +	writel_relaxed(0, base + QUP_OPERATIONAL_MASK);
> > +	writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN,
> > +		       base + SPI_ERROR_FLAGS_EN);
> > +
> > +	writel_relaxed(0, base + SPI_CONFIG);
> > +	writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL);
> > +
> > +	ret = devm_request_irq(dev, irq, spi_qup_qup_irq,
> > +			       IRQF_TRIGGER_HIGH, pdev->name, controller);
> > +	if (ret) {
> > +		dev_err(dev, "cannot request IRQ %d\n", irq);
> 
> unnecessary print

Will remove.

> 
> > +		goto error;
> > +	}
> > +
> > +	ret = devm_spi_register_master(dev, master);
> > +	if (!ret) {
> > +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> > +		pm_runtime_use_autosuspend(dev);
> > +		pm_runtime_set_active(dev);
> > +		pm_runtime_enable(dev);
> 
> Remove all the runtime stuff.  not supported right now.
> 
> > +		return ret;
> > +	}
> > +error:
> > +	clk_disable_unprepare(cclk);
> > +	clk_disable_unprepare(iclk);
> > +	spi_master_put(master);
> > +	return ret;
> > +}
> > +

<snip>

> 
> > +
> > +static int spi_qup_remove(struct platform_device *pdev)
> > +{
> > +	struct spi_master *master = dev_get_drvdata(&pdev->dev);
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +
> > +	pm_runtime_get_sync(&pdev->dev);
> > +
> 
> Do we need to wait for any current transactions to complete
> and do a devm_free_irq()?
> 
> > +	clk_disable_unprepare(controller->cclk);
> > +	clk_disable_unprepare(controller->iclk);

My understanding is:

Disabling clocks will timeout transaction, if any. Core Device driver
will call: devm_spi_unregister(), which will wait pending transactions
to complete and then remove the SPI master.

> > +
> > +	pm_runtime_put_noidle(&pdev->dev);
> > +	pm_runtime_disable(&pdev->dev);
> > +	return 0;
> > +}
> > +
> > +static struct of_device_id spi_qup_dt_match[] = {
> > +	{ .compatible = "qcom,spi-qup-v2", },
> 
> Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1
> (msm8974 v2)

I am not aware of the difference. My board report v.20020000. 
Is there difference of handling these controllers?


Thanks, 
Ivan

Re: [PATCH RESEND 1/2] spi: qup: Add device tree bindings information

[Mark Brown]

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

On Fri, Feb 07, 2014 at 09:43:27AM +0200, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> 
> The Qualcomm Universal Peripheral (QUP) core is an
> AHB slave that provides a common data path (an output
> FIFO and an input FIFO) for serial peripheral interface
> (SPI) mini-core.

Please fix the formatting of this document so the lines are less than 80
columns, it's hard to read as is.  Otherwise this is fine.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND 1/2] spi: qup: Add device tree bindings information

[Ivan T. Ivanov]

On Fri, 2014-02-07 at 12:27 +0000, Mark Brown wrote: 
> On Fri, Feb 07, 2014 at 09:43:27AM +0200, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> > 
> > The Qualcomm Universal Peripheral (QUP) core is an
> > AHB slave that provides a common data path (an output
> > FIFO and an input FIFO) for serial peripheral interface
> > (SPI) mini-core.
> 
> Please fix the formatting of this document so the lines are less than 80
> columns, it's hard to read as is.  Otherwise this is fine.

File is looking fine in editor with smart tab support

Regards,
Ivan

Re: [PATCH RESEND 1/2] spi: qup: Add device tree bindings information

[Mark Brown]

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

On Fri, Feb 07, 2014 at 03:00:43PM +0200, Ivan T. Ivanov wrote:
> On Fri, 2014-02-07 at 12:27 +0000, Mark Brown wrote: 

> > Please fix the formatting of this document so the lines are less than 80
> > columns, it's hard to read as is.  Otherwise this is fine.

> File is looking fine in editor with smart tab support

I don't know what "smart tab support" is intended to be in this context
but whatever it is it's not working well in the e-mail you sent, bear in
mind that you're looking for a readable patch.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND 1/2] spi: qup: Add device tree bindings information

[Ivan T. Ivanov]

On Fri, 2014-02-07 at 13:13 +0000, Mark Brown wrote: 
> On Fri, Feb 07, 2014 at 03:00:43PM +0200, Ivan T. Ivanov wrote:
> > On Fri, 2014-02-07 at 12:27 +0000, Mark Brown wrote: 
> 
> > > Please fix the formatting of this document so the lines are less than 80
> > > columns, it's hard to read as is.  Otherwise this is fine.
> 
> > File is looking fine in editor with smart tab support
> 
> I don't know what "smart tab support" is intended to be in this context
> but whatever it is it's not working well in the e-mail you sent, bear in
> mind that you're looking for a readable patch.

Right. will fix it.

Regards,
Ivan

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Josh Cartwright]

On Fri, Feb 07, 2014 at 01:39:52AM -0600, Andy Gross wrote:
> On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> > 
> > Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> > provides a common data path (an output FIFO and an input FIFO)
> > for serial peripheral interface (SPI) mini-core. SPI in master mode
> > support up to 50MHz, up to four chip selects, and a programmable
> > data path from 4 bits to 32 bits; MODE0..3 protocols
> > 
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > Cc: Alok Chauhan <alokc@codeaurora.org>
> > Cc: Gilad Avidov <gavidov@codeaurora.org>
> > Cc: Kiran Gunda <kgunda@codeaurora.org>
> > Cc: Sagar Dharia <sdharia@codeaurora.org>
> > ---
> >  drivers/spi/Kconfig   |   14 +
> >  drivers/spi/Makefile  |    1 +
> >  drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 913 insertions(+)
> >  create mode 100644 drivers/spi/spi-qup.c
> > 
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index ba9310b..bf8ce6b 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -381,6 +381,20 @@ config SPI_RSPI
> >  	help
> >  	  SPI driver for Renesas RSPI blocks.
> >  
> > +config SPI_QUP
> > +	tristate "Qualcomm SPI Support with QUP interface"
> > +	depends on ARCH_MSM
> 
> I'd change to ARCH_MSM_DT.  This ensures the OF component is there.

I'd rather explicitly include the CONFIG_OF dependency, but I'm not too
opinionated.

	config SPI_QUP
		tristate "Qualcomm SPI Support with QUP interface"
		depends on OF
		depends on ARM
		depends on ARCH_MSM_DT || COMPILE_TEST

With Kumar's pending the ARCH_MSM_DT -> ARCH_QCOM rename, we'll
introduce a arm-soc/spi tree dependency here that we'll need to keep
track of.

Kumar-

How would you like to handle this?  Would it make sense for this to go
through the SPI tree with depending on ARCH_QCOM instead of ARCH_MSM_DT?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Mark Brown]

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

On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:

This looks mostly good, there's a few odd things and missing use of
framework features.

> Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> provides a common data path (an output FIFO and an input FIFO)
> for serial peripheral interface (SPI) mini-core. SPI in master mode
> support up to 50MHz, up to four chip selects, and a programmable
> data path from 4 bits to 32 bits; MODE0..3 protocols

The grammar in this and the Kconfig text is a bit garbled, might want to
give it a once over (support -> supports for example).

> +static void spi_qup_deassert_cs(struct spi_qup *controller,
> +				struct spi_qup_device *chip)
> +{


> +	if (chip->mode & SPI_CS_HIGH)
> +		iocontol &= ~mask;
> +	else
> +		iocontol |= mask;

Implement a set_cs() operation and let the core worry about all this
for you as well as saving two implementations.

> +		word = 0;
> +		for (idx = 0; idx < controller->bytes_per_word &&
> +		     controller->tx_bytes < xfer->len; idx++,
> +		     controller->tx_bytes++) {
> +
> +			if (!tx_buf)
> +				continue;

Do you need to set the _MUST_TX flag?

> +	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> +	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> +	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +
> +	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> +	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> +	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> +
> +	if (!xfer)
> +		return IRQ_HANDLED;

Are you sure?  It seems wrong to just ignore interrupts, some comments
would help explain why.

> +static int spi_qup_transfer_do(struct spi_qup *controller,
> +			       struct spi_qup_device *chip,
> +			       struct spi_transfer *xfer)

This looks like a transfer_one() function, please use the framework
features where you can.

> +	if (controller->speed_hz != chip->speed_hz) {
> +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> +		if (ret) {
> +			dev_err(controller->dev, "fail to set frequency %d",
> +				chip->speed_hz);
> +			return -EIO;
> +		}
> +	}

Is calling into the clock framework really so expensive that we need to
avoid doing it?  You also shouldn't be interacting with the hardware in
setup().

> +	if (chip->bits_per_word <= 8)
> +		controller->bytes_per_word = 1;
> +	else if (chip->bits_per_word <= 16)
> +		controller->bytes_per_word = 2;
> +	else
> +		controller->bytes_per_word = 4;

This looks like a switch statement, and looking at the above it's not
clear that the device actually supports anything other than whole bytes.
I'm not sure what that would mean from an API point of view.

> +static int spi_qup_transfer_one(struct spi_master *master,
> +				struct spi_message *msg)
> +{

This entire function can be removed, the core can do it for you.

> +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> +		max_freq = 19200000;
> +
> +	if (!max_freq) {
> +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> +		return -ENXIO;
> +	}
> +
> +	ret = clk_set_rate(cclk, max_freq);
> +	if (ret)
> +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);

You set the clock rate per transfer so why bother setting it here,
perhaps we support the rate the devices request but not this maximum
rate?

> +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);

Are you *sure* the device supports anything other than whole bytes?

> +	ret = devm_spi_register_master(dev, master);
> +	if (!ret) {
> +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> +		pm_runtime_use_autosuspend(dev);
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);
> +		return ret;
> +	}

This is really unclearly written, the success case looks like error
handling.

> +#ifdef CONFIG_PM_RUNTIME
> +static int spi_qup_pm_suspend_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	disable_irq(controller->irq);

Why do you need to disable the interrupt?  Will the hardware generate
spurious interrupts, if so some documentation is in order.

> +static int spi_qup_pm_resume_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	clk_prepare_enable(controller->cclk);
> +	clk_prepare_enable(controller->iclk);
> +	enable_irq(controller->irq);

No error checking here...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Mark Brown]

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

On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:

> 	config SPI_QUP
> 		tristate "Qualcomm SPI Support with QUP interface"
> 		depends on OF
> 		depends on ARM

Does this really depend on ARM?  If so why?

> 		depends on ARCH_MSM_DT || COMPILE_TEST

> With Kumar's pending the ARCH_MSM_DT -> ARCH_QCOM rename, we'll
> introduce a arm-soc/spi tree dependency here that we'll need to keep
> track of.

It seems simpler to just depend on MSM_DT || ARCH_QCOM or whatever.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Josh Cartwright]

Hey Mark-

On Fri, Feb 07, 2014 at 05:18:34PM +0000, Mark Brown wrote:
> On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:
> 
> > 	config SPI_QUP
> > 		tristate "Qualcomm SPI Support with QUP interface"
> > 		depends on OF
> > 		depends on ARM
> 
> Does this really depend on ARM?  If so why?

The ARM dependency is there for the use of _relaxed io accessor
variants.

> > 		depends on ARCH_MSM_DT || COMPILE_TEST
> 
> > With Kumar's pending the ARCH_MSM_DT -> ARCH_QCOM rename, we'll
> > introduce a arm-soc/spi tree dependency here that we'll need to keep
> > track of.
> 
> It seems simpler to just depend on MSM_DT || ARCH_QCOM or whatever.

ARCH_MSM_DT is going away, so maybe this is the best option for the
short term (a later patch can remove ARCH_MSM_DT from here at some point
in the future).

  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Mark Brown]

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

On Fri, Feb 07, 2014 at 11:20:51AM -0600, Josh Cartwright wrote:
> On Fri, Feb 07, 2014 at 05:18:34PM +0000, Mark Brown wrote:
> > On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:

> > > 	config SPI_QUP
> > > 		tristate "Qualcomm SPI Support with QUP interface"
> > > 		depends on OF
> > > 		depends on ARM

> > Does this really depend on ARM?  If so why?

> The ARM dependency is there for the use of _relaxed io accessor
> variants.

That's not ARM only and I thought we were getting generic versions of it
anyway?  ARMv8, MIPS, Microblaze, Hexagon and SH also define it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Andy Gross]

On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:
> 
> Hi Andy,
> 
> On Fri, 2014-02-07 at 01:39 -0600, Andy Gross wrote: 
> > On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> > > 
> > > Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> > > provides a common data path (an output FIFO and an input FIFO)
> > > for serial peripheral interface (SPI) mini-core. SPI in master mode
> > > support up to 50MHz, up to four chip selects, and a programmable
> > > data path from 4 bits to 32 bits; MODE0..3 protocols
> > > 
> > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > > Cc: Alok Chauhan <alokc@codeaurora.org>
> > > Cc: Gilad Avidov <gavidov@codeaurora.org>
> > > Cc: Kiran Gunda <kgunda@codeaurora.org>
> > > Cc: Sagar Dharia <sdharia@codeaurora.org>
> > > ---
> > >  drivers/spi/Kconfig   |   14 +
> > >  drivers/spi/Makefile  |    1 +
> > >  drivers/spi/spi-qup.c |  898 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 913 insertions(+)
> > >  create mode 100644 drivers/spi/spi-qup.c
> > > 
> > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > > index ba9310b..bf8ce6b 100644
> > > --- a/drivers/spi/Kconfig
> > > +++ b/drivers/spi/Kconfig
> > > @@ -381,6 +381,20 @@ config SPI_RSPI
> > >  	help
> > >  	  SPI driver for Renesas RSPI blocks.
> > >  
> > > +config SPI_QUP
> > > +	tristate "Qualcomm SPI Support with QUP interface"
> > > +	depends on ARCH_MSM
> > 
> > I'd change to ARCH_MSM_DT.  This ensures the OF component is there.
> 
> Ok. will change.
> 
> > 
> > > +	help
> > > +	  Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> > > +	  provides a common data path (an output FIFO and an input FIFO)
> > > +	  for serial peripheral interface (SPI) mini-core. SPI in master
> > > +	  mode support up to 50MHz, up to four chip selects, and a
> > > +	  programmable data path from 4 bits to 32 bits; supports numerous
> > > +	  protocol variants.
> > > +
> > > +	  This driver can also be built as a module.  If so, the module
> > > +	  will be called spi_qup.
> > > +
> > >  config SPI_S3C24XX
> > >  	tristate "Samsung S3C24XX series SPI"
> > >  	depends on ARCH_S3C24XX
> > > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > > index 95af48d..e598147 100644
> > > --- a/drivers/spi/Makefile
> > > +++ b/drivers/spi/Makefile
> > > @@ -59,6 +59,7 @@ spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA)	+= spi-pxa2xx-pxadma.o
> > >  spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA)	+= spi-pxa2xx-dma.o
> > >  obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
> > >  obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
> > > +obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
> > >  obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
> > >  obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
> > >  spi-s3c24xx-hw-y			:= spi-s3c24xx.o
> > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> > > new file mode 100644
> > > index 0000000..5eb5e8f
> > > --- /dev/null
> > > +++ b/drivers/spi/spi-qup.c
> > > @@ -0,0 +1,898 @@
> > > +/*
> > > + * Copyright (c) 2008-2014, The Linux foundation. All rights reserved.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License rev 2 and
> > > + * only rev 2 as published by the free Software foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/err.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/list.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > 
> > Remove this for now.  No runtime support.
> 
> Did you see any particular issue with the implementation 
> or this is just because this platform didn't have support 
> for power management?  
> 

The platform doesn't have support for PM right now.  So it's probably better to
remove all this and revisit later when it is in place.

> > > +#include <linux/spi/spi.h>
> > > +
> 
> <snip>
> 
> > > +
> > > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > > +			       struct spi_qup_device *chip,
> > > +			       struct spi_transfer *xfer)
> > > +{
> > > +	unsigned long timeout;
> > > +	int ret = -EIO;
> > > +
> > > +	reinit_completion(&controller->done);
> > > +
> > > +	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> > > +	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> > > +	timeout = 100 * msecs_to_jiffies(timeout);
> > > +
> > > +	controller->rx_bytes = 0;
> > > +	controller->tx_bytes = 0;
> > > +	controller->error = 0;
> > > +	controller->xfer = xfer;
> > > +
> > > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > +		dev_warn(controller->dev, "cannot set RUN state\n");
> > > +		goto exit;
> > > +	}
> > > +
> > > +	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> > > +		dev_warn(controller->dev, "cannot set PAUSE state\n");
> > > +		goto exit;
> > > +	}
> > > +
> > > +	spi_qup_fifo_write(controller, xfer);
> > > +
> > > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > +		dev_warn(controller->dev, "cannot set EXECUTE state\n");
> > > +		goto exit;
> > > +	}
> > > +
> > > +	if (!wait_for_completion_timeout(&controller->done, timeout))
> > > +		ret = -ETIMEDOUT;
> > > +	else
> > > +		ret = controller->error;
> > > +exit:
> > > +	controller->xfer = NULL;
> > 
> > Should the manipulation of controller->xfer be protected by spinlock?
> 
> :-). Probably. I am wondering, could I avoid locking if firstly place
> QUP into RESET state and then access these field. This should stop
> all activities in it, right?

It's generally safest to not assume the hardware is going to do sane things.
I'm concerned about spurious IRQs.

> > 
> > > +	controller->error = 0;
> > > +	controller->rx_bytes = 0;
> > > +	controller->tx_bytes = 0;
> > > +	spi_qup_set_state(controller, QUP_STATE_RESET);
> > > +	return ret;
> > > +}
> > > +
> 
> <snip>
> 
> > > +
> > > +/* set clock freq, clock ramp, bits per work */
> > > +static int spi_qup_io_setup(struct spi_device *spi,
> > > +			  struct spi_transfer *xfer)
> > > +{
> 
> <snip>
> 
> > > +
> > > +	/*
> > > +	 * TODO: In BAM mode mask INPUT and OUTPUT service flags in
> > > +	 * to prevent IRQs on FIFO status change.
> > > +	 */
> > 
> > Remove the TODO.  Not necessary.  This stuff can be added when it becomes BAM
> > enabled.
> 
> Ok. 
> 
> > 
> > > +	writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int spi_qup_transfer_one(struct spi_master *master,
> > > +				struct spi_message *msg)
> > > +{
> > > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > > +	struct spi_qup_device *chip = spi_get_ctldata(msg->spi);
> > > +	struct spi_transfer *xfer;
> > > +	struct spi_device *spi;
> > > +	unsigned cs_change;
> > > +	int status;
> > > +
> > > +	spi = msg->spi;
> > > +	cs_change = 1;
> > > +	status = 0;
> > > +
> > > +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> > > +
> > > +		status = spi_qup_io_setup(spi, xfer);
> > > +		if (status)
> > > +			break;
> > > +
> > 
> > no locking?  This whole code block needs to have some type of mutex_lock to keep
> > others from trouncing the hardware while you are doing this transfer.
> 
> This is handled by SPI framework.
> 

Ah I looked through that and didn't see it the first time.  But looking again, I
see it.  You're right, you can ignore this comment.

> > 
> > > +		if (cs_change)
> > > +			spi_qup_assert_cs(controller, chip);
> > 
> > Should the CS be done outside the loop?  I'd expect the following sequence to
> > happen:
> > - change CS
> > - Loop and do some transfers
> > - deassert CS
> > 
> > In this code, you reinitialize and assert/deassert CS for every transaction.
> > 
> > > +
> > > +		cs_change = xfer->cs_change;
> 
> 
> Not exactly. It is allowed that CS goes inactive after every
> transaction. This is how I read struct spi_transfer description.

Ah ok.  This is fine then.

> 
> > > +
> > > +		/* Do actual transfer */
> > > +		status = spi_qup_transfer_do(controller, chip, xfer);
> > > +		if (status)
> > > +			break;
> > > +
> > > +		msg->actual_length += xfer->len;
> > > +
> > > +		if (xfer->delay_usecs)
> > > +			udelay(xfer->delay_usecs);
> > > +
> > > +		if (cs_change)
> > > +			spi_qup_deassert_cs(controller, chip);
> > > +	}
> > > +
> > > +	if (status || !cs_change)
> > > +		spi_qup_deassert_cs(controller, chip);
> > > +
> > > +	msg->status = status;
> > > +	spi_finalize_current_message(master);
> > > +	return status;
> > > +}
> > > +
> > > +static int spi_qup_probe(struct platform_device *pdev)
> > > +{
> > > +	struct spi_master *master;
> > > +	struct clk *iclk, *cclk;
> > > +	struct spi_qup *controller;
> > > +	struct resource *res;
> > > +	struct device *dev;
> > > +	void __iomem *base;
> > > +	u32 data, max_freq, iomode;
> > > +	int ret, irq, size;
> > > +
> > > +	dev = &pdev->dev;
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	base = devm_ioremap_resource(dev, res);
> > > +	if (IS_ERR(base))
> > > +		return PTR_ERR(base);
> > > +
> > > +	irq = platform_get_irq(pdev, 0);
> > > +
> > > +	if (irq < 0)
> > > +		return irq;
> > > +
> > > +	cclk = devm_clk_get(dev, "core");
> > > +	if (IS_ERR(cclk)) {
> > > +		dev_err(dev, "cannot get core clock\n");
> > No need to error print.  devm_clk_get already outputs something
> 
> Ok.
> 
> > > +		return PTR_ERR(cclk);
> > > +	}
> > > +
> > > +	iclk = devm_clk_get(dev, "iface");
> > > +	if (IS_ERR(iclk)) {
> > > +		dev_err(dev, "cannot get iface clock\n");
> > 
> > No need to error print.  devm_clk_get already outputs something
> 
> Ok.
> 
> > 
> > > +		return PTR_ERR(iclk);
> > > +	}
> > > +
> > > +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > > +		max_freq = 19200000;
> > 
> > I'd set the default to 50MHz as that is the max supported by hardware.  I'd just
> > set max_freq declaration to 50MHz and then check the value if it is changed via
> > DT.
> 
> 50MHz doesn't seems to be supported on all chip sets. Currently common
> denominator on all chip sets, that I can see, is 19.2MHz. I have tried 
> to test it with more than 19.2MHz on APQ8074 and it fails.
> 

I guess my stance is to set it to the hardware max supported frequency if it is
not specified.  If that needs to be lower on a board because of whatever reason,
they override it.

> > 
> > > +
> > > +	if (!max_freq) {
> > > +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> > > +		return -ENXIO;
> > > +	}
> > 
> > This is buggy.  Remove this and collapse into the of_property_read_u32 if
> > statement.  On non-zero, check the range for validity.
> 
> True. Will fix.
> 
> > 
> > > +
> > > +	ret = clk_set_rate(cclk, max_freq);
> > > +	if (ret)
> > > +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> > 
> > Bail here?
> 
> I don't know. What will be the consequences if controller continue to
> operate on its default rate?
> 

It is unclear.  But if you can't set the rate that is configured or if there is
a misconfiguration, it's probably better to exit the probe and catch it here.

> > > +
> > > +	ret = clk_prepare_enable(cclk);
> > > +	if (ret) {
> > > +		dev_err(dev, "cannot enable core clock\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(iclk);
> > > +	if (ret) {
> > > +		clk_disable_unprepare(cclk);
> > > +		dev_err(dev, "cannot enable iface clock\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	data = readl_relaxed(base + QUP_HW_VERSION);
> > > +
> > > +	if (data < QUP_HW_VERSION_2_1_1) {
> > > +		clk_disable_unprepare(cclk);
> > > +		clk_disable_unprepare(iclk);
> > > +		dev_err(dev, "v.%08x is not supported\n", data);
> > > +		return -ENXIO;
> > > +	}
> > > +
> > > +	master = spi_alloc_master(dev, sizeof(struct spi_qup));
> > > +	if (!master) {
> > > +		clk_disable_unprepare(cclk);
> > > +		clk_disable_unprepare(iclk);
> > > +		dev_err(dev, "cannot allocate master\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	master->bus_num = pdev->id;
> > > +	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
> > > +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> > > +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> > > +	master->setup = spi_qup_setup;
> > > +	master->cleanup = spi_qup_cleanup;
> > > +	master->transfer_one_message = spi_qup_transfer_one;
> > > +	master->dev.of_node = pdev->dev.of_node;
> > > +	master->auto_runtime_pm = true;
> > 
> > Remove this.  No runtime support
> > 
> > > +
> > > +	platform_set_drvdata(pdev, master);
> > > +
> > > +	controller = spi_master_get_devdata(master);
> > > +
> > > +	controller->dev  = dev;
> > > +	controller->base = base;
> > > +	controller->iclk = iclk;
> > > +	controller->cclk = cclk;
> > > +	controller->irq  = irq;
> > > +	controller->max_speed_hz = clk_get_rate(cclk);
> > > +	controller->speed_hz = controller->max_speed_hz;
> > > +
> > > +	init_completion(&controller->done);
> > > +
> > > +	iomode = readl_relaxed(base + QUP_IO_M_MODES);
> > > +
> > > +	size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode);
> > > +	if (size)
> > > +		controller->out_blk_sz = size * 16;
> > > +	else
> > > +		controller->out_blk_sz = 4;
> > > +
> > > +	size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode);
> > > +	if (size)
> > > +		controller->in_blk_sz = size * 16;
> > > +	else
> > > +		controller->in_blk_sz = 4;
> > > +
> > > +	size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode);
> > > +	controller->out_fifo_sz = controller->out_blk_sz * (2 << size);
> > > +
> > > +	size = QUP_IO_M_INPUT_FIFO_SIZE(iomode);
> > > +	controller->in_fifo_sz = controller->in_blk_sz * (2 << size);
> > > +
> > > +	dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
> > > +		 data, controller->in_blk_sz, controller->in_fifo_sz,
> > > +		 controller->out_blk_sz, controller->out_fifo_sz);
> > > +
> > > +	writel_relaxed(1, base + QUP_SW_RESET);
> > > +
> > > +	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
> > > +	if (ret) {
> > > +		dev_err(dev, "cannot set RESET state\n");
> > > +		goto error;
> > > +	}
> > > +
> > > +	writel_relaxed(0, base + QUP_OPERATIONAL);
> > > +	writel_relaxed(0, base + QUP_IO_M_MODES);
> > > +	writel_relaxed(0, base + QUP_OPERATIONAL_MASK);
> > > +	writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN,
> > > +		       base + SPI_ERROR_FLAGS_EN);
> > > +
> > > +	writel_relaxed(0, base + SPI_CONFIG);
> > > +	writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL);
> > > +
> > > +	ret = devm_request_irq(dev, irq, spi_qup_qup_irq,
> > > +			       IRQF_TRIGGER_HIGH, pdev->name, controller);
> > > +	if (ret) {
> > > +		dev_err(dev, "cannot request IRQ %d\n", irq);
> > 
> > unnecessary print
> 
> Will remove.
> 
> > 
> > > +		goto error;
> > > +	}
> > > +
> > > +	ret = devm_spi_register_master(dev, master);
> > > +	if (!ret) {
> > > +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> > > +		pm_runtime_use_autosuspend(dev);
> > > +		pm_runtime_set_active(dev);
> > > +		pm_runtime_enable(dev);
> > 
> > Remove all the runtime stuff.  not supported right now.
> > 
> > > +		return ret;
> > > +	}
> > > +error:
> > > +	clk_disable_unprepare(cclk);
> > > +	clk_disable_unprepare(iclk);
> > > +	spi_master_put(master);
> > > +	return ret;
> > > +}
> > > +
> 
> <snip>
> 
> > 
> > > +
> > > +static int spi_qup_remove(struct platform_device *pdev)
> > > +{
> > > +	struct spi_master *master = dev_get_drvdata(&pdev->dev);
> > > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > > +
> > > +	pm_runtime_get_sync(&pdev->dev);
> > > +
> > 
> > Do we need to wait for any current transactions to complete
> > and do a devm_free_irq()?
> > 
> > > +	clk_disable_unprepare(controller->cclk);
> > > +	clk_disable_unprepare(controller->iclk);
> 
> My understanding is:
> 
> Disabling clocks will timeout transaction, if any. Core Device driver
> will call: devm_spi_unregister(), which will wait pending transactions
> to complete and then remove the SPI master.

Disabling clocks will confuse the hardware.  We cannot disable clocks while the
spi core is active and transferring data.

> 
> > > +
> > > +	pm_runtime_put_noidle(&pdev->dev);
> > > +	pm_runtime_disable(&pdev->dev);
> > > +	return 0;
> > > +}
> > > +
> > > +static struct of_device_id spi_qup_dt_match[] = {
> > > +	{ .compatible = "qcom,spi-qup-v2", },
> > 
> > Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1
> > (msm8974 v2)
> 
> I am not aware of the difference. My board report v.20020000. 
> Is there difference of handling these controllers?

There were some bug fixes between versions.  None of those affect SPI (that I
can tell), but it's better to be more descriptive and use the full versions in
the compatible tags.

> 
> 
> Thanks, 
> Ivan
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Josh Cartwright]

On Fri, Feb 07, 2014 at 05:31:08PM +0000, Mark Brown wrote:
> On Fri, Feb 07, 2014 at 11:20:51AM -0600, Josh Cartwright wrote:
> > On Fri, Feb 07, 2014 at 05:18:34PM +0000, Mark Brown wrote:
> > > On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:
> > > > 	config SPI_QUP
> > > > 		tristate "Qualcomm SPI Support with QUP interface"
> > > > 		depends on OF
> > > > 		depends on ARM
> 
> > > Does this really depend on ARM?  If so why?
> 
> > The ARM dependency is there for the use of _relaxed io accessor
> > variants.
> 
> That's not ARM only and I thought we were getting generic versions of it
> anyway?  ARMv8, MIPS, Microblaze, Hexagon and SH also define it.

Okay, that's fair.  I'm only vaguely familiar with the generic _relaxed
variants, but until they land, how do we appropriately declare the
dependency to prevent breaking COMPILE_TEST builds on architectures that
don't have them?  Or should we either bother?

Do we need to introduce a HAVE_RELAXED_IO_ACCESSORS selected by those
architectures with support?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Mark Brown]

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

On Fri, Feb 07, 2014 at 11:32:07AM -0600, Andy Gross wrote:
> On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:

To repeat what I said in my earlier e-mail please delete irrelevant
context from your mails so any new content you are including is
discoverable.

> > Did you see any particular issue with the implementation 
> > or this is just because this platform didn't have support 
> > for power management?  

> The platform doesn't have support for PM right now.  So it's probably better to
> remove all this and revisit later when it is in place.

No, runtime PM does not require any platform support at all and is good
practice - look at what the driver is doing with it, it's useful as-is.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Mark Brown]

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

On Fri, Feb 07, 2014 at 11:46:43AM -0600, Josh Cartwright wrote:
> On Fri, Feb 07, 2014 at 05:31:08PM +0000, Mark Brown wrote:

> > That's not ARM only and I thought we were getting generic versions of it
> > anyway?  ARMv8, MIPS, Microblaze, Hexagon and SH also define it.

> Okay, that's fair.  I'm only vaguely familiar with the generic _relaxed
> variants, but until they land, how do we appropriately declare the
> dependency to prevent breaking COMPILE_TEST builds on architectures that
> don't have them?  Or should we either bother?

> Do we need to introduce a HAVE_RELAXED_IO_ACCESSORS selected by those
> architectures with support?

I think that or just getting generic versions done would be the way
forwards.  Right now it's a bit of a shambles.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Andy Gross]

On Fri, Feb 07, 2014 at 05:52:34PM +0000, Mark Brown wrote:
> On Fri, Feb 07, 2014 at 11:32:07AM -0600, Andy Gross wrote:
> > On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:
> 

[... snip ...]

> > The platform doesn't have support for PM right now.  So it's probably better to
> > remove all this and revisit later when it is in place.
> 
> No, runtime PM does not require any platform support at all and is good
> practice - look at what the driver is doing with it, it's useful as-is.

Fair enough.

-- 
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Ivan T. Ivanov]

Hi,

On Fri, 2014-02-07 at 17:12 +0000, Mark Brown wrote: 
> On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
> 
> This looks mostly good, there's a few odd things and missing use of
> framework features.
> 
> > Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> > provides a common data path (an output FIFO and an input FIFO)
> > for serial peripheral interface (SPI) mini-core. SPI in master mode
> > support up to 50MHz, up to four chip selects, and a programmable
> > data path from 4 bits to 32 bits; MODE0..3 protocols
> 
> The grammar in this and the Kconfig text is a bit garbled, might want to
> give it a once over (support -> supports for example).

Ok

> 
> > +static void spi_qup_deassert_cs(struct spi_qup *controller,
> > +				struct spi_qup_device *chip)
> > +{
> 
> 
> > +	if (chip->mode & SPI_CS_HIGH)
> > +		iocontol &= ~mask;
> > +	else
> > +		iocontol |= mask;
> 
> Implement a set_cs() operation and let the core worry about all this
> for you as well as saving two implementations.

Nice. Will us it.

> 
> > +		word = 0;
> > +		for (idx = 0; idx < controller->bytes_per_word &&
> > +		     controller->tx_bytes < xfer->len; idx++,
> > +		     controller->tx_bytes++) {
> > +
> > +			if (!tx_buf)
> > +				continue;
> 
> Do you need to set the _MUST_TX flag?
> 
> > +	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> > +	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> > +	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> > +
> > +	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> > +	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> > +	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> > +
> > +	if (!xfer)
> > +		return IRQ_HANDLED;
> 
> Are you sure?  It seems wrong to just ignore interrupts, some comments
> would help explain why.

Of course this should never happen. This was left from initial stage
of the implementation.

> 
> > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > +			       struct spi_qup_device *chip,
> > +			       struct spi_transfer *xfer)
> 
> This looks like a transfer_one() function, please use the framework
> features where you can.

Sure, will do. Somehow I have missed this.

> 
> > +	if (controller->speed_hz != chip->speed_hz) {
> > +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> > +		if (ret) {
> > +			dev_err(controller->dev, "fail to set frequency %d",
> > +				chip->speed_hz);
> > +			return -EIO;
> > +		}
> > +	}
> 
> Is calling into the clock framework really so expensive that we need to
> avoid doing it?  

Probably not. But why to call it if the frequency is the same.


> You also shouldn't be interacting with the hardware in
> setup().

This is internal hw-setup, not master::setup() or I am
missing something?

> 
> > +	if (chip->bits_per_word <= 8)
> > +		controller->bytes_per_word = 1;
> > +	else if (chip->bits_per_word <= 16)
> > +		controller->bytes_per_word = 2;
> > +	else
> > +		controller->bytes_per_word = 4;
> 
> This looks like a switch statement, and looking at the above it's not
> clear that the device actually supports anything other than whole bytes.
> I'm not sure what that would mean from an API point of view.

SPI API didn't validate struct spi_transfer::len field.

The whole sniped looks like this:

	chip->bits_per_word = spi->bits_per_word;
	if (xfer->bits_per_word)
		chip->bits_per_word = xfer->bits_per_word;

	if (chip->bits_per_word <= 8)
		controller->bytes_per_word = 1;
	else if (chip->bits_per_word <= 16)
		controller->bytes_per_word = 2;
	else
		controller->bytes_per_word = 4;

	if (controller->bytes_per_word > xfer->len ||
	    xfer->len % controller->bytes_per_word != 0){
		/* No partial transfers */
		dev_err(controller->dev, "invalid len %d for %d bits\n",
			xfer->len, chip->bits_per_word);
		return -EIO;
	}

	n_words = xfer->len / controller->bytes_per_word;

'bytes_per_word' have to be power of 2. This is my understanding of
struct spi_transfer description. So I am discarding all transfers with
'len' non multiple of word size.


> 
> > +static int spi_qup_transfer_one(struct spi_master *master,
> > +				struct spi_message *msg)
> > +{
> 
> This entire function can be removed, the core can do it for you.

Sure, will use it.

> 
> > +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > +		max_freq = 19200000;
> > +
> > +	if (!max_freq) {
> > +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> > +		return -ENXIO;
> > +	}
> > +
> > +	ret = clk_set_rate(cclk, max_freq);
> > +	if (ret)
> > +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> 
> You set the clock rate per transfer so why bother setting it here,

Only if differs from the current one.

> perhaps we support the rate the devices request but not this maximum
> rate?

Thats why it is just a warning. I will see how to handle this better.

> 
> > +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> > +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> 
> Are you *sure* the device supports anything other than whole bytes?

Yep. I see them on the scope.

> 
> > +	ret = devm_spi_register_master(dev, master);
> > +	if (!ret) {
> > +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> > +		pm_runtime_use_autosuspend(dev);
> > +		pm_runtime_set_active(dev);
> > +		pm_runtime_enable(dev);
> > +		return ret;
> > +	}
> 
> This is really unclearly written, the success case looks like error
> handling.

I suppose that if use a goto, I will have to do it consistently. 
Will rework it.

> 
> > +#ifdef CONFIG_PM_RUNTIME
> > +static int spi_qup_pm_suspend_runtime(struct device *device)
> > +{
> > +	struct spi_master *master = dev_get_drvdata(device);
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +
> > +	disable_irq(controller->irq);
> 
> Why do you need to disable the interrupt?  Will the hardware generate
> spurious interrupts, if so some documentation is in order.

I don't know. Just extra protection? I will have t o find how to 
test this.

> 
> > +static int spi_qup_pm_resume_runtime(struct device *device)
> > +{
> > +	struct spi_master *master = dev_get_drvdata(device);
> > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > +
> > +	clk_prepare_enable(controller->cclk);
> > +	clk_prepare_enable(controller->iclk);
> > +	enable_irq(controller->irq);
> 
> No error checking here...

Will fix.

Thanks,
Ivan

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Ivan T. Ivanov]

Hi Andy,

On Fri, 2014-02-07 at 11:32 -0600, Andy Gross wrote: 
> On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:
> > 

<snip>

> > > > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > > > +			       struct spi_qup_device *chip,
> > > > +			       struct spi_transfer *xfer)
> > > > +{
> > > > +	unsigned long timeout;
> > > > +	int ret = -EIO;
> > > > +
> > > > +	reinit_completion(&controller->done);
> > > > +
> > > > +	timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> > > > +	timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> > > > +	timeout = 100 * msecs_to_jiffies(timeout);
> > > > +
> > > > +	controller->rx_bytes = 0;
> > > > +	controller->tx_bytes = 0;
> > > > +	controller->error = 0;
> > > > +	controller->xfer = xfer;
> > > > +
> > > > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > > +		dev_warn(controller->dev, "cannot set RUN state\n");
> > > > +		goto exit;
> > > > +	}
> > > > +
> > > > +	if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> > > > +		dev_warn(controller->dev, "cannot set PAUSE state\n");
> > > > +		goto exit;
> > > > +	}
> > > > +
> > > > +	spi_qup_fifo_write(controller, xfer);
> > > > +
> > > > +	if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > > +		dev_warn(controller->dev, "cannot set EXECUTE state\n");
> > > > +		goto exit;
> > > > +	}
> > > > +
> > > > +	if (!wait_for_completion_timeout(&controller->done, timeout))
> > > > +		ret = -ETIMEDOUT;
> > > > +	else
> > > > +		ret = controller->error;
> > > > +exit:
> > > > +	controller->xfer = NULL;
> > > 
> > > Should the manipulation of controller->xfer be protected by spinlock?
> > 
> > :-). Probably. I am wondering, could I avoid locking if firstly place
> > QUP into RESET state and then access these field. This should stop
> > all activities in it, right?
> 
> It's generally safest to not assume the hardware is going to do sane things.
> I'm concerned about spurious IRQs.

Ok, will add protection. 

<snip>

> > > > +
> > > > +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > > > +		max_freq = 19200000;
> > > 
> > > I'd set the default to 50MHz as that is the max supported by hardware.  I'd just
> > > set max_freq declaration to 50MHz and then check the value if it is changed via
> > > DT.
> > 
> > 50MHz doesn't seems to be supported on all chip sets. Currently common
> > denominator on all chip sets, that I can see, is 19.2MHz. I have tried 
> > to test it with more than 19.2MHz on APQ8074 and it fails.
> > 
> 
> I guess my stance is to set it to the hardware max supported frequency if it is
> not specified.  If that needs to be lower on a board because of whatever reason,
> they override it.

Ok, I will do in this way.

<snip>

> > > 
> > > > +
> > > > +	ret = clk_set_rate(cclk, max_freq);
> > > > +	if (ret)
> > > > +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> > > 
> > > Bail here?
> > 
> > I don't know. What will be the consequences if controller continue to
> > operate on its default rate?
> > 
> 
> It is unclear.  But if you can't set the rate that is configured or if there is
> a misconfiguration, it's probably better to exit the probe and catch it here.


My preference is to delay clock speed change till first
SPI transfer. And use wherever transfer itself mandate.

<snip>

> > > > +
> > > > +static int spi_qup_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct spi_master *master = dev_get_drvdata(&pdev->dev);
> > > > +	struct spi_qup *controller = spi_master_get_devdata(master);
> > > > +
> > > > +	pm_runtime_get_sync(&pdev->dev);
> > > > +
> > > 
> > > Do we need to wait for any current transactions to complete
> > > and do a devm_free_irq()?
> > > 
> > > > +	clk_disable_unprepare(controller->cclk);
> > > > +	clk_disable_unprepare(controller->iclk);
> > 
> > My understanding is:
> > 
> > Disabling clocks will timeout transaction, if any. Core Device driver
> > will call: devm_spi_unregister(), which will wait pending transactions
> > to complete and then remove the SPI master.
> 
> Disabling clocks will confuse the hardware.  We cannot disable clocks while the
> spi core is active and transferring data.

I could follow approach taken by other SPI drivers, just reset
controller and disable clocks.

> 
> > 
> > > > +
> > > > +	pm_runtime_put_noidle(&pdev->dev);
> > > > +	pm_runtime_disable(&pdev->dev);
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static struct of_device_id spi_qup_dt_match[] = {
> > > > +	{ .compatible = "qcom,spi-qup-v2", },
> > > 
> > > Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1
> > > (msm8974 v2)
> > 
> > I am not aware of the difference. My board report v.20020000. 
> > Is there difference of handling these controllers?
> 
> There were some bug fixes between versions.  None of those affect SPI (that I
> can tell), but it's better to be more descriptive and use the full versions in
> the compatible tags.

No strong preference here. Should I add qcom,spi-qup-v2.2.0, then? :-)

Regards,
Ivan

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Mark Brown]

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

On Mon, Feb 10, 2014 at 06:29:26PM +0200, Ivan T. Ivanov wrote:
> On Fri, 2014-02-07 at 17:12 +0000, Mark Brown wrote: 

> > > +	if (!xfer)
> > > +		return IRQ_HANDLED;

> > Are you sure?  It seems wrong to just ignore interrupts, some comments
> > would help explain why.

> Of course this should never happen. This was left from initial stage
> of the implementation.

OK, so reporting them as errors seems better then.

> > > +	if (controller->speed_hz != chip->speed_hz) {
> > > +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> > > +		if (ret) {
> > > +			dev_err(controller->dev, "fail to set frequency %d",
> > > +				chip->speed_hz);
> > > +			return -EIO;
> > > +		}
> > > +	}

> > Is calling into the clock framework really so expensive that we need to
> > avoid doing it?  

> Probably not. But why to call it if the frequency is the same.

It's less code that could go wrong and the check is already there in the
clock framework hopefully.

> > You also shouldn't be interacting with the hardware in
> > setup().

> This is internal hw-setup, not master::setup() or I am
> missing something?

The naming could probably be clearer then - config or something.

> > > +	if (chip->bits_per_word <= 8)
> > > +		controller->bytes_per_word = 1;
> > > +	else if (chip->bits_per_word <= 16)
> > > +		controller->bytes_per_word = 2;
> > > +	else
> > > +		controller->bytes_per_word = 4;

> > This looks like a switch statement, and looking at the above it's not
> > clear that the device actually supports anything other than whole bytes.
> > I'm not sure what that would mean from an API point of view.

> SPI API didn't validate struct spi_transfer::len field.

It's supposed to; if the validation is incomplete then that should be
fixed.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Andy Gross]

On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:

[....]

> > > > Bail here?
> > > 
> > > I don't know. What will be the consequences if controller continue to
> > > operate on its default rate?
> > > 
> > 
> > It is unclear.  But if you can't set the rate that is configured or if there is
> > a misconfiguration, it's probably better to exit the probe and catch it here.
> 
> 
> My preference is to delay clock speed change till first
> SPI transfer. And use wherever transfer itself mandate.
> 

That works.  My only concern is that it might be nice to catch a configuration
problem early rather than wait for the SPI transfer to fail continuously.

[....]

> > > My understanding is:
> > > 
> > > Disabling clocks will timeout transaction, if any. Core Device driver
> > > will call: devm_spi_unregister(), which will wait pending transactions
> > > to complete and then remove the SPI master.
> > 
> > Disabling clocks will confuse the hardware.  We cannot disable clocks while the
> > spi core is active and transferring data.
> 
> I could follow approach taken by other SPI drivers, just reset
> controller and disable clocks.

You have to wait until the hardware is in a sane state.  For the QUP, that means
in a RUN/PAUSE/RESET state.  It cannot be in transition when you cut the clocks.
The safest thing to do is to get the QUP into the RESET state and then cut the
clocks.

[.....]

> > > I am not aware of the difference. My board report v.20020000. 
> > > Is there difference of handling these controllers?
> > 
> > There were some bug fixes between versions.  None of those affect SPI (that I
> > can tell), but it's better to be more descriptive and use the full versions in
> > the compatible tags.
> 
> No strong preference here. Should I add qcom,spi-qup-v2.2.0, then? :-)

According to the documentation, there is no v2.2.0.  It appears there is some
disconnect between the specific HW revision and the documentation.  I'll see if
I can get some clarification from the hardware guys.  For now, I think the 2.1.1
and 2.2.1 tags are fine.


-- 
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Ivan T. Ivanov]

Hi,

On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
> On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
> 
> [....]
> 
> > > > > Bail here?
> > > > 
> > > > I don't know. What will be the consequences if controller continue to
> > > > operate on its default rate?
> > > > 
> > > 
> > > It is unclear.  But if you can't set the rate that is configured or if there is
> > > a misconfiguration, it's probably better to exit the probe and catch it here.
> > 
> > 
> > My preference is to delay clock speed change till first
> > SPI transfer. And use wherever transfer itself mandate.
> > 
> 
> That works.  My only concern is that it might be nice to catch a configuration
> problem early rather than wait for the SPI transfer to fail continuously.

If developer is skilled enough to know which version controller is,
(s)he will be able to put the right frequency constrain here :-)

> 
> [....]
> 
> > > > My understanding is:
> > > > 
> > > > Disabling clocks will timeout transaction, if any. Core Device driver
> > > > will call: devm_spi_unregister(), which will wait pending transactions
> > > > to complete and then remove the SPI master.
> > > 
> > > Disabling clocks will confuse the hardware.  We cannot disable clocks while the
> > > spi core is active and transferring data.
> > 
> > I could follow approach taken by other SPI drivers, just reset
> > controller and disable clocks.
> 
> You have to wait until the hardware is in a sane state.  For the QUP, that means
> in a RUN/PAUSE/RESET state.  It cannot be in transition when you cut the clocks.
> The safest thing to do is to get the QUP into the RESET state and then cut the
> clocks.
> 

Sure. will do.

> [.....]
> 
> > > > I am not aware of the difference. My board report v.20020000. 
> > > > Is there difference of handling these controllers?
> > > 
> > > There were some bug fixes between versions.  None of those affect SPI (that I
> > > can tell), but it's better to be more descriptive and use the full versions in
> > > the compatible tags.
> > 
> > No strong preference here. Should I add qcom,spi-qup-v2.2.0, then? :-)
> 
> According to the documentation, there is no v2.2.0.  It appears there is some
> disconnect between the specific HW revision and the documentation.  I'll see if
> I can get some clarification from the hardware guys.  For now, I think the 2.1.1
> and 2.2.1 tags are fine.

Ok. Thanks,
Ivan

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Courtney Cavin]

On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote:
> 
> Hi,
> 
> On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
> > On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
> > 
> > [....]
> > 
> > > > > > Bail here?
> > > > > 
> > > > > I don't know. What will be the consequences if controller continue to
> > > > > operate on its default rate?
> > > > > 
> > > > 
> > > > It is unclear.  But if you can't set the rate that is configured or if there is
> > > > a misconfiguration, it's probably better to exit the probe and catch it here.
> > > 
> > > 
> > > My preference is to delay clock speed change till first
> > > SPI transfer. And use wherever transfer itself mandate.
> > > 
> > 
> > That works.  My only concern is that it might be nice to catch a configuration
> > problem early rather than wait for the SPI transfer to fail continuously.
> 
> If developer is skilled enough to know which version controller is,
> (s)he will be able to put the right frequency constrain here :-)

A developer doesn't have to have much skill at all to copy-paste DT
configurations around and muck with numbers....  I agree with Andy here,
early validation is a good idea here, at the very least, some sanity
checks.

-Courtney

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Ivan T. Ivanov]

Hi, 

On Mon, 2014-02-10 at 12:29 -0800, Courtney Cavin wrote:
> On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote:
> > 
> > Hi,
> > 
> > On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
> > > On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
> > > 
> > > [....]
> > > 
> > > > > > > Bail here?
> > > > > > 
> > > > > > I don't know. What will be the consequences if controller continue to
> > > > > > operate on its default rate?
> > > > > > 
> > > > > 
> > > > > It is unclear.  But if you can't set the rate that is configured or if there is
> > > > > a misconfiguration, it's probably better to exit the probe and catch it here.
> > > > 
> > > > 
> > > > My preference is to delay clock speed change till first
> > > > SPI transfer. And use wherever transfer itself mandate.
> > > > 
> > > 
> > > That works.  My only concern is that it might be nice to catch a configuration
> > > problem early rather than wait for the SPI transfer to fail continuously.
> > 
> > If developer is skilled enough to know which version controller is,
> > (s)he will be able to put the right frequency constrain here :-)
> 
> A developer doesn't have to have much skill at all to copy-paste DT
> configurations around and muck with numbers....  I agree with Andy here,
> early validation is a good idea here, at the very least, some sanity
> checks.
> 

So, probably first variant with just warning will be good enough? 

Regards,
Ivan


> -Courtney

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Mark Brown]

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

On Mon, Feb 10, 2014 at 10:59:54PM +0200, Ivan T. Ivanov wrote:
> On Mon, 2014-02-10 at 12:29 -0800, Courtney Cavin wrote:

> > A developer doesn't have to have much skill at all to copy-paste DT
> > configurations around and muck with numbers....  I agree with Andy here,
> > early validation is a good idea here, at the very least, some sanity
> > checks.

> So, probably first variant with just warning will be good enough? 

I'm not sure it actually adds anything meaningful here - if the error
reporting isn't clear enough on use then that's probably an issue anyway
and we may never even use the default.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Ivan T. Ivanov]

Hi, 

On Mon, 2014-02-10 at 12:29 -0800, Courtney Cavin wrote: 
> On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote:
> > 
> > Hi,
> > 
> > On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
> > > On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
> > > 
> > > [....]
> > > 
> > > > > > > Bail here?
> > > > > > 
> > > > > > I don't know. What will be the consequences if controller continue to
> > > > > > operate on its default rate?
> > > > > > 
> > > > > 
> > > > > It is unclear.  But if you can't set the rate that is configured or if there is
> > > > > a misconfiguration, it's probably better to exit the probe and catch it here.
> > > > 
> > > > 
> > > > My preference is to delay clock speed change till first
> > > > SPI transfer. And use wherever transfer itself mandate.
> > > > 
> > > 
> > > That works.  My only concern is that it might be nice to catch a configuration
> > > problem early rather than wait for the SPI transfer to fail continuously.
> > 
> > If developer is skilled enough to know which version controller is,
> > (s)he will be able to put the right frequency constrain here :-)
> 
> A developer doesn't have to have much skill at all to copy-paste DT
> configurations around and muck with numbers....  I agree with Andy here,
> early validation is a good idea here, at the very least, some sanity
> checks.

Actually, thinking more on this. Supplying SPI controller with, 
let say 50MHz, which is what success of clk_set_rate() means, doesn't
necessarily guaranteer that controller will be able to do transfers
properly, right? Setting frequency at this point didn't bring any
benefit. 

Regards,
Ivan


> 
> -Courtney