linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] Add support for Microchip QSPI controller
@ 2022-08-05  5:30 Naga Sureshkumar Relli
  2022-08-05  5:30 ` [PATCH v3 1/4] spi: dt-binding: document microchip coreQSPI Naga Sureshkumar Relli
                   ` (4 more replies)
  0 siblings, 5 replies; 20+ messages in thread
From: Naga Sureshkumar Relli @ 2022-08-05  5:30 UTC (permalink / raw)
  To: broonie, robh+dt, krzysztof.kozlowski+dt, conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis,
	Naga Sureshkumar Relli

This patch enables the Microchip's FPGA QSPI and Polarfire SoC QSPI
controller support.

Tested spi-nand (W25N01GV) and spi-nor (MT25QL256A) on Microchip's
ICICLE kit. tested using both FPGA QSPI and Polarfie SoC QSPI.

changes in v3
------------
1. Added dev_err_probe() at places like probe failures
2. Split the dt-bindings one for adding coreqspi compatible
   and other one to add coreqspi as fallback to mpfs-qspi.

changes in v2
------------
1. Replaced spi_alloc_master() with devm_spi_alloc_master()
2. Used dev_err_probe() when devm_spi_alloc_master() fails.
3. Added shared IRQ flag in the interrupt registration.
4. Updated the dt_bindings so that there is a differentiation
   between FPGA QSPI IP core and hard QSPI IP core.

Naga Sureshkumar Relli (4):
  spi: dt-binding: document microchip coreQSPI
  spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
  spi: microchip-core-qspi: Add support for microchip fpga qspi
    controllers
  MAINTAINERS: add qspi to Polarfire SoC entry

 .../bindings/spi/microchip,mpfs-spi.yaml      |  15 +-
 MAINTAINERS                                   |   1 +
 drivers/spi/Kconfig                           |   9 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-microchip-core-qspi.c         | 601 ++++++++++++++++++
 5 files changed, 623 insertions(+), 4 deletions(-)
 create mode 100644 drivers/spi/spi-microchip-core-qspi.c

-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 1/4] spi: dt-binding: document microchip coreQSPI
  2022-08-05  5:30 [PATCH v3 0/4] Add support for Microchip QSPI controller Naga Sureshkumar Relli
@ 2022-08-05  5:30 ` Naga Sureshkumar Relli
  2022-08-05  6:46   ` Krzysztof Kozlowski
  2022-08-05  5:30 ` [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi Naga Sureshkumar Relli
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 20+ messages in thread
From: Naga Sureshkumar Relli @ 2022-08-05  5:30 UTC (permalink / raw)
  To: broonie, robh+dt, krzysztof.kozlowski+dt, conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis,
	Naga Sureshkumar Relli

Add microchip coreQSPI compatible string and update the title/description
to reflect this addition.

Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
---
 .../devicetree/bindings/spi/microchip,mpfs-spi.yaml        | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
index 7326c0a28d16..a47d4923b51b 100644
--- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
+++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
@@ -4,7 +4,11 @@
 $id: http://devicetree.org/schemas/spi/microchip,mpfs-spi.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Microchip MPFS {Q,}SPI Controller Device Tree Bindings
+title: Microchip FPGA {Q,}SPI Controllers
+
+description:
+  SPI and QSPI controllers on Microchip PolarFire SoC and the "soft"/
+  fabric IP cores they are based on
 
 maintainers:
   - Conor Dooley <conor.dooley@microchip.com>
@@ -17,6 +21,7 @@ properties:
     enum:
       - microchip,mpfs-spi
       - microchip,mpfs-qspi
+      - microchip,coreqspi-rtl-v2 # FPGA QSPI
 
   reg:
     maxItems: 1
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
  2022-08-05  5:30 [PATCH v3 0/4] Add support for Microchip QSPI controller Naga Sureshkumar Relli
  2022-08-05  5:30 ` [PATCH v3 1/4] spi: dt-binding: document microchip coreQSPI Naga Sureshkumar Relli
@ 2022-08-05  5:30 ` Naga Sureshkumar Relli
  2022-08-05  6:47   ` Krzysztof Kozlowski
                     ` (2 more replies)
  2022-08-05  5:30 ` [PATCH v3 3/4] spi: microchip-core-qspi: Add support for microchip fpga qspi controllers Naga Sureshkumar Relli
                   ` (2 subsequent siblings)
  4 siblings, 3 replies; 20+ messages in thread
From: Naga Sureshkumar Relli @ 2022-08-05  5:30 UTC (permalink / raw)
  To: broonie, robh+dt, krzysztof.kozlowski+dt, conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis,
	Naga Sureshkumar Relli

Microchip's PolarFire SoC QSPI IP core is based on coreQSPI,
so add coreqspi as a fallback to mpfs-qspi.

Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
---
 .../devicetree/bindings/spi/microchip,mpfs-spi.yaml    | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
index a47d4923b51b..84d32c1a4d60 100644
--- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
+++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
@@ -18,10 +18,12 @@ allOf:
 
 properties:
   compatible:
-    enum:
-      - microchip,mpfs-spi
-      - microchip,mpfs-qspi
-      - microchip,coreqspi-rtl-v2 # FPGA QSPI
+   oneOf:
+    - items:
+        - const: microchip,mpfs-qspi
+        - const: microchip,coreqspi-rtl-v2
+    - const: microchip,coreqspi-rtl-v2 #FPGA QSPI
+    - const: microchip,mpfs-spi
 
   reg:
     maxItems: 1
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 3/4] spi: microchip-core-qspi: Add support for microchip fpga qspi controllers
  2022-08-05  5:30 [PATCH v3 0/4] Add support for Microchip QSPI controller Naga Sureshkumar Relli
  2022-08-05  5:30 ` [PATCH v3 1/4] spi: dt-binding: document microchip coreQSPI Naga Sureshkumar Relli
  2022-08-05  5:30 ` [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi Naga Sureshkumar Relli
@ 2022-08-05  5:30 ` Naga Sureshkumar Relli
  2022-08-05  6:50   ` Krzysztof Kozlowski
  2022-08-05  5:30 ` [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry Naga Sureshkumar Relli
  2022-08-05  9:14 ` [PATCH v3 0/4] Add support for Microchip QSPI controller Conor.Dooley
  4 siblings, 1 reply; 20+ messages in thread
From: Naga Sureshkumar Relli @ 2022-08-05  5:30 UTC (permalink / raw)
  To: broonie, robh+dt, krzysztof.kozlowski+dt, conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis,
	Naga Sureshkumar Relli

Add a driver for Microchip FPGA QSPI controllers. This driver also
supports "hard" QSPI controllers on Polarfire SoC.

Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
---
 drivers/spi/Kconfig                   |   9 +
 drivers/spi/Makefile                  |   1 +
 drivers/spi/spi-microchip-core-qspi.c | 601 ++++++++++++++++++++++++++
 3 files changed, 611 insertions(+)
 create mode 100644 drivers/spi/spi-microchip-core-qspi.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 9987c3f2bd1c..78e447327cc4 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -591,6 +591,15 @@ config SPI_MICROCHIP_CORE
 	  PolarFire SoC.
 	  If built as a module, it will be called spi-microchip-core.
 
+config SPI_MICROCHIP_CORE_QSPI
+	tristate "Microchip FPGA QSPI controllers"
+	depends on SPI_MASTER
+	help
+	  This enables the QSPI driver for Microchip FPGA QSPI controllers.
+	  Say Y or M here if you want to use the QSPI controllers on
+	  PolarFire SoC.
+	  If built as a module, it will be called spi-microchip-core-qspi.
+
 config SPI_MT65XX
 	tristate "MediaTek SPI controller"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 15d2f3835e45..4b34e855c841 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -73,6 +73,7 @@ obj-$(CONFIG_SPI_LP8841_RTC)		+= spi-lp8841-rtc.o
 obj-$(CONFIG_SPI_MESON_SPICC)		+= spi-meson-spicc.o
 obj-$(CONFIG_SPI_MESON_SPIFC)		+= spi-meson-spifc.o
 obj-$(CONFIG_SPI_MICROCHIP_CORE)	+= spi-microchip-core.o
+obj-$(CONFIG_SPI_MICROCHIP_CORE_QSPI)	+= spi-microchip-core-qspi.o
 obj-$(CONFIG_SPI_MPC512x_PSC)		+= spi-mpc512x-psc.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= spi-mpc52xx-psc.o
 obj-$(CONFIG_SPI_MPC52xx)		+= spi-mpc52xx.o
diff --git a/drivers/spi/spi-microchip-core-qspi.c b/drivers/spi/spi-microchip-core-qspi.c
new file mode 100644
index 000000000000..32188342440d
--- /dev/null
+++ b/drivers/spi/spi-microchip-core-qspi.c
@@ -0,0 +1,601 @@
+// SPDX-License-Identifier: (GPL-2.0)
+/*
+ * Microchip coreQSPI QSPI controller driver
+ *
+ * Copyright (C) 2018-2022 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+
+/*
+ * QSPI Control register mask defines
+ */
+#define CONTROL_ENABLE		BIT(0)
+#define CONTROL_MASTER		BIT(1)
+#define CONTROL_XIP		BIT(2)
+#define CONTROL_XIPADDR		BIT(3)
+#define CONTROL_CLKIDLE		BIT(10)
+#define CONTROL_SAMPLE_MASK	GENMASK(12, 11)
+#define CONTROL_MODE0		BIT(13)
+#define CONTROL_MODE12_MASK	GENMASK(15, 14)
+#define CONTROL_MODE12_EX_RO	BIT(14)
+#define CONTROL_MODE12_EX_RW	BIT(15)
+#define CONTROL_MODE12_FULL	GENMASK(15, 14)
+#define CONTROL_FLAGSX4		BIT(16)
+#define CONTROL_CLKRATE_MASK	GENMASK(27, 24)
+#define CONTROL_CLKRATE_SHIFT	24
+
+/*
+ * QSPI Frames register mask defines
+ */
+#define FRAMES_TOTALBYTES_MASK	GENMASK(15, 0)
+#define FRAMES_CMDBYTES_MASK	GENMASK(24, 16)
+#define FRAMES_CMDBYTES_SHIFT	16
+#define FRAMES_SHIFT		25
+#define FRAMES_IDLE_MASK	GENMASK(29, 26)
+#define FRAMES_IDLE_SHIFT	26
+#define FRAMES_FLAGBYTE		BIT(30)
+#define FRAMES_FLAGWORD		BIT(31)
+
+/*
+ * QSPI Interrupt Enable register mask defines
+ */
+#define IEN_TXDONE		BIT(0)
+#define IEN_RXDONE		BIT(1)
+#define IEN_RXAVAILABLE		BIT(2)
+#define IEN_TXAVAILABLE		BIT(3)
+#define IEN_RXFIFOEMPTY		BIT(4)
+#define IEN_TXFIFOFULL		BIT(5)
+
+/*
+ * QSPI Status register mask defines
+ */
+#define STATUS_TXDONE		BIT(0)
+#define STATUS_RXDONE		BIT(1)
+#define STATUS_RXAVAILABLE	BIT(2)
+#define STATUS_TXAVAILABLE	BIT(3)
+#define STATUS_RXFIFOEMPTY	BIT(4)
+#define STATUS_TXFIFOFULL	BIT(5)
+#define STATUS_READY		BIT(7)
+#define STATUS_FLAGSX4		BIT(8)
+#define STATUS_MASK		GENMASK(8, 0)
+
+#define BYTESUPPER_MASK		GENMASK(31, 16)
+#define BYTESLOWER_MASK		GENMASK(15, 0)
+
+#define MAX_DIVIDER		16
+#define MIN_DIVIDER		0
+#define MAX_DATA_CMD_LEN	256
+
+/* QSPI ready time out value */
+#define TIMEOUT_MS		500
+
+/*
+ * QSPI Register offsets.
+ */
+#define REG_CONTROL		(0x00)
+#define REG_FRAMES		(0x04)
+#define REG_IEN			(0x0c)
+#define REG_STATUS		(0x10)
+#define REG_DIRECT_ACCESS	(0x14)
+#define REG_UPPER_ACCESS	(0x18)
+#define REG_RX_DATA		(0x40)
+#define REG_TX_DATA		(0x44)
+#define REG_X4_RX_DATA		(0x48)
+#define REG_X4_TX_DATA		(0x4c)
+#define REG_FRAMESUP		(0x50)
+
+/**
+ * struct mchp_coreqspi - Defines qspi driver instance
+ * @regs:              Virtual address of the QSPI controller registers
+ * @clk:               QSPI Operating clock
+ * @data_completion:   completion structure
+ * @op_lock:           lock access to the device
+ * @txbuf:             TX buffer
+ * @rxbuf:             RX buffer
+ * @irq:               IRQ number
+ * @tx_len:            Number of bytes left to transfer
+ * @rx_len:            Number of bytes left to receive
+ */
+struct mchp_coreqspi {
+	void __iomem *regs;
+	struct clk *clk;
+	struct completion data_completion;
+	struct mutex op_lock; /* lock access to the device */
+	u8 *txbuf;
+	u8 *rxbuf;
+	int irq;
+	int tx_len;
+	int rx_len;
+};
+
+static int mchp_coreqspi_set_mode(struct mchp_coreqspi *qspi, const struct spi_mem_op *op)
+{
+	u32 control = readl_relaxed(qspi->regs + REG_CONTROL);
+
+	/*
+	 * The operating mode can be configured based on the command that needs to be send.
+	 * bits[15:14]: Sets whether multiple bit SPI operates in normal, extended or full modes.
+	 *		00: Normal (single DQ0 TX and single DQ1 RX lines)
+	 *		01: Extended RO (command and address bytes on DQ0 only)
+	 *		10: Extended RW (command byte on DQ0 only)
+	 *		11: Full. (command and address are on all DQ lines)
+	 * bit[13]:	Sets whether multiple bit SPI uses 2 or 4 bits of data
+	 *		0: 2-bits (BSPI)
+	 *		1: 4-bits (QSPI)
+	 */
+	if (op->data.buswidth == 4 || op->data.buswidth == 2) {
+		control &= ~CONTROL_MODE12_MASK;
+		if (op->cmd.buswidth == 1 && (op->addr.buswidth == 1 || op->addr.buswidth == 0))
+			control |= CONTROL_MODE12_EX_RO;
+		else if (op->cmd.buswidth == 1)
+			control |= CONTROL_MODE12_EX_RW;
+		else
+			control |= CONTROL_MODE12_FULL;
+
+		control |= CONTROL_MODE0;
+	} else {
+		control &= ~(CONTROL_MODE12_MASK |
+			     CONTROL_MODE0);
+	}
+
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	return 0;
+}
+
+static inline void mchp_coreqspi_read_op(struct mchp_coreqspi *qspi)
+{
+	u32 control, data;
+
+	if (!qspi->rx_len)
+		return;
+
+	control = readl_relaxed(qspi->regs + REG_CONTROL);
+
+	/*
+	 * Read 4-bytes from the SPI FIFO in single transaction and then read
+	 * the reamaining data byte wise.
+	 */
+	control |= CONTROL_FLAGSX4;
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	while (qspi->rx_len >= 4) {
+		while (readl_relaxed(qspi->regs + REG_STATUS) & STATUS_RXFIFOEMPTY)
+			;
+		data = readl_relaxed(qspi->regs + REG_X4_RX_DATA);
+		*(u32 *)qspi->rxbuf = data;
+		qspi->rxbuf += 4;
+		qspi->rx_len -= 4;
+	}
+
+	control &= ~CONTROL_FLAGSX4;
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	while (qspi->rx_len--) {
+		while (readl_relaxed(qspi->regs + REG_STATUS) & STATUS_RXFIFOEMPTY)
+			;
+		data = readl_relaxed(qspi->regs + REG_RX_DATA);
+		*qspi->rxbuf++ = (data & 0xFF);
+	}
+}
+
+static inline void mchp_coreqspi_write_op(struct mchp_coreqspi *qspi, bool word)
+{
+	u32 control, data;
+
+	control = readl_relaxed(qspi->regs + REG_CONTROL);
+	control |= CONTROL_FLAGSX4;
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	while (qspi->tx_len >= 4) {
+		while (readl_relaxed(qspi->regs + REG_STATUS) & STATUS_TXFIFOFULL)
+			;
+		data = *(u32 *)qspi->txbuf;
+		qspi->txbuf += 4;
+		qspi->tx_len -= 4;
+		writel_relaxed(data, qspi->regs + REG_X4_TX_DATA);
+	}
+
+	control &= ~CONTROL_FLAGSX4;
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	while (qspi->tx_len--) {
+		while (readl_relaxed(qspi->regs + REG_STATUS) & STATUS_TXFIFOFULL)
+			;
+		data =  *qspi->txbuf++;
+		writel_relaxed(data, qspi->regs + REG_TX_DATA);
+	}
+}
+
+static void mchp_coreqspi_enable_ints(struct mchp_coreqspi *qspi)
+{
+	u32 mask = IEN_TXDONE |
+		   IEN_RXDONE |
+		   IEN_RXAVAILABLE;
+
+	writel_relaxed(mask, qspi->regs + REG_IEN);
+}
+
+static void mchp_coreqspi_disable_ints(struct mchp_coreqspi *qspi)
+{
+	writel_relaxed(0, qspi->regs + REG_IEN);
+}
+
+static irqreturn_t mchp_coreqspi_isr(int irq, void *dev_id)
+{
+	struct mchp_coreqspi *qspi = (struct mchp_coreqspi *)dev_id;
+	irqreturn_t ret = IRQ_NONE;
+	int intfield = readl_relaxed(qspi->regs + REG_STATUS) & STATUS_MASK;
+
+	if (intfield == 0)
+		return ret;
+
+	if (intfield & IEN_TXDONE) {
+		writel_relaxed(IEN_TXDONE, qspi->regs + REG_STATUS);
+		ret = IRQ_HANDLED;
+	}
+
+	if (intfield & IEN_RXAVAILABLE) {
+		writel_relaxed(IEN_RXAVAILABLE, qspi->regs + REG_STATUS);
+		mchp_coreqspi_read_op(qspi);
+		ret = IRQ_HANDLED;
+	}
+
+	if (intfield & IEN_RXDONE) {
+		writel_relaxed(IEN_RXDONE, qspi->regs + REG_STATUS);
+		complete(&qspi->data_completion);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
+static int mchp_coreqspi_setup_clock(struct mchp_coreqspi *qspi, struct spi_device *spi)
+{
+	unsigned long clk_hz;
+	u32 control, baud_rate_val = 0;
+
+	clk_hz = clk_get_rate(qspi->clk);
+	if (!clk_hz)
+		return -EINVAL;
+
+	baud_rate_val = DIV_ROUND_UP(clk_hz, 2 * spi->max_speed_hz);
+	if (baud_rate_val > MAX_DIVIDER || baud_rate_val < MIN_DIVIDER) {
+		dev_err(&spi->dev,
+			"could not configure the clock for spi clock %d Hz & system clock %ld Hz\n",
+			spi->max_speed_hz, clk_hz);
+		return -EINVAL;
+	}
+
+	control = readl_relaxed(qspi->regs + REG_CONTROL);
+	control |= baud_rate_val << CONTROL_CLKRATE_SHIFT;
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+	control = readl_relaxed(qspi->regs + REG_CONTROL);
+
+	if ((spi->mode & SPI_CPOL) && (spi->mode & SPI_CPHA))
+		control |= CONTROL_CLKIDLE;
+	else
+		control &= ~CONTROL_CLKIDLE;
+
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	return 0;
+}
+
+static int mchp_coreqspi_setup_op(struct spi_device *spi_dev)
+{
+	struct spi_controller *ctlr = spi_dev->master;
+	struct mchp_coreqspi *qspi = spi_controller_get_devdata(ctlr);
+	u32 control = readl_relaxed(qspi->regs + REG_CONTROL);
+
+	control |= (CONTROL_MASTER | CONTROL_ENABLE);
+	control &= ~CONTROL_CLKIDLE;
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	return 0;
+}
+
+static inline void mchp_coreqspi_config_op(struct mchp_coreqspi *qspi, const struct spi_mem_op *op)
+{
+	u32 idle_cycles = 0;
+	int total_bytes, cmd_bytes, frames, ctrl;
+
+	cmd_bytes = op->cmd.nbytes + op->addr.nbytes;
+	total_bytes = cmd_bytes + op->data.nbytes;
+
+	/*
+	 * As per the coreQSPI IP spec,the number of command and data bytes are
+	 * controlled by the frames register for each SPI sequence. This supports
+	 * the SPI flash memory read and writes sequences as below. so configure
+	 * the cmd and total bytes accordingly.
+	 * ---------------------------------------------------------------------
+	 * TOTAL BYTES  |  CMD BYTES | What happens                             |
+	 * ______________________________________________________________________
+	 *              |            |                                          |
+	 *     1        |   1        | The SPI core will transmit a single byte |
+	 *              |            | and receive data is discarded            |
+	 *              |            |                                          |
+	 *     1        |   0        | The SPI core will transmit a single byte |
+	 *              |            | and return a single byte                 |
+	 *              |            |                                          |
+	 *     10       |   4        | The SPI core will transmit 4 command     |
+	 *              |            | bytes discarding the receive data and    |
+	 *              |            | transmits 6 dummy bytes returning the 6  |
+	 *              |            | received bytes and return a single byte  |
+	 *              |            |                                          |
+	 *     10       |   10       | The SPI core will transmit 10 command    |
+	 *              |            |                                          |
+	 *     10       |    0       | The SPI core will transmit 10 command    |
+	 *              |            | bytes and returning 10 received bytes    |
+	 * ______________________________________________________________________
+	 */
+	if (!(op->data.dir == SPI_MEM_DATA_IN))
+		cmd_bytes = total_bytes;
+
+	frames = total_bytes & BYTESUPPER_MASK;
+	writel_relaxed(frames, qspi->regs + REG_FRAMESUP);
+	frames = total_bytes & BYTESLOWER_MASK;
+	frames |= cmd_bytes << FRAMES_CMDBYTES_SHIFT;
+
+	if (op->dummy.buswidth)
+		idle_cycles = op->dummy.nbytes * 8 / op->dummy.buswidth;
+
+	frames |= idle_cycles << FRAMES_IDLE_SHIFT;
+	ctrl = readl_relaxed(qspi->regs + REG_CONTROL);
+
+	if (ctrl & CONTROL_MODE12_MASK)
+		frames |= (1 << FRAMES_SHIFT);
+
+	frames |= FRAMES_FLAGWORD;
+	writel_relaxed(frames, qspi->regs + REG_FRAMES);
+}
+
+static int mchp_qspi_wait_for_ready(struct spi_mem *mem)
+{
+	struct mchp_coreqspi *qspi = spi_controller_get_devdata
+				    (mem->spi->master);
+	u32 status;
+	int ret;
+
+	ret = readl_poll_timeout(qspi->regs + REG_STATUS, status,
+				 (status & STATUS_READY), 0,
+				 TIMEOUT_MS);
+	if (ret) {
+		dev_err(&mem->spi->dev,
+			"Timeout waiting on QSPI ready.\n");
+		return -ETIMEDOUT;
+	}
+
+	return ret;
+}
+
+static int mchp_coreqspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+	struct mchp_coreqspi *qspi = spi_controller_get_devdata
+				    (mem->spi->master);
+	u32 address = op->addr.val;
+	u8 opcode = op->cmd.opcode;
+	u8 opaddr[5];
+	int err, i;
+
+	mutex_lock(&qspi->op_lock);
+	err = mchp_qspi_wait_for_ready(mem);
+	if (err)
+		goto error;
+
+	err = mchp_coreqspi_setup_clock(qspi, mem->spi);
+	if (err)
+		goto error;
+
+	err = mchp_coreqspi_set_mode(qspi, op);
+	if (err)
+		goto error;
+
+	reinit_completion(&qspi->data_completion);
+	mchp_coreqspi_config_op(qspi, op);
+	if (op->cmd.opcode) {
+		qspi->txbuf = &opcode;
+		qspi->rxbuf = NULL;
+		qspi->tx_len = op->cmd.nbytes;
+		qspi->rx_len = 0;
+		mchp_coreqspi_write_op(qspi, false);
+	}
+
+	qspi->txbuf = &opaddr[0];
+	if (op->addr.nbytes) {
+		for (i = 0; i < op->addr.nbytes; i++)
+			qspi->txbuf[i] = address >> (8 * (op->addr.nbytes - i - 1));
+
+		qspi->rxbuf = NULL;
+		qspi->tx_len = op->addr.nbytes;
+		qspi->rx_len = 0;
+		mchp_coreqspi_write_op(qspi, false);
+	}
+
+	if (op->data.nbytes) {
+		if (op->data.dir == SPI_MEM_DATA_OUT) {
+			qspi->txbuf = (u8 *)op->data.buf.out;
+			qspi->rxbuf = NULL;
+			qspi->rx_len = 0;
+			qspi->tx_len = op->data.nbytes;
+			mchp_coreqspi_write_op(qspi, true);
+		} else {
+			qspi->txbuf = NULL;
+			qspi->rxbuf = (u8 *)op->data.buf.in;
+			qspi->rx_len = op->data.nbytes;
+			qspi->tx_len = 0;
+		}
+	}
+
+	mchp_coreqspi_enable_ints(qspi);
+
+	if (!wait_for_completion_timeout(&qspi->data_completion, msecs_to_jiffies(1000)))
+		err = -ETIMEDOUT;
+
+error:
+	mutex_unlock(&qspi->op_lock);
+	mchp_coreqspi_disable_ints(qspi);
+
+	return err;
+}
+
+static bool mchp_coreqspi_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+	if (!spi_mem_default_supports_op(mem, op))
+		return false;
+
+	if ((op->data.buswidth == 4 || op->data.buswidth == 2) &&
+	    (op->cmd.buswidth == 1 && (op->addr.buswidth == 1 || op->addr.buswidth == 0))) {
+		/*
+		 * If the command and address are on DQ0 only, then this
+		 * controller doesn't support sending data on dual and
+		 * quad lines. but it supports reading data on dual and
+		 * quad lines with same configuration as command and
+		 * address on DQ0.
+		 * i.e. The control register[15:13] :EX_RO(read only) is
+		 * meant only for the command and address are on DQ0 but
+		 * not to write data, it is just to read.
+		 * Ex: 0x34h is Quad Load Program Data which is not
+		 * supported. Then the spi-mem layer will iterate over
+		 * each command and it will chose the supported one.
+		 */
+		if (op->data.dir == SPI_MEM_DATA_OUT)
+			return false;
+	}
+
+	return true;
+}
+
+static int mchp_coreqspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
+{
+	if (op->data.dir == SPI_MEM_DATA_OUT || op->data.dir == SPI_MEM_DATA_IN) {
+		if (op->data.nbytes > MAX_DATA_CMD_LEN)
+			op->data.nbytes = MAX_DATA_CMD_LEN;
+	}
+
+	return 0;
+}
+
+static const struct spi_controller_mem_ops mchp_coreqspi_mem_ops = {
+	.adjust_op_size = mchp_coreqspi_adjust_op_size,
+	.supports_op = mchp_coreqspi_supports_op,
+	.exec_op = mchp_coreqspi_exec_op,
+};
+
+static int mchp_coreqspi_probe(struct platform_device *pdev)
+{
+	struct spi_controller *ctlr;
+	struct mchp_coreqspi *qspi;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	int ret;
+
+	ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(*qspi));
+	if (!ctlr)
+		return dev_err_probe(&pdev->dev, -ENOMEM,
+				     "unable to allocate master for QSPI controller\n");
+
+	qspi = spi_controller_get_devdata(ctlr);
+	platform_set_drvdata(pdev, qspi);
+
+	qspi->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(qspi->regs))
+		return dev_err_probe(&pdev->dev, PTR_ERR(qspi->regs),
+				     "failed to map registers\n");
+
+	qspi->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(qspi->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(qspi->clk),
+				     "could not get clock\n");
+
+	ret = clk_prepare_enable(qspi->clk);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to enable clock\n");
+
+	init_completion(&qspi->data_completion);
+	mutex_init(&qspi->op_lock);
+
+	qspi->irq = platform_get_irq(pdev, 0);
+	if (qspi->irq <= 0) {
+		ret = qspi->irq;
+		goto out;
+	}
+
+	ret = devm_request_irq(&pdev->dev, qspi->irq, mchp_coreqspi_isr,
+			       IRQF_SHARED, pdev->name, qspi);
+	if (ret) {
+		dev_err(&pdev->dev, "request_irq failed %d\n", ret);
+		goto out;
+	}
+
+	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
+	ctlr->mem_ops = &mchp_coreqspi_mem_ops;
+	ctlr->setup = mchp_coreqspi_setup_op;
+	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
+			  SPI_TX_DUAL | SPI_TX_QUAD;
+	ctlr->dev.of_node = np;
+
+	ret = devm_spi_register_controller(&pdev->dev, ctlr);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret,
+			      "spi_register_controller failed\n");
+		goto out;
+	}
+
+	return 0;
+
+out:
+	clk_disable_unprepare(qspi->clk);
+
+	return ret;
+}
+
+static int mchp_coreqspi_remove(struct platform_device *pdev)
+{
+	struct mchp_coreqspi *qspi = platform_get_drvdata(pdev);
+	u32 control = readl_relaxed(qspi->regs + REG_CONTROL);
+
+	mchp_coreqspi_disable_ints(qspi);
+	control &= ~CONTROL_ENABLE;
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+	clk_disable_unprepare(qspi->clk);
+
+	return 0;
+}
+
+static const struct of_device_id mchp_coreqspi_of_match[] = {
+	{ .compatible = "microchip,mpfs-qspi" },
+	{ .compatible = "microchip,coreqspi-rtl-v2" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mchp_coreqspi_of_match);
+
+static struct platform_driver mchp_coreqspi_driver = {
+	.probe = mchp_coreqspi_probe,
+	.driver = {
+		.name = "microchip,mpfs-qspi",
+		.of_match_table = mchp_coreqspi_of_match,
+	},
+	.remove = mchp_coreqspi_remove,
+};
+module_platform_driver(mchp_coreqspi_driver);
+
+MODULE_AUTHOR("Naga Sureshkumar Relli <nagasuresh.relli@microchip.com");
+MODULE_DESCRIPTION("Microchip coreQSPI QSPI controller driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry
  2022-08-05  5:30 [PATCH v3 0/4] Add support for Microchip QSPI controller Naga Sureshkumar Relli
                   ` (2 preceding siblings ...)
  2022-08-05  5:30 ` [PATCH v3 3/4] spi: microchip-core-qspi: Add support for microchip fpga qspi controllers Naga Sureshkumar Relli
@ 2022-08-05  5:30 ` Naga Sureshkumar Relli
  2022-08-05  6:50   ` Krzysztof Kozlowski
  2022-08-05 11:05   ` Mark Brown
  2022-08-05  9:14 ` [PATCH v3 0/4] Add support for Microchip QSPI controller Conor.Dooley
  4 siblings, 2 replies; 20+ messages in thread
From: Naga Sureshkumar Relli @ 2022-08-05  5:30 UTC (permalink / raw)
  To: broonie, robh+dt, krzysztof.kozlowski+dt, conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis,
	Naga Sureshkumar Relli

Add the qspi driver to existing Polarfire SoC entry.

Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 295ca16a415b..0329dca23fe2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17146,6 +17146,7 @@ S:	Supported
 F:	arch/riscv/boot/dts/microchip/
 F:	drivers/mailbox/mailbox-mpfs.c
 F:	drivers/soc/microchip/
+F:	drivers/spi/spi-microchip-core-qspi.c
 F:	drivers/spi/spi-microchip-core.c
 F:	include/soc/microchip/mpfs.h
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 1/4] spi: dt-binding: document microchip coreQSPI
  2022-08-05  5:30 ` [PATCH v3 1/4] spi: dt-binding: document microchip coreQSPI Naga Sureshkumar Relli
@ 2022-08-05  6:46   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-05  6:46 UTC (permalink / raw)
  To: Naga Sureshkumar Relli, broonie, robh+dt, krzysztof.kozlowski+dt,
	conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
> Add microchip coreQSPI compatible string and update the title/description
> to reflect this addition.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
  2022-08-05  5:30 ` [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi Naga Sureshkumar Relli
@ 2022-08-05  6:47   ` Krzysztof Kozlowski
  2022-08-05  6:49   ` Krzysztof Kozlowski
  2022-08-05 14:14   ` Rob Herring
  2 siblings, 0 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-05  6:47 UTC (permalink / raw)
  To: Naga Sureshkumar Relli, broonie, robh+dt, krzysztof.kozlowski+dt,
	conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
> Microchip's PolarFire SoC QSPI IP core is based on coreQSPI,
> so add coreqspi as a fallback to mpfs-qspi.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
  2022-08-05  5:30 ` [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi Naga Sureshkumar Relli
  2022-08-05  6:47   ` Krzysztof Kozlowski
@ 2022-08-05  6:49   ` Krzysztof Kozlowski
  2022-08-05  7:34     ` Conor.Dooley
  2022-08-05 14:14   ` Rob Herring
  2 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-05  6:49 UTC (permalink / raw)
  To: Naga Sureshkumar Relli, broonie, robh+dt, krzysztof.kozlowski+dt,
	conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
> diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
> index a47d4923b51b..84d32c1a4d60 100644
> --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
> +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
> @@ -18,10 +18,12 @@ allOf:
>  
>  properties:
>    compatible:
> -    enum:
> -      - microchip,mpfs-spi
> -      - microchip,mpfs-qspi
> -      - microchip,coreqspi-rtl-v2 # FPGA QSPI
> +   oneOf:
> +    - items:
> +        - const: microchip,mpfs-qspi
> +        - const: microchip,coreqspi-rtl-v2

Eh, this does not make sense after looking at your driver...

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 3/4] spi: microchip-core-qspi: Add support for microchip fpga qspi controllers
  2022-08-05  5:30 ` [PATCH v3 3/4] spi: microchip-core-qspi: Add support for microchip fpga qspi controllers Naga Sureshkumar Relli
@ 2022-08-05  6:50   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-05  6:50 UTC (permalink / raw)
  To: Naga Sureshkumar Relli, broonie, robh+dt, krzysztof.kozlowski+dt,
	conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
> Add a driver for Microchip FPGA QSPI controllers. This driver also
> supports "hard" QSPI controllers on Polarfire SoC.
> 

Thank you for your patch. There is something to discuss/improve.

> +	ret = clk_prepare_enable(qspi->clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to enable clock\n");
> +
> +	init_completion(&qspi->data_completion);
> +	mutex_init(&qspi->op_lock);
> +
> +	qspi->irq = platform_get_irq(pdev, 0);
> +	if (qspi->irq <= 0) {


< 0
Why did you change it to <=?

> +		ret = qspi->irq;
> +		goto out;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, qspi->irq, mchp_coreqspi_isr,
> +			       IRQF_SHARED, pdev->name, qspi);
> +	if (ret) {
> +		dev_err(&pdev->dev, "request_irq failed %d\n", ret);
> +		goto out;
> +	}
> +
> +	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
> +	ctlr->mem_ops = &mchp_coreqspi_mem_ops;
> +	ctlr->setup = mchp_coreqspi_setup_op;
> +	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
> +			  SPI_TX_DUAL | SPI_TX_QUAD;
> +	ctlr->dev.of_node = np;
> +
> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret,
> +			      "spi_register_controller failed\n");
> +		goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	clk_disable_unprepare(qspi->clk);
> +
> +	return ret;
> +}
> +
> +static int mchp_coreqspi_remove(struct platform_device *pdev)
> +{
> +	struct mchp_coreqspi *qspi = platform_get_drvdata(pdev);
> +	u32 control = readl_relaxed(qspi->regs + REG_CONTROL);
> +
> +	mchp_coreqspi_disable_ints(qspi);
> +	control &= ~CONTROL_ENABLE;
> +	writel_relaxed(control, qspi->regs + REG_CONTROL);
> +	clk_disable_unprepare(qspi->clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mchp_coreqspi_of_match[] = {
> +	{ .compatible = "microchip,mpfs-qspi" },
> +	{ .compatible = "microchip,coreqspi-rtl-v2" },

This is not what the binding is saying.


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry
  2022-08-05  5:30 ` [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry Naga Sureshkumar Relli
@ 2022-08-05  6:50   ` Krzysztof Kozlowski
  2022-08-05 11:04     ` Mark Brown
  2022-08-05 11:05   ` Mark Brown
  1 sibling, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-05  6:50 UTC (permalink / raw)
  To: Naga Sureshkumar Relli, broonie, robh+dt, krzysztof.kozlowski+dt,
	conor.dooley
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
> Add the qspi driver to existing Polarfire SoC entry.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> ---

This should be squashed with previous patch.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
  2022-08-05  6:49   ` Krzysztof Kozlowski
@ 2022-08-05  7:34     ` Conor.Dooley
  2022-08-05  8:12       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 20+ messages in thread
From: Conor.Dooley @ 2022-08-05  7:34 UTC (permalink / raw)
  To: krzysztof.kozlowski, Nagasuresh.Relli, broonie, robh+dt,
	krzysztof.kozlowski+dt
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 07:49, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
>> diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>> index a47d4923b51b..84d32c1a4d60 100644
>> --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>> @@ -18,10 +18,12 @@ allOf:
>>
>>   properties:
>>     compatible:
>> -    enum:
>> -      - microchip,mpfs-spi
>> -      - microchip,mpfs-qspi
>> -      - microchip,coreqspi-rtl-v2 # FPGA QSPI
>> +   oneOf:
>> +    - items:
>> +        - const: microchip,mpfs-qspi
>> +        - const: microchip,coreqspi-rtl-v2
> 
> Eh, this does not make sense after looking at your driver...

What is wrong with explicitly binding the driver to both of the
compatible strings? The "hard" peripheral in the SoC part of the
FPGA is a superset of version 2 of the coreQSPI IP so the fallback
used in the binding here makes sense to me. coreQSPI can be
instantiated in the FPGA fabric and used there, so it needs a
compatible of its own.

That brings me back to the original point question, why not
explicitly bind the driver to both of the compatible strings it
is known to work for?
Thanks,
Conor.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
  2022-08-05  7:34     ` Conor.Dooley
@ 2022-08-05  8:12       ` Krzysztof Kozlowski
  2022-08-05  8:44         ` Conor.Dooley
  0 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-05  8:12 UTC (permalink / raw)
  To: Conor.Dooley, Nagasuresh.Relli, broonie, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 09:34, Conor.Dooley@microchip.com wrote:
> On 05/08/2022 07:49, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
>>> diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>>> index a47d4923b51b..84d32c1a4d60 100644
>>> --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>>> @@ -18,10 +18,12 @@ allOf:
>>>
>>>   properties:
>>>     compatible:
>>> -    enum:
>>> -      - microchip,mpfs-spi
>>> -      - microchip,mpfs-qspi
>>> -      - microchip,coreqspi-rtl-v2 # FPGA QSPI
>>> +   oneOf:
>>> +    - items:
>>> +        - const: microchip,mpfs-qspi
>>> +        - const: microchip,coreqspi-rtl-v2
>>
>> Eh, this does not make sense after looking at your driver...
> 
> What is wrong with explicitly binding the driver to both of the
> compatible strings? The "hard" peripheral in the SoC part of the
> FPGA is a superset of version 2 of the coreQSPI IP so the fallback
> used in the binding here makes sense to me. coreQSPI can be
> instantiated in the FPGA fabric and used there, so it needs a
> compatible of its own.
> 
> That brings me back to the original point question, why not
> explicitly bind the driver to both of the compatible strings it
> is known to work for?

There is nothing particularly bad with matching to both of compatibles.
It is valid code. There are however questions/issues with that:

1. It is redundant. I did not look too much at the driver, but none of
the of_device_id entries have some driver data to differentiate,
therefore - for the driver - the devices are identical. If they are
identical and according to binding compatible, use less code and just
one compatible.

2. Otherwise, maybe the devices are not actually fully compatible?
That's the second problem. If one writes binding like that and codes it
in driver differently, it looks like it was not investigated really and
I ask questions...

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
  2022-08-05  8:12       ` Krzysztof Kozlowski
@ 2022-08-05  8:44         ` Conor.Dooley
  2022-08-05  8:47           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 20+ messages in thread
From: Conor.Dooley @ 2022-08-05  8:44 UTC (permalink / raw)
  To: krzysztof.kozlowski, Nagasuresh.Relli, broonie, robh+dt,
	krzysztof.kozlowski+dt
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 09:12, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 05/08/2022 09:34, Conor.Dooley@microchip.com wrote:
>> On 05/08/2022 07:49, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
>>>> diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>>>> index a47d4923b51b..84d32c1a4d60 100644
>>>> --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>>>> @@ -18,10 +18,12 @@ allOf:
>>>>
>>>>    properties:
>>>>      compatible:
>>>> -    enum:
>>>> -      - microchip,mpfs-spi
>>>> -      - microchip,mpfs-qspi
>>>> -      - microchip,coreqspi-rtl-v2 # FPGA QSPI
>>>> +   oneOf:
>>>> +    - items:
>>>> +        - const: microchip,mpfs-qspi
>>>> +        - const: microchip,coreqspi-rtl-v2
>>>
>>> Eh, this does not make sense after looking at your driver...
>>
>> What is wrong with explicitly binding the driver to both of the
>> compatible strings? The "hard" peripheral in the SoC part of the
>> FPGA is a superset of version 2 of the coreQSPI IP so the fallback
>> used in the binding here makes sense to me. coreQSPI can be
>> instantiated in the FPGA fabric and used there, so it needs a
>> compatible of its own.
>>
>> That brings me back to the original point question, why not
>> explicitly bind the driver to both of the compatible strings it
>> is known to work for?
> 
> There is nothing particularly bad with matching to both of compatibles.
> It is valid code. There are however questions/issues with that:
> 
> 1. It is redundant. I did not look too much at the driver, but none of
> the of_device_id entries have some driver data to differentiate,
> therefore - for the driver - the devices are identical. If they are
> identical and according to binding compatible, use less code and just
> one compatible.

Right. Then the binding is correct and the driver should only bind
against "microchip,coreqspi-rtl-v2".

> 
> 2. Otherwise, maybe the devices are not actually fully compatible?
> That's the second problem. If one writes binding like that and codes it
> in driver differently, it looks like it was not investigated really and
> I ask questions...
> 
> Best regards,
> Krzysztof


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
  2022-08-05  8:44         ` Conor.Dooley
@ 2022-08-05  8:47           ` Krzysztof Kozlowski
  0 siblings, 0 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-05  8:47 UTC (permalink / raw)
  To: Conor.Dooley, Nagasuresh.Relli, broonie, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 10:44, Conor.Dooley@microchip.com wrote:
>> 1. It is redundant. I did not look too much at the driver, but none of
>> the of_device_id entries have some driver data to differentiate,
>> therefore - for the driver - the devices are identical. If they are
>> identical and according to binding compatible, use less code and just
>> one compatible.
> 
> Right. Then the binding is correct and the driver should only bind
> against "microchip,coreqspi-rtl-v2".

Yes.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 0/4] Add support for Microchip QSPI controller
  2022-08-05  5:30 [PATCH v3 0/4] Add support for Microchip QSPI controller Naga Sureshkumar Relli
                   ` (3 preceding siblings ...)
  2022-08-05  5:30 ` [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry Naga Sureshkumar Relli
@ 2022-08-05  9:14 ` Conor.Dooley
  4 siblings, 0 replies; 20+ messages in thread
From: Conor.Dooley @ 2022-08-05  9:14 UTC (permalink / raw)
  To: Nagasuresh.Relli, broonie, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-spi, devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 06:30, Naga Sureshkumar Relli wrote:
> This patch enables the Microchip's FPGA QSPI and Polarfire SoC QSPI
> controller support.
> 
> Tested spi-nand (W25N01GV) and spi-nor (MT25QL256A) on Microchip's
> ICICLE kit. tested using both FPGA QSPI and Polarfie SoC QSPI.

Pending Krzysztof's requests etc:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

@Valentina could you test this version please?

Thanks,
Conor.
> 
> changes in v3
> ------------
> 1. Added dev_err_probe() at places like probe failures
> 2. Split the dt-bindings one for adding coreqspi compatible
>     and other one to add coreqspi as fallback to mpfs-qspi.
> 
> changes in v2
> ------------
> 1. Replaced spi_alloc_master() with devm_spi_alloc_master()
> 2. Used dev_err_probe() when devm_spi_alloc_master() fails.
> 3. Added shared IRQ flag in the interrupt registration.
> 4. Updated the dt_bindings so that there is a differentiation
>     between FPGA QSPI IP core and hard QSPI IP core.
> 
> Naga Sureshkumar Relli (4):
>    spi: dt-binding: document microchip coreQSPI
>    spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
>    spi: microchip-core-qspi: Add support for microchip fpga qspi
>      controllers
>    MAINTAINERS: add qspi to Polarfire SoC entry
> 
>   .../bindings/spi/microchip,mpfs-spi.yaml      |  15 +-
>   MAINTAINERS                                   |   1 +
>   drivers/spi/Kconfig                           |   9 +
>   drivers/spi/Makefile                          |   1 +
>   drivers/spi/spi-microchip-core-qspi.c         | 601 ++++++++++++++++++
>   5 files changed, 623 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/spi/spi-microchip-core-qspi.c
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry
  2022-08-05  6:50   ` Krzysztof Kozlowski
@ 2022-08-05 11:04     ` Mark Brown
  0 siblings, 0 replies; 20+ messages in thread
From: Mark Brown @ 2022-08-05 11:04 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Naga Sureshkumar Relli, robh+dt, krzysztof.kozlowski+dt,
	conor.dooley, linux-spi, devicetree, linux-kernel,
	Valentina.FernandezAlanis

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

On Fri, Aug 05, 2022 at 08:50:37AM +0200, Krzysztof Kozlowski wrote:
> On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
> > Add the qspi driver to existing Polarfire SoC entry.
> > 
> > Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>

> This should be squashed with previous patch.

It's perfectly fine to have a separate patch for MAINTAINERS like this.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry
  2022-08-05  5:30 ` [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry Naga Sureshkumar Relli
  2022-08-05  6:50   ` Krzysztof Kozlowski
@ 2022-08-05 11:05   ` Mark Brown
  2022-08-05 12:07     ` Conor.Dooley
  1 sibling, 1 reply; 20+ messages in thread
From: Mark Brown @ 2022-08-05 11:05 UTC (permalink / raw)
  To: Naga Sureshkumar Relli
  Cc: robh+dt, krzysztof.kozlowski+dt, conor.dooley, linux-spi,
	devicetree, linux-kernel, Valentina.FernandezAlanis

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

On Fri, Aug 05, 2022 at 11:00:19AM +0530, Naga Sureshkumar Relli wrote:
> Add the qspi driver to existing Polarfire SoC entry.

> +++ b/MAINTAINERS
> @@ -17146,6 +17146,7 @@ S:	Supported
>  F:	arch/riscv/boot/dts/microchip/
>  F:	drivers/mailbox/mailbox-mpfs.c
>  F:	drivers/soc/microchip/
> +F:	drivers/spi/spi-microchip-core-qspi.c
>  F:	drivers/spi/spi-microchip-core.c
>  F:	include/soc/microchip/mpfs.h

You should also add a pattern for the DT binding here.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry
  2022-08-05 11:05   ` Mark Brown
@ 2022-08-05 12:07     ` Conor.Dooley
  2022-08-05 12:11       ` Mark Brown
  0 siblings, 1 reply; 20+ messages in thread
From: Conor.Dooley @ 2022-08-05 12:07 UTC (permalink / raw)
  To: broonie, Nagasuresh.Relli
  Cc: robh+dt, krzysztof.kozlowski+dt, Conor.Dooley, linux-spi,
	devicetree, linux-kernel, Valentina.FernandezAlanis

On 05/08/2022 12:05, Mark Brown wrote:
> On Fri, Aug 05, 2022 at 11:00:19AM +0530, Naga Sureshkumar Relli wrote:
>> Add the qspi driver to existing Polarfire SoC entry.
> 
>> +++ b/MAINTAINERS
>> @@ -17146,6 +17146,7 @@ S:	Supported
>>   F:	arch/riscv/boot/dts/microchip/
>>   F:	drivers/mailbox/mailbox-mpfs.c
>>   F:	drivers/soc/microchip/
>> +F:	drivers/spi/spi-microchip-core-qspi.c
>>   F:	drivers/spi/spi-microchip-core.c
>>   F:	include/soc/microchip/mpfs.h
> 
> You should also add a pattern for the DT binding here.

All of the bindings for the platform should have entries then
right? I'll send a separate patch adding all of the missing
bindings. I have a deferred change to the entry that needs to
be sent to Arnd anyway so I can queue the two together.
Nothing to be gained by waiting until this driver lands in 6.1+
to have MAINTAINERS coverage of the bindings :)

Thanks,
Conor.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry
  2022-08-05 12:07     ` Conor.Dooley
@ 2022-08-05 12:11       ` Mark Brown
  0 siblings, 0 replies; 20+ messages in thread
From: Mark Brown @ 2022-08-05 12:11 UTC (permalink / raw)
  To: Conor.Dooley
  Cc: Nagasuresh.Relli, robh+dt, krzysztof.kozlowski+dt, linux-spi,
	devicetree, linux-kernel, Valentina.FernandezAlanis

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

On Fri, Aug 05, 2022 at 12:07:57PM +0000, Conor.Dooley@microchip.com wrote:
> On 05/08/2022 12:05, Mark Brown wrote:

> >> +++ b/MAINTAINERS
> >> @@ -17146,6 +17146,7 @@ S:	Supported
> >>   F:	arch/riscv/boot/dts/microchip/
> >>   F:	drivers/mailbox/mailbox-mpfs.c
> >>   F:	drivers/soc/microchip/
> >> +F:	drivers/spi/spi-microchip-core-qspi.c
> >>   F:	drivers/spi/spi-microchip-core.c
> >>   F:	include/soc/microchip/mpfs.h

> > You should also add a pattern for the DT binding here.

> All of the bindings for the platform should have entries then
> right? I'll send a separate patch adding all of the missing
> bindings. I have a deferred change to the entry that needs to
> be sent to Arnd anyway so I can queue the two together.
> Nothing to be gained by waiting until this driver lands in 6.1+
> to have MAINTAINERS coverage of the bindings :)

Yes, it's better if everything has coverage - that way the platform
maintainers are more likely to see any changes that are needed for the
bindings.  Sending as part of a bigger patch adding the rest sounds
good.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi
  2022-08-05  5:30 ` [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi Naga Sureshkumar Relli
  2022-08-05  6:47   ` Krzysztof Kozlowski
  2022-08-05  6:49   ` Krzysztof Kozlowski
@ 2022-08-05 14:14   ` Rob Herring
  2 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2022-08-05 14:14 UTC (permalink / raw)
  To: Naga Sureshkumar Relli
  Cc: devicetree, krzysztof.kozlowski+dt, linux-kernel, broonie,
	linux-spi, robh+dt, Valentina.FernandezAlanis, conor.dooley

On Fri, 05 Aug 2022 11:00:17 +0530, Naga Sureshkumar Relli wrote:
> Microchip's PolarFire SoC QSPI IP core is based on coreQSPI,
> so add coreqspi as a fallback to mpfs-qspi.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
> ---
>  .../devicetree/bindings/spi/microchip,mpfs-spi.yaml    | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml:21:4: [warning] wrong indentation: expected 4 but found 3 (indentation)
./Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml:22:5: [warning] wrong indentation: expected 5 but found 4 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2022-08-05 14:15 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-05  5:30 [PATCH v3 0/4] Add support for Microchip QSPI controller Naga Sureshkumar Relli
2022-08-05  5:30 ` [PATCH v3 1/4] spi: dt-binding: document microchip coreQSPI Naga Sureshkumar Relli
2022-08-05  6:46   ` Krzysztof Kozlowski
2022-08-05  5:30 ` [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi Naga Sureshkumar Relli
2022-08-05  6:47   ` Krzysztof Kozlowski
2022-08-05  6:49   ` Krzysztof Kozlowski
2022-08-05  7:34     ` Conor.Dooley
2022-08-05  8:12       ` Krzysztof Kozlowski
2022-08-05  8:44         ` Conor.Dooley
2022-08-05  8:47           ` Krzysztof Kozlowski
2022-08-05 14:14   ` Rob Herring
2022-08-05  5:30 ` [PATCH v3 3/4] spi: microchip-core-qspi: Add support for microchip fpga qspi controllers Naga Sureshkumar Relli
2022-08-05  6:50   ` Krzysztof Kozlowski
2022-08-05  5:30 ` [PATCH v3 4/4] MAINTAINERS: add qspi to Polarfire SoC entry Naga Sureshkumar Relli
2022-08-05  6:50   ` Krzysztof Kozlowski
2022-08-05 11:04     ` Mark Brown
2022-08-05 11:05   ` Mark Brown
2022-08-05 12:07     ` Conor.Dooley
2022-08-05 12:11       ` Mark Brown
2022-08-05  9:14 ` [PATCH v3 0/4] Add support for Microchip QSPI controller Conor.Dooley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).