[PATCH v3 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[PATCH v3 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Ryan Case]

From: Girish Mahadevan <girishm@codeaurora.org>

Bindings for Qualcomm Quad SPI used on SoCs such as sdm845.

Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Signed-off-by: Ryan Case <ryandcase@chromium.org>
---

Changes in v3:
- Added generic compatible string in addition to specific SoC

Changes in v2:
- Added commit text
- Removed invalid property
- Updated example to match sdm845 with attached spi-nor

 .../bindings/spi/qcom,spi-qcom-qspi.txt       | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt

diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
new file mode 100644
index 000000000000..e13f5bb314ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
@@ -0,0 +1,36 @@
+Qualcomm Quad Serial Peripheral Interface (QSPI)
+
+The QSPI controller allows SPI protocol communication in single, dual, or quad
+wire transmission modes for read/write access to slaves such as NOR flash.
+
+Required properties:
+- compatible:	An SoC specific identifier followed by "qcom,qspi-v1", such as
+		"qcom,sdm845-qspi", "qcom,qspi-v1"
+- reg:		Should contain the base register location and length.
+- interrupts:	Interrupt number used by the controller.
+- clocks:	Should contain the core and AHB clock.
+- clock-names:	Should be "core" for core clock and "iface" for AHB clock.
+
+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:
+
+	qspi: qspi@88df000 {
+		compatible = "qcom,sdm845-qspi", "qcom,qspi-v1";
+		reg = <0x88df000 0x600>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
+		clock-names = "iface", "core";
+		clocks = <&gcc GCC_QSPI_CNOC_PERIPH_AHB_CLK>,
+			 <&gcc GCC_QSPI_CORE_CLK>;
+
+		device@0 {
+			compatible = "jedec,spi-nor";
+			reg = <0>;
+			spi-max-frequency = <25000000>;
+			spi-tx-bus-width = <2>;
+			spi-rx-bus-width = <2>;
+		};
+	};
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

[Ryan Case]

From: Girish Mahadevan <girishm@codeaurora.org>

New driver for Qualcomm QuadSPI(QSPI) controller that is used to
communicate with slaves such flash memory devices. The QSPI controller
can operate in 2 or 4 wire mode but only supports SPI Mode 0. The
controller can also operate in Single or Dual data rate modes.

Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Signed-off-by: Ryan Case <ryandcase@chromium.org>
---

Changes in v3:
- Corrected QPSPI typo
- Removed setup function and moved configurations to prepare_message
- Added __maybe_unused to suspend and resume functions

Changes in v2:
- Addressed formatting feedback
- Squashed bug fixes and features from Doug
- Now uses transfer_one_message instead of mem_ops
- Fixed suspend/resume
- Added spinlocks

 drivers/spi/Kconfig         |   6 +
 drivers/spi/Makefile        |   1 +
 drivers/spi/spi-qcom-qspi.c | 598 ++++++++++++++++++++++++++++++++++++
 3 files changed, 605 insertions(+)
 create mode 100644 drivers/spi/spi-qcom-qspi.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index de03d67bcd2b..723d47bf281f 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -549,6 +549,12 @@ config SPI_RSPI
 	help
 	  SPI driver for Renesas RSPI and QSPI blocks.
 
+config SPI_QCOM_QSPI
+	tristate "QTI QSPI controller"
+	depends on ARCH_QCOM
+	help
+	  QSPI(Quad SPI) driver for Qualcomm QSPI controller.
+
 config SPI_QUP
 	tristate "Qualcomm SPI controller with QUP interface"
 	depends on ARCH_QCOM || (ARM && COMPILE_TEST)
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 876f8690fc47..f997c49255a6 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
 # SPI slave protocol handlers
 obj-$(CONFIG_SPI_SLAVE_TIME)		+= spi-slave-time.o
 obj-$(CONFIG_SPI_SLAVE_SYSTEM_CONTROL)	+= spi-slave-system-control.o
+obj-$(CONFIG_SPI_QCOM_QSPI)		+= spi-qcom-qspi.o
diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
new file mode 100644
index 000000000000..0ad2ef003068
--- /dev/null
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -0,0 +1,598 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+
+#include <asm/unaligned.h>
+
+#define AHB_MIN_HZ		9600000UL
+#define QSPI_NUM_CS		2
+#define QSPI_BYTES_PER_WORD	4
+#define MSTR_CONFIG		0x0000
+#define AHB_MASTER_CFG		0x0004
+#define MSTR_INT_EN		0x000C
+#define MSTR_INT_STATUS		0x0010
+#define PIO_XFER_CTRL		0x0014
+#define PIO_XFER_CFG		0x0018
+#define PIO_XFER_STATUS		0x001c
+#define PIO_DATAOUT_1B		0x0020
+#define PIO_DATAOUT_4B		0x0024
+#define RD_FIFO_CFG		0x0028
+#define RD_FIFO_STATUS		0x002c
+#define RD_FIFO_RESET		0x0030
+#define CUR_MEM_ADDR		0x0048
+#define HW_VERSION		0x004c
+#define RD_FIFO			0x0050
+#define SAMPLING_CLK_CFG	0x0090
+#define SAMPLING_CLK_STATUS	0x0094
+
+/* Macros to help set/get fields in MSTR_CONFIG register */
+#define	FULL_CYCLE_MODE		BIT(3)
+#define	FB_CLK_EN		BIT(4)
+#define	PIN_HOLDN		BIT(6)
+#define	PIN_WPN			BIT(7)
+#define	DMA_ENABLE		BIT(8)
+#define	BIG_ENDIAN_MODE		BIT(9)
+#define	SPI_MODE_MSK		0xc00
+#define	SPI_MODE_SHFT		10
+#define	CHIP_SELECT_NUM		BIT(12)
+#define	SBL_EN			BIT(13)
+#define	LPA_BASE_MSK		0x3c000
+#define	LPA_BASE_SHFT		14
+#define	TX_DATA_DELAY_MSK	0xc0000
+#define	TX_DATA_DELAY_SHFT	18
+#define	TX_CLK_DELAY_MSK	0x300000
+#define	TX_CLK_DELAY_SHFT	20
+#define	TX_CS_N_DELAY_MSK	0xc00000
+#define	TX_CS_N_DELAY_SHFT	22
+#define	TX_DATA_OE_DELAY_MSK	0x3000000
+#define	TX_DATA_OE_DELAY_SHFT	24
+
+/* Macros to help set/get fields in AHB_MSTR_CFG register */
+#define	HMEM_TYPE_START_MID_TRANS_MSK		0x7
+#define	HMEM_TYPE_START_MID_TRANS_SHFT		0
+#define	HMEM_TYPE_LAST_TRANS_MSK		0x38
+#define	HMEM_TYPE_LAST_TRANS_SHFT		3
+#define	USE_HMEMTYPE_LAST_ON_DESC_OR_CHAIN_MSK	0xc0
+#define	USE_HMEMTYPE_LAST_ON_DESC_OR_CHAIN_SHFT	6
+#define	HMEMTYPE_READ_TRANS_MSK			0x700
+#define	HMEMTYPE_READ_TRANS_SHFT		8
+#define	HSHARED					BIT(11)
+#define	HINNERSHARED				BIT(12)
+
+/* Macros to help set/get fields in MSTR_INT_EN/MSTR_INT_STATUS registers */
+#define	RESP_FIFO_UNDERRUN	BIT(0)
+#define	RESP_FIFO_NOT_EMPTY	BIT(1)
+#define	RESP_FIFO_RDY		BIT(2)
+#define	HRESP_FROM_NOC_ERR	BIT(3)
+#define	WR_FIFO_EMPTY		BIT(9)
+#define	WR_FIFO_FULL		BIT(10)
+#define	WR_FIFO_OVERRUN		BIT(11)
+#define	TRANSACTION_DONE	BIT(16)
+#define QSPI_ERR_IRQS		(RESP_FIFO_UNDERRUN | HRESP_FROM_NOC_ERR | \
+				 WR_FIFO_OVERRUN)
+#define	QSPI_ALL_IRQS		(QSPI_ERR_IRQS | RESP_FIFO_RDY | \
+				 WR_FIFO_EMPTY | WR_FIFO_FULL | \
+				 TRANSACTION_DONE)
+
+/* Macros to help set/get fields in RD_FIFO_CONFIG register */
+#define	CONTINUOUS_MODE		BIT(0)
+
+/* Macros to help set/get fields in RD_FIFO_RESET register */
+#define	RESET_FIFO		BIT(0)
+
+/* Macros to help set/get fields in PIO_TRANSFER_CONFIG register */
+#define	TRANSFER_DIRECTION	BIT(0)
+#define	MULTI_IO_MODE_MSK	0xe
+#define	MULTI_IO_MODE_SHFT	1
+#define	TRANSFER_FRAGMENT	BIT(8)
+
+/* Macros to help set/get fields in PIO_TRANSFER_CONTROL register */
+#define	REQUEST_COUNT_MSK	0xffff
+
+/* Macros to help set/get fields in PIO_TRANSFER_STATUS register */
+#define	WR_FIFO_BYTES_MSK	0xffff0000
+#define	WR_FIFO_BYTES_SHFT	16
+
+/* Macros to help set/get fields in RD_FIFO_STATUS register */
+#define	FIFO_EMPTY	BIT(11)
+#define	WR_CNTS_MSK	0x7f0
+#define	WR_CNTS_SHFT	4
+#define	RDY_64BYTE	BIT(3)
+#define	RDY_32BYTE	BIT(2)
+#define	RDY_16BYTE	BIT(1)
+#define	FIFO_RDY	BIT(0)
+
+/*
+ * The Mode transfer macros, the values are programmed to the HW registers
+ * when doing PIO mode of transfers.
+ */
+#define	SDR_1BIT	1
+#define	SDR_2BIT	2
+#define	SDR_4BIT	3
+#define	DDR_1BIT	5
+#define	DDR_2BIT	6
+#define	DDR_4BIT	7
+
+/* The Mode transfer macros when setting up DMA descriptors */
+#define	DMA_DESC_SINGLE_SPI	1
+#define	DMA_DESC_DUAL_SPI	2
+#define	DMA_DESC_QUAD_SPI	3
+
+enum qspi_dir {
+	QSPI_READ,
+	QSPI_WRITE,
+};
+
+struct qspi_xfer {
+	union {
+		const void *tx_buf;
+		void *rx_buf;
+	};
+	unsigned int rem_bytes;
+	int buswidth;
+	enum qspi_dir dir;
+	bool is_last;
+};
+
+enum qspi_clocks {
+	QSPI_CLK_CORE,
+	QSPI_CLK_IFACE,
+	QSPI_NUM_CLKS
+};
+
+struct qcom_qspi {
+	void __iomem *base;
+	struct device *dev;
+	struct clk_bulk_data clks[QSPI_NUM_CLKS];
+	struct qspi_xfer xfer;
+	spinlock_t lock;
+};
+
+static int qspi_buswidth_to_iomode(struct qcom_qspi *ctrl, int buswidth)
+{
+	switch (buswidth) {
+	case 1:
+		return SDR_1BIT;
+	case 2:
+		return SDR_2BIT;
+	case 4:
+		return SDR_4BIT;
+	default:
+		dev_warn_once(ctrl->dev,
+				"Unexpected bus width: %d\n", buswidth);
+		return SDR_1BIT;
+	}
+}
+
+static void qcom_qspi_pio_xfer_cfg(struct qcom_qspi *ctrl)
+{
+	u32 pio_xfer_cfg = 0;
+	struct qspi_xfer *xfer;
+
+	xfer = &ctrl->xfer;
+	pio_xfer_cfg = readl(ctrl->base + PIO_XFER_CFG);
+	pio_xfer_cfg &= ~TRANSFER_DIRECTION;
+	pio_xfer_cfg |= xfer->dir;
+	if (xfer->is_last)
+		pio_xfer_cfg &= ~TRANSFER_FRAGMENT;
+	else
+		pio_xfer_cfg |= TRANSFER_FRAGMENT;
+	pio_xfer_cfg &= ~MULTI_IO_MODE_MSK;
+	pio_xfer_cfg |= qspi_buswidth_to_iomode(ctrl, xfer->buswidth) <<
+			MULTI_IO_MODE_SHFT;
+
+	writel(pio_xfer_cfg, ctrl->base + PIO_XFER_CFG);
+}
+
+static void qcom_qspi_pio_xfer_ctrl(struct qcom_qspi *ctrl)
+{
+	u32 pio_xfer_ctrl;
+
+	pio_xfer_ctrl = readl(ctrl->base + PIO_XFER_CTRL);
+	pio_xfer_ctrl &= ~REQUEST_COUNT_MSK;
+	pio_xfer_ctrl |= ctrl->xfer.rem_bytes;
+	writel(pio_xfer_ctrl, ctrl->base + PIO_XFER_CTRL);
+}
+
+static void qcom_qspi_pio_xfer(struct qcom_qspi *ctrl)
+{
+	u32 ints;
+
+	qcom_qspi_pio_xfer_cfg(ctrl);
+
+	/* Ack any previous interrupts that might be hanging around */
+	writel(QSPI_ALL_IRQS, ctrl->base + MSTR_INT_STATUS);
+
+	/* Setup new interrupts */
+	if (ctrl->xfer.dir == QSPI_WRITE)
+		ints = QSPI_ERR_IRQS | WR_FIFO_EMPTY;
+	else
+		ints = QSPI_ERR_IRQS | RESP_FIFO_RDY;
+	writel(ints, ctrl->base + MSTR_INT_EN);
+
+	/* Kick off the transfer */
+	qcom_qspi_pio_xfer_ctrl(ctrl);
+}
+
+static void qcom_qspi_handle_err(struct spi_master *master,
+				 struct spi_message *msg)
+{
+	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctrl->lock, flags);
+	writel(0, ctrl->base + MSTR_INT_EN);
+	ctrl->xfer.rem_bytes = 0;
+	spin_unlock_irqrestore(&ctrl->lock, flags);
+}
+
+static int qcom_qspi_transfer_one(struct spi_master *master,
+				  struct spi_device *slv,
+				  struct spi_transfer *xfer)
+{
+	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
+	int ret;
+	unsigned long speed_hz;
+	unsigned long flags;
+
+	speed_hz = slv->max_speed_hz;
+	if (xfer->speed_hz)
+		speed_hz = xfer->speed_hz;
+
+	ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4);
+	if (ret) {
+		dev_err(ctrl->dev, "Failed to set core clk %d\n", ret);
+		return ret;
+	}
+
+	spin_lock_irqsave(&ctrl->lock, flags);
+
+	/* We are half duplex, so either rx or tx will be set */
+	if (xfer->rx_buf) {
+		ctrl->xfer.dir = QSPI_READ;
+		ctrl->xfer.buswidth = xfer->rx_nbits;
+		ctrl->xfer.rx_buf = xfer->rx_buf;
+	} else {
+		ctrl->xfer.dir = QSPI_WRITE;
+		ctrl->xfer.buswidth = xfer->tx_nbits;
+		ctrl->xfer.tx_buf = xfer->tx_buf;
+	}
+	ctrl->xfer.is_last = list_is_last(&xfer->transfer_list,
+					  &master->cur_msg->transfers);
+	ctrl->xfer.rem_bytes = xfer->len;
+	qcom_qspi_pio_xfer(ctrl);
+
+	spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	/* We'll call spi_finalize_current_transfer() when done */
+	return 1;
+}
+
+static int qcom_qspi_prepare_message(struct spi_master *master,
+				     struct spi_message *message)
+{
+	u32 mstr_cfg;
+	struct qcom_qspi *ctrl;
+	int tx_data_oe_delay = 1;
+	int tx_data_delay = 1;
+	unsigned long flags;
+
+	ctrl = spi_master_get_devdata(master);
+	spin_lock_irqsave(&ctrl->lock, flags);
+
+	mstr_cfg = readl(ctrl->base + MSTR_CONFIG);
+	mstr_cfg &= ~CHIP_SELECT_NUM;
+	if (message->spi->chip_select)
+		mstr_cfg |= CHIP_SELECT_NUM;
+
+	mstr_cfg = (mstr_cfg & ~SPI_MODE_MSK) |
+				(message->spi->mode << SPI_MODE_SHFT);
+	mstr_cfg |= FB_CLK_EN | PIN_WPN | PIN_HOLDN | SBL_EN | FULL_CYCLE_MODE;
+	mstr_cfg |= (mstr_cfg & ~TX_DATA_OE_DELAY_MSK) |
+				(tx_data_oe_delay << TX_DATA_OE_DELAY_SHFT);
+	mstr_cfg |= (mstr_cfg & ~TX_DATA_DELAY_MSK) |
+				(tx_data_delay << TX_DATA_DELAY_SHFT);
+	mstr_cfg &= ~DMA_ENABLE;
+
+	writel(mstr_cfg, ctrl->base + MSTR_CONFIG);
+	spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static irqreturn_t pio_read(struct qcom_qspi *ctrl)
+{
+	u32 rd_fifo_status;
+	u32 rd_fifo;
+	unsigned int wr_cnts;
+	unsigned int bytes_to_read;
+	unsigned int words_to_read;
+	u32 *word_buf;
+	u8 *byte_buf;
+	int i;
+
+	rd_fifo_status = readl(ctrl->base + RD_FIFO_STATUS);
+
+	if (!(rd_fifo_status & FIFO_RDY)) {
+		dev_dbg(ctrl->dev, "Spurious IRQ %#x\n", rd_fifo_status);
+		return IRQ_NONE;
+	}
+
+	wr_cnts = (rd_fifo_status & WR_CNTS_MSK) >> WR_CNTS_SHFT;
+
+	if (wr_cnts > ctrl->xfer.rem_bytes)
+		wr_cnts = ctrl->xfer.rem_bytes;
+
+	words_to_read = wr_cnts / QSPI_BYTES_PER_WORD;
+	bytes_to_read = wr_cnts % QSPI_BYTES_PER_WORD;
+
+	if (words_to_read) {
+		word_buf = ctrl->xfer.rx_buf;
+		ctrl->xfer.rem_bytes -= words_to_read * QSPI_BYTES_PER_WORD;
+		for (i = 0; i < words_to_read; i++) {
+			rd_fifo = readl(ctrl->base + RD_FIFO);
+			put_unaligned(rd_fifo, word_buf++);
+		}
+		ctrl->xfer.rx_buf = word_buf;
+	}
+
+	if (bytes_to_read) {
+		byte_buf = ctrl->xfer.rx_buf;
+		rd_fifo = readl(ctrl->base + RD_FIFO);
+		ctrl->xfer.rem_bytes -= bytes_to_read;
+		for (i = 0; i < bytes_to_read; i++)
+			*byte_buf++ = rd_fifo >> (i * BITS_PER_BYTE);
+		ctrl->xfer.rx_buf = byte_buf;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t pio_write(struct qcom_qspi *ctrl)
+{
+	const void *xfer_buf = ctrl->xfer.tx_buf;
+	const int *word_buf;
+	const char *byte_buf;
+	unsigned int wr_fifo_bytes;
+	unsigned int wr_fifo_words;
+	unsigned int wr_size;
+	unsigned int rem_words;
+
+	wr_fifo_bytes = readl(ctrl->base + PIO_XFER_STATUS)
+				>> WR_FIFO_BYTES_SHFT;
+
+	if (ctrl->xfer.rem_bytes < QSPI_BYTES_PER_WORD) {
+		/* Process the last 1-3 bytes */
+		wr_size = min(wr_fifo_bytes, ctrl->xfer.rem_bytes);
+		ctrl->xfer.rem_bytes -= wr_size;
+
+		byte_buf = xfer_buf;
+		while (wr_size--)
+			writel(*byte_buf++,
+			       ctrl->base + PIO_DATAOUT_1B);
+		ctrl->xfer.tx_buf = byte_buf;
+	} else {
+		/*
+		 * Process all the whole words; to keep things simple we'll
+		 * just wait for the next interrupt to handle the last 1-3
+		 * bytes if we don't have an even number of words.
+		 */
+		rem_words = ctrl->xfer.rem_bytes / QSPI_BYTES_PER_WORD;
+		wr_fifo_words = wr_fifo_bytes / QSPI_BYTES_PER_WORD;
+
+		wr_size = min(rem_words, wr_fifo_words);
+		ctrl->xfer.rem_bytes -= wr_size * QSPI_BYTES_PER_WORD;
+
+		word_buf = xfer_buf;
+		while (wr_size--)
+			writel(get_unaligned(word_buf++),
+			       ctrl->base + PIO_DATAOUT_4B);
+		ctrl->xfer.tx_buf = word_buf;
+
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t qcom_qspi_irq(int irq, void *dev_id)
+{
+	u32 int_status;
+	struct qcom_qspi *ctrl = dev_id;
+	irqreturn_t ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctrl->lock, flags);
+
+	int_status = readl(ctrl->base + MSTR_INT_STATUS);
+	writel(int_status, ctrl->base + MSTR_INT_STATUS);
+
+	if (ctrl->xfer.dir == QSPI_WRITE) {
+		if (int_status & WR_FIFO_EMPTY)
+			ret = pio_write(ctrl);
+	} else {
+		if (int_status & RESP_FIFO_RDY)
+			ret = pio_read(ctrl);
+	}
+
+	if (!ctrl->xfer.rem_bytes) {
+		writel(0, ctrl->base + MSTR_INT_EN);
+		spi_finalize_current_transfer(dev_get_drvdata(ctrl->dev));
+	}
+
+	spin_unlock_irqrestore(&ctrl->lock, flags);
+	return ret;
+}
+
+static int qcom_qspi_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct device *dev;
+	struct resource *res;
+	struct spi_master *master;
+	struct qcom_qspi *ctrl;
+
+	dev = &pdev->dev;
+
+	master = spi_alloc_master(dev, sizeof(struct qcom_qspi));
+	if (!master) {
+		dev_err(dev, "Failed to alloc spi master\n");
+		return -ENOMEM;
+	}
+	platform_set_drvdata(pdev, master);
+
+	ctrl = spi_master_get_devdata(master);
+
+	spin_lock_init(&ctrl->lock);
+	ctrl->dev = dev;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ctrl->base = devm_ioremap(dev, res->start, resource_size(res));
+	if (IS_ERR(ctrl->base)) {
+		ret = PTR_ERR(ctrl->base);
+		dev_err(dev, "Failed to get base addr %d\n", ret);
+		goto exit_probe_master_put;
+	}
+
+	ctrl->clks[QSPI_CLK_CORE].id = "core";
+	ctrl->clks[QSPI_CLK_IFACE].id = "iface";
+	ret = devm_clk_bulk_get(dev, QSPI_NUM_CLKS, ctrl->clks);
+	if (ret)
+		goto exit_probe_master_put;
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get irq %d\n", ret);
+		goto exit_probe_master_put;
+	}
+	ret = devm_request_irq(dev, ret, qcom_qspi_irq,
+			IRQF_TRIGGER_HIGH, dev_name(dev), ctrl);
+	if (ret) {
+		dev_err(dev, "Failed to request irq %d\n", ret);
+		goto exit_probe_master_put;
+	}
+
+	master->max_speed_hz = 300000000;
+	master->num_chipselect = QSPI_NUM_CS;
+	master->bus_num = pdev->id;
+	master->dev.of_node = pdev->dev.of_node;
+	master->mode_bits = SPI_MODE_0 |
+			    SPI_TX_DUAL | SPI_RX_DUAL |
+			    SPI_TX_QUAD | SPI_RX_QUAD;
+	master->flags = SPI_MASTER_HALF_DUPLEX;
+	master->prepare_message = qcom_qspi_prepare_message;
+	master->transfer_one = qcom_qspi_transfer_one;
+	master->handle_err = qcom_qspi_handle_err;
+	master->auto_runtime_pm = true;
+
+	pm_runtime_enable(dev);
+
+	ret = spi_register_master(master);
+	if (ret)
+		goto exit_probe_runtime_disable;
+
+	return 0;
+
+exit_probe_runtime_disable:
+	pm_runtime_disable(dev);
+
+exit_probe_master_put:
+	spi_master_put(master);
+
+	return ret;
+}
+
+static int qcom_qspi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+
+	/* Unregister _before_ disabling pm_runtime() so we stop transfers */
+	spi_unregister_master(master);
+
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev)
+{
+	struct spi_master *master = dev_get_drvdata(dev);
+	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
+
+	clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks);
+
+	return 0;
+}
+
+static int __maybe_unused qcom_qspi_runtime_resume(struct device *dev)
+{
+	struct spi_master *master = dev_get_drvdata(dev);
+	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
+
+	return clk_bulk_prepare_enable(QSPI_NUM_CLKS, ctrl->clks);
+}
+
+static int __maybe_unused qcom_qspi_suspend(struct device *dev)
+{
+	struct spi_master *master = dev_get_drvdata(dev);
+	int ret;
+
+	ret = spi_master_suspend(master);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		spi_master_resume(master);
+
+	return ret;
+}
+
+static int __maybe_unused qcom_qspi_resume(struct device *dev)
+{
+	struct spi_master *master = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		return ret;
+
+	ret = spi_master_resume(master);
+	if (ret)
+		pm_runtime_force_suspend(dev);
+
+	return ret;
+}
+
+static const struct dev_pm_ops qcom_qspi_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(qcom_qspi_runtime_suspend,
+			   qcom_qspi_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(qcom_qspi_suspend, qcom_qspi_resume)
+};
+
+static const struct of_device_id qcom_qspi_dt_match[] = {
+	{ .compatible = "qcom,qspi-v1", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, qcom_qspi_dt_match);
+
+static struct platform_driver qcom_qspi_driver = {
+	.driver = {
+		.name		= "qcom_qspi",
+		.pm		= &qcom_qspi_dev_pm_ops,
+		.of_match_table = qcom_qspi_dt_match,
+	},
+	.probe = qcom_qspi_probe,
+	.remove = qcom_qspi_remove,
+};
+module_platform_driver(qcom_qspi_driver);
+
+MODULE_DESCRIPTION("SPI driver for QSPI cores");
+MODULE_LICENSE("GPL v2");
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH v3 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Doug Anderson]

Hi,

On Wed, Sep 26, 2018 at 1:54 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> From: Girish Mahadevan <girishm@codeaurora.org>
>
> Bindings for Qualcomm Quad SPI used on SoCs such as sdm845.
>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
> Changes in v3:
> - Added generic compatible string in addition to specific SoC
>
> Changes in v2:
> - Added commit text
> - Removed invalid property
> - Updated example to match sdm845 with attached spi-nor
>
>  .../bindings/spi/qcom,spi-qcom-qspi.txt       | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt

This looks good to me and change in v3 to add the SoC-specific string
in addition to the more generic string identifying the IP block
version matches my understanding of the correct things to do (as
discussed in the v2 patch).

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v3 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

[Doug Anderson]

Ryan,

On Wed, Sep 26, 2018 at 1:54 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> From: Girish Mahadevan <girishm@codeaurora.org>
>
> New driver for Qualcomm QuadSPI(QSPI) controller that is used to
> communicate with slaves such flash memory devices. The QSPI controller
> can operate in 2 or 4 wire mode but only supports SPI Mode 0. The
> controller can also operate in Single or Dual data rate modes.
>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
> Changes in v3:
> - Corrected QPSPI typo
> - Removed setup function and moved configurations to prepare_message
> - Added __maybe_unused to suspend and resume functions
>
> Changes in v2:
> - Addressed formatting feedback
> - Squashed bug fixes and features from Doug
> - Now uses transfer_one_message instead of mem_ops
> - Fixed suspend/resume
> - Added spinlocks
>
>  drivers/spi/Kconfig         |   6 +
>  drivers/spi/Makefile        |   1 +
>  drivers/spi/spi-qcom-qspi.c | 598 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 605 insertions(+)
>  create mode 100644 drivers/spi/spi-qcom-qspi.c

This looks good to me and addresses all outstanding feedback I'm aware
of from v2.  I've also tested this patch and it's working fine.  Thus:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

NOTE to Mark: please be aware that there are currently _two_ SPI
drivers in flight for sdm845 since there are two totally different SPI
IP blocks in SDM845.  We need both this driver (the Quad SPI one) and
also the other driver (the GENI SPI one).  As I understand it Dilip
plans to send the next spin of the GENI SPI driver some time in the
next day or two.

-Doug

Re: [PATCH v3 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Stephen Boyd]

Quoting Ryan Case (2018-09-26 13:52:03)
> From: Girish Mahadevan <girishm@codeaurora.org>
> 
> Bindings for Qualcomm Quad SPI used on SoCs such as sdm845.
> 
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v3 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

[Stephen Boyd]

Quoting Ryan Case (2018-09-26 13:52:04)
> From: Girish Mahadevan <girishm@codeaurora.org>
> 
> New driver for Qualcomm QuadSPI(QSPI) controller that is used to
> communicate with slaves such flash memory devices. The QSPI controller

s/such/such as/?

> can operate in 2 or 4 wire mode but only supports SPI Mode 0. The
> controller can also operate in Single or Dual data rate modes.
> 
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 876f8690fc47..f997c49255a6 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_SPI_ZYNQMP_GQSPI)              += spi-zynqmp-gqspi.o
>  # SPI slave protocol handlers
>  obj-$(CONFIG_SPI_SLAVE_TIME)           += spi-slave-time.o
>  obj-$(CONFIG_SPI_SLAVE_SYSTEM_CONTROL) += spi-slave-system-control.o
> +obj-$(CONFIG_SPI_QCOM_QSPI)            += spi-qcom-qspi.o

Move this to alphabetical in the SPI master controller list of this
file please.

> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
> new file mode 100644
> index 000000000000..0ad2ef003068
> --- /dev/null
> +++ b/drivers/spi/spi-qcom-qspi.c
> @@ -0,0 +1,598 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define AHB_MIN_HZ             9600000UL

Is this used?

> +#define QSPI_NUM_CS            2
> +#define QSPI_BYTES_PER_WORD    4
> +#define MSTR_CONFIG            0x0000
> +#define AHB_MASTER_CFG         0x0004
> +#define MSTR_INT_EN            0x000C
> +#define MSTR_INT_STATUS                0x0010
> +#define PIO_XFER_CTRL          0x0014
> +#define PIO_XFER_CFG           0x0018
> +#define PIO_XFER_STATUS                0x001c
> +#define PIO_DATAOUT_1B         0x0020
> +#define PIO_DATAOUT_4B         0x0024
> +#define RD_FIFO_CFG            0x0028
> +#define RD_FIFO_STATUS         0x002c
> +#define RD_FIFO_RESET          0x0030
> +#define CUR_MEM_ADDR           0x0048
> +#define HW_VERSION             0x004c
> +#define RD_FIFO                        0x0050
> +#define SAMPLING_CLK_CFG       0x0090
> +#define SAMPLING_CLK_STATUS    0x0094
> +
> +/* Macros to help set/get fields in MSTR_CONFIG register */
> +#define        FULL_CYCLE_MODE         BIT(3)
> +#define        FB_CLK_EN               BIT(4)
> +#define        PIN_HOLDN               BIT(6)
> +#define        PIN_WPN                 BIT(7)
> +#define        DMA_ENABLE              BIT(8)
> +#define        BIG_ENDIAN_MODE         BIT(9)
> +#define        SPI_MODE_MSK            0xc00
> +#define        SPI_MODE_SHFT           10
> +#define        CHIP_SELECT_NUM         BIT(12)
> +#define        SBL_EN                  BIT(13)
> +#define        LPA_BASE_MSK            0x3c000
> +#define        LPA_BASE_SHFT           14
> +#define        TX_DATA_DELAY_MSK       0xc0000
> +#define        TX_DATA_DELAY_SHFT      18
> +#define        TX_CLK_DELAY_MSK        0x300000
> +#define        TX_CLK_DELAY_SHFT       20
> +#define        TX_CS_N_DELAY_MSK       0xc00000
> +#define        TX_CS_N_DELAY_SHFT      22
> +#define        TX_DATA_OE_DELAY_MSK    0x3000000
> +#define        TX_DATA_OE_DELAY_SHFT   24
> +
> +/* Macros to help set/get fields in AHB_MSTR_CFG register */
> +#define        HMEM_TYPE_START_MID_TRANS_MSK           0x7
> +#define        HMEM_TYPE_START_MID_TRANS_SHFT          0
> +#define        HMEM_TYPE_LAST_TRANS_MSK                0x38
> +#define        HMEM_TYPE_LAST_TRANS_SHFT               3
> +#define        USE_HMEMTYPE_LAST_ON_DESC_OR_CHAIN_MSK  0xc0
> +#define        USE_HMEMTYPE_LAST_ON_DESC_OR_CHAIN_SHFT 6
> +#define        HMEMTYPE_READ_TRANS_MSK                 0x700
> +#define        HMEMTYPE_READ_TRANS_SHFT                8
> +#define        HSHARED                                 BIT(11)
> +#define        HINNERSHARED                            BIT(12)
> +
> +/* Macros to help set/get fields in MSTR_INT_EN/MSTR_INT_STATUS registers */
> +#define        RESP_FIFO_UNDERRUN      BIT(0)
> +#define        RESP_FIFO_NOT_EMPTY     BIT(1)
> +#define        RESP_FIFO_RDY           BIT(2)
> +#define        HRESP_FROM_NOC_ERR      BIT(3)
> +#define        WR_FIFO_EMPTY           BIT(9)
> +#define        WR_FIFO_FULL            BIT(10)
> +#define        WR_FIFO_OVERRUN         BIT(11)
> +#define        TRANSACTION_DONE        BIT(16)
> +#define QSPI_ERR_IRQS          (RESP_FIFO_UNDERRUN | HRESP_FROM_NOC_ERR | \
> +                                WR_FIFO_OVERRUN)
> +#define        QSPI_ALL_IRQS           (QSPI_ERR_IRQS | RESP_FIFO_RDY | \
> +                                WR_FIFO_EMPTY | WR_FIFO_FULL | \
> +                                TRANSACTION_DONE)
> +
> +/* Macros to help set/get fields in RD_FIFO_CONFIG register */
> +#define        CONTINUOUS_MODE         BIT(0)
> +
> +/* Macros to help set/get fields in RD_FIFO_RESET register */
> +#define        RESET_FIFO              BIT(0)
> +
> +/* Macros to help set/get fields in PIO_TRANSFER_CONFIG register */
> +#define        TRANSFER_DIRECTION      BIT(0)
> +#define        MULTI_IO_MODE_MSK       0xe
> +#define        MULTI_IO_MODE_SHFT      1
> +#define        TRANSFER_FRAGMENT       BIT(8)
> +
> +/* Macros to help set/get fields in PIO_TRANSFER_CONTROL register */
> +#define        REQUEST_COUNT_MSK       0xffff
> +
> +/* Macros to help set/get fields in PIO_TRANSFER_STATUS register */
> +#define        WR_FIFO_BYTES_MSK       0xffff0000
> +#define        WR_FIFO_BYTES_SHFT      16
> +
> +/* Macros to help set/get fields in RD_FIFO_STATUS register */

Typically we put these definitions immediately after the register offset
that uses them so we don't need these comments to tell us what registers
they apply to.

> +#define        FIFO_EMPTY      BIT(11)
> +#define        WR_CNTS_MSK     0x7f0
> +#define        WR_CNTS_SHFT    4
> +#define        RDY_64BYTE      BIT(3)
> +#define        RDY_32BYTE      BIT(2)
> +#define        RDY_16BYTE      BIT(1)
> +#define        FIFO_RDY        BIT(0)
> +
> +/*
> + * The Mode transfer macros, the values are programmed to the HW registers
> + * when doing PIO mode of transfers.
> + */
> +#define        SDR_1BIT        1
> +#define        SDR_2BIT        2
> +#define        SDR_4BIT        3
> +#define        DDR_1BIT        5
> +#define        DDR_2BIT        6
> +#define        DDR_4BIT        7
> +
> +/* The Mode transfer macros when setting up DMA descriptors */
> +#define        DMA_DESC_SINGLE_SPI     1
> +#define        DMA_DESC_DUAL_SPI       2
> +#define        DMA_DESC_QUAD_SPI       3
> +
> +enum qspi_dir {
> +       QSPI_READ,
> +       QSPI_WRITE,
> +};
> +
> +struct qspi_xfer {
> +       union {
> +               const void *tx_buf;
> +               void *rx_buf;
> +       };
> +       unsigned int rem_bytes;
> +       int buswidth;

unsigned int?

> +       enum qspi_dir dir;
> +       bool is_last;
> +};
> +
> +enum qspi_clocks {
> +       QSPI_CLK_CORE,
> +       QSPI_CLK_IFACE,
> +       QSPI_NUM_CLKS
> +};
> +
> +struct qcom_qspi {
> +       void __iomem *base;
> +       struct device *dev;
> +       struct clk_bulk_data clks[QSPI_NUM_CLKS];
> +       struct qspi_xfer xfer;
> +       spinlock_t lock;

What is the lock protecting? Hardware interrupt state? Maybe add a small
comment to help the reader know what needs protecting.

> +};
> +
> +static int qspi_buswidth_to_iomode(struct qcom_qspi *ctrl, int buswidth)
> +{
> +       switch (buswidth) {
> +       case 1:
> +               return SDR_1BIT;
> +       case 2:
> +               return SDR_2BIT;
> +       case 4:
> +               return SDR_4BIT;
> +       default:
> +               dev_warn_once(ctrl->dev,
> +                               "Unexpected bus width: %d\n", buswidth);
> +               return SDR_1BIT;
> +       }
> +}
> +
> +static void qcom_qspi_pio_xfer_cfg(struct qcom_qspi *ctrl)
> +{
> +       u32 pio_xfer_cfg = 0;

Please remove useless initial assignment.

> +       struct qspi_xfer *xfer;

const?

> +
> +       xfer = &ctrl->xfer;
> +       pio_xfer_cfg = readl(ctrl->base + PIO_XFER_CFG);
> +       pio_xfer_cfg &= ~TRANSFER_DIRECTION;
> +       pio_xfer_cfg |= xfer->dir;
> +       if (xfer->is_last)
> +               pio_xfer_cfg &= ~TRANSFER_FRAGMENT;
> +       else
> +               pio_xfer_cfg |= TRANSFER_FRAGMENT;
> +       pio_xfer_cfg &= ~MULTI_IO_MODE_MSK;
> +       pio_xfer_cfg |= qspi_buswidth_to_iomode(ctrl, xfer->buswidth) <<
> +                       MULTI_IO_MODE_SHFT;

Style nitpick, this looks very odd split across two lines. Probably
better to make qspi_buswidth_to_iomode() return the shifted value
because this is the only call-site and then everything fits on one line.
Could also rename 'pio_xfer_cfg' to just 'cfg' and probably everything
would be short too.

> +
> +       writel(pio_xfer_cfg, ctrl->base + PIO_XFER_CFG);
> +}
> +
> +static void qcom_qspi_pio_xfer_ctrl(struct qcom_qspi *ctrl)
> +{
> +       u32 pio_xfer_ctrl;
> +
> +       pio_xfer_ctrl = readl(ctrl->base + PIO_XFER_CTRL);
> +       pio_xfer_ctrl &= ~REQUEST_COUNT_MSK;
> +       pio_xfer_ctrl |= ctrl->xfer.rem_bytes;
> +       writel(pio_xfer_ctrl, ctrl->base + PIO_XFER_CTRL);
> +}
> +
> +static void qcom_qspi_pio_xfer(struct qcom_qspi *ctrl)
> +{
> +       u32 ints;
> +
> +       qcom_qspi_pio_xfer_cfg(ctrl);
> +
> +       /* Ack any previous interrupts that might be hanging around */
> +       writel(QSPI_ALL_IRQS, ctrl->base + MSTR_INT_STATUS);
> +
> +       /* Setup new interrupts */
> +       if (ctrl->xfer.dir == QSPI_WRITE)
> +               ints = QSPI_ERR_IRQS | WR_FIFO_EMPTY;
> +       else
> +               ints = QSPI_ERR_IRQS | RESP_FIFO_RDY;
> +       writel(ints, ctrl->base + MSTR_INT_EN);
> +
> +       /* Kick off the transfer */
> +       qcom_qspi_pio_xfer_ctrl(ctrl);
> +}
> +
> +static void qcom_qspi_handle_err(struct spi_master *master,
> +                                struct spi_message *msg)
> +{
> +       struct qcom_qspi *ctrl = spi_master_get_devdata(master);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&ctrl->lock, flags);
> +       writel(0, ctrl->base + MSTR_INT_EN);
> +       ctrl->xfer.rem_bytes = 0;
> +       spin_unlock_irqrestore(&ctrl->lock, flags);
> +}
> +
> +static int qcom_qspi_transfer_one(struct spi_master *master,
> +                                 struct spi_device *slv,
> +                                 struct spi_transfer *xfer)
> +{
> +       struct qcom_qspi *ctrl = spi_master_get_devdata(master);
> +       int ret;
> +       unsigned long speed_hz;
> +       unsigned long flags;
> +
> +       speed_hz = slv->max_speed_hz;
> +       if (xfer->speed_hz)
> +               speed_hz = xfer->speed_hz;
> +
> +       ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4);

Why 4? Is that related to the number of wires?

> +       if (ret) {
> +               dev_err(ctrl->dev, "Failed to set core clk %d\n", ret);
> +               return ret;
> +       }
> +
> +       spin_lock_irqsave(&ctrl->lock, flags);
> +
> +       /* We are half duplex, so either rx or tx will be set */
> +       if (xfer->rx_buf) {
> +               ctrl->xfer.dir = QSPI_READ;
> +               ctrl->xfer.buswidth = xfer->rx_nbits;
> +               ctrl->xfer.rx_buf = xfer->rx_buf;
> +       } else {
> +               ctrl->xfer.dir = QSPI_WRITE;
> +               ctrl->xfer.buswidth = xfer->tx_nbits;
> +               ctrl->xfer.tx_buf = xfer->tx_buf;
> +       }
> +       ctrl->xfer.is_last = list_is_last(&xfer->transfer_list,
> +                                         &master->cur_msg->transfers);
> +       ctrl->xfer.rem_bytes = xfer->len;
> +       qcom_qspi_pio_xfer(ctrl);
> +
> +       spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +       /* We'll call spi_finalize_current_transfer() when done */
> +       return 1;
> +}
> +
> +static int qcom_qspi_prepare_message(struct spi_master *master,
> +                                    struct spi_message *message)
> +{
> +       u32 mstr_cfg;
> +       struct qcom_qspi *ctrl;
> +       int tx_data_oe_delay = 1;
> +       int tx_data_delay = 1;
> +       unsigned long flags;
> +
> +       ctrl = spi_master_get_devdata(master);
> +       spin_lock_irqsave(&ctrl->lock, flags);
> +
> +       mstr_cfg = readl(ctrl->base + MSTR_CONFIG);
> +       mstr_cfg &= ~CHIP_SELECT_NUM;
> +       if (message->spi->chip_select)
> +               mstr_cfg |= CHIP_SELECT_NUM;
> +
> +       mstr_cfg = (mstr_cfg & ~SPI_MODE_MSK) |
> +                               (message->spi->mode << SPI_MODE_SHFT);
> +       mstr_cfg |= FB_CLK_EN | PIN_WPN | PIN_HOLDN | SBL_EN | FULL_CYCLE_MODE;
> +       mstr_cfg |= (mstr_cfg & ~TX_DATA_OE_DELAY_MSK) |
> +                               (tx_data_oe_delay << TX_DATA_OE_DELAY_SHFT);
> +       mstr_cfg |= (mstr_cfg & ~TX_DATA_DELAY_MSK) |
> +                               (tx_data_delay << TX_DATA_DELAY_SHFT);

Maybe just write them all on one line with mstr_cfg |=? Then it's not
indendented like that:

       mstr_cfg &= ~(SPI_MODE_MSK | TX_DATA_OE_DELAY_MSK | TX_DATA_DELAY_MSK);
       mstr_cfg |= message->spi->mode << SPI_MODE_SHFT;
       mstr_cfg |= FB_CLK_EN | PIN_WPN | PIN_HOLDN | SBL_EN | FULL_CYCLE_MODE;
       mstr_cfg |= tx_data_oe_delay << TX_DATA_OE_DELAY_SHFT;
       mstr_cfg |= tx_data_delay << TX_DATA_DELAY_SHFT;

> +       mstr_cfg &= ~DMA_ENABLE;
> +
> +       writel(mstr_cfg, ctrl->base + MSTR_CONFIG);
> +       spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +       return 0;
> +}
> +
> +static irqreturn_t pio_read(struct qcom_qspi *ctrl)
> +{
> +       u32 rd_fifo_status;
> +       u32 rd_fifo;
> +       unsigned int wr_cnts;
> +       unsigned int bytes_to_read;
> +       unsigned int words_to_read;
> +       u32 *word_buf;
> +       u8 *byte_buf;
> +       int i;
> +
> +       rd_fifo_status = readl(ctrl->base + RD_FIFO_STATUS);
> +
> +       if (!(rd_fifo_status & FIFO_RDY)) {
> +               dev_dbg(ctrl->dev, "Spurious IRQ %#x\n", rd_fifo_status);
> +               return IRQ_NONE;
> +       }
> +
> +       wr_cnts = (rd_fifo_status & WR_CNTS_MSK) >> WR_CNTS_SHFT;
> +
> +       if (wr_cnts > ctrl->xfer.rem_bytes)
> +               wr_cnts = ctrl->xfer.rem_bytes;

Could be

	wr_cnts = min(wr_cnts, ctrl->xfer.rem_bytes)

> +
> +       words_to_read = wr_cnts / QSPI_BYTES_PER_WORD;
> +       bytes_to_read = wr_cnts % QSPI_BYTES_PER_WORD;
> +
> +       if (words_to_read) {
> +               word_buf = ctrl->xfer.rx_buf;
> +               ctrl->xfer.rem_bytes -= words_to_read * QSPI_BYTES_PER_WORD;
> +               for (i = 0; i < words_to_read; i++) {
> +                       rd_fifo = readl(ctrl->base + RD_FIFO);

This will mess things up on big endian CPUs and make the CPU buffer byte
swapped. Use readsl() or ioread32_rep().

> +                       put_unaligned(rd_fifo, word_buf++);
> +               }
> +               ctrl->xfer.rx_buf = word_buf;
> +       }
> +
> +       if (bytes_to_read) {
> +               byte_buf = ctrl->xfer.rx_buf;

Does this need to move forward by words_to_read bytes so that the left
over bytes are tacked onto the end? Or this should be an else if
statement?

> +               rd_fifo = readl(ctrl->base + RD_FIFO);
> +               ctrl->xfer.rem_bytes -= bytes_to_read;
> +               for (i = 0; i < bytes_to_read; i++)
> +                       *byte_buf++ = rd_fifo >> (i * BITS_PER_BYTE);
> +               ctrl->xfer.rx_buf = byte_buf;
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t pio_write(struct qcom_qspi *ctrl)
> +{
> +       const void *xfer_buf = ctrl->xfer.tx_buf;
> +       const int *word_buf;
> +       const char *byte_buf;
> +       unsigned int wr_fifo_bytes;
> +       unsigned int wr_fifo_words;
> +       unsigned int wr_size;
> +       unsigned int rem_words;
> +
> +       wr_fifo_bytes = readl(ctrl->base + pio_xfer_status)
> +                               >> wr_fifo_bytes_shft;

Just write this as:

       wr_fifo_bytes = readl(ctrl->base + pio_xfer_status);
       wr_fifo_bytes >>= WR_FIFO_BYTES_SHFT;

and is that supposed to be uppercase but it's lower case? Or did I
somehow mess that up when replying?

> +
> +       if (ctrl->xfer.rem_bytes < QSPI_BYTES_PER_WORD) {
> +               /* Process the last 1-3 bytes */
> +               wr_size = min(wr_fifo_bytes, ctrl->xfer.rem_bytes);
> +               ctrl->xfer.rem_bytes -= wr_size;
> +
> +               byte_buf = xfer_buf;
> +               while (wr_size--)
> +                       writel(*byte_buf++,
> +                              ctrl->base + PIO_DATAOUT_1B);
> +               ctrl->xfer.tx_buf = byte_buf;
> +       } else {
> +               /*
> +                * Process all the whole words; to keep things simple we'll
> +                * just wait for the next interrupt to handle the last 1-3
> +                * bytes if we don't have an even number of words.
> +                */
> +               rem_words = ctrl->xfer.rem_bytes / QSPI_BYTES_PER_WORD;
> +               wr_fifo_words = wr_fifo_bytes / QSPI_BYTES_PER_WORD;
> +
> +               wr_size = min(rem_words, wr_fifo_words);
> +               ctrl->xfer.rem_bytes -= wr_size * QSPI_BYTES_PER_WORD;
> +
> +               word_buf = xfer_buf;
> +               while (wr_size--)
> +                       writel(get_unaligned(word_buf++),
> +                              ctrl->base + PIO_DATAOUT_4B);

Is this a FIFO? Should use writesl() or iowrite32_rep() then when
throwing words at a time into the FIFO so that it works on any CPU
endianess for a little endian device.

> +               ctrl->xfer.tx_buf = word_buf;
> +
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t qcom_qspi_irq(int irq, void *dev_id)
> +{
> +       u32 int_status;
> +       struct qcom_qspi *ctrl = dev_id;
> +       irqreturn_t ret;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&ctrl->lock, flags);
> +
> +       int_status = readl(ctrl->base + MSTR_INT_STATUS);
> +       writel(int_status, ctrl->base + MSTR_INT_STATUS);
> +
> +       if (ctrl->xfer.dir == QSPI_WRITE) {
> +               if (int_status & WR_FIFO_EMPTY)
> +                       ret = pio_write(ctrl);
> +       } else {
> +               if (int_status & RESP_FIFO_RDY)
> +                       ret = pio_read(ctrl);

What if int_status & RESP_FIFO_RDY isn't set? Then 'ret' is never
assigned? Should have ret = IRQ_NONE up above I guess.

> +       }

And should the error interrupt bits be checked and printed if they
happen? We seem to unmask them, but then we don't really do anything
with them if they happen.

> +
> +       if (!ctrl->xfer.rem_bytes) {
> +               writel(0, ctrl->base + MSTR_INT_EN);
> +               spi_finalize_current_transfer(dev_get_drvdata(ctrl->dev));
> +       }
> +
> +       spin_unlock_irqrestore(&ctrl->lock, flags);
> +       return ret;
> +}
> +
> +static int qcom_qspi_probe(struct platform_device *pdev)
> +{
> +       int ret = 0;

Should be able to drop the assignment here. Hopefully compiler doesn't
complain.

> +       struct device *dev;
> +       struct resource *res;
> +       struct spi_master *master;
> +       struct qcom_qspi *ctrl;
> +
> +       dev = &pdev->dev;
> +
> +       master = spi_alloc_master(dev, sizeof(struct qcom_qspi));

sizeof(*ctrl) so we know what is being stored inside?

> +       if (!master) {
> +               dev_err(dev, "Failed to alloc spi master\n");

We don't need allocation error messages. Just return -ENOMEM here.

> +               return -ENOMEM;
> +       }
> +       platform_set_drvdata(pdev, master);
> +
> +       ctrl = spi_master_get_devdata(master);
> +
> +       spin_lock_init(&ctrl->lock);
> +       ctrl->dev = dev;
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       ctrl->base = devm_ioremap(dev, res->start, resource_size(res));
> +       if (IS_ERR(ctrl->base)) {
> +               ret = PTR_ERR(ctrl->base);
> +               dev_err(dev, "Failed to get base addr %d\n", ret);

Use devm_ioremap_resource()? And then drop this error print?

> +               goto exit_probe_master_put;
> +       }
> +
> +       ctrl->clks[QSPI_CLK_CORE].id = "core";
> +       ctrl->clks[QSPI_CLK_IFACE].id = "iface";
> +       ret = devm_clk_bulk_get(dev, QSPI_NUM_CLKS, ctrl->clks);
> +       if (ret)
> +               goto exit_probe_master_put;
> +
> +       ret = platform_get_irq(pdev, 0);
> +       if (ret < 0) {
> +               dev_err(dev, "Failed to get irq %d\n", ret);
> +               goto exit_probe_master_put;
> +       }
> +       ret = devm_request_irq(dev, ret, qcom_qspi_irq,
> +                       IRQF_TRIGGER_HIGH, dev_name(dev), ctrl);
> +       if (ret) {
> +               dev_err(dev, "Failed to request irq %d\n", ret);
> +               goto exit_probe_master_put;
> +       }
> +
> +       master->max_speed_hz = 300000000;
> +       master->num_chipselect = QSPI_NUM_CS;
> +       master->bus_num = pdev->id;

Can this come from DT aliases? I've never thought about qspi and
"regular" spi being in the same spi bus numbering system, but I suppose
that will happen now and we need to make sure that qspi numbers start
after the regular ones?

> +       master->dev.of_node = pdev->dev.of_node;
> +       master->mode_bits = SPI_MODE_0 |
> +                           SPI_TX_DUAL | SPI_RX_DUAL |
> +                           SPI_TX_QUAD | SPI_RX_QUAD;
> +       master->flags = SPI_MASTER_HALF_DUPLEX;
> +       master->prepare_message = qcom_qspi_prepare_message;
> +       master->transfer_one = qcom_qspi_transfer_one;
> +       master->handle_err = qcom_qspi_handle_err;
> +       master->auto_runtime_pm = true;
> +
> +       pm_runtime_enable(dev);
> +
> +       ret = spi_register_master(master);
> +       if (ret)
> +               goto exit_probe_runtime_disable;
> +
> +       return 0;

Or 

	if (!ret)
		return 0;
	
> +
> +exit_probe_runtime_disable:

And then drop this label.

> +       pm_runtime_disable(dev);
> +
> +exit_probe_master_put:
> +       spi_master_put(master);
> +
> +       return ret;
> +}

Re: [PATCH v3 1/2] dt-bindings: spi: Qualcomm Quad SPI(QSPI) documentation

[Rob Herring]

On Wed, Sep 26, 2018 at 01:52:03PM -0700, Ryan Case wrote:
> From: Girish Mahadevan <girishm@codeaurora.org>
> 
> Bindings for Qualcomm Quad SPI used on SoCs such as sdm845.
> 
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
> 
> Changes in v3:
> - Added generic compatible string in addition to specific SoC
> 
> Changes in v2:
> - Added commit text
> - Removed invalid property
> - Updated example to match sdm845 with attached spi-nor
> 
>  .../bindings/spi/qcom,spi-qcom-qspi.txt       | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> new file mode 100644
> index 000000000000..e13f5bb314ad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-qcom-qspi.txt
> @@ -0,0 +1,36 @@
> +Qualcomm Quad Serial Peripheral Interface (QSPI)
> +
> +The QSPI controller allows SPI protocol communication in single, dual, or quad
> +wire transmission modes for read/write access to slaves such as NOR flash.
> +
> +Required properties:
> +- compatible:	An SoC specific identifier followed by "qcom,qspi-v1", such as
> +		"qcom,sdm845-qspi", "qcom,qspi-v1"
> +- reg:		Should contain the base register location and length.
> +- interrupts:	Interrupt number used by the controller.
> +- clocks:	Should contain the core and AHB clock.
> +- clock-names:	Should be "core" for core clock and "iface" for AHB clock.
> +
> +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:
> +
> +	qspi: qspi@88df000 {

spi@...

> +		compatible = "qcom,sdm845-qspi", "qcom,qspi-v1";
> +		reg = <0x88df000 0x600>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
> +		clock-names = "iface", "core";
> +		clocks = <&gcc GCC_QSPI_CNOC_PERIPH_AHB_CLK>,
> +			 <&gcc GCC_QSPI_CORE_CLK>;
> +
> +		device@0 {

flash@0

With that,

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

> +			compatible = "jedec,spi-nor";
> +			reg = <0>;
> +			spi-max-frequency = <25000000>;
> +			spi-tx-bus-width = <2>;
> +			spi-rx-bus-width = <2>;
> +		};
> +	};
> -- 
> 2.19.0.605.g01d371f741-goog
> 

Re: [PATCH v3 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

[Ryan Case]

On Wed, Sep 26, 2018 at 11:43 PM Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Ryan Case (2018-09-26 13:52:04)
> > From: Girish Mahadevan <girishm@codeaurora.org>
> > +#include <asm/unaligned.h>
> > +
> > +#define AHB_MIN_HZ             9600000UL
>
> Is this used?

Nope. Do you want all currently unused defines removed or specifically this
one? I saw precedent in other drivers for defining registers/flags/values of
supported but unused functionality so I left these (big endian, DDR, ...).


> > +       speed_hz = slv->max_speed_hz;
> > +       if (xfer->speed_hz)
> > +               speed_hz = xfer->speed_hz;
> > +
> > +       ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4);
>
> Why 4? Is that related to the number of wires?

In normal operation the core clock should be running at 4x the rate of the
transfer clock regardless of number of wires used.

> > +                       put_unaligned(rd_fifo, word_buf++);
> > +               }
> > +               ctrl->xfer.rx_buf = word_buf;
> > +       }
> > +
> > +       if (bytes_to_read) {
> > +               byte_buf = ctrl->xfer.rx_buf;
>
> Does this need to move forward by words_to_read bytes so that the left
> over bytes are tacked onto the end? Or this should be an else if
> statement?

When the words block completes it updates the rx_buf location so it is already
at the correct offset for bytes.

> > +
> > +       master->max_speed_hz = 300000000;
> > +       master->num_chipselect = QSPI_NUM_CS;
> > +       master->bus_num = pdev->id;
>
> Can this come from DT aliases? I've never thought about qspi and
> "regular" spi being in the same spi bus numbering system, but I suppose
> that will happen now and we need to make sure that qspi numbers start
> after the regular ones?

I'm not sure. Can look into it.

>
> > +       master->dev.of_node = pdev->dev.of_node;
> > +       master->mode_bits = SPI_MODE_0 |
> > +                           SPI_TX_DUAL | SPI_RX_DUAL |
> > +                           SPI_TX_QUAD | SPI_RX_QUAD;
> > +       master->flags = SPI_MASTER_HALF_DUPLEX;
> > +       master->prepare_message = qcom_qspi_prepare_message;
> > +       master->transfer_one = qcom_qspi_transfer_one;

Re: [PATCH v3 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

[Doug Anderson]

Hi,

On Fri, Sep 28, 2018 at 11:20 AM Ryan Case <ryandcase@chromium.org> wrote:
> > > +       master->max_speed_hz = 300000000;
> > > +       master->num_chipselect = QSPI_NUM_CS;
> > > +       master->bus_num = pdev->id;
> >
> > Can this come from DT aliases? I've never thought about qspi and
> > "regular" spi being in the same spi bus numbering system, but I suppose
> > that will happen now and we need to make sure that qspi numbers start
> > after the regular ones?
>
> I'm not sure. Can look into it.

In a previous response for the other Qualcomm SPI driver Mark said to
set this to -1.  Specifically the code was checking the alias and Mark
said:

> Don't do this, just set bus_num to -1 and let the core assign an ID.

[1] https://lkml.kernel.org/r/20180503233849.GF13402@sirena.org.uk

-Doug

Re: [PATCH v3 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

[Stephen Boyd]

Quoting Ryan Case (2018-09-28 11:19:51)
> On Wed, Sep 26, 2018 at 11:43 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > Quoting Ryan Case (2018-09-26 13:52:04)
> > > From: Girish Mahadevan <girishm@codeaurora.org>
> > > +#include <asm/unaligned.h>
> > > +
> > > +#define AHB_MIN_HZ             9600000UL
> >
> > Is this used?
> 
> Nope. Do you want all currently unused defines removed or specifically this
> one? I saw precedent in other drivers for defining registers/flags/values of
> supported but unused functionality so I left these (big endian, DDR, ...).

I guess it's fine but I don't know if it will ever be used so remove it?
I'd leave the others if they help someone know what register bits exist.
That's usually how I handle it.

> 
> 
> > > +       speed_hz = slv->max_speed_hz;
> > > +       if (xfer->speed_hz)
> > > +               speed_hz = xfer->speed_hz;
> > > +
> > > +       ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4);
> >
> > Why 4? Is that related to the number of wires?
> 
> In normal operation the core clock should be running at 4x the rate of the
> transfer clock regardless of number of wires used.

Ok. Maybe add a comment so we understand that.

> 
> > > +                       put_unaligned(rd_fifo, word_buf++);
> > > +               }
> > > +               ctrl->xfer.rx_buf = word_buf;
> > > +       }
> > > +
> > > +       if (bytes_to_read) {
> > > +               byte_buf = ctrl->xfer.rx_buf;
> >
> > Does this need to move forward by words_to_read bytes so that the left
> > over bytes are tacked onto the end? Or this should be an else if
> > statement?
> 
> When the words block completes it updates the rx_buf location so it is already
> at the correct offset for bytes.
> 

Ok I see. Subtle!