2018-09-20 22:43:19

by Ryan Case

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

From: Girish Mahadevan <[email protected]>

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

Signed-off-by: Girish Mahadevan <[email protected]>
Signed-off-by: Ryan Case <[email protected]>
---

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



2018-09-20 22:43:20

by Ryan Case

[permalink] [raw]
Subject: [PATCH v2 2/2] spi: Introduce new driver for Qualcomm QuadSPI controller

From: Girish Mahadevan <[email protected]>

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 <[email protected]>
Signed-off-by: Ryan Case <[email protected]>
---

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


2018-09-20 22:48:13

by Randy Dunlap

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

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

2018-09-20 23:48:34

by Ryan Case

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

On Thu, Sep 20, 2018 at 3:46 PM Randy Dunlap <[email protected]> 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.

2018-09-21 16:34:21

by Mark Brown

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

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.


Attachments:
(No filename) (564.00 B)
signature.asc (499.00 B)
Download all attachments

2018-09-21 17:33:04

by Stephen Boyd

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

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

2018-09-21 17:39:33

by Mark Brown

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

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.


Attachments:
(No filename) (516.00 B)
signature.asc (499.00 B)
Download all attachments

2018-09-21 17:41:50

by Doug Anderson

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

Hi,

On Fri, Sep 21, 2018 at 10:31 AM Stephen Boyd <[email protected]> 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.
> > +

2018-09-21 18:34:03

by Trent Piepho

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

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.

2018-09-21 18:40:48

by Stephen Boyd

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

Quoting Doug Anderson (2018-09-21 10:40:14)
> Hi,
>
> On Fri, Sep 21, 2018 at 10:31 AM Stephen Boyd <[email protected]> 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.


2018-09-21 18:50:12

by Doug Anderson

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

Stephen
On Fri, Sep 21, 2018 at 11:40 AM Stephen Boyd <[email protected]> wrote:
>
> Quoting Doug Anderson (2018-09-21 10:40:14)
> > Hi,
> >
> > On Fri, Sep 21, 2018 at 10:31 AM Stephen Boyd <[email protected]> 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

2018-09-21 18:51:53

by Mark Brown

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

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.


Attachments:
(No filename) (878.00 B)
signature.asc (499.00 B)
Download all attachments

2018-09-23 03:45:22

by Stephen Boyd

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

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


2018-09-24 17:14:19

by Doug Anderson

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

Hi,

On Sat, Sep 22, 2018 at 8:45 PM Stephen Boyd <[email protected]> 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

2018-09-24 19:32:48

by Trent Piepho

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

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.

2018-09-25 16:03:08

by Doug Anderson

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

Hi,
On Mon, Sep 24, 2018 at 11:23 AM Trent Piepho <[email protected]> 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