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

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

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