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

[Sean Anderson <sean.anderson@linux.dev>]

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