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

[PATCH v2 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 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..ecfb1e2bd520
--- /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:	Should contain:
+		"qcom,sdm845-qspi"
+- 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";
+		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.444.g18242da7ef-goog

[PATCH v2 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 and SPI
Mode 3. 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 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 | 608 ++++++++++++++++++++++++++++++++++++
 3 files changed, 615 insertions(+)
 create mode 100644 drivers/spi/spi-qcom-qspi.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index de03d67bcd2b..36922e12c3b0 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 QPSPI 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..1bf2720a7b6f
--- /dev/null
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -0,0 +1,608 @@
+// 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_setup(struct spi_device *spi)
+{
+	u32 mstr_cfg;
+	struct qcom_qspi *ctrl;
+	int tx_data_oe_delay = 1;
+	int tx_data_delay = 1;
+	int ret;
+
+	ctrl = spi_master_get_devdata(spi->master);
+	ret = pm_runtime_get_sync(ctrl->dev);
+	if (ret) {
+		dev_err(ctrl->dev, "Runtime PM get failed in setup: %d\n", ret);
+		pm_runtime_put(ctrl->dev);
+		return ret;
+	}
+
+	mstr_cfg = readl(ctrl->base + MSTR_CONFIG);
+	mstr_cfg &= ~CHIP_SELECT_NUM;
+	if (spi->chip_select)
+		mstr_cfg |= CHIP_SELECT_NUM;
+
+	mstr_cfg = (mstr_cfg & ~SPI_MODE_MSK) | (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);
+
+	/*
+	 * Ensure that the configuration goes through by reading back
+	 * a register from the IO space.
+	 */
+	mstr_cfg = readl(ctrl->base + MSTR_CONFIG);
+
+	pm_runtime_put(ctrl->dev);
+
+	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->setup = qcom_qspi_setup;
+	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 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 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,sdm845-qspi", },
+	{ }
+};
+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.444.g18242da7ef-goog

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

[Randy Dunlap]

Hi,

Just curious:

On 9/20/18 3:40 PM, Ryan Case wrote:
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index de03d67bcd2b..36922e12c3b0 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 QPSPI controller"

	Is that       QPSPI correct?
	This is the only place that QPSPI or PSPI is used in this patch.


> +	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)


-- 
~Randy

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

[Ryan Case]

On Thu, Sep 20, 2018 at 3:46 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> On 9/20/18 3:40 PM, Ryan Case wrote:
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index de03d67bcd2b..36922e12c3b0 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 QPSPI controller"
>
>         Is that       QPSPI correct?
>         This is the only place that QPSPI or PSPI is used in this patch.
>

Thanks Randy, that is a typo. I'll fix this in the next revision in a
day or two pending further feedback.

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

[Mark Brown]

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

On Thu, Sep 20, 2018 at 03:40:55PM -0700, Ryan Case wrote:

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

> +	/*
> +	 * Ensure that the configuration goes through by reading back
> +	 * a register from the IO space.
> +	 */
> +	mstr_cfg = readl(ctrl->base + MSTR_CONFIG);

Your setup() function shouldn't be affecting the status of the hardware
for any other SPI devices using the controller, otherwise it might
disturb an active transfer.  prepare_message() is typically the best
place to do this stuff.

Otherwise this looks good.

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

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

[Stephen Boyd]

Quoting Ryan Case (2018-09-20 15:40:54)
> 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..ecfb1e2bd520
> --- /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:  Should contain:
> +               "qcom,sdm845-qspi"

Does someone have a more generic compatible string that can be added
here to indicate the type of quad SPI controller this is? I really doubt
this is a one-off hardware block for the specific SDM845 SoC. 

> +- 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.
> +

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

[Stephen Boyd]

Quoting Ryan Case (2018-09-20 15:40:54)
> 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..ecfb1e2bd520
> --- /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:  Should contain:
> +               "qcom,sdm845-qspi"

Does someone have a more generic compatible string that can be added
here to indicate the type of quad SPI controller this is? I really doubt
this is a one-off hardware block for the specific SDM845 SoC. 

> +- 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.
> +

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

[Mark Brown]

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

On Fri, Sep 21, 2018 at 10:30:57AM -0700, Stephen Boyd wrote:
> Quoting Ryan Case (2018-09-20 15:40:54)

> > +Required properties:
> > +- compatible:  Should contain:
> > +               "qcom,sdm845-qspi"

> Does someone have a more generic compatible string that can be added
> here to indicate the type of quad SPI controller this is? I really doubt
> this is a one-off hardware block for the specific SDM845 SoC. 

The idiom for DT is supposed to be to use only device specific names
unfortunately.

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

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

[Doug Anderson]

Hi,

On Fri, Sep 21, 2018 at 10:31 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Ryan Case (2018-09-20 15:40:54)
> > 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..ecfb1e2bd520
> > --- /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:  Should contain:
> > +               "qcom,sdm845-qspi"
>
> Does someone have a more generic compatible string that can be added
> here to indicate the type of quad SPI controller this is? I really doubt
> this is a one-off hardware block for the specific SDM845 SoC.

The compatible string used to be "qcom,qspi-v1".  ...but Rob Herring
requested [1] "an SoC specific compatible string".  While we could do
a compatible string like:

  "qcom,sdm845-qspi", "qcom,qspi-v1".

I'm curious if that buys us anything.  From all my previous experience
with device tree it is fine to name a compatible string for a
component based on the first SoC that used it.  If we later find that
this is also used in an "msm1234" we could always later do the
compatible string for that device as:

  "qcom, msm1234-qspi", "qcom,sdm845-qspi"

...and we don't need to try to come up with a generic name.
Obviously, though, I'll cede to whatever Rob says here though.

-Doug


[1] http://lkml.kernel.org/r/20180716222721.GA12854@rob-hp-laptop


>
> > +- 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.
> > +

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

[Trent Piepho]

On Fri, 2018-09-21 at 10:39 -0700, Mark Brown wrote:
> On Fri, Sep 21, 2018 at 10:30:57AM -0700, Stephen Boyd wrote:
> > Quoting Ryan Case (2018-09-20 15:40:54)
> > > +Required properties:
> > > +- compatible:  Should contain:
> > > +               "qcom,sdm845-qspi"
> > Does someone have a more generic compatible string that can be added
> > here to indicate the type of quad SPI controller this is? I really doubt
> > this is a one-off hardware block for the specific SDM845 SoC. 
> 
> The idiom for DT is supposed to be to use only device specific names
> unfortunately.

Basically the "first" device the driver can control has it's specific
name used as the generic string.  This is used in place of some
internal codename for the core.

Then a newer device will have "foo,XYZ200", "foo,XYZ100" as compatible,
where the 100 was the first device and the 200 is new one.  Maybe the
driver cares, or will care, about what device this or maybe it can
drive the device fine without needing to know more than the generic.

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

[Stephen Boyd]

Quoting Doug Anderson (2018-09-21 10:40:14)
> Hi,
> 
> On Fri, Sep 21, 2018 at 10:31 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Ryan Case (2018-09-20 15:40:54)
> > > 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..ecfb1e2bd520
> > > --- /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:  Should contain:
> > > +               "qcom,sdm845-qspi"
> >
> > Does someone have a more generic compatible string that can be added
> > here to indicate the type of quad SPI controller this is? I really doubt
> > this is a one-off hardware block for the specific SDM845 SoC.
> 
> The compatible string used to be "qcom,qspi-v1".  ...but Rob Herring
> requested [1] "an SoC specific compatible string".  While we could do
> a compatible string like:
> 
>   "qcom,sdm845-qspi", "qcom,qspi-v1".
> 
> I'm curious if that buys us anything.  From all my previous experience
> with device tree it is fine to name a compatible string for a
> component based on the first SoC that used it.  If we later find that
> this is also used in an "msm1234" we could always later do the
> compatible string for that device as:
> 
>   "qcom, msm1234-qspi", "qcom,sdm845-qspi"
> 
> ...and we don't need to try to come up with a generic name.
> Obviously, though, I'll cede to whatever Rob says here though.
> 

It seems that everybody has misunderstood my email. Let me try to
clarify.

I'm not saying to replace the sdm845 qspi compatible with a generic one.
I'm recommending that a generic one is added in addition to the SoC
specific one. That way, we get to put the generic compatible string in
the driver and not need to update the driver compatible string array
each time a new SoC comes out with a new compatible string.

If it turns out later that we need to handle some bug in that specific
SoC compatible string then we're good to go and we can handle it by
matching the more specific SoC version compatible.

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

[Doug Anderson]

Stephen
On Fri, Sep 21, 2018 at 11:40 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2018-09-21 10:40:14)
> > Hi,
> >
> > On Fri, Sep 21, 2018 at 10:31 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Ryan Case (2018-09-20 15:40:54)
> > > > 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..ecfb1e2bd520
> > > > --- /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:  Should contain:
> > > > +               "qcom,sdm845-qspi"
> > >
> > > Does someone have a more generic compatible string that can be added
> > > here to indicate the type of quad SPI controller this is? I really doubt
> > > this is a one-off hardware block for the specific SDM845 SoC.
> >
> > The compatible string used to be "qcom,qspi-v1".  ...but Rob Herring
> > requested [1] "an SoC specific compatible string".  While we could do
> > a compatible string like:
> >
> >   "qcom,sdm845-qspi", "qcom,qspi-v1".
> >
> > I'm curious if that buys us anything.  From all my previous experience
> > with device tree it is fine to name a compatible string for a
> > component based on the first SoC that used it.  If we later find that
> > this is also used in an "msm1234" we could always later do the
> > compatible string for that device as:
> >
> >   "qcom, msm1234-qspi", "qcom,sdm845-qspi"
> >
> > ...and we don't need to try to come up with a generic name.
> > Obviously, though, I'll cede to whatever Rob says here though.
> >
>
> It seems that everybody has misunderstood my email. Let me try to
> clarify.
>
> I'm not saying to replace the sdm845 qspi compatible with a generic one.
> I'm recommending that a generic one is added in addition to the SoC
> specific one. That way, we get to put the generic compatible string in
> the driver and not need to update the driver compatible string array
> each time a new SoC comes out with a new compatible string.
>
> If it turns out later that we need to handle some bug in that specific
> SoC compatible string then we're good to go and we can handle it by
> matching the more specific SoC version compatible.

I don't think I misunderstood.  I was suggesting that I believe that
the device tree way is to use the first SoC as the generic one.  In
other words to support sdm845 and msm1234, we do:

A)
on sdm845: "qcom,sdm845-qspi"
on msm1234 (later): "qcom, msm1234-qspi", "qcom,sdm845-qspi"

What you are suggesting (I think) is:

B)
on sdm845: "qcom,sdm845-qspi", "qcom,qspi-v1"
on msm1234 (later): "qcom, msm1234-qspi", "qcom,qspi-v1"

If Rob likes B) better then it's fine with me, it was just my
understanding that A) was the suggested way to do things (even if it
is decidedly non-symmetric).


NOTE: Even with A) there's no need to update the driver for msm1234
since it has the fallback to sdm845.

-Doug

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

[Mark Brown]

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

On Fri, Sep 21, 2018 at 11:40:24AM -0700, Stephen Boyd wrote:

> It seems that everybody has misunderstood my email. Let me try to
> clarify.

> I'm not saying to replace the sdm845 qspi compatible with a generic one.
> I'm recommending that a generic one is added in addition to the SoC
> specific one. That way, we get to put the generic compatible string in
> the driver and not need to update the driver compatible string array
> each time a new SoC comes out with a new compatible string.

> If it turns out later that we need to handle some bug in that specific
> SoC compatible string then we're good to go and we can handle it by
> matching the more specific SoC version compatible.

Right, the policy is generally not to have these strings at all.  IIRC
the argument is that they tend to either become unclear as the marketing
and technology changes.

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

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

[Stephen Boyd]

Quoting Mark Brown (2018-09-21 11:51:06)
> On Fri, Sep 21, 2018 at 11:40:24AM -0700, Stephen Boyd wrote:
> 
> > It seems that everybody has misunderstood my email. Let me try to
> > clarify.
> 
> > I'm not saying to replace the sdm845 qspi compatible with a generic one.
> > I'm recommending that a generic one is added in addition to the SoC
> > specific one. That way, we get to put the generic compatible string in
> > the driver and not need to update the driver compatible string array
> > each time a new SoC comes out with a new compatible string.
> 
> > If it turns out later that we need to handle some bug in that specific
> > SoC compatible string then we're good to go and we can handle it by
> > matching the more specific SoC version compatible.
> 
> Right, the policy is generally not to have these strings at all.  IIRC
> the argument is that they tend to either become unclear as the marketing
> and technology changes.

Where is this policy documented? Is it on the list somewhere or written
in Documentation/devicetree/? From my read of Rob's comment in the
previous version of this patch, all that was asked was to add another
compatible string for the specific SoC.

I find the approach of picking the first SoC that the driver works on to
be obtuse. I don't want to be reading some SoC DTS and see another SoC
marketing number in the compatible string because it makes it confusing
to explain to someone that yes these different SoCs are related to each
other, but no, that SoC isn't this SoC. Sure it all works and everything
is technically fine, but my aesthetically pleasing alarms go off and I
don't see any particular downside to having two compatibles.

The upside is that things aren't confusing and drivers don't get
continual SoC churn updates because the compatible describes the SoC
(qcom,sdm845-qspi) and the programming model (qcom,qspi-v1).

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

[Doug Anderson]

Hi,

On Sat, Sep 22, 2018 at 8:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Mark Brown (2018-09-21 11:51:06)
> > On Fri, Sep 21, 2018 at 11:40:24AM -0700, Stephen Boyd wrote:
> >
> > > It seems that everybody has misunderstood my email. Let me try to
> > > clarify.
> >
> > > I'm not saying to replace the sdm845 qspi compatible with a generic one.
> > > I'm recommending that a generic one is added in addition to the SoC
> > > specific one. That way, we get to put the generic compatible string in
> > > the driver and not need to update the driver compatible string array
> > > each time a new SoC comes out with a new compatible string.
> >
> > > If it turns out later that we need to handle some bug in that specific
> > > SoC compatible string then we're good to go and we can handle it by
> > > matching the more specific SoC version compatible.
> >
> > Right, the policy is generally not to have these strings at all.  IIRC
> > the argument is that they tend to either become unclear as the marketing
> > and technology changes.
>
> Where is this policy documented? Is it on the list somewhere or written
> in Documentation/devicetree/?

I don't of it being documented anywhere, but it's what I've been told
before.  I spent a bit of time to find a specific example but I
couldn't.  As with a lot of device tree stuff it historically has been
a bunch of word-of-mouth type stuff.  It does look like people are
moving towards a more formal spec at
<https://github.com/devicetree-org/devicetree-specification/> but it
doesn't include this guideline.


> From my read of Rob's comment in the
> previous version of this patch, all that was asked was to add another
> compatible string for the specific SoC.
>
> I find the approach of picking the first SoC that the driver works on to
> be obtuse. I don't want to be reading some SoC DTS and see another SoC
> marketing number in the compatible string because it makes it confusing
> to explain to someone that yes these different SoCs are related to each
> other, but no, that SoC isn't this SoC. Sure it all works and everything
> is technically fine, but my aesthetically pleasing alarms go off and I
> don't see any particular downside to having two compatibles.
>
> The upside is that things aren't confusing and drivers don't get
> continual SoC churn updates because the compatible describes the SoC
> (qcom,sdm845-qspi) and the programming model (qcom,qspi-v1).

IIUC previous suggestions about just naming it based on the first SoC
was due to the difficulty of coming up with a good generic name to
give something.  For instance you definitely wouldn't want to name it
"qcom-qspi-sdm8xx" because you have no idea what future SoCs will be
numbered.

In the case here calling it "qcom,qspi-v1" is better than that and if
Rob gives the thumbs up then I won't object to it.  In general though
looking at other device tree bindings this doesn't seem like a thing
commonly done.  Perhaps if we decide it's useful here we should start
suggesting it everywhere...

-Doug



-Doug

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

[Trent Piepho]

On Mon, 2018-09-24 at 10:13 -0700, Doug Anderson wrote:
> IIUC previous suggestions about just naming it based on the first SoC
> was due to the difficulty of coming up with a good generic name to
> give something.  For instance you definitely wouldn't want to name it
> "qcom-qspi-sdm8xx" because you have no idea what future SoCs will be
> numbered.

And the hypothetical sdm899 might use a non-compatible device that uses
a different driver, and that really makes "qcom-qspi-sdm8xx" look dumb.

> 
> In the case here calling it "qcom,qspi-v1" is better than that and if
> Rob gives the thumbs up then I won't object to it.  In general though
> looking at other device tree bindings this doesn't seem like a thing
> commonly done.  Perhaps if we decide it's useful here we should start
> suggesting it everywhere...

It would help if the programming model or IP core name or whatever this
is using was mentioned in the public reference manual for the SoC. 
Then is a lot more clear that a number of different SoCs all have the
same quad spi controller inside them.

Usually it's not like that.  The RMs just say, "it's got a SPI master
with these registers."  What SoCs use the same IP module, which
different?  When did they make a new version?  The silicon vendors
don't tell you this.  So any name we make up for the IP module likely
doesn't match reality.

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

[Doug Anderson]

Hi,
On Mon, Sep 24, 2018 at 11:23 AM Trent Piepho <tpiepho@impinj.com> wrote:
>
> On Mon, 2018-09-24 at 10:13 -0700, Doug Anderson wrote:
> > IIUC previous suggestions about just naming it based on the first SoC
> > was due to the difficulty of coming up with a good generic name to
> > give something.  For instance you definitely wouldn't want to name it
> > "qcom-qspi-sdm8xx" because you have no idea what future SoCs will be
> > numbered.
>
> And the hypothetical sdm899 might use a non-compatible device that uses
> a different driver, and that really makes "qcom-qspi-sdm8xx" look dumb.
>
> >
> > In the case here calling it "qcom,qspi-v1" is better than that and if
> > Rob gives the thumbs up then I won't object to it.  In general though
> > looking at other device tree bindings this doesn't seem like a thing
> > commonly done.  Perhaps if we decide it's useful here we should start
> > suggesting it everywhere...
>
> It would help if the programming model or IP core name or whatever this
> is using was mentioned in the public reference manual for the SoC.
> Then is a lot more clear that a number of different SoCs all have the
> same quad spi controller inside them.
>
> Usually it's not like that.  The RMs just say, "it's got a SPI master
> with these registers."  What SoCs use the same IP module, which
> different?  When did they make a new version?  The silicon vendors
> don't tell you this.  So any name we make up for the IP module likely
> doesn't match reality.

Note that Rob did recently give a positive review to a similar binding.  See:

https://lore.kernel.org/patchwork/patch/979432/

Specifically the text from that binding was:

+                  Qcom SoCs must contain, as below, SoC-specific compatibles
+                  along with "qcom,smmu-v2":
+                  "qcom,msm8996-smmu-v2", "qcom,smmu-v2",
+                  "qcom,sdm845-smmu-v2", "qcom,smmu-v2".

Given Rob's positive review there, it seems like it would be fine to do:

  "qcom,sdm845-qspi", "qcom,qspi-v1".

NOTE: In our case we don't need the "-v1" in SoC-specific case since
there's only one Quad SPI driver there.  As I understand it the reason
we needed the -v2 in the SoC-specific case for the SMMU was that there
are two totally different SMMUs in SDM845.  You can see history in the
v15 patch <https://lore.kernel.org/patchwork/patch/977888/>


-Doug