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

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

--Sean