[PATCH 0/7] spi: zynqmp-gqspi: Split the bus and add GPIO support

[PATCH 0/7] spi: zynqmp-gqspi: Split the bus and add GPIO support

[Sean Anderson]

As discussed in patches 1 and FIXME, this device really has two SPI
busses. Split the existing "merged" bus in twain, and add support for
GPIO chipselects.

This series depends on [1, 2].

[1] https://lore.kernel.org/linux-spi/20250116224130.2684544-1-sean.anderson@linux.dev/T/#t
[2] https://lore.kernel.org/linux-spi/20250116225521.2688224-1-sean.anderson@linux.dev/T/#t


Sean Anderson (7):
  dt-bindings: spi: zynqmp-qspi: Split the bus
  spi: zynqmp-gqspi: Pass speed/mode directly to config_op
  spi: zynqmp-gqspi: Configure SPI mode dynamically
  spi: zynqmp-gqspi: Refactor out controller initialization
  spi: zynqmp-gqspi: Split the bus
  spi: zynqmp-gqspi: Support GPIO chip selects
  ARM64: xilinx: zynqmp: Convert to split QSPI bus

 .../bindings/spi/spi-zynqmp-qspi.yaml         |  43 ++-
 .../boot/dts/xilinx/zynqmp-sm-k26-revA.dts    |   5 +-
 .../boot/dts/xilinx/zynqmp-zc1232-revA.dts    |   5 +-
 .../boot/dts/xilinx/zynqmp-zc1254-revA.dts    |   5 +-
 .../dts/xilinx/zynqmp-zc1751-xm015-dc1.dts    |   5 +-
 .../dts/xilinx/zynqmp-zc1751-xm018-dc4.dts    |   5 +-
 .../boot/dts/xilinx/zynqmp-zcu102-revA.dts    |   5 +-
 .../boot/dts/xilinx/zynqmp-zcu104-revA.dts    |   5 +-
 .../boot/dts/xilinx/zynqmp-zcu104-revC.dts    |   5 +-
 .../boot/dts/xilinx/zynqmp-zcu106-revA.dts    |   5 +-
 .../boot/dts/xilinx/zynqmp-zcu111-revA.dts    |   5 +-
 .../boot/dts/xilinx/zynqmp-zcu1275-revA.dts   |   5 +-
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi        |  15 +-
 drivers/spi/spi-zynqmp-gqspi.c                | 328 ++++++++++++++----
 14 files changed, 359 insertions(+), 82 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[Sean Anderson]

This device supports two separate SPI busses: "lower" (SPI0) and "upper"
(SPI1). Each SPI bus has separate clock and data lines, as well as a
hardware-controlled chip select. The current binding does not model this
situation. It exposes one bus, where CS 0 uses the lower bus and the
lower chip select, and CS 1 uses the upper bus and the upper chip
select. It is not possible to use the upper chip select with the lower
bus (or vice versa). GPIO chip selects are unsupported, and there would
be no way to specify which bus to use if they were.

Split the "merged" bus into an upper and lower bus, each with their own
subnodes.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 .../bindings/spi/spi-zynqmp-qspi.yaml         | 43 +++++++++++++++++--
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
index 901e15fcce2d..12c547c4f1ba 100644
--- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
@@ -39,6 +39,18 @@ properties:
   resets:
     maxItems: 1
 
+  spi-lower:
+    type: object
+    $ref: spi-controller.yaml#
+    unevaluatedProperties: false
+    description: The "lower" bus (SPI0). On the ZynqMP this uses MIO pins 0-5.
+
+  spi-upper:
+    type: object
+    $ref: spi-controller.yaml#
+    unevaluatedProperties: false
+    description: The "upper" bus (SPI1). On the ZynqMP this uses MIO pins 7-12.
+
 required:
   - compatible
   - reg
@@ -50,8 +62,6 @@ required:
 unevaluatedProperties: false
 
 allOf:
-  - $ref: spi-controller.yaml#
-
   - if:
       properties:
         compatible:
@@ -75,7 +85,7 @@ examples:
       #address-cells = <2>;
       #size-cells = <2>;
 
-      qspi: spi@ff0f0000 {
+      qspi: spi-controller@ff0f0000 {
         compatible = "xlnx,zynqmp-qspi-1.0";
         clocks = <&zynqmp_clk QSPI_REF>, <&zynqmp_clk LPD_LSBUS>;
         clock-names = "ref_clk", "pclk";
@@ -84,5 +94,32 @@ examples:
         resets = <&zynqmp_reset ZYNQMP_RESET_QSPI>;
         reg = <0x0 0xff0f0000 0x0 0x1000>,
               <0x0 0xc0000000 0x0 0x8000000>;
+
+        spi-lower {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          num-cs = <2>;
+          cs-gpios = <0>, <&gpio 5>;
+
+          flash@0 {
+            reg = <0>;
+            compatible = "jedec,spi-nor";
+          };
+
+          flash@1 {
+            reg = <1>;
+            compatible = "jedec,spi-nor";
+          };
+        };
+
+        spi-upper {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          flash@0 {
+            reg = <0>;
+            compatible = "jedec,spi-nor";
+          };
+        };
       };
     };
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 2/7] spi: zynqmp-gqspi: Pass speed/mode directly to config_op

[Sean Anderson]

In preparation for supporting transfer_one, which supplies the speed
from the spi_transfer instead of the spi_device, convert config_op to
take the speed and mode directly.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/spi/spi-zynqmp-gqspi.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 95eea7d75f71..ba12adec8632 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -545,8 +545,8 @@ static inline u32 zynqmp_qspi_selectspimode(struct zynqmp_qspi *xqspi,
 /**
  * zynqmp_qspi_config_op - Configure QSPI controller for specified
  *				transfer
- * @xqspi:	Pointer to the zynqmp_qspi structure
- * @qspi:	Pointer to the spi_device structure
+ * @xqspi: Pointer to the zynqmp_qspi structure
+ * @req_speed_hz: Requested frequency
  *
  * Sets the operational mode of QSPI controller for the next QSPI transfer and
  * sets the requested clock frequency.
@@ -563,13 +563,10 @@ static inline u32 zynqmp_qspi_selectspimode(struct zynqmp_qspi *xqspi,
  *	by the QSPI controller the driver will set the highest or lowest
  *	frequency supported by controller.
  */
-static int zynqmp_qspi_config_op(struct zynqmp_qspi *xqspi,
-				 struct spi_device *qspi)
+static int zynqmp_qspi_config_op(struct zynqmp_qspi *xqspi, u32 req_speed_hz)
 {
 	ulong clk_rate;
-	u32 config_reg, req_speed_hz, baud_rate_val = 0;
-
-	req_speed_hz = qspi->max_speed_hz;
+	u32 config_reg, baud_rate_val = 0;
 
 	if (xqspi->speed_hz != req_speed_hz) {
 		xqspi->speed_hz = req_speed_hz;
@@ -1094,7 +1091,7 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
 		op->dummy.buswidth, op->data.buswidth);
 
 	mutex_lock(&xqspi->op_lock);
-	zynqmp_qspi_config_op(xqspi, mem->spi);
+	zynqmp_qspi_config_op(xqspi, mem->spi->max_speed_hz);
 	zynqmp_qspi_chipselect(mem->spi, false);
 	genfifoentry |= xqspi->genfifocs;
 	genfifoentry |= xqspi->genfifobus;
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 3/7] spi: zynqmp-gqspi: Configure SPI mode dynamically

[Sean Anderson]

The SPI mode (phase /polarity) can change between spi_transfers.  In
preparation for transfer_one support, program the SPI mode on every
operation instead of once during init.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/spi/spi-zynqmp-gqspi.c | 43 +++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index ba12adec8632..a1233897dc88 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -186,7 +186,8 @@ struct qspi_platform_data {
  * @mode:		Defines the mode in which QSPI is operating
  * @data_completion:	completion structure
  * @op_lock:		Operational lock
- * @speed_hz:          Current SPI bus clock speed in hz
+ * @speed_hz:		Current SPI bus clock speed in hz
+ * @spi_mode:		Current SPI bus mode
  * @has_tapdelay:	Used for tapdelay register available in qspi
  */
 struct zynqmp_qspi {
@@ -210,6 +211,7 @@ struct zynqmp_qspi {
 	struct completion data_completion;
 	struct mutex op_lock;
 	u32 speed_hz;
+	u32 spi_mode;
 	bool has_tapdelay;
 };
 
@@ -397,16 +399,11 @@ static int zynqmp_qspi_init_hw(struct zynqmp_qspi *xqspi)
 	config_reg |= GQSPI_CFG_WP_HOLD_MASK;
 	/* Clear pre-scalar by default */
 	config_reg &= ~GQSPI_CFG_BAUD_RATE_DIV_MASK;
-	/* Set CPHA */
-	if (xqspi->ctlr->mode_bits & SPI_CPHA)
-		config_reg |= GQSPI_CFG_CLK_PHA_MASK;
-	else
-		config_reg &= ~GQSPI_CFG_CLK_PHA_MASK;
-	/* Set CPOL */
-	if (xqspi->ctlr->mode_bits & SPI_CPOL)
-		config_reg |= GQSPI_CFG_CLK_POL_MASK;
-	else
-		config_reg &= ~GQSPI_CFG_CLK_POL_MASK;
+
+	/* Set default mode */
+	xqspi->spi_mode = SPI_MODE_3;
+	config_reg |= GQSPI_CFG_CLK_PHA_MASK;
+	config_reg |= GQSPI_CFG_CLK_POL_MASK;
 
 	/* Set the clock frequency */
 	clk_rate = clk_get_rate(xqspi->refclk);
@@ -547,6 +544,7 @@ static inline u32 zynqmp_qspi_selectspimode(struct zynqmp_qspi *xqspi,
  *				transfer
  * @xqspi: Pointer to the zynqmp_qspi structure
  * @req_speed_hz: Requested frequency
+ * @mode: Requested SPI mode
  *
  * Sets the operational mode of QSPI controller for the next QSPI transfer and
  * sets the requested clock frequency.
@@ -563,7 +561,8 @@ static inline u32 zynqmp_qspi_selectspimode(struct zynqmp_qspi *xqspi,
  *	by the QSPI controller the driver will set the highest or lowest
  *	frequency supported by controller.
  */
-static int zynqmp_qspi_config_op(struct zynqmp_qspi *xqspi, u32 req_speed_hz)
+static int zynqmp_qspi_config_op(struct zynqmp_qspi *xqspi, u32 req_speed_hz,
+				 u32 mode)
 {
 	ulong clk_rate;
 	u32 config_reg, baud_rate_val = 0;
@@ -589,7 +588,23 @@ static int zynqmp_qspi_config_op(struct zynqmp_qspi *xqspi, u32 req_speed_hz)
 		zynqmp_qspi_set_tapdelay(xqspi, baud_rate_val);
 	}
 
-	dev_dbg(xqspi->dev, "config speed %u\n", req_speed_hz);
+	mode &= SPI_MODE_X_MASK;
+	if (xqspi->spi_mode != mode) {
+		xqspi->spi_mode = mode;
+
+		config_reg = zynqmp_gqspi_read(xqspi, GQSPI_CONFIG_OFST);
+		if (mode & SPI_CPHA)
+			config_reg |= GQSPI_CFG_CLK_PHA_MASK;
+		else
+			config_reg &= ~GQSPI_CFG_CLK_PHA_MASK;
+		if (mode & SPI_CPOL)
+			config_reg |= GQSPI_CFG_CLK_POL_MASK;
+		else
+			config_reg &= ~GQSPI_CFG_CLK_POL_MASK;
+		zynqmp_gqspi_write(xqspi, GQSPI_CONFIG_OFST, config_reg);
+	}
+
+	dev_dbg(xqspi->dev, "config speed %u mode %x\n", req_speed_hz, mode);
 	return 0;
 }
 
@@ -1091,7 +1106,7 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
 		op->dummy.buswidth, op->data.buswidth);
 
 	mutex_lock(&xqspi->op_lock);
-	zynqmp_qspi_config_op(xqspi, mem->spi->max_speed_hz);
+	zynqmp_qspi_config_op(xqspi, mem->spi->max_speed_hz, mem->spi->mode);
 	zynqmp_qspi_chipselect(mem->spi, false);
 	genfifoentry |= xqspi->genfifocs;
 	genfifoentry |= xqspi->genfifobus;
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 4/7] spi: zynqmp-gqspi: Refactor out controller initialization

[Sean Anderson]

In preparation for having multiple SPI busses, refactor out the
controller initialization into a separate function. No functional change
intended.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/spi/spi-zynqmp-gqspi.c | 42 +++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index a1233897dc88..d78e114e17e0 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1253,6 +1253,29 @@ static const struct spi_controller_mem_ops zynqmp_qspi_mem_ops = {
 	.exec_op = zynqmp_qspi_exec_op,
 };
 
+static int zynqmp_qspi_register_ctlr(struct zynqmp_qspi *xqspi,
+				     struct spi_controller *ctlr)
+{
+	int ret;
+
+	if (!ctlr)
+		return 0;
+
+	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
+		SPI_TX_DUAL | SPI_TX_QUAD;
+	ctlr->max_speed_hz = xqspi->speed_hz;
+	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
+	ctlr->mem_ops = &zynqmp_qspi_mem_ops;
+	ctlr->setup = zynqmp_qspi_setup_op;
+	ctlr->auto_runtime_pm = true;
+
+	ret = devm_spi_register_controller(xqspi->dev, ctlr);
+	if (ret)
+		dev_err_probe(xqspi->dev, ret, "could not register %pOF\n",
+			      ctlr->dev.of_node);
+	return ret;
+}
+
 /**
  * zynqmp_qspi_probe - Probe method for the QSPI driver
  * @pdev:	Pointer to the platform_device structure
@@ -1329,12 +1352,8 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 		goto clk_dis_all;
 	}
 
-	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
-		SPI_TX_DUAL | SPI_TX_QUAD;
-	ctlr->max_speed_hz = clk_get_rate(xqspi->refclk) / 2;
-	xqspi->speed_hz = ctlr->max_speed_hz;
-
 	/* QSPI controller initializations */
+	xqspi->speed_hz = clk_get_rate(xqspi->refclk) / 2;
 	ret = zynqmp_qspi_init_hw(xqspi);
 	if (ret)
 		goto clk_dis_all;
@@ -1368,18 +1387,9 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 		ctlr->num_chipselect = num_cs;
 	}
 
-	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
-	ctlr->mem_ops = &zynqmp_qspi_mem_ops;
-	ctlr->setup = zynqmp_qspi_setup_op;
-	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
-	ctlr->dev.of_node = np;
-	ctlr->auto_runtime_pm = true;
-
-	ret = devm_spi_register_controller(&pdev->dev, ctlr);
-	if (ret) {
-		dev_err(&pdev->dev, "spi_register_controller failed\n");
+	ret = zynqmp_qspi_register_ctlr(xqspi, ctlr);
+	if (ret)
 		goto clk_dis_all;
-	}
 
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 5/7] spi: zynqmp-gqspi: Split the bus

[Sean Anderson]

This device supports two separate SPI busses: "lower" (SPI0) and "upper"
(SPI1). Each SPI bus has separate clock and data lines, as well as a
hardware-controlled chip select. The busses may be driven independently,
with only one bus active at a time, or in concert, with both busses
active. If both busses are driven at once, data may either be duplicated
on each bus or striped (bitwise) across both busses.

The current driver does not model this situation. It exposes one bus,
where CS 0 uses the lower bus and the lower chip select, and CS 1 uses
the upper bus and the upper chip select. It is not possible to use the
upper chip select with the lower bus (or vice versa). GPIO chip selects
are unsupported, and there would be no way to specify which bus to use
if they were.

To conserve pins, designers may wish to place multiple devices on a
single SPI bus. Add support for this by splitting the "merged" bus into
an upper and lower bus. Each bus uses a separate devicetree node and has
a single native chipselect 0. If "lower" and "upper" nodes are absent
from the devicetree, we register the merged bus instead, which maintains
the current behavior.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/spi/spi-zynqmp-gqspi.c | 155 ++++++++++++++++++++++++++-------
 1 file changed, 125 insertions(+), 30 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index d78e114e17e0..9823d710c4d6 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -167,6 +167,10 @@ struct qspi_platform_data {
 
 /**
  * struct zynqmp_qspi - Defines qspi driver instance
+ * @lower		Pointer to "lower" SPI bus
+ * @upper		Pointer to "upper" SPI bus
+ * @merged		Pointer to legacy SPI bus which is a combination of
+ * 			@lower and @upper
  * @ctlr:		Pointer to the spi controller information
  * @regs:		Virtual address of the QSPI controller registers
  * @refclk:		Pointer to the peripheral clock
@@ -191,7 +195,7 @@ struct qspi_platform_data {
  * @has_tapdelay:	Used for tapdelay register available in qspi
  */
 struct zynqmp_qspi {
-	struct spi_controller *ctlr;
+	struct spi_controller *lower, *upper, *merged;
 	void __iomem *regs;
 	struct clk *refclk;
 	struct clk *pclk;
@@ -467,20 +471,33 @@ static void zynqmp_qspi_copy_read_data(struct zynqmp_qspi *xqspi,
  */
 static void zynqmp_qspi_chipselect(struct spi_device *qspi, bool is_high)
 {
-	struct zynqmp_qspi *xqspi = spi_controller_get_devdata(qspi->controller);
+	struct spi_controller *ctlr = qspi->controller;
+	struct zynqmp_qspi *xqspi = spi_controller_get_devdata(ctlr);
 	ulong timeout;
 	u32 genfifoentry = 0, statusreg;
 
 	genfifoentry |= GQSPI_GENFIFO_MODE_SPI;
 
 	if (!is_high) {
-		if (!spi_get_chipselect(qspi, 0)) {
-			xqspi->genfifobus = GQSPI_GENFIFO_BUS_LOWER;
-			xqspi->genfifocs = GQSPI_GENFIFO_CS_LOWER;
+		bool upper;
+
+		if (ctlr == xqspi->lower) {
+			upper = false;
+		} else if (ctlr == xqspi->upper) {
+			upper = true;
 		} else {
+			WARN_ON_ONCE(ctlr != xqspi->merged);
+			upper = spi_get_chipselect(qspi, 0);
+		}
+
+		if (upper) {
 			xqspi->genfifobus = GQSPI_GENFIFO_BUS_UPPER;
 			xqspi->genfifocs = GQSPI_GENFIFO_CS_UPPER;
+		} else {
+			xqspi->genfifobus = GQSPI_GENFIFO_BUS_LOWER;
+			xqspi->genfifocs = GQSPI_GENFIFO_CS_LOWER;
 		}
+
 		genfifoentry |= xqspi->genfifobus;
 		genfifoentry |= xqspi->genfifocs;
 		genfifoentry |= GQSPI_GENFIFO_CS_SETUP;
@@ -962,12 +979,28 @@ static int zynqmp_qspi_read_op(struct zynqmp_qspi *xqspi, u8 rx_nbits,
 static int __maybe_unused zynqmp_qspi_suspend(struct device *dev)
 {
 	struct zynqmp_qspi *xqspi = dev_get_drvdata(dev);
-	struct spi_controller *ctlr = xqspi->ctlr;
 	int ret;
 
-	ret = spi_controller_suspend(ctlr);
-	if (ret)
-		return ret;
+	if (xqspi->merged) {
+		ret = spi_controller_suspend(xqspi->merged);
+		if (ret)
+			return ret;
+	} else {
+		if (xqspi->lower) {
+			ret = spi_controller_suspend(xqspi->lower);
+			if (ret)
+				return ret;
+		}
+
+		if (xqspi->upper) {
+			ret = spi_controller_suspend(xqspi->upper);
+			if (ret) {
+				if (xqspi->lower)
+					spi_controller_resume(xqspi->lower);
+				return ret;
+			}
+		}
+	}
 
 	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, 0x0);
 
@@ -986,13 +1019,18 @@ static int __maybe_unused zynqmp_qspi_suspend(struct device *dev)
 static int __maybe_unused zynqmp_qspi_resume(struct device *dev)
 {
 	struct zynqmp_qspi *xqspi = dev_get_drvdata(dev);
-	struct spi_controller *ctlr = xqspi->ctlr;
+	int ret = 0;
 
 	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, GQSPI_EN_MASK);
 
-	spi_controller_resume(ctlr);
+	if (xqspi->merged)
+		ret = spi_controller_resume(xqspi->merged);
+	if (xqspi->lower)
+		ret = spi_controller_resume(xqspi->lower) ?: ret;
+	if (xqspi->upper)
+		ret = spi_controller_resume(xqspi->upper) ?: ret;
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -1253,6 +1291,41 @@ static const struct spi_controller_mem_ops zynqmp_qspi_mem_ops = {
 	.exec_op = zynqmp_qspi_exec_op,
 };
 
+static void zynqmp_qspi_release_node(void *of_node)
+{
+	of_node_put(of_node);
+}
+
+static struct spi_controller *
+zynqmp_qspi_alloc_split(struct zynqmp_qspi *xqspi, const char *name)
+{
+	struct spi_controller *ctlr;
+	struct device_node *np;
+	u32 num_cs;
+	int err;
+
+	np = of_get_child_by_name(xqspi->dev->of_node, name);
+	if (!np)
+		return NULL;
+
+	err = devm_add_action_or_reset(xqspi->dev, zynqmp_qspi_release_node,
+				       np);
+	if (err)
+		return ERR_PTR(err);
+
+	ctlr = devm_spi_alloc_host(xqspi->dev, 0);
+	if (!ctlr)
+		return ERR_PTR(-ENOMEM);
+
+	ctlr->dev.of_node = np;
+	if (of_property_read_u32(np, "num-cs", &num_cs))
+		ctlr->num_chipselect = GQSPI_DEFAULT_NUM_CS;
+	else
+		ctlr->num_chipselect = num_cs;
+
+	return ctlr;
+}
+
 static int zynqmp_qspi_register_ctlr(struct zynqmp_qspi *xqspi,
 				     struct spi_controller *ctlr)
 {
@@ -1261,6 +1334,7 @@ static int zynqmp_qspi_register_ctlr(struct zynqmp_qspi *xqspi,
 	if (!ctlr)
 		return 0;
 
+	spi_controller_set_devdata(ctlr, xqspi);
 	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
 		SPI_TX_DUAL | SPI_TX_QUAD;
 	ctlr->max_speed_hz = xqspi->speed_hz;
@@ -1287,22 +1361,47 @@ static int zynqmp_qspi_register_ctlr(struct zynqmp_qspi *xqspi,
 static int zynqmp_qspi_probe(struct platform_device *pdev)
 {
 	int ret = 0;
-	struct spi_controller *ctlr;
 	struct zynqmp_qspi *xqspi;
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
-	u32 num_cs;
 	const struct qspi_platform_data *p_data;
 
-	ctlr = devm_spi_alloc_host(&pdev->dev, sizeof(*xqspi));
-	if (!ctlr)
+	xqspi = devm_kzalloc(dev, sizeof(*xqspi), GFP_KERNEL);
+	if (!xqspi)
 		return -ENOMEM;
 
-	xqspi = spi_controller_get_devdata(ctlr);
 	xqspi->dev = dev;
-	xqspi->ctlr = ctlr;
 	platform_set_drvdata(pdev, xqspi);
 
+	xqspi->lower = zynqmp_qspi_alloc_split(xqspi, "spi-lower");
+	if (IS_ERR(xqspi->lower))
+		return PTR_ERR(xqspi->lower);
+
+	xqspi->upper = zynqmp_qspi_alloc_split(xqspi, "spi-upper");
+	if (IS_ERR(xqspi->upper))
+		return PTR_ERR(xqspi->upper);
+
+	if (!xqspi->lower && !xqspi->upper) {
+		struct spi_controller *ctlr = devm_spi_alloc_host(dev, 0);
+		u32 num_cs;
+
+		if (!ctlr)
+			return -ENOMEM;
+
+		ret = of_property_read_u32(dev->of_node, "num-cs", &num_cs);
+		if (ret < 0) {
+			ctlr->num_chipselect = GQSPI_DEFAULT_NUM_CS;
+		} else if (num_cs > GQSPI_MAX_NUM_CS) {
+			dev_err(dev, "only %d chip selects are available\n",
+				GQSPI_MAX_NUM_CS);
+			return -EINVAL;
+		} else {
+			ctlr->num_chipselect = num_cs;
+		}
+
+		ctlr->dev.of_node = dev->of_node;
+		xqspi->merged = ctlr;
+	}
+
 	p_data = of_device_get_match_data(&pdev->dev);
 	if (p_data && (p_data->quirks & QSPI_QUIRK_HAS_TAPDELAY))
 		xqspi->has_tapdelay = true;
@@ -1375,19 +1474,15 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 	if (ret)
 		goto clk_dis_all;
 
-	ret = of_property_read_u32(np, "num-cs", &num_cs);
-	if (ret < 0) {
-		ctlr->num_chipselect = GQSPI_DEFAULT_NUM_CS;
-	} else if (num_cs > GQSPI_MAX_NUM_CS) {
-		ret = -EINVAL;
-		dev_err(&pdev->dev, "only %d chip selects are available\n",
-			GQSPI_MAX_NUM_CS);
+	ret = zynqmp_qspi_register_ctlr(xqspi, xqspi->lower);
+	if (ret)
 		goto clk_dis_all;
-	} else {
-		ctlr->num_chipselect = num_cs;
-	}
 
-	ret = zynqmp_qspi_register_ctlr(xqspi, ctlr);
+	ret = zynqmp_qspi_register_ctlr(xqspi, xqspi->upper);
+	if (ret)
+		goto clk_dis_all;
+
+	ret = zynqmp_qspi_register_ctlr(xqspi, xqspi->merged);
 	if (ret)
 		goto clk_dis_all;
 
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 6/7] spi: zynqmp-gqspi: Support GPIO chip selects

[Sean Anderson]

GPIO chipselects use the traditional SPU API instead of the SPIMEM API.
Implement it with transfer_one and set_cs (for non-GPIO chipselects). At
the moment we only support half-duplex transfers, which is good enough
to access SPI flashes.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/spi/spi-zynqmp-gqspi.c | 83 ++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 9823d710c4d6..efd01e06b77a 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -528,6 +528,15 @@ static void zynqmp_qspi_chipselect(struct spi_device *qspi, bool is_high)
 		dev_err(xqspi->dev, "Chip select timed out\n");
 }
 
+static void zynqmp_qspi_set_cs(struct spi_device *qspi, bool is_high)
+{
+	struct zynqmp_qspi *xqspi = spi_controller_get_devdata(qspi->controller);
+
+	mutex_lock(&xqspi->op_lock);
+	zynqmp_qspi_chipselect(qspi, is_high);
+	mutex_unlock(&xqspi->op_lock);
+}
+
 /**
  * zynqmp_qspi_selectspimode - Selects SPI mode - x1 or x2 or x4.
  * @xqspi:	xqspi is a pointer to the GQSPI instance
@@ -1271,6 +1280,75 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
 	return err;
 }
 
+static int zynqmp_qspi_transfer_one(struct spi_controller *ctlr,
+				    struct spi_device *spi,
+				    struct spi_transfer *transfer)
+{
+	struct zynqmp_qspi *xqspi = spi_controller_get_devdata(ctlr);
+	unsigned long timeout;
+	u32 genfifoentry;
+	u32 mask = 0;
+	int ret;
+
+	dev_dbg(xqspi->dev, "xfer %u/%u %u\n", transfer->tx_nbits,
+		transfer->rx_nbits, transfer->len);
+
+	if (transfer->tx_nbits && transfer->rx_nbits)
+		return -EOPNOTSUPP;
+
+	guard(mutex)(&xqspi->op_lock);
+	zynqmp_qspi_config_op(xqspi, transfer->speed_hz, spi->mode);
+	if (spi_get_csgpiod(spi, 0)) {
+		if (ctlr == xqspi->lower) {
+			xqspi->genfifobus = GQSPI_GENFIFO_BUS_LOWER;
+		} else {
+			WARN_ON_ONCE(ctlr != xqspi->upper);
+			xqspi->genfifobus = GQSPI_GENFIFO_BUS_UPPER;
+		}
+		xqspi->genfifocs = 0;
+	}
+	genfifoentry = xqspi->genfifocs | xqspi->genfifobus;
+
+	reinit_completion(&xqspi->data_completion);
+	if (transfer->tx_nbits) {
+		xqspi->txbuf = transfer->tx_buf;
+		xqspi->rxbuf = NULL;
+		xqspi->bytes_to_transfer = transfer->len;
+		xqspi->bytes_to_receive = 0;
+		zynqmp_qspi_write_op(xqspi, transfer->tx_nbits, genfifoentry);
+		mask = GQSPI_IER_TXEMPTY_MASK | GQSPI_IER_GENFIFOEMPTY_MASK |
+		       GQSPI_IER_TXNOT_FULL_MASK;
+		timeout = zynqmp_qspi_timeout(xqspi, transfer->tx_nbits,
+					      transfer->len);
+	} else {
+		xqspi->txbuf = NULL;
+		xqspi->rxbuf = transfer->rx_buf;
+		xqspi->bytes_to_transfer = 0;
+		xqspi->bytes_to_receive = transfer->len;
+		ret = zynqmp_qspi_read_op(xqspi, transfer->rx_nbits,
+					  genfifoentry);
+		if (ret)
+			return ret;
+
+		if (xqspi->mode != GQSPI_MODE_DMA)
+			mask = GQSPI_IER_GENFIFOEMPTY_MASK |
+			       GQSPI_IER_RXNEMPTY_MASK | GQSPI_IER_RXEMPTY_MASK;
+		timeout = zynqmp_qspi_timeout(xqspi, transfer->rx_nbits,
+					      transfer->len);
+	}
+
+	zynqmp_gqspi_write(xqspi, GQSPI_CONFIG_OFST,
+			   zynqmp_gqspi_read(xqspi, GQSPI_CONFIG_OFST) |
+			   GQSPI_CFG_START_GEN_FIFO_MASK);
+	if (mask)
+		zynqmp_gqspi_write(xqspi, GQSPI_IER_OFST, mask);
+	else
+		zynqmp_gqspi_write(xqspi, GQSPI_QSPIDMA_DST_I_EN_OFST,
+				   GQSPI_QSPIDMA_DST_I_EN_DONE_MASK);
+
+	return zynqmp_qspi_wait(xqspi, timeout);
+}
+
 static const struct dev_pm_ops zynqmp_qspi_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(zynqmp_runtime_suspend,
 			   zynqmp_runtime_resume, NULL)
@@ -1318,6 +1396,7 @@ zynqmp_qspi_alloc_split(struct zynqmp_qspi *xqspi, const char *name)
 		return ERR_PTR(-ENOMEM);
 
 	ctlr->dev.of_node = np;
+	ctlr->max_native_cs = 1;
 	if (of_property_read_u32(np, "num-cs", &num_cs))
 		ctlr->num_chipselect = GQSPI_DEFAULT_NUM_CS;
 	else
@@ -1337,11 +1416,15 @@ static int zynqmp_qspi_register_ctlr(struct zynqmp_qspi *xqspi,
 	spi_controller_set_devdata(ctlr, xqspi);
 	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
 		SPI_TX_DUAL | SPI_TX_QUAD;
+	ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX;
 	ctlr->max_speed_hz = xqspi->speed_hz;
 	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
 	ctlr->mem_ops = &zynqmp_qspi_mem_ops;
 	ctlr->setup = zynqmp_qspi_setup_op;
+	ctlr->set_cs = zynqmp_qspi_set_cs;
+	ctlr->transfer_one = zynqmp_qspi_transfer_one;
 	ctlr->auto_runtime_pm = true;
+	ctlr->use_gpio_descriptors = true;
 
 	ret = devm_spi_register_controller(xqspi->dev, ctlr);
 	if (ret)
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 7/7] ARM64: xilinx: zynqmp: Convert to split QSPI bus

[Sean Anderson]

Convert the ZynqMP devicetrees to use the split QSPI bus binding. This
is pretty simple, since all boards use only CS0.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts |  5 ++++-
 arch/arm64/boot/dts/xilinx/zynqmp-zc1232-revA.dts |  5 ++++-
 arch/arm64/boot/dts/xilinx/zynqmp-zc1254-revA.dts |  5 ++++-
 .../boot/dts/xilinx/zynqmp-zc1751-xm015-dc1.dts   |  5 ++++-
 .../boot/dts/xilinx/zynqmp-zc1751-xm018-dc4.dts   |  5 ++++-
 arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts |  5 ++++-
 arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts |  5 ++++-
 arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts |  5 ++++-
 arch/arm64/boot/dts/xilinx/zynqmp-zcu106-revA.dts |  5 ++++-
 arch/arm64/boot/dts/xilinx/zynqmp-zcu111-revA.dts |  5 ++++-
 .../arm64/boot/dts/xilinx/zynqmp-zcu1275-revA.dts |  5 ++++-
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi            | 15 +++++++++++----
 12 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
index bfa7ea6b9224..64b90de5b4ce 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
@@ -35,7 +35,7 @@ aliases {
 		serial0 = &uart0;
 		serial1 = &uart1;
 		serial2 = &dcc;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 		spi1 = &spi0;
 		spi2 = &spi1;
 		usb0 = &usb0;
@@ -129,6 +129,9 @@ mux {
 
 &qspi { /* MIO 0-5 - U143 */
 	status = "okay";
+};
+
+&qspi_lower {
 	spi_flash: flash@0 { /* MT25QU512A */
 		compatible = "jedec,spi-nor"; /* 64MB */
 		reg = <0>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zc1232-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zc1232-revA.dts
index 04079d1704f1..8927e0463cf4 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zc1232-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zc1232-revA.dts
@@ -19,7 +19,7 @@ / {
 	aliases {
 		serial0 = &uart0;
 		serial1 = &dcc;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 	};
 
 	chosen {
@@ -39,6 +39,9 @@ &dcc {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor"; /* 32MB */
 		#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zc1254-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zc1254-revA.dts
index 3dec57cf18be..da07b58706f0 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zc1254-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zc1254-revA.dts
@@ -20,7 +20,7 @@ / {
 	aliases {
 		serial0 = &uart0;
 		serial1 = &dcc;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 	};
 
 	chosen {
@@ -40,6 +40,9 @@ &dcc {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor"; /* 32MB */
 		#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm015-dc1.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm015-dc1.dts
index 6aff22d43361..ec570d68a4ae 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm015-dc1.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm015-dc1.dts
@@ -27,7 +27,7 @@ aliases {
 		mmc1 = &sdhci1;
 		rtc0 = &rtc;
 		serial0 = &uart0;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 		usb0 = &usb0;
 	};
 
@@ -354,6 +354,9 @@ &psgtr {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor"; /* Micron MT25QU512ABB8ESF */
 		#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm018-dc4.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm018-dc4.dts
index 6ec1d9813973..e1cfdc0db51e 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm018-dc4.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm018-dc4.dts
@@ -26,7 +26,7 @@ aliases {
 		rtc0 = &rtc;
 		serial0 = &uart0;
 		serial1 = &uart1;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 	};
 
 	chosen {
@@ -172,6 +172,9 @@ &i2c1 {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor"; /* 32MB */
 		#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts
index 7e26489a1539..18e323e2aad7 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts
@@ -31,7 +31,7 @@ aliases {
 		serial0 = &uart0;
 		serial1 = &uart1;
 		serial2 = &dcc;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 		usb0 = &usb0;
 	};
 
@@ -953,6 +953,9 @@ &psgtr {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor"; /* 16MB + 16MB */
 		#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts
index eb2090673ec1..026053c4116a 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts
@@ -29,7 +29,7 @@ aliases {
 		serial0 = &uart0;
 		serial1 = &uart1;
 		serial2 = &dcc;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 		usb0 = &usb0;
 	};
 
@@ -439,6 +439,9 @@ &psgtr {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor"; /* n25q512a 128MiB */
 		#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts
index 4694d0a841f1..da56e532dc2b 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts
@@ -29,7 +29,7 @@ aliases {
 		serial0 = &uart0;
 		serial1 = &uart1;
 		serial2 = &dcc;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 		usb0 = &usb0;
 	};
 
@@ -451,6 +451,9 @@ &psgtr {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor"; /* n25q512a 128MiB */
 		#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu106-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu106-revA.dts
index 7beedd730f94..8dd73b035969 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu106-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu106-revA.dts
@@ -31,7 +31,7 @@ aliases {
 		serial0 = &uart0;
 		serial1 = &uart1;
 		serial2 = &dcc;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 		usb0 = &usb0;
 	};
 
@@ -959,6 +959,9 @@ &psgtr {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor"; /* 16MB + 16MB */
 		#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu111-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu111-revA.dts
index b67ff7ecf3c3..9ed7972c3b4e 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu111-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu111-revA.dts
@@ -30,7 +30,7 @@ aliases {
 		rtc0 = &rtc;
 		serial0 = &uart0;
 		serial1 = &dcc;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 		usb0 = &usb0;
 	};
 
@@ -789,6 +789,9 @@ &psgtr {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor"; /* 16MB + 16MB */
 		#address-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu1275-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu1275-revA.dts
index a38c2baeba6c..99d007b3bfae 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu1275-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu1275-revA.dts
@@ -20,7 +20,7 @@ / {
 	aliases {
 		serial0 = &uart0;
 		serial1 = &dcc;
-		spi0 = &qspi;
+		spi0 = &qspi_lower;
 	};
 
 	chosen {
@@ -44,6 +44,9 @@ &gpio {
 
 &qspi {
 	status = "okay";
+};
+
+&qspi_lower {
 	flash@0 {
 		compatible = "m25p80", "jedec,spi-nor";
 		reg = <0x0>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 5dac0542a48d..470e0b90382f 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -972,21 +972,28 @@ pcie_intc: legacy-interrupt-controller {
 			};
 		};
 
-		qspi: spi@ff0f0000 {
+		qspi: spi-controller@ff0f0000 {
 			bootph-all;
 			compatible = "xlnx,zynqmp-qspi-1.0";
 			status = "disabled";
 			clock-names = "ref_clk", "pclk";
 			interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-parent = <&gic>;
-			num-cs = <1>;
 			reg = <0x0 0xff0f0000 0x0 0x1000>,
 			      <0x0 0xc0000000 0x0 0x8000000>;
-			#address-cells = <1>;
-			#size-cells = <0>;
 			/* iommus = <&smmu 0x873>; */
 			power-domains = <&zynqmp_firmware PD_QSPI>;
 			resets = <&zynqmp_reset ZYNQMP_RESET_QSPI>;
+
+			qspi_lower: spi-lower {
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			qspi_upper: spi-upper {
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
 		};
 
 		psgtr: phy@fd400000 {
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 0/7] spi: zynqmp-gqspi: Split the bus and add GPIO support

[Sean Anderson]

On 1/16/25 18:21, Sean Anderson wrote:
> As discussed in patches 1 and FIXME, this device really has two SPI

This should be "patches 1 and 5" (oops)

> busses. Split the existing "merged" bus in twain, and add support for
> GPIO chipselects.

RE: [PATCH 5/7] spi: zynqmp-gqspi: Split the bus

[Mahapatra, Amit Kumar]

Hello Andreson,

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Friday, January 17, 2025 4:51 AM
> To: Mark Brown <broonie@kernel.org>; Simek, Michal <michal.simek@amd.com>;
> linux-spi@vger.kernel.org
> Cc: Jinjie Ruan <ruanjinjie@huawei.com>; linux-arm-kernel@lists.infradead.org;
> Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; linux-
> kernel@vger.kernel.org; Miquel Raynal <miquel.raynal@bootlin.com>; Sean
> Anderson <sean.anderson@linux.dev>
> Subject: [PATCH 5/7] spi: zynqmp-gqspi: Split the bus
> 
> This device supports two separate SPI busses: "lower" (SPI0) and "upper"
> (SPI1). Each SPI bus has separate clock and data lines, as well as a hardware-
> controlled chip select. The busses may be driven independently, with only one bus
> active at a time, or in concert, with both busses active. If both busses are driven at
> once, data may either be duplicated on each bus or striped (bitwise) across both
> busses.
> 
> The current driver does not model this situation. It exposes one bus, where CS 0
> uses the lower bus and the lower chip select, and CS 1 uses the upper bus and the
> upper chip select. It is not possible to use the upper chip select with the lower bus
> (or vice versa). GPIO chip selects are unsupported, and there would be no way to
> specify which bus to use if they were.
> 
> To conserve pins, designers may wish to place multiple devices on a single SPI bus.
> Add support for this by splitting the "merged" bus into an upper and lower bus. Each
> bus uses a separate devicetree node and has a single native chipselect 0. If "lower"

IMHO, restricting users to fixed names is not ideal. A better approach would be to 
introduce a Device Tree (DT) property for the bus number and select the bus 
accordingly.

Regards,
Amit
 
> and "upper" nodes are absent from the devicetree, we register the merged bus
> instead, which maintains the current behavior.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  drivers/spi/spi-zynqmp-gqspi.c | 155 ++++++++++++++++++++++++++-------
>  1 file changed, 125 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c index
> d78e114e17e0..9823d710c4d6 100644
> --- a/drivers/spi/spi-zynqmp-gqspi.c
> +++ b/drivers/spi/spi-zynqmp-gqspi.c
> @@ -167,6 +167,10 @@ struct qspi_platform_data {
> 
>  /**
>   * struct zynqmp_qspi - Defines qspi driver instance
> + * @lower		Pointer to "lower" SPI bus
> + * @upper		Pointer to "upper" SPI bus
> + * @merged		Pointer to legacy SPI bus which is a combination of
> + * 			@lower and @upper
>   * @ctlr:		Pointer to the spi controller information
>   * @regs:		Virtual address of the QSPI controller registers
>   * @refclk:		Pointer to the peripheral clock
> @@ -191,7 +195,7 @@ struct qspi_platform_data {
>   * @has_tapdelay:	Used for tapdelay register available in qspi
>   */
>  struct zynqmp_qspi {
> -	struct spi_controller *ctlr;
> +	struct spi_controller *lower, *upper, *merged;
>  	void __iomem *regs;
>  	struct clk *refclk;
>  	struct clk *pclk;
> @@ -467,20 +471,33 @@ static void zynqmp_qspi_copy_read_data(struct
> zynqmp_qspi *xqspi,
>   */
>  static void zynqmp_qspi_chipselect(struct spi_device *qspi, bool is_high)  {
> -	struct zynqmp_qspi *xqspi = spi_controller_get_devdata(qspi->controller);
> +	struct spi_controller *ctlr = qspi->controller;
> +	struct zynqmp_qspi *xqspi = spi_controller_get_devdata(ctlr);
>  	ulong timeout;
>  	u32 genfifoentry = 0, statusreg;
> 
>  	genfifoentry |= GQSPI_GENFIFO_MODE_SPI;
> 
>  	if (!is_high) {
> -		if (!spi_get_chipselect(qspi, 0)) {
> -			xqspi->genfifobus = GQSPI_GENFIFO_BUS_LOWER;
> -			xqspi->genfifocs = GQSPI_GENFIFO_CS_LOWER;
> +		bool upper;
> +
> +		if (ctlr == xqspi->lower) {
> +			upper = false;
> +		} else if (ctlr == xqspi->upper) {
> +			upper = true;
>  		} else {
> +			WARN_ON_ONCE(ctlr != xqspi->merged);
> +			upper = spi_get_chipselect(qspi, 0);
> +		}
> +
> +		if (upper) {
>  			xqspi->genfifobus = GQSPI_GENFIFO_BUS_UPPER;
>  			xqspi->genfifocs = GQSPI_GENFIFO_CS_UPPER;
> +		} else {
> +			xqspi->genfifobus = GQSPI_GENFIFO_BUS_LOWER;
> +			xqspi->genfifocs = GQSPI_GENFIFO_CS_LOWER;
>  		}
> +
>  		genfifoentry |= xqspi->genfifobus;
>  		genfifoentry |= xqspi->genfifocs;
>  		genfifoentry |= GQSPI_GENFIFO_CS_SETUP; @@ -962,12
> +979,28 @@ static int zynqmp_qspi_read_op(struct zynqmp_qspi *xqspi, u8
> rx_nbits,  static int __maybe_unused zynqmp_qspi_suspend(struct device *dev)  {
>  	struct zynqmp_qspi *xqspi = dev_get_drvdata(dev);
> -	struct spi_controller *ctlr = xqspi->ctlr;
>  	int ret;
> 
> -	ret = spi_controller_suspend(ctlr);
> -	if (ret)
> -		return ret;
> +	if (xqspi->merged) {
> +		ret = spi_controller_suspend(xqspi->merged);
> +		if (ret)
> +			return ret;
> +	} else {
> +		if (xqspi->lower) {
> +			ret = spi_controller_suspend(xqspi->lower);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		if (xqspi->upper) {
> +			ret = spi_controller_suspend(xqspi->upper);
> +			if (ret) {
> +				if (xqspi->lower)
> +					spi_controller_resume(xqspi->lower);
> +				return ret;
> +			}
> +		}
> +	}
> 
>  	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, 0x0);
> 
> @@ -986,13 +1019,18 @@ static int __maybe_unused
> zynqmp_qspi_suspend(struct device *dev)  static int __maybe_unused
> zynqmp_qspi_resume(struct device *dev)  {
>  	struct zynqmp_qspi *xqspi = dev_get_drvdata(dev);
> -	struct spi_controller *ctlr = xqspi->ctlr;
> +	int ret = 0;
> 
>  	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, GQSPI_EN_MASK);
> 
> -	spi_controller_resume(ctlr);
> +	if (xqspi->merged)
> +		ret = spi_controller_resume(xqspi->merged);
> +	if (xqspi->lower)
> +		ret = spi_controller_resume(xqspi->lower) ?: ret;
> +	if (xqspi->upper)
> +		ret = spi_controller_resume(xqspi->upper) ?: ret;
> 
> -	return 0;
> +	return ret;
>  }
> 
>  /**
> @@ -1253,6 +1291,41 @@ static const struct spi_controller_mem_ops
> zynqmp_qspi_mem_ops = {
>  	.exec_op = zynqmp_qspi_exec_op,
>  };
> 
> +static void zynqmp_qspi_release_node(void *of_node) {
> +	of_node_put(of_node);
> +}
> +
> +static struct spi_controller *
> +zynqmp_qspi_alloc_split(struct zynqmp_qspi *xqspi, const char *name) {
> +	struct spi_controller *ctlr;
> +	struct device_node *np;
> +	u32 num_cs;
> +	int err;
> +
> +	np = of_get_child_by_name(xqspi->dev->of_node, name);
> +	if (!np)
> +		return NULL;
> +
> +	err = devm_add_action_or_reset(xqspi->dev, zynqmp_qspi_release_node,
> +				       np);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	ctlr = devm_spi_alloc_host(xqspi->dev, 0);
> +	if (!ctlr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ctlr->dev.of_node = np;
> +	if (of_property_read_u32(np, "num-cs", &num_cs))
> +		ctlr->num_chipselect = GQSPI_DEFAULT_NUM_CS;
> +	else
> +		ctlr->num_chipselect = num_cs;
> +
> +	return ctlr;
> +}
> +
>  static int zynqmp_qspi_register_ctlr(struct zynqmp_qspi *xqspi,
>  				     struct spi_controller *ctlr)
>  {
> @@ -1261,6 +1334,7 @@ static int zynqmp_qspi_register_ctlr(struct zynqmp_qspi
> *xqspi,
>  	if (!ctlr)
>  		return 0;
> 
> +	spi_controller_set_devdata(ctlr, xqspi);
>  	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL |
> SPI_RX_QUAD |
>  		SPI_TX_DUAL | SPI_TX_QUAD;
>  	ctlr->max_speed_hz = xqspi->speed_hz;
> @@ -1287,22 +1361,47 @@ static int zynqmp_qspi_register_ctlr(struct
> zynqmp_qspi *xqspi,  static int zynqmp_qspi_probe(struct platform_device *pdev)  {
>  	int ret = 0;
> -	struct spi_controller *ctlr;
>  	struct zynqmp_qspi *xqspi;
>  	struct device *dev = &pdev->dev;
> -	struct device_node *np = dev->of_node;
> -	u32 num_cs;
>  	const struct qspi_platform_data *p_data;
> 
> -	ctlr = devm_spi_alloc_host(&pdev->dev, sizeof(*xqspi));
> -	if (!ctlr)
> +	xqspi = devm_kzalloc(dev, sizeof(*xqspi), GFP_KERNEL);
> +	if (!xqspi)
>  		return -ENOMEM;
> 
> -	xqspi = spi_controller_get_devdata(ctlr);
>  	xqspi->dev = dev;
> -	xqspi->ctlr = ctlr;
>  	platform_set_drvdata(pdev, xqspi);
> 
> +	xqspi->lower = zynqmp_qspi_alloc_split(xqspi, "spi-lower");
> +	if (IS_ERR(xqspi->lower))
> +		return PTR_ERR(xqspi->lower);
> +
> +	xqspi->upper = zynqmp_qspi_alloc_split(xqspi, "spi-upper");
> +	if (IS_ERR(xqspi->upper))
> +		return PTR_ERR(xqspi->upper);
> +
> +	if (!xqspi->lower && !xqspi->upper) {
> +		struct spi_controller *ctlr = devm_spi_alloc_host(dev, 0);
> +		u32 num_cs;
> +
> +		if (!ctlr)
> +			return -ENOMEM;
> +
> +		ret = of_property_read_u32(dev->of_node, "num-cs", &num_cs);
> +		if (ret < 0) {
> +			ctlr->num_chipselect = GQSPI_DEFAULT_NUM_CS;
> +		} else if (num_cs > GQSPI_MAX_NUM_CS) {
> +			dev_err(dev, "only %d chip selects are available\n",
> +				GQSPI_MAX_NUM_CS);
> +			return -EINVAL;
> +		} else {
> +			ctlr->num_chipselect = num_cs;
> +		}
> +
> +		ctlr->dev.of_node = dev->of_node;
> +		xqspi->merged = ctlr;
> +	}
> +
>  	p_data = of_device_get_match_data(&pdev->dev);
>  	if (p_data && (p_data->quirks & QSPI_QUIRK_HAS_TAPDELAY))
>  		xqspi->has_tapdelay = true;
> @@ -1375,19 +1474,15 @@ static int zynqmp_qspi_probe(struct platform_device
> *pdev)
>  	if (ret)
>  		goto clk_dis_all;
> 
> -	ret = of_property_read_u32(np, "num-cs", &num_cs);
> -	if (ret < 0) {
> -		ctlr->num_chipselect = GQSPI_DEFAULT_NUM_CS;
> -	} else if (num_cs > GQSPI_MAX_NUM_CS) {
> -		ret = -EINVAL;
> -		dev_err(&pdev->dev, "only %d chip selects are available\n",
> -			GQSPI_MAX_NUM_CS);
> +	ret = zynqmp_qspi_register_ctlr(xqspi, xqspi->lower);
> +	if (ret)
>  		goto clk_dis_all;
> -	} else {
> -		ctlr->num_chipselect = num_cs;
> -	}
> 
> -	ret = zynqmp_qspi_register_ctlr(xqspi, ctlr);
> +	ret = zynqmp_qspi_register_ctlr(xqspi, xqspi->upper);
> +	if (ret)
> +		goto clk_dis_all;
> +
> +	ret = zynqmp_qspi_register_ctlr(xqspi, xqspi->merged);
>  	if (ret)
>  		goto clk_dis_all;
> 
> --
> 2.35.1.1320.gc452695387.dirty

Re: [PATCH 5/7] spi: zynqmp-gqspi: Split the bus

[Sean Anderson]

On 1/21/25 08:19, Mahapatra, Amit Kumar wrote:
> Hello Andreson,
> 
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Friday, January 17, 2025 4:51 AM
>> To: Mark Brown <broonie@kernel.org>; Simek, Michal <michal.simek@amd.com>;
>> linux-spi@vger.kernel.org
>> Cc: Jinjie Ruan <ruanjinjie@huawei.com>; linux-arm-kernel@lists.infradead.org;
>> Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; linux-
>> kernel@vger.kernel.org; Miquel Raynal <miquel.raynal@bootlin.com>; Sean
>> Anderson <sean.anderson@linux.dev>
>> Subject: [PATCH 5/7] spi: zynqmp-gqspi: Split the bus
>> 
>> This device supports two separate SPI busses: "lower" (SPI0) and "upper"
>> (SPI1). Each SPI bus has separate clock and data lines, as well as a hardware-
>> controlled chip select. The busses may be driven independently, with only one bus
>> active at a time, or in concert, with both busses active. If both busses are driven at
>> once, data may either be duplicated on each bus or striped (bitwise) across both
>> busses.
>> 
>> The current driver does not model this situation. It exposes one bus, where CS 0
>> uses the lower bus and the lower chip select, and CS 1 uses the upper bus and the
>> upper chip select. It is not possible to use the upper chip select with the lower bus
>> (or vice versa). GPIO chip selects are unsupported, and there would be no way to
>> specify which bus to use if they were.
>> 
>> To conserve pins, designers may wish to place multiple devices on a single SPI bus.
>> Add support for this by splitting the "merged" bus into an upper and lower bus. Each
>> bus uses a separate devicetree node and has a single native chipselect 0. If "lower"
> 
> IMHO, restricting users to fixed names is not ideal. A better approach would be to 
> introduce a Device Tree (DT) property for the bus number and select the bus 
> accordingly.

Why? It's not an artificial restriction; it reflects the hardware. And this is how
SPI busses are typically represented. If you have two SPI busses, there should be
two devicetree nodes.

--Sean

Re: [PATCH 5/7] spi: zynqmp-gqspi: Split the bus

[Mark Brown]

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

On Tue, Jan 21, 2025 at 10:53:53AM -0500, Sean Anderson wrote:
> On 1/21/25 08:19, Mahapatra, Amit Kumar wrote:

> > IMHO, restricting users to fixed names is not ideal. A better approach would be to 
> > introduce a Device Tree (DT) property for the bus number and select the bus 
> > accordingly.

> Why? It's not an artificial restriction; it reflects the hardware. And this is how
> SPI busses are typically represented. If you have two SPI busses, there should be
> two devicetree nodes.

Perhaps the thing is more that the buses are named instead of numbered?

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

Re: [PATCH 5/7] spi: zynqmp-gqspi: Split the bus

[Sean Anderson]

On 1/21/25 11:01, Mark Brown wrote:
> On Tue, Jan 21, 2025 at 10:53:53AM -0500, Sean Anderson wrote:
>> On 1/21/25 08:19, Mahapatra, Amit Kumar wrote:
> 
>> > IMHO, restricting users to fixed names is not ideal. A better approach would be to 
>> > introduce a Device Tree (DT) property for the bus number and select the bus 
>> > accordingly.
> 
>> Why? It's not an artificial restriction; it reflects the hardware. And this is how
>> SPI busses are typically represented. If you have two SPI busses, there should be
>> two devicetree nodes.
> 
> Perhaps the thing is more that the buses are named instead of numbered?

Well, we could number them, but it's not like they have different
addresses in memory. They use all the same registers except for a bit in
the genfifo. So it doesn't make sense to do address translation (e.g.
with reg/ranges).

As for the names, these are the names used by the documentation, as well
as the exising driver. There are some references to SPI0/SPI1, but there
are more to upper/lower. If you want to bikeshed spi-0/spi-1 instead of
spi-lower/spi-upper, be my guest.

--Sean

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[David Lechner]

On 1/16/25 5:21 PM, Sean Anderson wrote:
> This device supports two separate SPI busses: 

...

> @@ -84,5 +94,32 @@ examples:
>          resets = <&zynqmp_reset ZYNQMP_RESET_QSPI>;
>          reg = <0x0 0xff0f0000 0x0 0x1000>,
>                <0x0 0xc0000000 0x0 0x8000000>;
> +
> +        spi-lower {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          num-cs = <2>;
> +          cs-gpios = <0>, <&gpio 5>;
> +
> +          flash@0 {
> +            reg = <0>;
> +            compatible = "jedec,spi-nor";
> +          };
> +
> +          flash@1 {
> +            reg = <1>;
> +            compatible = "jedec,spi-nor";
> +          };
> +        };
> +
> +        spi-upper {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          flash@0 {
> +            reg = <0>;
> +            compatible = "jedec,spi-nor";
> +          };
> +        };
>        };
>      };

In the IIO subsystem, we've been recently working on several "advanced" ADCs
that could use a SPI controller like this. These ADCs have multiple input
channels that perform conversions in parallel and the data for each channel
needs to be read back on a separate serial line (MISO) at the same time. Another
similar case is to have two separate chips, but they share a conversion trigger
and essentially operate as a single composite device rather than two distinct
devices [1]. This would be similar to some ADCs that are daisy-chained where we
consider all of the chips in the chain as a single composite device, but they
would be in parallel rather than chained.

[1]: https://lore.kernel.org/linux-iio/e5e8eba7-2455-488b-a36f-e246844e11fd@baylibre.com/

For those use cases though, as mentioned above, we only have a single device
that would be connected to both buses. So for such a SPI controller with
multiple buses, I was envisioning that instead of adding child nodes for each
of the child buses, that we would do something like add a spi-buses property
to the spi peripheral bindings where you could specify one or more buses that
a device was connected to.

e.g. a device connected to the lower bus would be spi-buses = <0>; one connected
to the upper bus would be spi-buses = <1>; and a device connected to both would
be spi-buses = <0>, <1>;.  This would also work for SPI controllers that have
4 or 8 busses.

SPI controllers like these have a striping feature that allows to control both
buses at the same to either mirror the same data on both buses at the same
time when writing, e.g. for configuration or to read and write two different
bytes at the same time. A peripheral driver for device that was connected to
both buses could make use of this feature to craft a single SPI message with
transfers containing (new) parameters that specify which bus to use (one or
both) and, in the case of using both buses, to mirror or stripe the data.

Could we make a single device connected to both buses like this work using
the proposed spi-lower and spi-upper or should we consider a different binding
like the one I suggested?
 

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[Sean Anderson]

On 1/21/25 19:16, David Lechner wrote:
> On 1/16/25 5:21 PM, Sean Anderson wrote:
>> This device supports two separate SPI busses: 
> 
> ...
> 
>> @@ -84,5 +94,32 @@ examples:
>>          resets = <&zynqmp_reset ZYNQMP_RESET_QSPI>;
>>          reg = <0x0 0xff0f0000 0x0 0x1000>,
>>                <0x0 0xc0000000 0x0 0x8000000>;
>> +
>> +        spi-lower {
>> +          #address-cells = <1>;
>> +          #size-cells = <0>;
>> +          num-cs = <2>;
>> +          cs-gpios = <0>, <&gpio 5>;
>> +
>> +          flash@0 {
>> +            reg = <0>;
>> +            compatible = "jedec,spi-nor";
>> +          };
>> +
>> +          flash@1 {
>> +            reg = <1>;
>> +            compatible = "jedec,spi-nor";
>> +          };
>> +        };
>> +
>> +        spi-upper {
>> +          #address-cells = <1>;
>> +          #size-cells = <0>;
>> +
>> +          flash@0 {
>> +            reg = <0>;
>> +            compatible = "jedec,spi-nor";
>> +          };
>> +        };
>>        };
>>      };
> 
> In the IIO subsystem, we've been recently working on several "advanced" ADCs
> that could use a SPI controller like this. These ADCs have multiple input
> channels that perform conversions in parallel and the data for each channel
> needs to be read back on a separate serial line (MISO) at the same time. Another
> similar case is to have two separate chips, but they share a conversion trigger
> and essentially operate as a single composite device rather than two distinct
> devices [1]. This would be similar to some ADCs that are daisy-chained where we
> consider all of the chips in the chain as a single composite device, but they
> would be in parallel rather than chained.
> 
> [1]: https://lore.kernel.org/linux-iio/e5e8eba7-2455-488b-a36f-e246844e11fd@baylibre.com/
>
> For those use cases though, as mentioned above, we only have a single device
> that would be connected to both buses. So for such a SPI controller with
> multiple buses, I was envisioning that instead of adding child nodes for each
> of the child buses, that we would do something like add a spi-buses property
> to the spi peripheral bindings where you could specify one or more buses that
> a device was connected to.
> 
> e.g. a device connected to the lower bus would be spi-buses = <0>; one connected
> to the upper bus would be spi-buses = <1>; and a device connected to both would
> be spi-buses = <0>, <1>;.  This would also work for SPI controllers that have
> 4 or 8 busses.
> 
> SPI controllers like these have a striping feature that allows to control both
> buses at the same to either mirror the same data on both buses at the same
> time when writing, e.g. for configuration or to read and write two different
> bytes at the same time. A peripheral driver for device that was connected to
> both buses could make use of this feature to craft a single SPI message with
> transfers containing (new) parameters that specify which bus to use (one or
> both) and, in the case of using both buses, to mirror or stripe the data.
> 
> Could we make a single device connected to both buses like this work using
> the proposed spi-lower and spi-upper or should we consider a different binding
> like the one I suggested?

If you are willing to do the work to rewrite the SPI subsystem to handle
this, then I don't object to it in principle. Using a property would
also help with forwards compatibility. On the other hand, separate
busses are easier to implement since they integrate better with the SPI
subsystem (e.g. you can just call spi_register_controller to create all
the slaves).

There have been some previous patches from Xilinx to handle this
use case [1], but IMO they were pretty hacky. They got this feature
merged into U-Boot and it broke many other boards and took a lot of
cleanup to fix. So I have intentionally only tackled the unsynchronized
use case since that requires no modification to areas outside of this
driver. I don't need the "parallel" use case and I am not interested in
doing the work required to implement it.

--Sean

[1] https://lore.kernel.org/linux-spi/20221017121249.19061-1-amit.kumar-mahapatra@amd.com/

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[David Lechner]

On 1/23/25 10:24 AM, Sean Anderson wrote:
> On 1/21/25 19:16, David Lechner wrote:
>> On 1/16/25 5:21 PM, Sean Anderson wrote:

...

>> Could we make a single device connected to both buses like this work using
>> the proposed spi-lower and spi-upper or should we consider a different binding
>> like the one I suggested?
> 
> If you are willing to do the work to rewrite the SPI subsystem to handle
> this, then I don't object to it in principle. Using a property would
> also help with forwards compatibility. On the other hand, separate
> busses are easier to implement since they integrate better with the SPI
> subsystem (e.g. you can just call spi_register_controller to create all
> the slaves).
> 
> There have been some previous patches from Xilinx to handle this
> use case [1], but IMO they were pretty hacky. They got this feature
> merged into U-Boot and it broke many other boards and took a lot of
> cleanup to fix. So I have intentionally only tackled the unsynchronized
> use case since that requires no modification to areas outside of this
> driver. I don't need the "parallel" use case and I am not interested in
> doing the work required to implement it.
> 
> --Sean
> 
> [1] https://lore.kernel.org/linux-spi/20221017121249.19061-1-amit.kumar-mahapatra@amd.com/

Fair enough, and I think it can be done without breaking things like the multi
CS support did.

Here are a couple of patches. Feel free to resubmit them with your series if
they work for you. To make it work with your series, you should just need to
modify the .dts to look like this:

+          flash@0 {
+            compatible = "jedec,spi-nor";
+            reg = <0>;
+            spi-buses = <0>; /* lower */
+          };
+
+          flash@1 {
+            reg = <1>;
+            compatible = "jedec,spi-nor";
+            /* also OK to omit property in case of spi-buses = <0>; */
+          };
+
+          flash@2 {
+            reg = <2>;
+            compatible = "jedec,spi-nor";
+            spi-buses = <1>; /* upper */
+          };


Then drop patch "spi: zynqmp-gqspi: Split the bus" of course.

In zynqmp_qspi_probe(), add a line:

	ctlr->num_buses = 2;

And in the zynqmp_qspi_transfer_one() function, use spi->buses to select the
correct bus:

	xqspi->genfifobus = FIELD_PREP(GQSPI_GENFIFO_BUS_MASK, spi->buses);

I don't have a SPI controller on hand with multiple buses, so I don't have
any patch adding support to a specific controller. But I did build and run this
and hacked in some stuff to the drivers I am working on to make sure it is
working as advertised as best as I could with a single bus.


---
From: David Lechner <dlechner@baylibre.com>
Date: Thu, 23 Jan 2025 15:32:08 -0600
Subject: [PATCH 1/2] spi: dt-bindings: spi-peripheral-props: add spi-buses
 property

Add a spi-buses property to the spi-peripheral-props binding to allow
specifying the SPI bus or buses that a peripheral is connected to in
cases where the SPI controller has more than one physical SPI bus.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 .../devicetree/bindings/spi/spi-peripheral-props.yaml  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 0bb443b8decd..a69d368a8ae6 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -88,6 +88,16 @@ properties:
     description:
       Delay, in microseconds, after a write transfer.
 
+  spi-buses:
+    description:
+      Array of bus numbers that describes which SPI buses of the controller are
+      connected to the peripheral. This only applies to peripherals connected
+      to specialized SPI controllers that have multiple SPI buses on a single
+      controller.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    default: [0]
+
   stacked-memories:
     description: Several SPI memories can be wired in stacked mode.
       This basically means that either a device features several chip
-- 
2.43.0

---
From: David Lechner <dlechner@baylibre.com>
Date: Thu, 23 Jan 2025 15:35:19 -0600
Subject: [PATCH 2/2] spi: add support for multi-bus controllers

Add support for SPI controllers with multiple physical SPI buses.

This is common in the type of controller that can be used with parallel
flash memories, but can be used for general purpose SPI as well.

To indicate support, a controller just needs to set ctlr->num_buses to
something greater than 1. Peripherals indicate which bus they are
connected to via device tree (ACPI support can be added if needed).

In the future, this can be extended to support peripherals that also
have multiple SPI buses to use those buses at the same time by adding
a similar bus flags field to struct spi_transfer.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/spi/spi.c       | 26 +++++++++++++++++++++++++-
 include/linux/spi/spi.h | 13 +++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 10c365e9100a..f7722e5e906d 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2364,7 +2364,7 @@ static void of_spi_parse_dt_cs_delay(struct device_node *nc,
 static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 			   struct device_node *nc)
 {
-	u32 value, cs[SPI_CS_CNT_MAX];
+	u32 value, buses[8], cs[SPI_CS_CNT_MAX];
 	int rc, idx;
 
 	/* Mode (clock phase/polarity/etc.) */
@@ -2379,6 +2379,29 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	if (of_property_read_bool(nc, "spi-cs-high"))
 		spi->mode |= SPI_CS_HIGH;
 
+	rc = of_property_read_variable_u32_array(nc, "spi-buses", buses, 1,
+						 ARRAY_SIZE(buses));
+	if (rc < 0 && rc != -EINVAL) {
+		dev_err(&ctlr->dev, "%pOF has invalid 'spi-buses' property (%d)\n",
+			nc, rc);
+		return rc;
+	}
+
+	if (rc == -EINVAL) {
+		/* Default when property is omitted. */
+		spi->buses = BIT(0);
+	} else {
+		for (idx = 0; idx < rc; idx++) {
+			if (buses[idx] >= ctlr->num_buses) {
+				dev_err(&ctlr->dev,
+					"%pOF has out of range 'spi-buses' property (%d)\n",
+					nc, buses[idx]);
+				return -EINVAL;
+			}
+			spi->buses |= BIT(buses[idx]);
+		}
+	}
+
 	/* Device DUAL/QUAD mode */
 	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
 		switch (value) {
@@ -3072,6 +3095,7 @@ struct spi_controller *__spi_alloc_controller(struct device *dev,
 	mutex_init(&ctlr->add_lock);
 	ctlr->bus_num = -1;
 	ctlr->num_chipselect = 1;
+	ctlr->num_buses = 1;
 	ctlr->slave = slave;
 	if (IS_ENABLED(CONFIG_SPI_SLAVE) && slave)
 		ctlr->dev.class = &spi_slave_class;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4c087009cf97..bc45d70e8c45 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -187,6 +187,11 @@ struct spi_device {
 	struct device		dev;
 	struct spi_controller	*controller;
 	u32			max_speed_hz;
+	/*
+	 * Bit flags indicating which buses this device is connected to. Only
+	 * applicable to multi-bus controllers.
+	 */
+	u8 			buses;
 	u8			chip_select[SPI_CS_CNT_MAX];
 	u8			bits_per_word;
 	bool			rt;
@@ -570,6 +575,14 @@ struct spi_controller {
 	 */
 	u16			num_chipselect;
 
+	/*
+	 * Some specialized SPI controllers can have more than one physical
+	 * bus interface per controller. This specifies the number of buses
+	 * in that case. Other controllers do not need to set this (defaults
+	 * to 1).
+	 */
+	u16			num_buses;
+
 	/* Some SPI controllers pose alignment requirements on DMAable
 	 * buffers; let protocol drivers know about these requirements.
 	 */
-- 
2.43.0

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[Sean Anderson]

On 1/23/25 16:59, David Lechner wrote:
> On 1/23/25 10:24 AM, Sean Anderson wrote:
>> On 1/21/25 19:16, David Lechner wrote:
>>> On 1/16/25 5:21 PM, Sean Anderson wrote:
> 
> ...
> 
>>> Could we make a single device connected to both buses like this work using
>>> the proposed spi-lower and spi-upper or should we consider a different binding
>>> like the one I suggested?
>> 
>> If you are willing to do the work to rewrite the SPI subsystem to handle
>> this, then I don't object to it in principle. Using a property would
>> also help with forwards compatibility. On the other hand, separate
>> busses are easier to implement since they integrate better with the SPI
>> subsystem (e.g. you can just call spi_register_controller to create all
>> the slaves).
>> 
>> There have been some previous patches from Xilinx to handle this
>> use case [1], but IMO they were pretty hacky. They got this feature
>> merged into U-Boot and it broke many other boards and took a lot of
>> cleanup to fix. So I have intentionally only tackled the unsynchronized
>> use case since that requires no modification to areas outside of this
>> driver. I don't need the "parallel" use case and I am not interested in
>> doing the work required to implement it.
>> 
>> --Sean
>> 
>> [1] https://lore.kernel.org/linux-spi/20221017121249.19061-1-amit.kumar-mahapatra@amd.com/
> 
> Fair enough, and I think it can be done without breaking things like the multi
> CS support did.
> 
> Here are a couple of patches. Feel free to resubmit them with your series if
> they work for you. To make it work with your series, you should just need to
> modify the .dts to look like this:
> 
> +          flash@0 {
> +            compatible = "jedec,spi-nor";
> +            reg = <0>;
> +            spi-buses = <0>; /* lower */
> +          };
> +
> +          flash@1 {
> +            reg = <1>;
> +            compatible = "jedec,spi-nor";
> +            /* also OK to omit property in case of spi-buses = <0>; */
> +          };
> +
> +          flash@2 {
> +            reg = <2>;
> +            compatible = "jedec,spi-nor";
> +            spi-buses = <1>; /* upper */
> +          };
> 
> 
> Then drop patch "spi: zynqmp-gqspi: Split the bus" of course.
> 
> In zynqmp_qspi_probe(), add a line:
> 
> 	ctlr->num_buses = 2;
> 
> And in the zynqmp_qspi_transfer_one() function, use spi->buses to select the
> correct bus:
> 
> 	xqspi->genfifobus = FIELD_PREP(GQSPI_GENFIFO_BUS_MASK, spi->buses);
> 
> I don't have a SPI controller on hand with multiple buses, so I don't have
> any patch adding support to a specific controller. But I did build and run this
> and hacked in some stuff to the drivers I am working on to make sure it is
> working as advertised as best as I could with a single bus.

Your patches LGTM. I will use them for v2. Mark do you have any comments on this
approach?

--Sean

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[Mark Brown]

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

On Thu, Jan 23, 2025 at 05:37:16PM -0500, Sean Anderson wrote:

> Your patches LGTM. I will use them for v2. Mark do you have any comments on this
> approach?

It looks fine.

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

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[Sean Anderson]

Hi David,

I am (finally!) getting around to doing v2 of this series, and I ran
into a small problem with your proposed solution.

On 1/23/25 16:59, David Lechner wrote:
> ---
> From: David Lechner <dlechner@baylibre.com>
> Date: Thu, 23 Jan 2025 15:35:19 -0600
> Subject: [PATCH 2/2] spi: add support for multi-bus controllers
> 
> Add support for SPI controllers with multiple physical SPI buses.
> 
> This is common in the type of controller that can be used with parallel
> flash memories, but can be used for general purpose SPI as well.
> 
> To indicate support, a controller just needs to set ctlr->num_buses to
> something greater than 1. Peripherals indicate which bus they are
> connected to via device tree (ACPI support can be added if needed).
> 
> In the future, this can be extended to support peripherals that also
> have multiple SPI buses to use those buses at the same time by adding
> a similar bus flags field to struct spi_transfer.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/spi/spi.c       | 26 +++++++++++++++++++++++++-
>  include/linux/spi/spi.h | 13 +++++++++++++
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 10c365e9100a..f7722e5e906d 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2364,7 +2364,7 @@ static void of_spi_parse_dt_cs_delay(struct device_node *nc,
>  static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>  			   struct device_node *nc)
>  {
> -	u32 value, cs[SPI_CS_CNT_MAX];
> +	u32 value, buses[8], cs[SPI_CS_CNT_MAX];
>  	int rc, idx;
>  
>  	/* Mode (clock phase/polarity/etc.) */
> @@ -2379,6 +2379,29 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>  	if (of_property_read_bool(nc, "spi-cs-high"))
>  		spi->mode |= SPI_CS_HIGH;
>  
> +	rc = of_property_read_variable_u32_array(nc, "spi-buses", buses, 1,
> +						 ARRAY_SIZE(buses));
> +	if (rc < 0 && rc != -EINVAL) {
> +		dev_err(&ctlr->dev, "%pOF has invalid 'spi-buses' property (%d)\n",
> +			nc, rc);
> +		return rc;
> +	}
> +
> +	if (rc == -EINVAL) {
> +		/* Default when property is omitted. */
> +		spi->buses = BIT(0);

For backwards compatibility, the default bus for CS 1 on gqspi must be 1
and not 0. Ideally there would be some hook for the master to fix things
up when the slaves are probed, but that doesn't seem to exist. I was
thinking about doing this with OF changesets. Do you have any better
ideas?

--Sean

> +	} else {
> +		for (idx = 0; idx < rc; idx++) {
> +			if (buses[idx] >= ctlr->num_buses) {
> +				dev_err(&ctlr->dev,
> +					"%pOF has out of range 'spi-buses' property (%d)\n",
> +					nc, buses[idx]);
> +				return -EINVAL;
> +			}
> +			spi->buses |= BIT(buses[idx]);
> +		}
> +	}
> +
>  	/* Device DUAL/QUAD mode */
>  	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
>  		switch (value) {
> @@ -3072,6 +3095,7 @@ struct spi_controller *__spi_alloc_controller(struct device *dev,
>  	mutex_init(&ctlr->add_lock);
>  	ctlr->bus_num = -1;
>  	ctlr->num_chipselect = 1;
> +	ctlr->num_buses = 1;
>  	ctlr->slave = slave;
>  	if (IS_ENABLED(CONFIG_SPI_SLAVE) && slave)
>  		ctlr->dev.class = &spi_slave_class;
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 4c087009cf97..bc45d70e8c45 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -187,6 +187,11 @@ struct spi_device {
>  	struct device		dev;
>  	struct spi_controller	*controller;
>  	u32			max_speed_hz;
> +	/*
> +	 * Bit flags indicating which buses this device is connected to. Only
> +	 * applicable to multi-bus controllers.
> +	 */
> +	u8 			buses;
>  	u8			chip_select[SPI_CS_CNT_MAX];
>  	u8			bits_per_word;
>  	bool			rt;
> @@ -570,6 +575,14 @@ struct spi_controller {
>  	 */
>  	u16			num_chipselect;
>  
> +	/*
> +	 * Some specialized SPI controllers can have more than one physical
> +	 * bus interface per controller. This specifies the number of buses
> +	 * in that case. Other controllers do not need to set this (defaults
> +	 * to 1).
> +	 */
> +	u16			num_buses;
> +
>  	/* Some SPI controllers pose alignment requirements on DMAable
>  	 * buffers; let protocol drivers know about these requirements.
>  	 */
> -- 
> 2.43.0
> 
> 
> 

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[David Lechner]

On 6/12/25 6:44 PM, Sean Anderson wrote:
> Hi David,
> 
> I am (finally!) getting around to doing v2 of this series, and I ran
> into a small problem with your proposed solution.
> 
> On 1/23/25 16:59, David Lechner wrote:
>> ---
>> From: David Lechner <dlechner@baylibre.com>
>> Date: Thu, 23 Jan 2025 15:35:19 -0600
>> Subject: [PATCH 2/2] spi: add support for multi-bus controllers
>>
>> Add support for SPI controllers with multiple physical SPI buses.
>>
>> This is common in the type of controller that can be used with parallel
>> flash memories, but can be used for general purpose SPI as well.
>>
>> To indicate support, a controller just needs to set ctlr->num_buses to
>> something greater than 1. Peripherals indicate which bus they are
>> connected to via device tree (ACPI support can be added if needed).
>>
>> In the future, this can be extended to support peripherals that also
>> have multiple SPI buses to use those buses at the same time by adding
>> a similar bus flags field to struct spi_transfer.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>  drivers/spi/spi.c       | 26 +++++++++++++++++++++++++-
>>  include/linux/spi/spi.h | 13 +++++++++++++
>>  2 files changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 10c365e9100a..f7722e5e906d 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -2364,7 +2364,7 @@ static void of_spi_parse_dt_cs_delay(struct device_node *nc,
>>  static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>>  			   struct device_node *nc)
>>  {
>> -	u32 value, cs[SPI_CS_CNT_MAX];
>> +	u32 value, buses[8], cs[SPI_CS_CNT_MAX];
>>  	int rc, idx;
>>  
>>  	/* Mode (clock phase/polarity/etc.) */
>> @@ -2379,6 +2379,29 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>>  	if (of_property_read_bool(nc, "spi-cs-high"))
>>  		spi->mode |= SPI_CS_HIGH;
>>  
>> +	rc = of_property_read_variable_u32_array(nc, "spi-buses", buses, 1,
>> +						 ARRAY_SIZE(buses));
>> +	if (rc < 0 && rc != -EINVAL) {
>> +		dev_err(&ctlr->dev, "%pOF has invalid 'spi-buses' property (%d)\n",
>> +			nc, rc);
>> +		return rc;
>> +	}
>> +
>> +	if (rc == -EINVAL) {
>> +		/* Default when property is omitted. */
>> +		spi->buses = BIT(0);
> 
> For backwards compatibility, the default bus for CS 1 on gqspi must be 1
> and not 0. Ideally there would be some hook for the master to fix things
> up when the slaves are probed, but that doesn't seem to exist. I was
> thinking about doing this with OF changesets. Do you have any better
> ideas?
> 

Does this work? 

		spi->buses = BIT(cs[0]);

(would have to move all the new code after cs[0] is assigned of course)

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[Sean Anderson]

On 6/13/25 10:20, David Lechner wrote:
> On 6/12/25 6:44 PM, Sean Anderson wrote:
>> Hi David,
>> 
>> I am (finally!) getting around to doing v2 of this series, and I ran
>> into a small problem with your proposed solution.
>> 
>> On 1/23/25 16:59, David Lechner wrote:
>>> ---
>>> From: David Lechner <dlechner@baylibre.com>
>>> Date: Thu, 23 Jan 2025 15:35:19 -0600
>>> Subject: [PATCH 2/2] spi: add support for multi-bus controllers
>>>
>>> Add support for SPI controllers with multiple physical SPI buses.
>>>
>>> This is common in the type of controller that can be used with parallel
>>> flash memories, but can be used for general purpose SPI as well.
>>>
>>> To indicate support, a controller just needs to set ctlr->num_buses to
>>> something greater than 1. Peripherals indicate which bus they are
>>> connected to via device tree (ACPI support can be added if needed).
>>>
>>> In the future, this can be extended to support peripherals that also
>>> have multiple SPI buses to use those buses at the same time by adding
>>> a similar bus flags field to struct spi_transfer.
>>>
>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>> ---
>>>  drivers/spi/spi.c       | 26 +++++++++++++++++++++++++-
>>>  include/linux/spi/spi.h | 13 +++++++++++++
>>>  2 files changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>>> index 10c365e9100a..f7722e5e906d 100644
>>> --- a/drivers/spi/spi.c
>>> +++ b/drivers/spi/spi.c
>>> @@ -2364,7 +2364,7 @@ static void of_spi_parse_dt_cs_delay(struct device_node *nc,
>>>  static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>>>  			   struct device_node *nc)
>>>  {
>>> -	u32 value, cs[SPI_CS_CNT_MAX];
>>> +	u32 value, buses[8], cs[SPI_CS_CNT_MAX];
>>>  	int rc, idx;
>>>  
>>>  	/* Mode (clock phase/polarity/etc.) */
>>> @@ -2379,6 +2379,29 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>>>  	if (of_property_read_bool(nc, "spi-cs-high"))
>>>  		spi->mode |= SPI_CS_HIGH;
>>>  
>>> +	rc = of_property_read_variable_u32_array(nc, "spi-buses", buses, 1,
>>> +						 ARRAY_SIZE(buses));
>>> +	if (rc < 0 && rc != -EINVAL) {
>>> +		dev_err(&ctlr->dev, "%pOF has invalid 'spi-buses' property (%d)\n",
>>> +			nc, rc);
>>> +		return rc;
>>> +	}
>>> +
>>> +	if (rc == -EINVAL) {
>>> +		/* Default when property is omitted. */
>>> +		spi->buses = BIT(0);
>> 
>> For backwards compatibility, the default bus for CS 1 on gqspi must be 1
>> and not 0. Ideally there would be some hook for the master to fix things
>> up when the slaves are probed, but that doesn't seem to exist. I was
>> thinking about doing this with OF changesets. Do you have any better
>> ideas?
>> 
> 
> Does this work? 
> 
> 		spi->buses = BIT(cs[0]);
> 
> (would have to move all the new code after cs[0] is assigned of course)

Yeah, but do we really want to make this the default for all drivers?
This is really a quirk of the existing gqspi binding and I don't think
it makes sense in general.

--Sean

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[Sean Anderson]

On 6/13/25 11:57, Sean Anderson wrote:
> On 6/13/25 10:20, David Lechner wrote:
>> On 6/12/25 6:44 PM, Sean Anderson wrote:
>>> Hi David,
>>> 
>>> I am (finally!) getting around to doing v2 of this series, and I ran
>>> into a small problem with your proposed solution.
>>> 
>>> On 1/23/25 16:59, David Lechner wrote:
>>>> ---
>>>> From: David Lechner <dlechner@baylibre.com>
>>>> Date: Thu, 23 Jan 2025 15:35:19 -0600
>>>> Subject: [PATCH 2/2] spi: add support for multi-bus controllers
>>>>
>>>> Add support for SPI controllers with multiple physical SPI buses.
>>>>
>>>> This is common in the type of controller that can be used with parallel
>>>> flash memories, but can be used for general purpose SPI as well.
>>>>
>>>> To indicate support, a controller just needs to set ctlr->num_buses to
>>>> something greater than 1. Peripherals indicate which bus they are
>>>> connected to via device tree (ACPI support can be added if needed).
>>>>
>>>> In the future, this can be extended to support peripherals that also
>>>> have multiple SPI buses to use those buses at the same time by adding
>>>> a similar bus flags field to struct spi_transfer.
>>>>
>>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>>> ---
>>>>  drivers/spi/spi.c       | 26 +++++++++++++++++++++++++-
>>>>  include/linux/spi/spi.h | 13 +++++++++++++
>>>>  2 files changed, 38 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>>>> index 10c365e9100a..f7722e5e906d 100644
>>>> --- a/drivers/spi/spi.c
>>>> +++ b/drivers/spi/spi.c
>>>> @@ -2364,7 +2364,7 @@ static void of_spi_parse_dt_cs_delay(struct device_node *nc,
>>>>  static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>>>>  			   struct device_node *nc)
>>>>  {
>>>> -	u32 value, cs[SPI_CS_CNT_MAX];
>>>> +	u32 value, buses[8], cs[SPI_CS_CNT_MAX];
>>>>  	int rc, idx;
>>>>  
>>>>  	/* Mode (clock phase/polarity/etc.) */
>>>> @@ -2379,6 +2379,29 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>>>>  	if (of_property_read_bool(nc, "spi-cs-high"))
>>>>  		spi->mode |= SPI_CS_HIGH;
>>>>  
>>>> +	rc = of_property_read_variable_u32_array(nc, "spi-buses", buses, 1,
>>>> +						 ARRAY_SIZE(buses));
>>>> +	if (rc < 0 && rc != -EINVAL) {
>>>> +		dev_err(&ctlr->dev, "%pOF has invalid 'spi-buses' property (%d)\n",
>>>> +			nc, rc);
>>>> +		return rc;
>>>> +	}
>>>> +
>>>> +	if (rc == -EINVAL) {
>>>> +		/* Default when property is omitted. */
>>>> +		spi->buses = BIT(0);
>>> 
>>> For backwards compatibility, the default bus for CS 1 on gqspi must be 1
>>> and not 0. Ideally there would be some hook for the master to fix things
>>> up when the slaves are probed, but that doesn't seem to exist. I was
>>> thinking about doing this with OF changesets. Do you have any better
>>> ideas?
>>> 
>> 
>> Does this work? 
>> 
>> 		spi->buses = BIT(cs[0]);
>> 
>> (would have to move all the new code after cs[0] is assigned of course)
> 
> Yeah, but do we really want to make this the default for all drivers?
> This is really a quirk of the existing gqspi binding and I don't think
> it makes sense in general.

I think I will add a flag like

		/* Default when property is omitted. */
		if (ctlr->flags & SPI_CONTROLLER_DEFAULT_BUS_IS_CS)
			spi->buses = BIT(cs[0]);
		else
			spi->buses = BIT(0);

which should keep the defaults sane for everyone else.

--Sean

Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

[David Lechner]

On 6/13/25 10:57 AM, Sean Anderson wrote:
> On 6/13/25 10:20, David Lechner wrote:
>> On 6/12/25 6:44 PM, Sean Anderson wrote:
>>> Hi David,
>>>
>>> I am (finally!) getting around to doing v2 of this series, and I ran
>>> into a small problem with your proposed solution.
>>>
>>> On 1/23/25 16:59, David Lechner wrote:
>>>> ---
>>>> From: David Lechner <dlechner@baylibre.com>
>>>> Date: Thu, 23 Jan 2025 15:35:19 -0600
>>>> Subject: [PATCH 2/2] spi: add support for multi-bus controllers
>>>>
>>>> Add support for SPI controllers with multiple physical SPI buses.
>>>>
>>>> This is common in the type of controller that can be used with parallel
>>>> flash memories, but can be used for general purpose SPI as well.
>>>>
>>>> To indicate support, a controller just needs to set ctlr->num_buses to
>>>> something greater than 1. Peripherals indicate which bus they are
>>>> connected to via device tree (ACPI support can be added if needed).
>>>>
>>>> In the future, this can be extended to support peripherals that also
>>>> have multiple SPI buses to use those buses at the same time by adding
>>>> a similar bus flags field to struct spi_transfer.
>>>>
>>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>>> ---
>>>>  drivers/spi/spi.c       | 26 +++++++++++++++++++++++++-
>>>>  include/linux/spi/spi.h | 13 +++++++++++++
>>>>  2 files changed, 38 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>>>> index 10c365e9100a..f7722e5e906d 100644
>>>> --- a/drivers/spi/spi.c
>>>> +++ b/drivers/spi/spi.c
>>>> @@ -2364,7 +2364,7 @@ static void of_spi_parse_dt_cs_delay(struct device_node *nc,
>>>>  static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>>>>  			   struct device_node *nc)
>>>>  {
>>>> -	u32 value, cs[SPI_CS_CNT_MAX];
>>>> +	u32 value, buses[8], cs[SPI_CS_CNT_MAX];
>>>>  	int rc, idx;
>>>>  
>>>>  	/* Mode (clock phase/polarity/etc.) */
>>>> @@ -2379,6 +2379,29 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>>>>  	if (of_property_read_bool(nc, "spi-cs-high"))
>>>>  		spi->mode |= SPI_CS_HIGH;
>>>>  
>>>> +	rc = of_property_read_variable_u32_array(nc, "spi-buses", buses, 1,
>>>> +						 ARRAY_SIZE(buses));
>>>> +	if (rc < 0 && rc != -EINVAL) {
>>>> +		dev_err(&ctlr->dev, "%pOF has invalid 'spi-buses' property (%d)\n",
>>>> +			nc, rc);
>>>> +		return rc;
>>>> +	}
>>>> +
>>>> +	if (rc == -EINVAL) {
>>>> +		/* Default when property is omitted. */
>>>> +		spi->buses = BIT(0);
>>>
>>> For backwards compatibility, the default bus for CS 1 on gqspi must be 1
>>> and not 0. Ideally there would be some hook for the master to fix things
>>> up when the slaves are probed, but that doesn't seem to exist. I was
>>> thinking about doing this with OF changesets. Do you have any better
>>> ideas?
>>>
>>
>> Does this work? 
>>
>> 		spi->buses = BIT(cs[0]);
>>
>> (would have to move all the new code after cs[0] is assigned of course)
> 
> Yeah, but do we really want to make this the default for all drivers?
> This is really a quirk of the existing gqspi binding and I don't think
> it makes sense in general.
> 

Can we just leave spi->buses unset then and leave it up to the controller
driver to interpret that as "default" and handle it appropriately?

OF changessets seems overkill to me.