On Tue, May 14, 2024 at 1:46 PM Conor Dooley <conor@xxxxxxxxxx> wrote: > > On Mon, May 13, 2024 at 12:06:17PM -0500, David Lechner wrote: > > On Mon, May 13, 2024 at 11:46 AM Conor Dooley <conor@xxxxxxxxxx> wrote: > > > > > > On Fri, May 10, 2024 at 07:44:24PM -0500, David Lechner wrote: > > > > This adds a new property to the spi-peripheral-props binding for use > > > > with peripherals connected to controllers that support offloading. > > > > > > > > Here, offloading means that the controller has the ability to perform > > > > complex SPI transactions without CPU intervention in some shape or form. > > > > > > > > This property will be used to assign controller offload resources to > > > > each peripheral that needs them. What these resources are will be > > > > defined by each specific controller binding. > > > > > > > > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx> > > > > --- > > > > > > > > v2 changes: > > > > > > > > In v1, instead of generic SPI bindings, there were only controller- > > > > specific bindings, so this is a new patch. > > > > > > > > In the previous version I also had an offloads object node that described > > > > what the offload capabilities were but it was suggested that this was > > > > not necessary/overcomplicated. So I've gone to the other extreme and > > > > made it perhaps over-simplified now by requiring all information about > > > > how each offload is used to be encoded in a single u32. > > > > > > The property is a u32-array, so I guess, not a single u32? > > > > It is an array to handle cases where a peripheral might need more than > > one offload. But the idea was it put everything about each individual > > offload in a single u32. e.g. 0x0101 could be offload 1 with hardware > > trigger 1 and 0x0201 could be offload 1 with hardware trigger 2. Then > > a peripheral could have spi-offloads = <0x0101>, <0x0201>; if it > > needed to select between both triggers at runtime. > > > > > > > > > We could of course consider using #spi-offload-cells instead for > > > > allowing encoding multiple parameters for each offload instance if that > > > > would be preferable. > > > > > > A -cells property was my gut reaction to what you'd written here and > > > seems especially appropriate if there's any likelihood of some future > > > device using some external resources for spi-offloading. > > > However, -cells properties go in providers, not consumers, so it wouldn't > > > end up in spi-periph-props.yaml, but rather in the controller binding, > > > and instead there'd be a cell array type property in here. I think you > > > know that though and I'm interpreting what's been written rather than > > > what you meant. > > > > Indeed you guess correctly. So the next question is if it should be > > the kind of #-cells that implies a phandle like most providers or > > without phandles like #address-cells? > > I'm trying to understand if the offload could ever refer to something > beyond the controller that you'd need the phandle for. I think it would > be really helpful to see an example dt of a non-trivial example for how > this would work. The example in the ad7944 patch has a stub controller > node & the clocks/dmas in the peripheral node so it is difficult to > reason about the spi-offloads property there. The fully implemented and tested version of the .dts corresponding to the hardware pictured in the cover letter can be found at [1]. [1]: https://github.com/dlech/linux/blob/axi-spi-engine-offload-v2/arch/arm/boot/dts/xilinx/zynq-zed-adv7511-ad7986.dts To be clear though, the idea that I am proposing here is that if there is something beyond the SPI controller directly connected to the offload, then we would add those things in the peripheral node along with the spi-offloads property that specifies the offload those other things are connected to. Tangent on phandle vs. no phandle: If we add #spi-offload-cells, I would expect that it would always be in the SPI controller node. And the consumers would always be SPI peripheral nodes. So having a phandle seems redundant since it would always point to the controller which is the parent of the peripheral. example_spi: spi { ... #spi-offload-cells = <1>; adc@0 { ... /* fine, but not sure phandle is needed since it always the parent */ spi-offloads = <&example_spi 0>; }; }; spi { ... #spi-offload-cells = <1>; adc@0 { ... /* simpler is better? */ spi-offloads = <0>; }; }; Back to "something beyond the SPI controller": Here are some examples of how I envision this would work. Let's suppose we have a SPI controller that has some sort of offload capability with a configurable trigger source. The trigger can either be an internal software trigger (i.e. writing a register of the SPI controller) or and external trigger (i.e. a input signal from a pin on the SoC). The SPI controller has a lookup table with 8 slots where it can store a series of SPI commands that can be played back by asserting the trigger (this is what provides the "offloading"). So this SPI controller would have #spi-offload-cells = <2>; where the first cell would be the index in the lookup table 0 to 7 and the second cell would be the trigger source 0 for software or 1 for hardware. Application 1: a network controller This could use two offloads, one for TX and one for RX. For TX, we use the first slot with a software trigger because the data is coming from Linux. For RX we use the second slot with a hardware trigger since data is coming from the network controller (i.e. a data ready signal that would normally be wired to a gpio for interrupt but wired to the SPI offload trigger input pin instead). So the peripheral bindings would be: #define SOFTWARE_TRIGGER 0 #define HARDWARE_TRIGGER 1 can@0 { ... spi-offloads = <0 SOFTWARE_TRIGGER>, <1 HARDWARE_TRIGGER>; /* maybe we need names too? */ spi-offload-names = "tx", "rx"; }; In this case, there is nothing extra beyond the SPI controller and the network controller, so no extra bindings beyond this are needed. Application 2: an advanced ADC + FPGA This is basically the same as the ad7944 case seen already with one extra feature. In this case, the sample data also contains a CRC byte for error checking. So instead of SPI RX data going directly to DMA, the FPGA removes the CRC byte from the data stream an only the sample data goes to the DMA buffer. The CRC is checked and if bad, an interrupt is asserted. Since this is an FPGA, most everything is hardwired rather than having any kind of mux selection so #spi-offload-cells = <1>; for this controller. By adding spi-offloads to the peripheral node, it also extends the peripheral binding to include the additional properties needed for the extra features provided by the FPGA. In other words, we are saying this DT node now represents the ADC chip plus everything connected to the offload instance used by the ADC chip. adc@0 { ... spi-offloads = <0>; dmas = <&dma 0>; /* channel receiving split out sample data */ dma-names = "rx"; interrupts = <&intc 99>; /* interrupt for bad CRC */ interrupt-names = "crc"; }; > > > Asking because I got pushback on > > v1 for using a phandle with offloads (although in that case, the > > phandle was for the offload instance itself instead for the SPI > > controller, so maybe this is different in this case?). > > Do you have a link to this v1 pushback? Hmm... maybe that was from some internal review before v1 that I was remembering and confusing with the resistance of different aspects you mention below. > I had looked at the v1's binding > comments and didn't see that type of property being resisted - although > I did see some resistance to the spi peripheral node containing any of > the information about the offloads it had been assigned and instead > doing that mapping in the controller so that the cs was sufficient. I > don't think that'd work with the scenario you describe above though > where there could be two different triggers per device tho. I think most of the objection was to having an offloads object node with offload@ subnodes in the SPI controller node along side the peripheral nodes.
