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@xxxxxxx/ 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> --- .../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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> --- 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
