Message-ID: <46a7eba6-a705-4543-b967-e83ccc89e7d4@linux.dev>
Date: Thu, 23 Jan 2025 11:24:36 -0500
From: Sean Anderson <sean.anderson@...ux.dev>
To: David Lechner <dlechner@...libre.com>, Mark Brown <broonie@...nel.org>,
 Michal Simek <michal.simek@....com>, linux-spi@...r.kernel.org
Cc: Jinjie Ruan <ruanjinjie@...wei.com>,
 linux-arm-kernel@...ts.infradead.org,
 Amit Kumar Mahapatra <amit.kumar-mahapatra@....com>,
 linux-kernel@...r.kernel.org, Miquel Raynal <miquel.raynal@...tlin.com>,
 Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Rob Herring <robh@...nel.org>,
 devicetree@...r.kernel.org,
 "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
 Jonathan Cameron <jic23@...nel.org>, Nuno Sá
 <nuno.sa@...log.com>
Subject: Re: [PATCH 1/7] dt-bindings: spi: zynqmp-qspi: Split the bus

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/

Powered by blists - more mailing lists