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

[Stephen Boyd]

Quoting Ryan Case (2018-10-02 14:47:08)
> From: Girish Mahadevan 
> 
> New driver for Qualcomm QuadSPI(QSPI) controller that is used to
> communicate with slaves such as 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 
> Signed-off-by: Ryan Case 
> ---

Reviewed-by: Stephen Boyd 

One nitpick below. Looks better now, thanks!

> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
> new file mode 100644
> index ..b8163b40bb92
> --- /dev/null
> +++ b/drivers/spi/spi-qcom-qspi.c
> @@ -0,0 +1,581 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
[...]
> +
> +struct qspi_xfer {
> +   union {
> +   const void *tx_buf;
> +   void *rx_buf;
> +   };
> +   unsigned int rem_bytes;
> +   unsigned 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;
> +   /* Lock to protect data accessed by IRQs */

Nitpick: What data? Registers? The SPI core can't do it?

> +   spinlock_t lock;
> +};

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

[Doug Anderson]

Hi,

On Tue, Oct 2, 2018 at 2:48 PM Ryan Case  wrote:
> +#define RD_FIFO_STATUS 0x002c
> +#define FIFO_EMPTY BIT(11)
> +#define WR_CNTS_MSK0x7f0
> +#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)
> +
> +#define RD_FIFO_CFG0x0028
> +#define CONTINUOUS_MODEBIT(0)

You missed the above when re-sorting.  The section for 0x0028 should
be above the section for 0x002c.

IMO there's been enough spins of this patch and it's a pretty minor
change.  Mark: maybe you'd be OK with applying the current patch and
either fixing up the sort order as you apply or letting us know to
post a followup patch?  ...or if you'd like a v7 then please yell.


> +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;
> +}

As per my new understanding (now that I've been educated by Rafael)
[1] possibly the error handling here could be revamped to handle the
case where suspend() may be called again after a failed resume().  I'd
rather not block this patch based on that discussion though, so this
feels like something to address by a follow-up patch.  Thus:

Reviewed-by: Douglas Anderson 


[1] 
https://lkml.kernel.org/r/cajz5v0hsohuzvjcvcqzy_ddsypvppoabk0helsa3dpvf12l...@mail.gmail.com

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

[Ryan Case]

From: Girish Mahadevan 

New driver for Qualcomm QuadSPI(QSPI) controller that is used to
communicate with slaves such as 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 
Signed-off-by: Ryan Case 
---

Changes in v6:
- Fixed IRQ error logging accessibility
- Minor cleanup suggested by Doug

Changes in v5:
- Fixed tabbing/spacing errors in defines

Changes in v4:
- Set bus_num to -1
- Alphabetized Makefile entry
- Grouped register and associated flag defines
- Read and write transfers now use ioread32_rep and iowrite32_rep
- Added IRQ error logging
- Updated types, formatting, and other cleanup feedback from Stephen

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 | 581 
 3 files changed, 588 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..a71c18a146a1 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_SPI_PPC4xx)  += spi-ppc4xx.o
 spi-pxa2xx-platform-objs   := spi-pxa2xx.o spi-pxa2xx-dma.o
 obj-$(CONFIG_SPI_PXA2XX)   += spi-pxa2xx-platform.o
 obj-$(CONFIG_SPI_PXA2XX_PCI)   += spi-pxa2xx-pci.o
+obj-$(CONFIG_SPI_QCOM_QSPI)+= spi-qcom-qspi.o
 obj-$(CONFIG_SPI_QUP)  += spi-qup.o
 obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
 obj-$(CONFIG_SPI_RB4XX)+= spi-rb4xx.o
diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
new file mode 100644
index ..b8163b40bb92
--- /dev/null
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -0,0 +1,581 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#define QSPI_NUM_CS2
+#define QSPI_BYTES_PER_WORD4
+
+#define MSTR_CONFIG0x
+#define FULL_CYCLE_MODEBIT(3)
+#define FB_CLK_EN  BIT(4)
+#define PIN_HOLDN  BIT(6)
+#define PIN_WPNBIT(7)
+#define DMA_ENABLE BIT(8)
+#define BIG_ENDIAN_MODEBIT(9)
+#define SPI_MODE_MSK   0xc00
+#define SPI_MODE_SHFT  10
+#define CHIP_SELECT_NUMBIT(12)
+#define SBL_EN BIT(13)
+#define LPA_BASE_MSK   0x3c000
+#define LPA_BASE_SHFT  14
+#define TX_DATA_DELAY_MSK  0xc
+#define TX_DATA_DELAY_SHFT 18
+#define TX_CLK_DELAY_MSK   0x30
+#define TX_CLK_DELAY_SHFT  20
+#define TX_CS_N_DELAY_MSK  0xc0
+#define TX_CS_N_DELAY_SHFT 22
+#define TX_DATA_OE_DELAY_MSK   0x300
+#define TX_DATA_OE_DELAY_SHFT  24
+
+#define AHB_MASTER_CFG 0x0004
+#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_SHFT6
+#define HMEMTYPE_READ_TRANS_MSK0x700
+#define HMEMTYPE_READ_TRANS_SHFT   8
+#define HSHAREDBIT(11)
+#define HINNERSHARED   BIT(12)
+
+#define MSTR_INT_EN0x000C
+#define MSTR_INT_STATUS0x0010
+#define RESP_FIFO_UNDERRUN BIT(0)
+#define RESP_FIFO_NOT_EMPTYBIT(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_OVERRUNBIT(11)
+#define TRANSACTION_DONE   BIT(16)
+#define QSPI_ERR_IRQS  (RESP_FIFO_UNDERRUN | HRESP_FROM_NOC_ERR | \
+