Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner <dlechner@xxxxxxxxxxxx>]

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.