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
