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

Commit Message

David Lechner May 11, 2024, 12:44 a.m. UTC

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 <[e-mail]>
---

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.

We could of course consider using #spi-offload-cells instead for
allowing encoding multiple parameters for each offload instance if that
would be preferable.

I also considered adding spi-offload-names that could be used as sort
of a compatible string (more of an interface name really) in case some
peripherals may want to support more than 1 specialized type of offload.
---
 .../devicetree/bindings/spi/spi-peripheral-props.yaml          | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Conor Dooley May 13, 2024, 4:46 p.m. UTC | #1

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 <[e-mail]>
> ---
> 
> 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?

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

> I also considered adding spi-offload-names that could be used as sort
> of a compatible string (more of an interface name really) in case some
> peripherals may want to support more than 1 specialized type of offload.
> ---
>  .../devicetree/bindings/spi/spi-peripheral-props.yaml          | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 15938f81fdce..32991a2d2264 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -113,6 +113,16 @@ properties:
>      minItems: 2
>      maxItems: 4
>  
> +  spi-offloads:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      Array of controller offload instances that are reserved for use by the
> +      peripheral device. The semantic meaning of the values of the array
> +      elements is defined by the controller. For example, it could be a simple
> +      0-based index of the offload instance, or it could be a bitfield where
> +      a few bits represent the assigned hardware trigger, a few bits represent
> +      the assigned RX stream, etc.
> +
>    st,spi-midi-ns:
>      description: |
>        Only for STM32H7, (Master Inter-Data Idleness) minimum time
> 
> -- 
> 2.43.2
>

David Lechner May 13, 2024, 5:06 p.m. UTC | #2

On Mon, May 13, 2024 at 11:46 AM Conor Dooley <[e-mail]> 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 <[e-mail]>
> > ---
> >
> > 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? 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?).

>
> > I also considered adding spi-offload-names that could be used as sort
> > of a compatible string (more of an interface name really) in case some
> > peripherals may want to support more than 1 specialized type of offload.
> > ---
> >  .../devicetree/bindings/spi/spi-peripheral-props.yaml          | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > index 15938f81fdce..32991a2d2264 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > @@ -113,6 +113,16 @@ properties:
> >      minItems: 2
> >      maxItems: 4
> >
> > +  spi-offloads:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description:
> > +      Array of controller offload instances that are reserved for use by the
> > +      peripheral device. The semantic meaning of the values of the array
> > +      elements is defined by the controller. For example, it could be a simple
> > +      0-based index of the offload instance, or it could be a bitfield where
> > +      a few bits represent the assigned hardware trigger, a few bits represent
> > +      the assigned RX stream, etc.
> > +
> >    st,spi-midi-ns:
> >      description: |
> >        Only for STM32H7, (Master Inter-Data Idleness) minimum time
> >
> > --
> > 2.43.2
> >

Conor Dooley May 14, 2024, 6:46 p.m. UTC | #3

On Mon, May 13, 2024 at 12:06:17PM -0500, David Lechner wrote:
> On Mon, May 13, 2024 at 11:46 AM Conor Dooley <[e-mail]> 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 <[e-mail]>
> > > ---
> > >
> > > 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.

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

Cheers,
Conor.

David Lechner May 14, 2024, 10:56 p.m. UTC | #4

On Tue, May 14, 2024 at 1:46 PM Conor Dooley <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > > ---
> > > >
> > > > 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.

Conor Dooley May 16, 2024, 9:32 p.m. UTC | #5

Yo,

Sorry for the delay, long reply deserved some time to sit and think
about it.

On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> On Tue, May 14, 2024 at 1:46 PM Conor Dooley <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > > > ---
> > > > >
> > > > > 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

Unfortunately this is a trivial example, so there's not much to be
gained in new information from the example in the bindings :/ Your
examples below are good though, which makes up for that and more.

> 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:

Yeah, I think not having a phandle makes sense based on what you've
said.

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

It seems very strange to me that the dmas and the clock triggers are
going into the spi device nodes. The description is 
| +  dmas:
| +    maxItems: 1
| +    description: RX DMA Channel for receiving a samples from the SPI offload.
But as far as I can tell this device is in a package of its own and not
some IP provided by Analog that an engine on the FPGA can actually do
DMA to, and the actual connection of the device is "just" SPI.
The dmas and clock triggers etc appear to be resources belonging to the
controller that can "assigned" to a particular spi device. If the adc
gets disconnected from the system, the dmas and clock triggers are still
connected to the spi controller/offload engine, they don't end up n/c,
right? (Well maybe they would in the case of a fancy SPI device that
provides it's own sampling clock or w/e, but then it'd be a clock
provider of sorts). I'd be expecting the spi-offloads property to be
responsible for selecting which of the various resources belonging to
the controller are to be used by a device.
Maybe it overcomplicates the shit out of things and Rob or Mark are
gonna start screaming at me but w/e, looking at it from the point of
view of how the hardware is laid out (or at least how it is described
in your FPGA case above) the dma/clock properties looks like they're
misplaced. IOW, I don't think that adding the spi-offloads property
should convert a node from representing an ADC in a qfn-20 or w/e
to "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.

I dunno, that was my reading of Rob's comments at least. I know he had
more than one objection though, so maybe we're just looking at different
portions of it - I did note that you removed the offload@ though.

Cheers,
Conor.

David Lechner May 17, 2024, 4:51 p.m. UTC | #6

On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
>
> Yo,
>
> Sorry for the delay, long reply deserved some time to sit and think
> about it.
>
> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > On Tue, May 14, 2024 at 1:46 PM Conor Dooley <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > > > > ---
> > > > > >
> > > > > > 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
>
> Unfortunately this is a trivial example, so there's not much to be
> gained in new information from the example in the bindings :/ Your
> examples below are good though, which makes up for that and more.
>
> > 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:
>
> Yeah, I think not having a phandle makes sense based on what you've
> said.
>
> > 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.
>
> It seems very strange to me that the dmas and the clock triggers are
> going into the spi device nodes. The description is
> | +  dmas:
> | +    maxItems: 1
> | +    description: RX DMA Channel for receiving a samples from the SPI offload.
> But as far as I can tell this device is in a package of its own and not
> some IP provided by Analog that an engine on the FPGA can actually do
> DMA to, and the actual connection of the device is "just" SPI.
> The dmas and clock triggers etc appear to be resources belonging to the
> controller that can "assigned" to a particular spi device. If the adc
> gets disconnected from the system, the dmas and clock triggers are still
> connected to the spi controller/offload engine, they don't end up n/c,
> right? (Well maybe they would in the case of a fancy SPI device that
> provides it's own sampling clock or w/e, but then it'd be a clock
> provider of sorts). I'd be expecting the spi-offloads property to be
> responsible for selecting which of the various resources belonging to
> the controller are to be used by a device.
> Maybe it overcomplicates the shit out of things and Rob or Mark are
> gonna start screaming at me but w/e, looking at it from the point of
> view of how the hardware is laid out (or at least how it is described
> in your FPGA case above) the dma/clock properties looks like they're
> misplaced. IOW, I don't think that adding the spi-offloads property
> should convert a node from representing an ADC in a qfn-20 or w/e
> to "the ADC chip plus everything connected to the offload instance
> used by the ADC chip".

This is the same reasoning that led me to the binding proposed in v1.
Rob suggested that these extras (dmas/clocks) should just be
properties directly of the SPI controller. But the issue I have with
that is that since this is an FPGA, these properties are not fixed.
Maybe there are more clocks or no clocks or interrupts or something we
didn't think of yet. So it doesn't really seem possible to write a
binding for the SPI controller node to cover all of these cases. These
extras are included in the FPGA bitstream only for a specific type of
peripheral, not for general use of the SPI controller with any type of
peripheral.

Another idea I had was to perhaps use the recently added IIO backend
framework for the "extras". The idea there is that we are creating a
"composite" IIO device that consists of the ADC chip (frontend) plus
these extra hardware trigger and hardware buffer functions provided by
the FPGA (backend).

offload_backend: adc0-backend {
    /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */
    compatible = "adi,pulsar-adc-offload";
    #io-backend-cells = <0>;
    dmas = <&dma 0>;
    dma-names = "rx";
    clocks = <&trigger_clock>;
};

spi {
    ...
    adc@0 {
        ...
        spi-offloads = <0>;
        io-backends = <&offload_backend>;
    };
};

While this could be a solution for IIO devices, this wouldn't solve
the issue in general though for SPI offloads used with non-IIO
peripherals. So I don't think it is the right thing to do here. But, I
think this idea of a "composite" device helps explain why we are
pushing for putting the "extras" with the peripheral node rather than
the controller node, at least for the specific case of the AXI SPI
Engine controller.

>
> > 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.
>
> I dunno, that was my reading of Rob's comments at least. I know he had
> more than one objection though, so maybe we're just looking at different
> portions of it - I did note that you removed the offload@ though.
>
> Cheers,
> Conor.

Conor Dooley May 19, 2024, 12:53 p.m. UTC | #7

On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
> > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:

> > > 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.
> >
> > It seems very strange to me that the dmas and the clock triggers are
> > going into the spi device nodes. The description is
> > | +  dmas:
> > | +    maxItems: 1
> > | +    description: RX DMA Channel for receiving a samples from the SPI offload.
> > But as far as I can tell this device is in a package of its own and not
> > some IP provided by Analog that an engine on the FPGA can actually do
> > DMA to, and the actual connection of the device is "just" SPI.
> > The dmas and clock triggers etc appear to be resources belonging to the
> > controller that can "assigned" to a particular spi device. If the adc
> > gets disconnected from the system, the dmas and clock triggers are still
> > connected to the spi controller/offload engine, they don't end up n/c,
> > right? (Well maybe they would in the case of a fancy SPI device that
> > provides it's own sampling clock or w/e, but then it'd be a clock
> > provider of sorts). I'd be expecting the spi-offloads property to be
> > responsible for selecting which of the various resources belonging to
> > the controller are to be used by a device.
> > Maybe it overcomplicates the shit out of things and Rob or Mark are
> > gonna start screaming at me but w/e, looking at it from the point of
> > view of how the hardware is laid out (or at least how it is described
> > in your FPGA case above) the dma/clock properties looks like they're
> > misplaced. IOW, I don't think that adding the spi-offloads property
> > should convert a node from representing an ADC in a qfn-20 or w/e
> > to "the ADC chip plus everything connected to the offload instance
> > used by the ADC chip".
> 
> This is the same reasoning that led me to the binding proposed in v1.
> Rob suggested that these extras (dmas/clocks) should just be
> properties directly of the SPI controller.

> But the issue I have with
> that is that since this is an FPGA, these properties are not fixed.

I don't think whether or not we're talking about an FPGA or an ASIC
matters at all here to be honest. In my view the main thing that FPGA
impact in terms of bindings is the number of properties required to
represent the configurability of a particular IP. Sure, you're gonna
have to change the dts around in a way you'll never have to with an
ASIC, but the description of individual devices or relations between
them doesn't change.

> Maybe there are more clocks or no clocks or interrupts or something we
> didn't think of yet.

This could happen with a new generation of an ASIC and is not specific
to an FPGA IP core. Even with FPGA IP, while there may be lots of
different configuration parameters, they are known & limited - you can
look at the IP's documentation (or failing that, the HDL) to figure out
what the points of variability are. It's not particularly difficult to
account for there being a configurable number of clocks or interrupts.
For "something we didn't think of yet" to be a factor, someone has to
think of it and add it to the IP core, and which point we can absolutely
change the bindings and software to account for the revised IP.

> So it doesn't really seem possible to write a
> binding for the SPI controller node to cover all of these cases.

I dunno, I don't think your concerns about numbers of clocks (or any
other such property) are unique to this particular use-case.

> These
> extras are included in the FPGA bitstream only for a specific type of
> peripheral, not for general use of the SPI controller with any type of
> peripheral.

I accept that, but what I was trying to get across was that you could
configure the FPGA with a bitstream that contains these extra resources
and then connect a peripheral device that does not make use of them at
all. Or you could connect a range of different peripherals that do use
them. Whether or not the resources are there and how they are connected
in the FPGA bitstream is not a property of the device connected to the
SPI controller, only whether or not you use them is.

In fact, looking at the documentation for the "spi engine" some more, I
am even less convinced that the right place for describing the offload is
the peripheral as I (finally?) noticed that the registers for the offload
engine are mapped directly into the "spi engine"'s memory map, rather than
it being a entirely separate IP in the FPGA fabric.

Further, what resources are available depends on what the SPI offload
engine IP is. If my employer decides to start shipping some SPI offload
IP in our catalogue that can work with ADI's ADCs, the set of offload
related properties or their names may well differ.

> Another idea I had was to perhaps use the recently added IIO backend
> framework for the "extras". The idea there is that we are creating a
> "composite" IIO device that consists of the ADC chip (frontend) plus
> these extra hardware trigger and hardware buffer functions provided by
> the FPGA (backend).
> 
> offload_backend: adc0-backend {
>     /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */
>     compatible = "adi,pulsar-adc-offload";
>     #io-backend-cells = <0>;
>     dmas = <&dma 0>;
>     dma-names = "rx";
>     clocks = <&trigger_clock>;
> };
> 
> spi {
>     ...
>     adc@0 {
>         ...
>         spi-offloads = <0>;
>         io-backends = <&offload_backend>;
>     };
> };
> 
> While this could be a solution for IIO devices, this wouldn't solve
> the issue in general though for SPI offloads used with non-IIO
> peripherals.

Yeah, I agree that using something specific to IIO isn't really a good
solution.

Cheers,
Conor.

> So I don't think it is the right thing to do here. But, I
> think this idea of a "composite" device helps explain why we are
> pushing for putting the "extras" with the peripheral node rather than
> the controller node, at least for the specific case of the AXI SPI
> Engine controller.

David Lechner May 21, 2024, 2:54 p.m. UTC | #8

On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
>
> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
> > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
>
> > > > 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.
> > >
> > > It seems very strange to me that the dmas and the clock triggers are
> > > going into the spi device nodes. The description is
> > > | +  dmas:
> > > | +    maxItems: 1
> > > | +    description: RX DMA Channel for receiving a samples from the SPI offload.
> > > But as far as I can tell this device is in a package of its own and not
> > > some IP provided by Analog that an engine on the FPGA can actually do
> > > DMA to, and the actual connection of the device is "just" SPI.
> > > The dmas and clock triggers etc appear to be resources belonging to the
> > > controller that can "assigned" to a particular spi device. If the adc
> > > gets disconnected from the system, the dmas and clock triggers are still
> > > connected to the spi controller/offload engine, they don't end up n/c,
> > > right? (Well maybe they would in the case of a fancy SPI device that
> > > provides it's own sampling clock or w/e, but then it'd be a clock
> > > provider of sorts). I'd be expecting the spi-offloads property to be
> > > responsible for selecting which of the various resources belonging to
> > > the controller are to be used by a device.
> > > Maybe it overcomplicates the shit out of things and Rob or Mark are
> > > gonna start screaming at me but w/e, looking at it from the point of
> > > view of how the hardware is laid out (or at least how it is described
> > > in your FPGA case above) the dma/clock properties looks like they're
> > > misplaced. IOW, I don't think that adding the spi-offloads property
> > > should convert a node from representing an ADC in a qfn-20 or w/e
> > > to "the ADC chip plus everything connected to the offload instance
> > > used by the ADC chip".
> >
> > This is the same reasoning that led me to the binding proposed in v1.
> > Rob suggested that these extras (dmas/clocks) should just be
> > properties directly of the SPI controller.
>
> > But the issue I have with
> > that is that since this is an FPGA, these properties are not fixed.
>
> I don't think whether or not we're talking about an FPGA or an ASIC
> matters at all here to be honest. In my view the main thing that FPGA
> impact in terms of bindings is the number of properties required to
> represent the configurability of a particular IP. Sure, you're gonna
> have to change the dts around in a way you'll never have to with an
> ASIC, but the description of individual devices or relations between
> them doesn't change.
>
> > Maybe there are more clocks or no clocks or interrupts or something we
> > didn't think of yet.
>
> This could happen with a new generation of an ASIC and is not specific
> to an FPGA IP core. Even with FPGA IP, while there may be lots of
> different configuration parameters, they are known & limited - you can
> look at the IP's documentation (or failing that, the HDL) to figure out
> what the points of variability are. It's not particularly difficult to
> account for there being a configurable number of clocks or interrupts.
> For "something we didn't think of yet" to be a factor, someone has to
> think of it and add it to the IP core, and which point we can absolutely
> change the bindings and software to account for the revised IP.
>
> > So it doesn't really seem possible to write a
> > binding for the SPI controller node to cover all of these cases.
>
> I dunno, I don't think your concerns about numbers of clocks (or any
> other such property) are unique to this particular use-case.
>
> > These
> > extras are included in the FPGA bitstream only for a specific type of
> > peripheral, not for general use of the SPI controller with any type of
> > peripheral.
>
> I accept that, but what I was trying to get across was that you could
> configure the FPGA with a bitstream that contains these extra resources
> and then connect a peripheral device that does not make use of them at
> all.

Sure, in this case the peripheral has no spi-offloads property and the
SPI controller acts as a typical SPI controller.

> Or you could connect a range of different peripherals that do use
> them.

OK, you've got me thinking about something I hadn't really thought about yet.

> Whether or not the resources are there and how they are connected
> in the FPGA bitstream is not a property of the device connected to the
> SPI controller, only whether or not you use them is.
>

Even when those things are connected directly to a specific peripheral
device? Or not connected to the offload?

Here is another potential ADC trigger case to consider.

+-------------------------------+   +------------------+
|                               |   |                  |
|  SOC/FPGA                     |   |  ADC             |
|  +---------------------+      |   |                  |
|  | AXI SPI Engine      |      |   |                  |
|  |             SPI Bus ============ SPI Bus          |
|  |                     |      |   |                  |
|  |  +---------------+  |  < < < < < BUSY             |
|  |  | Offload 0     |  | v    |   |                  |
|  |  |               |  | v  > > > > CNV              |
|  |  |    TRIGGER IN < < <   ^ |   |                  |
|  |  +---------------+  |    ^ |   +------------------+
|  +---------------------+    ^ |
|  | AXI PWM             |    ^ |
|  |                 CH0 >  > ^ |
|  +---------------------+      |
|                               |
+-------------------------------+

This time, the periodic trigger (PWM) is connected to the pin on the
ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
that will go high at the start of the conversion and go low at the end
of the conversion. The BUSY output of the ADC is wired as the hardware
trigger input of the offload.

In this case would we still consider the PWM as part of the SPI
controller/offload since it can only be used in conjunction with the
SPI offload? It isn't connected to it at all though.

Another case could be a self-clocked ADC. Here, the ADC has it's own
periodic sample trigger on the chip and the RDY output goes high
whenever a sample is ready to read.

+-------------------------------+   +------------------+
|                               |   |                  |
|  SOC/FPGA                     |   |  ADC             |
|  +---------------------+      |   |                  |
|  | AXI SPI Engine      |      |   |                  |
|  |             SPI Bus ============ SPI Bus          |
|  |                     |      |   |                  |
|  |  +---------------+  |  < < < < < RDY              |
|  |  | Offload 0     |  | v    |   |                  |
|  |  |               |  | v    |   |                  |
|  |  |    TRIGGER IN < < <     |   |                  |
|  |  +---------------+  |      |   +------------------+
|  +---------------------+      |
|                               |
+-------------------------------+

If both of these are valid wiring configurations for the same ADC,
wouldn't it make more sense to describe this in the peripheral node
rather than having to query the controller to see how the peripheral
is wired up?

> In fact, looking at the documentation for the "spi engine" some more, I
> am even less convinced that the right place for describing the offload is
> the peripheral as I (finally?) noticed that the registers for the offload
> engine are mapped directly into the "spi engine"'s memory map, rather than
> it being a entirely separate IP in the FPGA fabric.

True, but we don't use these registers, e.g., to configure the
sampling frequency of a trigger (if it can even be done). That is done
in a completely separate IP block with it's own registers.

>
> Further, what resources are available depends on what the SPI offload
> engine IP is. If my employer decides to start shipping some SPI offload
> IP in our catalogue that can work with ADI's ADCs, the set of offload
> related properties or their names may well differ.

If all of these resources were fairly generic, like the periodic
trigger we've been discussing, then I could see the case for trying to
accommodate this the same way we do for other common features of SPI
controllers. But for cases like "Application 2" that I described
previously, these resources can get very specific to a peripheral
(like the example given of having a data processing unit that knows
about the exact data format and CRC algorithm used by a specific ADC).
These seems like too specific of a thing to say that a SPI controller
"supports".

But, OK, let's go with the idea of putting everything related to the
offloads in the SPI controller node an see where it takes us...

spi@1000 {
    compatible = "adi,axi-spi-engine";
    #spi-offload-cells = <1>;
    /* PWM is connected to offload hardware trigger. DMA for streaming sample
     * data can only handle 16-bit words. IIO hardware buffer will be CPU-
     * endian because data is streamed one word at a time. */
    spi-offload-0-capabilities = "pwm-trigger", "16-bit-rx-dma";

    /* pwm properties are only allowed because spi-offload-0-capabilities
     * contains "pwm-trigger" */
    pwms = <&pwm 0>;
    pwm-names = "offload-0-pwm-trigger";

    /* dma properties are only allowed because spi-offload-0-capabilities
     * contains "16-bit-rx-dma" */
    dmas = <&dma 0>;
    dma-names = "offload-0-16-bit-rx";

    ...

    adc@0 {
        compatible = "adi,ad7944";
        spi-offloads = <0>;
        ...
    };
};

spi@2000 {
    compatible = "not-adi,other-spi-engine";
    #spi-offload-cells = <1>;
    /* Clock is connected to offload hardware trigger. DMA for streaming sample
     * data can only handle one byte at a time. IIO hardware buffer will be big-
     * endian because data is streamed one byte at a time. */
    spi-offload-0-capabilities = "clock-trigger", "8-bit-rx-dma";

    /* Clock properties are extended because spi-offload-0-capabilities contains
     * "clock-trigger" and SPI controller itself has a clock */
    clocks = <&cpu_clock 0>, <&extra_clock 0>;
    clock-names = "sclk", "offload-0-pwm-trigger";

    /* DMA properties are extended because spi-offload-0-capabilities contains
     * "8-bit-rx-dma". "tx" and "rx" are for non-offload use. */
    dmas = <&dma1 0>, <&dma1 1>, <&dma2 0>;
    dma-names = "tx", "rx", "offload-0-16-bit-rx";

    ...

    adc@0 {
        compatible = "adi,ad7944";
        spi-offloads = <0>;
        ...
    };
};

spi@3000 {
    compatible = "adi,axi-spi-engine";
    #spi-offload-cells = <1>;
    /* Sample ready signal (~BSY) from ADC is connected to offload hardware
     * trigger. DMA for streaming sample data can only handle 16-bit words. */
    spi-offload-0-capabilities = "sample-trigger", "16-bit-rx-dma";

    /* Do we need a property to say that the sample trigger is connected to
     * adc@0 so that if adc@1 tries to use it, it will fail? */

    /* dma properties are only allowed because spi-offload-0-capabilities
     * contains "16-bit-rx-dma" */
    dmas = <&dma 0>;
    dma-names = "offload-0-16-bit-rx";

    ...

    adc@0 {
        compatible = "adi,ad7944";
        spi-offloads = <0>;
        ...

        /* PWM connected to the conversion pin (CNV). This only makes sense
         * when offload is used with BSY signal, otherwise we would have CNV
         * connected to SPI CS. */
        pwms = <&pwm 0>;
        pwm-names = "cnv";
    };
};

spi@4000 {
    compatible = "adi,axi-spi-engine";
    #spi-offload-cells = <1>;
    /* Sample ready signal (~BSY) from ADC is connected to offload hardware
     * trigger. DMA for streaming sample data can only handle 32-bit words.
     * This also has the CRC validation that strips off the CRC byte of the
     * raw data before passing the sample to DMA. */
    spi-offload-0-capabilities = "sample-trigger",
                                 "32-bit-rx-dma-with-ad4630-crc-check";

    /* dma properties are only allowed because spi-offload-0-capabilities
     * contains "16-bit-rx-dma" */
    dmas = <&dma 0>;
    dma-names = "offload-0-16-bit-rx";

    interrupt-parent = <&intc>;
    /* First interrupt is for the SPI controller (always present), second
     * interrupt is CRC error from the "32-bit-rx-dma-with-ad4630-crc-check"
     * of offload 0. */
    interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>, <0 58 IRQ_TYPE_LEVEL_HIGH>;
    interrupt-names = "controller", "offload-0-crc-error";

    ...

    adc@0 {
        compatible = "adi,ad4630";
        spi-offloads = <0>;
        ...

        /* PWM connected to the conversion pin (CNV). Without offload, we would
         * have cnv-gpios instead. */
        pwms = <&pwm 0>;
        pwm-names = "cnv";
    };
};

So this is what I came up with of how things could look (combining
suggestions from Rob in v1 and Conor's suggestions here). I can see
how we can make this work. But the thing I don't like about it is that
the peripheral drivers still need to know all of the information about
the offload capabilities and need to interact with the
pwms/clocks/interrupts/dmas/etc. So this is just adding layers of
indirection where all of this stuff has to go through the SPI
controller driver.


>
> > Another idea I had was to perhaps use the recently added IIO backend
> > framework for the "extras". The idea there is that we are creating a
> > "composite" IIO device that consists of the ADC chip (frontend) plus
> > these extra hardware trigger and hardware buffer functions provided by
> > the FPGA (backend).
> >
> > offload_backend: adc0-backend {
> >     /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */
> >     compatible = "adi,pulsar-adc-offload";
> >     #io-backend-cells = <0>;
> >     dmas = <&dma 0>;
> >     dma-names = "rx";
> >     clocks = <&trigger_clock>;
> > };
> >
> > spi {
> >     ...
> >     adc@0 {
> >         ...
> >         spi-offloads = <0>;
> >         io-backends = <&offload_backend>;
> >     };
> > };
> >
> > While this could be a solution for IIO devices, this wouldn't solve
> > the issue in general though for SPI offloads used with non-IIO
> > peripherals.
>
> Yeah, I agree that using something specific to IIO isn't really a good
> solution.
>
> Cheers,
> Conor.
>
> > So I don't think it is the right thing to do here. But, I
> > think this idea of a "composite" device helps explain why we are
> > pushing for putting the "extras" with the peripheral node rather than
> > the controller node, at least for the specific case of the AXI SPI
> > Engine controller.
>

Conor Dooley May 22, 2024, 6:24 p.m. UTC | #9

On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
> >
> > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
> > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> >
> > > > > 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.
> > > >
> > > > It seems very strange to me that the dmas and the clock triggers are
> > > > going into the spi device nodes. The description is
> > > > | +  dmas:
> > > > | +    maxItems: 1
> > > > | +    description: RX DMA Channel for receiving a samples from the SPI offload.
> > > > But as far as I can tell this device is in a package of its own and not
> > > > some IP provided by Analog that an engine on the FPGA can actually do
> > > > DMA to, and the actual connection of the device is "just" SPI.
> > > > The dmas and clock triggers etc appear to be resources belonging to the
> > > > controller that can "assigned" to a particular spi device. If the adc
> > > > gets disconnected from the system, the dmas and clock triggers are still
> > > > connected to the spi controller/offload engine, they don't end up n/c,
> > > > right? (Well maybe they would in the case of a fancy SPI device that
> > > > provides it's own sampling clock or w/e, but then it'd be a clock
> > > > provider of sorts). I'd be expecting the spi-offloads property to be
> > > > responsible for selecting which of the various resources belonging to
> > > > the controller are to be used by a device.
> > > > Maybe it overcomplicates the shit out of things and Rob or Mark are
> > > > gonna start screaming at me but w/e, looking at it from the point of
> > > > view of how the hardware is laid out (or at least how it is described
> > > > in your FPGA case above) the dma/clock properties looks like they're
> > > > misplaced. IOW, I don't think that adding the spi-offloads property
> > > > should convert a node from representing an ADC in a qfn-20 or w/e
> > > > to "the ADC chip plus everything connected to the offload instance
> > > > used by the ADC chip".
> > >
> > > This is the same reasoning that led me to the binding proposed in v1.
> > > Rob suggested that these extras (dmas/clocks) should just be
> > > properties directly of the SPI controller.
> >
> > > But the issue I have with
> > > that is that since this is an FPGA, these properties are not fixed.
> >
> > I don't think whether or not we're talking about an FPGA or an ASIC
> > matters at all here to be honest. In my view the main thing that FPGA
> > impact in terms of bindings is the number of properties required to
> > represent the configurability of a particular IP. Sure, you're gonna
> > have to change the dts around in a way you'll never have to with an
> > ASIC, but the description of individual devices or relations between
> > them doesn't change.
> >
> > > Maybe there are more clocks or no clocks or interrupts or something we
> > > didn't think of yet.
> >
> > This could happen with a new generation of an ASIC and is not specific
> > to an FPGA IP core. Even with FPGA IP, while there may be lots of
> > different configuration parameters, they are known & limited - you can
> > look at the IP's documentation (or failing that, the HDL) to figure out
> > what the points of variability are. It's not particularly difficult to
> > account for there being a configurable number of clocks or interrupts.
> > For "something we didn't think of yet" to be a factor, someone has to
> > think of it and add it to the IP core, and which point we can absolutely
> > change the bindings and software to account for the revised IP.
> >
> > > So it doesn't really seem possible to write a
> > > binding for the SPI controller node to cover all of these cases.
> >
> > I dunno, I don't think your concerns about numbers of clocks (or any
> > other such property) are unique to this particular use-case.
> >
> > > These
> > > extras are included in the FPGA bitstream only for a specific type of
> > > peripheral, not for general use of the SPI controller with any type of
> > > peripheral.
> >
> > I accept that, but what I was trying to get across was that you could
> > configure the FPGA with a bitstream that contains these extra resources
> > and then connect a peripheral device that does not make use of them at
> > all.
> 
> Sure, in this case the peripheral has no spi-offloads property and the
> SPI controller acts as a typical SPI controller.
> 
> > Or you could connect a range of different peripherals that do use
> > them.
> 
> OK, you've got me thinking about something I hadn't really thought about yet.
> 
> > Whether or not the resources are there and how they are connected
> > in the FPGA bitstream is not a property of the device connected to the
> > SPI controller, only whether or not you use them is.
> >
> 
> Even when those things are connected directly to a specific peripheral
> device? Or not connected to the offload?
> 
> Here is another potential ADC trigger case to consider.
> 
> +-------------------------------+   +------------------+
> |                               |   |                  |
> |  SOC/FPGA                     |   |  ADC             |
> |  +---------------------+      |   |                  |
> |  | AXI SPI Engine      |      |   |                  |
> |  |             SPI Bus ============ SPI Bus          |
> |  |                     |      |   |                  |
> |  |  +---------------+  |  < < < < < BUSY             |
> |  |  | Offload 0     |  | v    |   |                  |
> |  |  |               |  | v  > > > > CNV              |
> |  |  |    TRIGGER IN < < <   ^ |   |                  |
> |  |  +---------------+  |    ^ |   +------------------+
> |  +---------------------+    ^ |
> |  | AXI PWM             |    ^ |
> |  |                 CH0 >  > ^ |
> |  +---------------------+      |
> |                               |
> +-------------------------------+
> 
> This time, the periodic trigger (PWM) is connected to the pin on the
> ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
> that will go high at the start of the conversion and go low at the end
> of the conversion. The BUSY output of the ADC is wired as the hardware
> trigger input of the offload.
> 
> In this case would we still consider the PWM as part of the SPI
> controller/offload since it can only be used in conjunction with the
> SPI offload? It isn't connected to it at all though.

No, in this case the ADC is a PWM consumer and the offload engine is
not. The ADC is a "trigger" provider and the SPI offload engine is a
trigger consumer.

> Another case could be a self-clocked ADC. Here, the ADC has it's own
> periodic sample trigger on the chip and the RDY output goes high
> whenever a sample is ready to read.
> 
> +-------------------------------+   +------------------+
> |                               |   |                  |
> |  SOC/FPGA                     |   |  ADC             |
> |  +---------------------+      |   |                  |
> |  | AXI SPI Engine      |      |   |                  |
> |  |             SPI Bus ============ SPI Bus          |
> |  |                     |      |   |                  |
> |  |  +---------------+  |  < < < < < RDY              |
> |  |  | Offload 0     |  | v    |   |                  |
> |  |  |               |  | v    |   |                  |
> |  |  |    TRIGGER IN < < <     |   |                  |
> |  |  +---------------+  |      |   +------------------+
> |  +---------------------+      |
> |                               |
> +-------------------------------+
> 
> If both of these are valid wiring configurations for the same ADC,
> wouldn't it make more sense to describe this in the peripheral node
> rather than having to query the controller to see how the peripheral
> is wired up?

In both of these cases, the offload works in the same way, it gets a
trigger from the ADC and acts upon it. I would expect that in this case
the ADC driver would look for an optional pwm property in the devicetree
and if it is present configure the ADC to use that and if absent do then
do whatever configuration is required for self clocking. I would expect
that both cases would be handled identically by the SPI [offload] engine
side of things, other than inverting the polarity of the trigger. (If I
understand correctly, you want to trigger the offload engine on a
falling edge of BUSY but a rising edge of RDY).


> > In fact, looking at the documentation for the "spi engine" some more, I
> > am even less convinced that the right place for describing the offload is
> > the peripheral as I (finally?) noticed that the registers for the offload
> > engine are mapped directly into the "spi engine"'s memory map, rather than
> > it being a entirely separate IP in the FPGA fabric.
> 
> True, but we don't use these registers, e.g., to configure the
> sampling frequency of a trigger (if it can even be done). That is done
> in a completely separate IP block with it's own registers.

Where is the binding for that IP block? I think describing that is
pretty key. goto continuation;

> > Further, what resources are available depends on what the SPI offload
> > engine IP is. If my employer decides to start shipping some SPI offload
> > IP in our catalogue that can work with ADI's ADCs, the set of offload
> > related properties or their names may well differ.
> 
> If all of these resources were fairly generic, like the periodic
> trigger we've been discussing, then I could see the case for trying to
> accommodate this the same way we do for other common features of SPI
> controllers. But for cases like "Application 2" that I described
> previously, these resources can get very specific to a peripheral
> (like the example given of having a data processing unit that knows
> about the exact data format and CRC algorithm used by a specific ADC).
> These seems like too specific of a thing to say that a SPI controller
> "supports".

To remind myself, "Application 2" featured an offload engine designed
specifically to work with a particular data format that would strip a
CRC byte and check the validity of the data stream.

I think you're right something like that is a stretch to say that that
is a feature of the SPI controller - but I still don't believe that
modelling it as part of the ADC is correct. I don't fully understand the
io-backends and how they work yet, but the features you describe there
seem like something that should/could be modelled as one, with its own
node and compatible etc. Describing custom RTL stuff ain't always
strightforward, but the stuff from Analog is versioned and documented
etc so it shouldn't be quite that hard.

continuation:
If offload engines have their own register region in the memory map,
their own resources (the RTL is gonna need at the very least a clock)
and potentially also provide other services (like acting as an
io-backend type device that pre-processes data) it doesn't sound like
either the controller or peripheral nodes are the right place for these
properties. And uh, spi-offloads gets a phandle once more...

FWIW, I did read these examples but didn't feel it was worth commenting
on them given the above. I'll comment on them if they stay "accurate".

Cheers,
Conor.

> But, OK, let's go with the idea of putting everything related to the
> offloads in the SPI controller node an see where it takes us...
> 
> spi@1000 {
>     compatible = "adi,axi-spi-engine";
>     #spi-offload-cells = <1>;
>     /* PWM is connected to offload hardware trigger. DMA for streaming sample
>      * data can only handle 16-bit words. IIO hardware buffer will be CPU-
>      * endian because data is streamed one word at a time. */
>     spi-offload-0-capabilities = "pwm-trigger", "16-bit-rx-dma";
> 
>     /* pwm properties are only allowed because spi-offload-0-capabilities
>      * contains "pwm-trigger" */
>     pwms = <&pwm 0>;
>     pwm-names = "offload-0-pwm-trigger";
> 
>     /* dma properties are only allowed because spi-offload-0-capabilities
>      * contains "16-bit-rx-dma" */
>     dmas = <&dma 0>;
>     dma-names = "offload-0-16-bit-rx";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad7944";
>         spi-offloads = <0>;
>         ...
>     };
> };
> 
> spi@2000 {
>     compatible = "not-adi,other-spi-engine";
>     #spi-offload-cells = <1>;
>     /* Clock is connected to offload hardware trigger. DMA for streaming sample
>      * data can only handle one byte at a time. IIO hardware buffer will be big-
>      * endian because data is streamed one byte at a time. */
>     spi-offload-0-capabilities = "clock-trigger", "8-bit-rx-dma";
> 
>     /* Clock properties are extended because spi-offload-0-capabilities contains
>      * "clock-trigger" and SPI controller itself has a clock */
>     clocks = <&cpu_clock 0>, <&extra_clock 0>;
>     clock-names = "sclk", "offload-0-pwm-trigger";
> 
>     /* DMA properties are extended because spi-offload-0-capabilities contains
>      * "8-bit-rx-dma". "tx" and "rx" are for non-offload use. */
>     dmas = <&dma1 0>, <&dma1 1>, <&dma2 0>;
>     dma-names = "tx", "rx", "offload-0-16-bit-rx";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad7944";
>         spi-offloads = <0>;
>         ...
>     };
> };
> 
> spi@3000 {
>     compatible = "adi,axi-spi-engine";
>     #spi-offload-cells = <1>;
>     /* Sample ready signal (~BSY) from ADC is connected to offload hardware
>      * trigger. DMA for streaming sample data can only handle 16-bit words. */
>     spi-offload-0-capabilities = "sample-trigger", "16-bit-rx-dma";
> 
>     /* Do we need a property to say that the sample trigger is connected to
>      * adc@0 so that if adc@1 tries to use it, it will fail? */
> 
>     /* dma properties are only allowed because spi-offload-0-capabilities
>      * contains "16-bit-rx-dma" */
>     dmas = <&dma 0>;
>     dma-names = "offload-0-16-bit-rx";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad7944";
>         spi-offloads = <0>;
>         ...
> 
>         /* PWM connected to the conversion pin (CNV). This only makes sense
>          * when offload is used with BSY signal, otherwise we would have CNV
>          * connected to SPI CS. */
>         pwms = <&pwm 0>;
>         pwm-names = "cnv";
>     };
> };
> 
> spi@4000 {
>     compatible = "adi,axi-spi-engine";
>     #spi-offload-cells = <1>;
>     /* Sample ready signal (~BSY) from ADC is connected to offload hardware
>      * trigger. DMA for streaming sample data can only handle 32-bit words.
>      * This also has the CRC validation that strips off the CRC byte of the
>      * raw data before passing the sample to DMA. */
>     spi-offload-0-capabilities = "sample-trigger",
>                                  "32-bit-rx-dma-with-ad4630-crc-check";
> 
>     /* dma properties are only allowed because spi-offload-0-capabilities
>      * contains "16-bit-rx-dma" */
>     dmas = <&dma 0>;
>     dma-names = "offload-0-16-bit-rx";
> 
>     interrupt-parent = <&intc>;
>     /* First interrupt is for the SPI controller (always present), second
>      * interrupt is CRC error from the "32-bit-rx-dma-with-ad4630-crc-check"
>      * of offload 0. */
>     interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>, <0 58 IRQ_TYPE_LEVEL_HIGH>;
>     interrupt-names = "controller", "offload-0-crc-error";
> 
>     ...
> 
>     adc@0 {
>         compatible = "adi,ad4630";
>         spi-offloads = <0>;
>         ...
> 
>         /* PWM connected to the conversion pin (CNV). Without offload, we would
>          * have cnv-gpios instead. */
>         pwms = <&pwm 0>;
>         pwm-names = "cnv";
>     };
> };
> 
> So this is what I came up with of how things could look (combining
> suggestions from Rob in v1 and Conor's suggestions here). I can see
> how we can make this work. But the thing I don't like about it is that
> the peripheral drivers still need to know all of the information about
> the offload capabilities and need to interact with the
> pwms/clocks/interrupts/dmas/etc. So this is just adding layers of
> indirection where all of this stuff has to go through the SPI
> controller driver.
> 
> 
> >
> > > Another idea I had was to perhaps use the recently added IIO backend
> > > framework for the "extras". The idea there is that we are creating a
> > > "composite" IIO device that consists of the ADC chip (frontend) plus
> > > these extra hardware trigger and hardware buffer functions provided by
> > > the FPGA (backend).
> > >
> > > offload_backend: adc0-backend {
> > >     /* http://analogdevicesinc.github.io/hdl/projects/pulsar_adc/index.html */
> > >     compatible = "adi,pulsar-adc-offload";
> > >     #io-backend-cells = <0>;
> > >     dmas = <&dma 0>;
> > >     dma-names = "rx";
> > >     clocks = <&trigger_clock>;
> > > };
> > >
> > > spi {
> > >     ...
> > >     adc@0 {
> > >         ...
> > >         spi-offloads = <0>;
> > >         io-backends = <&offload_backend>;
> > >     };
> > > };
> > >
> > > While this could be a solution for IIO devices, this wouldn't solve
> > > the issue in general though for SPI offloads used with non-IIO
> > > peripherals.
> >
> > Yeah, I agree that using something specific to IIO isn't really a good
> > solution.
> >
> > Cheers,
> > Conor.
> >
> > > So I don't think it is the right thing to do here. But, I
> > > think this idea of a "composite" device helps explain why we are
> > > pushing for putting the "extras" with the peripheral node rather than
> > > the controller node, at least for the specific case of the AXI SPI
> > > Engine controller.
> >

Nuno Sá May 23, 2024, 12:15 p.m. UTC | #10

On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
> > > 
> > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
> > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > > 
> > > > > > 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.
> > > > > 
> > > > > It seems very strange to me that the dmas and the clock triggers are
> > > > > going into the spi device nodes. The description is
> > > > > > +  dmas:
> > > > > > +    maxItems: 1
> > > > > > +    description: RX DMA Channel for receiving a samples from the SPI
> > > > > > offload.
> > > > > But as far as I can tell this device is in a package of its own and not
> > > > > some IP provided by Analog that an engine on the FPGA can actually do
> > > > > DMA to, and the actual connection of the device is "just" SPI.
> > > > > The dmas and clock triggers etc appear to be resources belonging to the
> > > > > controller that can "assigned" to a particular spi device. If the adc
> > > > > gets disconnected from the system, the dmas and clock triggers are still
> > > > > connected to the spi controller/offload engine, they don't end up n/c,
> > > > > right? (Well maybe they would in the case of a fancy SPI device that
> > > > > provides it's own sampling clock or w/e, but then it'd be a clock
> > > > > provider of sorts). I'd be expecting the spi-offloads property to be
> > > > > responsible for selecting which of the various resources belonging to
> > > > > the controller are to be used by a device.
> > > > > Maybe it overcomplicates the shit out of things and Rob or Mark are
> > > > > gonna start screaming at me but w/e, looking at it from the point of
> > > > > view of how the hardware is laid out (or at least how it is described
> > > > > in your FPGA case above) the dma/clock properties looks like they're
> > > > > misplaced. IOW, I don't think that adding the spi-offloads property
> > > > > should convert a node from representing an ADC in a qfn-20 or w/e
> > > > > to "the ADC chip plus everything connected to the offload instance
> > > > > used by the ADC chip".
> > > > 
> > > > This is the same reasoning that led me to the binding proposed in v1.
> > > > Rob suggested that these extras (dmas/clocks) should just be
> > > > properties directly of the SPI controller.
> > > 
> > > > But the issue I have with
> > > > that is that since this is an FPGA, these properties are not fixed.
> > > 
> > > I don't think whether or not we're talking about an FPGA or an ASIC
> > > matters at all here to be honest. In my view the main thing that FPGA
> > > impact in terms of bindings is the number of properties required to
> > > represent the configurability of a particular IP. Sure, you're gonna
> > > have to change the dts around in a way you'll never have to with an
> > > ASIC, but the description of individual devices or relations between
> > > them doesn't change.
> > > 
> > > > Maybe there are more clocks or no clocks or interrupts or something we
> > > > didn't think of yet.
> > > 
> > > This could happen with a new generation of an ASIC and is not specific
> > > to an FPGA IP core. Even with FPGA IP, while there may be lots of
> > > different configuration parameters, they are known & limited - you can
> > > look at the IP's documentation (or failing that, the HDL) to figure out
> > > what the points of variability are. It's not particularly difficult to
> > > account for there being a configurable number of clocks or interrupts.
> > > For "something we didn't think of yet" to be a factor, someone has to
> > > think of it and add it to the IP core, and which point we can absolutely
> > > change the bindings and software to account for the revised IP.
> > > 
> > > > So it doesn't really seem possible to write a
> > > > binding for the SPI controller node to cover all of these cases.
> > > 
> > > I dunno, I don't think your concerns about numbers of clocks (or any
> > > other such property) are unique to this particular use-case.
> > > 
> > > > These
> > > > extras are included in the FPGA bitstream only for a specific type of
> > > > peripheral, not for general use of the SPI controller with any type of
> > > > peripheral.
> > > 
> > > I accept that, but what I was trying to get across was that you could
> > > configure the FPGA with a bitstream that contains these extra resources
> > > and then connect a peripheral device that does not make use of them at
> > > all.
> > 
> > Sure, in this case the peripheral has no spi-offloads property and the
> > SPI controller acts as a typical SPI controller.
> > 
> > > Or you could connect a range of different peripherals that do use
> > > them.
> > 
> > OK, you've got me thinking about something I hadn't really thought about yet.
> > 
> > > Whether or not the resources are there and how they are connected
> > > in the FPGA bitstream is not a property of the device connected to the
> > > SPI controller, only whether or not you use them is.
> > > 
> > 
> > Even when those things are connected directly to a specific peripheral
> > device? Or not connected to the offload?
> > 
> > Here is another potential ADC trigger case to consider.
> > 
> > +-------------------------------+   +------------------+
> > >                               |   |                  |
> > >  SOC/FPGA                     |   |  ADC             |
> > >  +---------------------+      |   |                  |
> > >  | AXI SPI Engine      |      |   |                  |
> > >  |             SPI Bus ============ SPI Bus          |
> > >  |                     |      |   |                  |
> > >  |  +---------------+  |  < < < < < BUSY             |
> > >  |  | Offload 0     |  | v    |   |                  |
> > >  |  |               |  | v  > > > > CNV              |
> > >  |  |    TRIGGER IN < < <   ^ |   |                  |
> > >  |  +---------------+  |    ^ |   +------------------+
> > >  +---------------------+    ^ |
> > >  | AXI PWM             |    ^ |
> > >  |                 CH0 >  > ^ |
> > >  +---------------------+      |
> > >                               |
> > +-------------------------------+
> > 
> > This time, the periodic trigger (PWM) is connected to the pin on the
> > ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
> > that will go high at the start of the conversion and go low at the end
> > of the conversion. The BUSY output of the ADC is wired as the hardware
> > trigger input of the offload.
> > 
> > In this case would we still consider the PWM as part of the SPI
> > controller/offload since it can only be used in conjunction with the
> > SPI offload? It isn't connected to it at all though.
> 
> No, in this case the ADC is a PWM consumer and the offload engine is
> not. The ADC is a "trigger" provider and the SPI offload engine is a
> trigger consumer.
> 
> > Another case could be a self-clocked ADC. Here, the ADC has it's own
> > periodic sample trigger on the chip and the RDY output goes high
> > whenever a sample is ready to read.
> > 
> > +-------------------------------+   +------------------+
> > >                               |   |                  |
> > >  SOC/FPGA                     |   |  ADC             |
> > >  +---------------------+      |   |                  |
> > >  | AXI SPI Engine      |      |   |                  |
> > >  |             SPI Bus ============ SPI Bus          |
> > >  |                     |      |   |                  |
> > >  |  +---------------+  |  < < < < < RDY              |
> > >  |  | Offload 0     |  | v    |   |                  |
> > >  |  |               |  | v    |   |                  |
> > >  |  |    TRIGGER IN < < <     |   |                  |
> > >  |  +---------------+  |      |   +------------------+
> > >  +---------------------+      |
> > >                               |
> > +-------------------------------+
> > 
> > If both of these are valid wiring configurations for the same ADC,
> > wouldn't it make more sense to describe this in the peripheral node
> > rather than having to query the controller to see how the peripheral
> > is wired up?
> 
> In both of these cases, the offload works in the same way, it gets a
> trigger from the ADC and acts upon it. I would expect that in this case
> the ADC driver would look for an optional pwm property in the devicetree
> and if it is present configure the ADC to use that and if absent do then
> do whatever configuration is required for self clocking. I would expect
> that both cases would be handled identically by the SPI [offload] engine
> side of things, other than inverting the polarity of the trigger. (If I
> understand correctly, you want to trigger the offload engine on a
> falling edge of BUSY but a rising edge of RDY).
> 
> 
> > > In fact, looking at the documentation for the "spi engine" some more, I
> > > am even less convinced that the right place for describing the offload is
> > > the peripheral as I (finally?) noticed that the registers for the offload
> > > engine are mapped directly into the "spi engine"'s memory map, rather than
> > > it being a entirely separate IP in the FPGA fabric.
> > 
> > True, but we don't use these registers, e.g., to configure the
> > sampling frequency of a trigger (if it can even be done). That is done
> > in a completely separate IP block with it's own registers.
> 
> Where is the binding for that IP block? I think describing that is
> pretty key. goto continuation;
> 
> > > Further, what resources are available depends on what the SPI offload
> > > engine IP is. If my employer decides to start shipping some SPI offload
> > > IP in our catalogue that can work with ADI's ADCs, the set of offload
> > > related properties or their names may well differ.
> > 
> > If all of these resources were fairly generic, like the periodic
> > trigger we've been discussing, then I could see the case for trying to
> > accommodate this the same way we do for other common features of SPI
> > controllers. But for cases like "Application 2" that I described
> > previously, these resources can get very specific to a peripheral
> > (like the example given of having a data processing unit that knows
> > about the exact data format and CRC algorithm used by a specific ADC).
> > These seems like too specific of a thing to say that a SPI controller
> > "supports".
> 
> To remind myself, "Application 2" featured an offload engine designed
> specifically to work with a particular data format that would strip a
> CRC byte and check the validity of the data stream.
> 

I think the data manipulation is not really a property of the engine. Typically data
going out of the offload engine goes into another "data reorder" block that is pure
HW.

> I think you're right something like that is a stretch to say that that
> is a feature of the SPI controller - but I still don't believe that
> modelling it as part of the ADC is correct. I don't fully understand the
> io-backends and how they work yet, but the features you describe there
> seem like something that should/could be modelled as one, with its own
> node and compatible etc. Describing custom RTL stuff ain't always
> strightforward, but the stuff from Analog is versioned and documented
> etc so it shouldn't be quite that hard.
> 

Putting this in io-backends is likely a stretch but one thing to add is that the
peripheral is always (I think) kind of the consumer of the resources. Taking the
trigger (PWM) as an example and even when it is directly connected with the offload
block, the peripheral still needs to know about it. Think of sampling frequency...
The period of the trigger signal is strictly connected with the sampling frequency of
the peripheral for example. So I see 2 things:

1) Enabling/Disabling the trigger could be easily done from the peripheral even with
the resource in the spi engine. I think David already has some code in the series
that would make this trivial and so having the property in the spi controller brings
no added complexity.

2) Controlling things like the trigger period/sample_rate. This could be harder to do
over SPI (or making it generic enough) so we would still need to have the same
property on the peripheral (even if not directly connected to it). I kind of agree
with David that having the property both in the peripheral and controller is a bit
weird.

And the DMA block is a complete different story. Sharing that data back with the
peripheral driver (in this case, the IIO subsystem) would be very interesting at the
very least. Note that the DMA block is not really something that is part of the
controller nor the offload block. It is an external block that gets the data coming
out of the offload engine (or the data reorder block). In IIO, we already have a DMA
buffer interface so users of the peripheral can get the data without any intervention
of the driver (on the data). We "just" enable buffering and then everything happens
on HW and userspace can start requesting data. If we are going to attach the DMA in
the controller, I have no idea how we can handle it. Moreover, the offload it's
really just a way of replaying the same spi transfer over and over and that happens
in HW so I'm not sure how we could "integrate" that with dmaengine.

But maybe I'm overlooking things... And thinking more in how this can be done in SW
rather than what makes sense from an HW perspective.


> continuation:
> If offload engines have their own register region in the memory map,


Don't think it has it's own register region... David?

> their own resources (the RTL is gonna need at the very least a clock)
> and potentially also provide other services (like acting as an
> io-backend type device that pre-processes data) it doesn't sound like
> either the controller or peripheral nodes are the right place for these
> properties. And uh, spi-offloads gets a phandle once more...
> 

But then it would be valid for both peripheral and controller to reference that
phandle right (and get the resources of interest)?

FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful
to you:

https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl

- Nuno Sá

Mark Brown May 23, 2024, 12:45 p.m. UTC | #11

On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
> > > > 
> > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

David Lechner May 23, 2024, 2:28 p.m. UTC | #12

On 5/22/24 1:24 PM, Conor Dooley wrote:
> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
>>>
>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
>>>

...

>> This time, the periodic trigger (PWM) is connected to the pin on the
>> ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
>> that will go high at the start of the conversion and go low at the end
>> of the conversion. The BUSY output of the ADC is wired as the hardware
>> trigger input of the offload.
>>
>> In this case would we still consider the PWM as part of the SPI
>> controller/offload since it can only be used in conjunction with the
>> SPI offload? It isn't connected to it at all though.
> 
> No, in this case the ADC is a PWM consumer and the offload engine is
> not. The ADC is a "trigger" provider and the SPI offload engine is a
> trigger consumer.

Makes sense.

...


> 
>>> In fact, looking at the documentation for the "spi engine" some more, I
>>> am even less convinced that the right place for describing the offload is
>>> the peripheral as I (finally?) noticed that the registers for the offload
>>> engine are mapped directly into the "spi engine"'s memory map, rather than
>>> it being a entirely separate IP in the FPGA fabric.
>>
>> True, but we don't use these registers, e.g., to configure the
>> sampling frequency of a trigger (if it can even be done). That is done
>> in a completely separate IP block with it's own registers.
> 
> Where is the binding for that IP block? I think describing that is
> pretty key. goto continuation;

For the real-world case I used to test this series, it is an AXI PWMGEN
[1] that is providing the trigger event source. It has a typical PWM
provider binding with #pwm-cells [2].

Calling this a "trigger" provider to the SPI offload instance just like the
case above where the ADC is directly connected as the offload trigger makes
sense to me.

What I was going for in this patch (slightly revised to use #cells) is that
this trigger provider, whatever it is, is selected by one of the cells of
#spi-offload-cells. It doesn't seem like there should be a special case for
if the trigger provider is a clock or PWM where the SPI controller node
becomes a consumer of the clock or PWM provider rather than just describing
the trigger relationship.

For example, supposing we had an FPGA/HDL that could handle all 3 wiring
configurations we have discussed so far. A) PWM connected directly to SPI
offload as trigger, B) PWM connected to CNV of ADC and BSY of ADC connected
to SPI offload as trigger, C) self clocked ADC with RDY of ADC connected
to SPI offload as trigger. So the DT would have:

controller:
#spi-offload-cells = <2>: /* 1st cell = offload instance
                           * 2nd cell = trigger provider */

peripheral (choose one based on actual wiring):
spi-offloads = <0 0>; /* case A */
spi-offloads = <0 1>; /* case B */
spi-offloads = <0 2>; /* case C */


As to what is the actual consumer of the PWM provider in each of these
cases...

* C is easy. There isn't a PWM provider since the ADC is self-clocked.
* B, as discussed elsewhere is fairly straight forward. The ADC node is
  the consumer since the PWM is connected directly to the ADC.
* A is the one we need to figure out. I'm proposing that the PWM consumer
  should be whatever kind of composite device node we come up with that
  also solves the issue described below about where does the CRC checker
  (or whatever) go. I think we are in agreement here at least on the point
  that it doesn't belong in the SPI controller node?

[1]: http://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html
[2]: https://lore.kernel.org/linux-pwm/[e-mail]/



> 
> I think you're right something like that is a stretch to say that that
> is a feature of the SPI controller - but I still don't believe that
> modelling it as part of the ADC is correct. I don't fully understand the
> io-backends and how they work yet, but the features you describe there
> seem like something that should/could be modelled as one, with its own
> node and compatible etc. Describing custom RTL stuff ain't always
> strightforward, but the stuff from Analog is versioned and documented
> etc so it shouldn't be quite that hard.
> 
> continuation:
> If offload engines have their own register region in the memory map,
> their own resources (the RTL is gonna need at the very least a clock)
> and potentially also provide other services (like acting as an
> io-backend type device that pre-processes data) it doesn't sound like
> either the controller or peripheral nodes are the right place for these
> properties. And uh, spi-offloads gets a phandle once more...
>

Conor Dooley May 23, 2024, 2:31 p.m. UTC | #13

On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
> > > > 
> > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
> > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > > > 
> > I think you're right something like that is a stretch to say that that
> > is a feature of the SPI controller - but I still don't believe that
> > modelling it as part of the ADC is correct. I don't fully understand the
> > io-backends and how they work yet, but the features you describe there
> > seem like something that should/could be modelled as one, with its own
> > node and compatible etc. Describing custom RTL stuff ain't always
> > strightforward, but the stuff from Analog is versioned and documented
> > etc so it shouldn't be quite that hard.
> > 
> 
> Putting this in io-backends is likely a stretch but one thing to add is that the
> peripheral is always (I think) kind of the consumer of the resources. Taking the
> trigger (PWM) as an example and even when it is directly connected with the offload
> block, the peripheral still needs to know about it. Think of sampling frequency...
> The period of the trigger signal is strictly connected with the sampling frequency of
> the peripheral for example. So I see 2 things:

Cherry picking this one thing to reply to cos I'm not sure if it was
understood as I intended. When I talked about io-backends I was not
suggesting that we drop the spi-offload idea, I was suggesting that if
something has a dedicated register region & resources that handles both
offloading and some usecase specific data processing that it should be a
device of its own, acting as a provider of both spi-offloads and
io-backends.
I'll look at the rest of the mail soonTM.

Nuno Sá May 23, 2024, 2:57 p.m. UTC | #14

On Thu, 2024-05-23 at 09:28 -0500, David Lechner wrote:
> On 5/22/24 1:24 PM, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
> > > > 
> > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
> > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > > > 
> 

...

> 
> controller:
> #spi-offload-cells = <2>: /* 1st cell = offload instance
>                            * 2nd cell = trigger provider */
> 

What about things like DMA? I'm mentioning it a lot because it's way more complex
having it on the controller (from a SW perspective). But from an HW point of view,
it's always very similar (if not the same) as your case A.

- Nuno Sá
>

David Lechner May 23, 2024, 3:05 p.m. UTC | #15

On 5/23/24 7:15 AM, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
>> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
>>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
>>>>
>>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
>>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
>>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
>>>>

...

>> To remind myself, "Application 2" featured an offload engine designed
>> specifically to work with a particular data format that would strip a
>> CRC byte and check the validity of the data stream.
>>
> 
> I think the data manipulation is not really a property of the engine. Typically data
> going out of the offload engine goes into another "data reorder" block that is pure
> HW.
> 
>> I think you're right something like that is a stretch to say that that
>> is a feature of the SPI controller - but I still don't believe that
>> modelling it as part of the ADC is correct. I don't fully understand the
>> io-backends and how they work yet, but the features you describe there
>> seem like something that should/could be modelled as one, with its own
>> node and compatible etc. Describing custom RTL stuff ain't always
>> strightforward, but the stuff from Analog is versioned and documented
>> etc so it shouldn't be quite that hard.
>>
> 
> Putting this in io-backends is likely a stretch but one thing to add is that the
> peripheral is always (I think) kind of the consumer of the resources. Taking the
> trigger (PWM) as an example and even when it is directly connected with the offload
> block, the peripheral still needs to know about it. Think of sampling frequency...
> The period of the trigger signal is strictly connected with the sampling frequency of
> the peripheral for example. So I see 2 things:
> 
> 1) Enabling/Disabling the trigger could be easily done from the peripheral even with
> the resource in the spi engine. I think David already has some code in the series
> that would make this trivial and so having the property in the spi controller brings
> no added complexity.
> 
> 2) Controlling things like the trigger period/sample_rate. This could be harder to do
> over SPI (or making it generic enough) so we would still need to have the same
> property on the peripheral (even if not directly connected to it). I kind of agree
> with David that having the property both in the peripheral and controller is a bit
> weird.
> 
> And the DMA block is a complete different story. Sharing that data back with the
> peripheral driver (in this case, the IIO subsystem) would be very interesting at the
> very least. Note that the DMA block is not really something that is part of the
> controller nor the offload block. It is an external block that gets the data coming
> out of the offload engine (or the data reorder block). In IIO, we already have a DMA
> buffer interface so users of the peripheral can get the data without any intervention
> of the driver (on the data). We "just" enable buffering and then everything happens
> on HW and userspace can start requesting data. If we are going to attach the DMA in
> the controller, I have no idea how we can handle it. Moreover, the offload it's
> really just a way of replaying the same spi transfer over and over and that happens
> in HW so I'm not sure how we could "integrate" that with dmaengine.
> 
> But maybe I'm overlooking things... And thinking more in how this can be done in SW
> rather than what makes sense from an HW perspective.
> 
> 
>> continuation:
>> If offload engines have their own register region in the memory map,
> 
> 
> Don't think it has it's own register region... David?

I think the question here was if the CRC checker IP block (or descrambler shown
in the link below, or whatever) had registers in the offload/SPI controller
to control that extra part or if they had their own dedicated registers. So far,
these have been fixed-purpose, so have no registers at all. But I could see
needing a register, e.g. for turning it on or off. In this case, I think it
does become something like an io-backend. Or would we add this on/off switch
to the AXI SPI Engine registers?

Also, as shown in the link below, the extra bits share a clock domain with the
AXI SPI Engine. So, yes, technically I suppose they could/should? be independent
consumers of the same clock like Conor suggests below. It does seems kind of
goofy if we have to write a driver just to turn on a clock that is already
guaranteed to be on though.

> 
>> their own resources (the RTL is gonna need at the very least a clock)
>> and potentially also provide other services (like acting as an
>> io-backend type device that pre-processes data) it doesn't sound like
>> either the controller or peripheral nodes are the right place for these
>> properties. And uh, spi-offloads gets a phandle once more...
>>
> 
> But then it would be valid for both peripheral and controller to reference that
> phandle right (and get the resources of interest)?
> 
> FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful
> to you:
> 
> https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl
> 
> - Nuno Sá
> 
>

David Lechner May 23, 2024, 3:09 p.m. UTC | #16

On 5/23/24 9:57 AM, Nuno Sá wrote:
> On Thu, 2024-05-23 at 09:28 -0500, David Lechner wrote:
>> On 5/22/24 1:24 PM, Conor Dooley wrote:
>>> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
>>>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
>>>>>
>>>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
>>>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
>>>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
>>>>>
>>
> 
> ...
> 
>>
>> controller:
>> #spi-offload-cells = <2>: /* 1st cell = offload instance
>>                            * 2nd cell = trigger provider */
>>
> 
> What about things like DMA? I'm mentioning it a lot because it's way more complex
> having it on the controller (from a SW perspective). But from an HW point of view,
> it's always very similar (if not the same) as your case A.
> 

If we had a setup where there was more than one place that, e.g. the
RX stream from the offload could be piped, then I would add a 3rd
cell to describe that. If the hardware is fixed and the RX stream
always goes to a specific DMA channel, then it doesn't seem like we
need to describe that in the SPI controller node because the hardware
is fixed.

Nuno Sá May 23, 2024, 3:30 p.m. UTC | #17

On Thu, 2024-05-23 at 10:09 -0500, David Lechner wrote:
> On 5/23/24 9:57 AM, Nuno Sá wrote:
> > On Thu, 2024-05-23 at 09:28 -0500, David Lechner wrote:
> > > On 5/22/24 1:24 PM, Conor Dooley wrote:
> > > > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> > > > > On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
> > > > > > 
> > > > > > On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> > > > > > > On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
> > > > > > > > On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> > > > > > 
> > > 
> > 
> > ...
> > 
> > > 
> > > controller:
> > > #spi-offload-cells = <2>: /* 1st cell = offload instance
> > >                            * 2nd cell = trigger provider */
> > > 
> > 
> > What about things like DMA? I'm mentioning it a lot because it's way more complex
> > having it on the controller (from a SW perspective). But from an HW point of
> > view,
> > it's always very similar (if not the same) as your case A.
> > 
> 
> If we had a setup where there was more than one place that, e.g. the
> RX stream from the offload could be piped, then I would add a 3rd
> cell to describe that. If the hardware is fixed and the RX stream
> always goes to a specific DMA channel, then it doesn't seem like we
> need to describe that in the SPI controller node because the hardware
> is fixed.
> 

Well, yes, still the DMA channel is connected on the controller and not the
peripheral. Same deal as we discussed on the IIO backends stuff. But there, since
it's all IIO it was easy to make the DMA a property of the backend device. That said,
I can surely see having the property in the peripheral.

Another thing that came to mind for the trigger case. What about an additional spi
interface for configuring/setting the trigger rate? Sounds generic and then we would
not really need the trigger on the peripheral right (did not checked the CRC issue
you mentioned so not sure if it's somehow trigger related)?

Hmm but sometimes there's other things than rate/period (like offset) to care about
so maybe not doable... Bah!

- Nuno Sá

Conor Dooley May 26, 2024, 3:42 p.m. UTC | #18

On Thu, May 23, 2024 at 10:05:49AM -0500, David Lechner wrote:
> On 5/23/24 7:15 AM, Nuno Sá wrote:
> > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> >> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> >>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
> >>>>
> >>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> >>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
> >>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> >>>>
> 
> ...
> 
> >> To remind myself, "Application 2" featured an offload engine designed
> >> specifically to work with a particular data format that would strip a
> >> CRC byte and check the validity of the data stream.
> >>
> > 
> > I think the data manipulation is not really a property of the engine. Typically data
> > going out of the offload engine goes into another "data reorder" block that is pure
> > HW.
> > 
> >> I think you're right something like that is a stretch to say that that
> >> is a feature of the SPI controller - but I still don't believe that
> >> modelling it as part of the ADC is correct. I don't fully understand the
> >> io-backends and how they work yet, but the features you describe there
> >> seem like something that should/could be modelled as one, with its own
> >> node and compatible etc. Describing custom RTL stuff ain't always
> >> strightforward, but the stuff from Analog is versioned and documented
> >> etc so it shouldn't be quite that hard.
> >>
> > 
> > Putting this in io-backends is likely a stretch but one thing to add is that the
> > peripheral is always (I think) kind of the consumer of the resources. Taking the
> > trigger (PWM) as an example and even when it is directly connected with the offload
> > block, the peripheral still needs to know about it. Think of sampling frequency...
> > The period of the trigger signal is strictly connected with the sampling frequency of
> > the peripheral for example. So I see 2 things:
> > 
> > 1) Enabling/Disabling the trigger could be easily done from the peripheral even with
> > the resource in the spi engine. I think David already has some code in the series
> > that would make this trivial and so having the property in the spi controller brings
> > no added complexity.
> > 
> > 2) Controlling things like the trigger period/sample_rate. This could be harder to do
> > over SPI (or making it generic enough) so we would still need to have the same
> > property on the peripheral (even if not directly connected to it). I kind of agree
> > with David that having the property both in the peripheral and controller is a bit
> > weird.
> > 
> > And the DMA block is a complete different story. Sharing that data back with the
> > peripheral driver (in this case, the IIO subsystem) would be very interesting at the
> > very least. Note that the DMA block is not really something that is part of the
> > controller nor the offload block. It is an external block that gets the data coming
> > out of the offload engine (or the data reorder block). In IIO, we already have a DMA
> > buffer interface so users of the peripheral can get the data without any intervention
> > of the driver (on the data). We "just" enable buffering and then everything happens
> > on HW and userspace can start requesting data. If we are going to attach the DMA in
> > the controller, I have no idea how we can handle it. Moreover, the offload it's
> > really just a way of replaying the same spi transfer over and over and that happens
> > in HW so I'm not sure how we could "integrate" that with dmaengine.
> > 
> > But maybe I'm overlooking things... And thinking more in how this can be done in SW
> > rather than what makes sense from an HW perspective.
> > 
> > 
> >> continuation:
> >> If offload engines have their own register region in the memory map,
> > 
> > 
> > Don't think it has it's own register region... David?
> 
> I think the question here was if the CRC checker IP block (or descrambler shown
> in the link below, or whatever) had registers in the offload/SPI controller
> to control that extra part or if they had their own dedicated registers.

I don't think there was a question here at all. I was simply stating
that if the offload engine was not just a subordinate feature of the SPI
controller, but also provided additional data processing features then
treating the offload engine as a component of the SPI controller would
not be accurate.

> So far,
> these have been fixed-purpose, so have no registers at all. But I could see
> needing a register, e.g. for turning it on or off. In this case, I think it
> does become something like an io-backend. Or would we add this on/off switch
> to the AXI SPI Engine registers?

Seems to be that the CRC checking is a separate piece of IP though, and
so is not part of the offload engine at all, so my concern was
misplaced. I think whether or not you have registers to control it, it
should be represented in DT. How do you know it is there otherwise?

> Also, as shown in the link below, the extra bits share a clock domain with the
> AXI SPI Engine. So, yes, technically I suppose they could/should? be independent
> consumers of the same clock like Conor suggests below. It does seems kind of
> goofy if we have to write a driver just to turn on a clock that is already
> guaranteed to be on though.

You wouldn't necessarily need to write a driver for it, you could reach
out and turn it on from the backend consumer for example. And, obviously
there may be other ways that the FPGA design is configured where the
clock is not shared with the SPI controller or the offload engine.

Conor Dooley May 26, 2024, 3:45 p.m. UTC | #19

On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote:
> On 5/22/24 1:24 PM, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> >> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:
> >>>
> >>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
> >>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <[e-mail]> wrote:
> >>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
> >>>
> 
> ...
> 
> >> This time, the periodic trigger (PWM) is connected to the pin on the
> >> ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
> >> that will go high at the start of the conversion and go low at the end
> >> of the conversion. The BUSY output of the ADC is wired as the hardware
> >> trigger input of the offload.
> >>
> >> In this case would we still consider the PWM as part of the SPI
> >> controller/offload since it can only be used in conjunction with the
> >> SPI offload? It isn't connected to it at all though.
> > 
> > No, in this case the ADC is a PWM consumer and the offload engine is
> > not. The ADC is a "trigger" provider and the SPI offload engine is a
> > trigger consumer.
> 
> Makes sense.
> 
> ...
> 
> 
> > 
> >>> In fact, looking at the documentation for the "spi engine" some more, I
> >>> am even less convinced that the right place for describing the offload is
> >>> the peripheral as I (finally?) noticed that the registers for the offload
> >>> engine are mapped directly into the "spi engine"'s memory map, rather than
> >>> it being a entirely separate IP in the FPGA fabric.
> >>
> >> True, but we don't use these registers, e.g., to configure the
> >> sampling frequency of a trigger (if it can even be done). That is done
> >> in a completely separate IP block with it's own registers.
> > 
> > Where is the binding for that IP block? I think describing that is
> > pretty key. goto continuation;
> 
> For the real-world case I used to test this series, it is an AXI PWMGEN
> [1] that is providing the trigger event source. It has a typical PWM
> provider binding with #pwm-cells [2].
> 
> Calling this a "trigger" provider to the SPI offload instance just like the
> case above where the ADC is directly connected as the offload trigger makes
> sense to me.
> 
> What I was going for in this patch (slightly revised to use #cells) is that
> this trigger provider, whatever it is, is selected by one of the cells of
> #spi-offload-cells. It doesn't seem like there should be a special case for
> if the trigger provider is a clock or PWM where the SPI controller node
> becomes a consumer of the clock or PWM provider rather than just describing
> the trigger relationship.
> 
> For example, supposing we had an FPGA/HDL that could handle all 3 wiring
> configurations we have discussed so far. A) PWM connected directly to SPI
> offload as trigger, B) PWM connected to CNV of ADC and BSY of ADC connected
> to SPI offload as trigger, C) self clocked ADC with RDY of ADC connected
> to SPI offload as trigger. So the DT would have:
> 
> controller:
> #spi-offload-cells = <2>: /* 1st cell = offload instance
>                            * 2nd cell = trigger provider */
> 
> peripheral (choose one based on actual wiring):
> spi-offloads = <0 0>; /* case A */
> spi-offloads = <0 1>; /* case B */
> spi-offloads = <0 2>; /* case C */
> 
> 
> As to what is the actual consumer of the PWM provider in each of these
> cases...
> 
> * C is easy. There isn't a PWM provider since the ADC is self-clocked.
> * B, as discussed elsewhere is fairly straight forward. The ADC node is
>   the consumer since the PWM is connected directly to the ADC.
> * A is the one we need to figure out. I'm proposing that the PWM consumer
>   should be whatever kind of composite device node we come up with that
>   also solves the issue described below about where does the CRC checker
>   (or whatever) go. I think we are in agreement here at least on the point
>   that it doesn't belong in the SPI controller node?

To be clear, you're saying that we agree that the CRC checker doesnt
belong in the SPI controller node, right?

Conor Dooley May 26, 2024, 5:35 p.m. UTC | #20

On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:

> > 
> > To remind myself, "Application 2" featured an offload engine designed
> > specifically to work with a particular data format that would strip a
> > CRC byte and check the validity of the data stream.
> > 
> 
> I think the data manipulation is not really a property of the engine. Typically data
> going out of the offload engine goes into another "data reorder" block that is pure
> HW.
> 
> > I think you're right something like that is a stretch to say that that
> > is a feature of the SPI controller - but I still don't believe that
> > modelling it as part of the ADC is correct. I don't fully understand the
> > io-backends and how they work yet, but the features you describe there
> > seem like something that should/could be modelled as one, with its own
> > node and compatible etc. Describing custom RTL stuff ain't always
> > strightforward, but the stuff from Analog is versioned and documented
> > etc so it shouldn't be quite that hard.
> > 
> 
> Putting this in io-backends is likely a stretch but one thing to add is that the
> peripheral is always (I think) kind of the consumer of the resources.

Could you explain you think why making some additional processing done to
the data an io-backend is a stretch? Where else can this RTL be
represented? hint: it's not part of the ADC, just like how if you have
some custom RTL that does video processing that is not part of the
camera!

> Taking the
> trigger (PWM) as an example and even when it is directly connected with the offload
> block, the peripheral still needs to know about it. Think of sampling frequency...
> The period of the trigger signal is strictly connected with the sampling frequency of
> the peripheral for example. So I see 2 things:
> 
> 1) Enabling/Disabling the trigger could be easily done from the peripheral even with
> the resource in the spi engine. I think David already has some code in the series
> that would make this trivial and so having the property in the spi controller brings
> no added complexity.
> 
> 2) Controlling things like the trigger period/sample_rate. This could be harder to do
> over SPI (or making it generic enough) so we would still need to have the same
> property on the peripheral (even if not directly connected to it). I kind of agree
> with David that having the property both in the peripheral and controller is a bit
> weird.

Can you explain what you mean by "same property on the peripheral"? I
would expect a peripheral to state its trigger period (just like how it
states the max frequency) and for the trigger period not to appear in
the controller.

I think a way that this could be modelled to reduce some software
complexity is considering that the periodic trigger is a clock, not
a PWM, provided you are only interested in the period. That'd give you
an interface that was less concerned about what the provider of the
periodic trigger is. After all, I doubt the ADC cares how you decide to
generate the trigger, as long as the periodicity is correct.
With the examples provided, you'd get something like:

pwm {
}

pclk {
	compatible = pwm-clock
	pwms = <&pwm 0 x>
}

spi {
	compatible = spi-engine
	clocks = <&clks SPI>, <&pwm>
	clock-names = "bus", "offload"
}

The pwm-clock driver (clk-pwm.c) doesn't implement .set_rate though, but
maybe you don't need that or maybe it could be added if needed.

> And the DMA block is a complete different story. Sharing that data back with the
> peripheral driver (in this case, the IIO subsystem) would be very interesting at the
> very least. Note that the DMA block is not really something that is part of the
> controller nor the offload block. It is an external block that gets the data coming
> out of the offload engine (or the data reorder block). In IIO, we already have a DMA
> buffer interface so users of the peripheral can get the data without any intervention
> of the driver (on the data). We "just" enable buffering and then everything happens
> on HW and userspace can start requesting data. If we are going to attach the DMA in
> the controller, I have no idea how we can handle it. Moreover, the offload it's
> really just a way of replaying the same spi transfer over and over and that happens
> in HW so I'm not sure how we could "integrate" that with dmaengine.
> 
> But maybe I'm overlooking things... And thinking more in how this can be done in SW
> rather than what makes sense from an HW perspective.

I don't think you're overlooking things at all, I'm intentionally being
a bit difficult and ignoring what may be convenient for the adc driver.
This is being presented as a solution to a generic problem (and I think
you're right to do that), but it feels as if the one implementation is
all that's being considered here and I'm trying to ensure that what we
end up with doesn't make limiting assumptions.

Part of me says "sure, hook the DMAs up to the devices, as that's what
happens for other IIO devices" but at the same time I recognise that the
DMA isn't actually hooked up like that and the other IIO devices I see
like that are all actually on the SoC, rather than connected over SPI.
It might be easy to do it this way right now, but be problematic for a
future device or if someone wants to chuck away the ADI provided RTL and
do their own thing for this device. Really it just makes me wonder if
what's needed to describe more complex data pipelines uses an of_graph,
just like how video pipelines are handled, rather than the implementation
of io-backends that don't really seem to model the flow of data.

> > continuation:
> > If offload engines have their own register region in the memory map,
> 
> 
> Don't think it has it's own register region... David?
> 
> > their own resources (the RTL is gonna need at the very least a clock)
> > and potentially also provide other services (like acting as an
> > io-backend type device that pre-processes data) it doesn't sound like
> > either the controller or peripheral nodes are the right place for these
> > properties. And uh, spi-offloads gets a phandle once more...
> > 
> 
> But then it would be valid for both peripheral and controller to reference that
> phandle right (and get the resources of interest)?

Sure. But to assume that, for example, the provider of data processing
services also provided the periodic polling trigger would be incorrect
even if in the only currently existing case it does. Nothing is stopping
someone splitting those into different blocks, after all this is "random"
RTL in an FPGA :)

> FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful
> to you:
> 
> https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl

Yeah, they're a good reference, thanks.

I had to go AFK in the middle of this, I have a nagging feeling that I
forgot to say something but I cannot recall what.

Cheers,
Conor.

Nuno Sá May 29, 2024, 8:07 a.m. UTC | #21

On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> 
> > > 
> > > To remind myself, "Application 2" featured an offload engine designed
> > > specifically to work with a particular data format that would strip a
> > > CRC byte and check the validity of the data stream.
> > > 
> > 
> > I think the data manipulation is not really a property of the engine. Typically
> > data
> > going out of the offload engine goes into another "data reorder" block that is
> > pure
> > HW.
> > 
> > > I think you're right something like that is a stretch to say that that
> > > is a feature of the SPI controller - but I still don't believe that
> > > modelling it as part of the ADC is correct. I don't fully understand the
> > > io-backends and how they work yet, but the features you describe there
> > > seem like something that should/could be modelled as one, with its own
> > > node and compatible etc. Describing custom RTL stuff ain't always
> > > strightforward, but the stuff from Analog is versioned and documented
> > > etc so it shouldn't be quite that hard.
> > > 
> > 
> > Putting this in io-backends is likely a stretch but one thing to add is that the
> > peripheral is always (I think) kind of the consumer of the resources.
> 
> Could you explain you think why making some additional processing done to
> the data an io-backend is a stretch? Where else can this RTL be
> represented? hint: it's not part of the ADC, just like how if you have
> some custom RTL that does video processing that is not part of the
> camera!

Maybe we are speaking about two different things... I do agree with the video
processing example you gave but for this case I'm not sure there#s any data
manipulation involved. i mean, there is but nothing controlled by SW at this point.
Or maybe there's already a future usecase that I'm not aware about (maybe the CRC
stuff David mentioned).

I'm more focusing on where the trigger (PWMS in this particular case but could be
something else) and the DMA properties belong. I do agree that, Hardware wise, the
trigger is a property of the offload engine even though intrinsically connected with
the peripheral.

The DMA is also directly connected to the offload but I'm not sure if in this case we
should say it's a property of it? It's an external block that we do not control at
all and the data is to be consumed by users of the peripheral.

> 
> > Taking the
> > trigger (PWM) as an example and even when it is directly connected with the
> > offload
> > block, the peripheral still needs to know about it. Think of sampling
> > frequency...
> > The period of the trigger signal is strictly connected with the sampling
> > frequency of
> > the peripheral for example. So I see 2 things:
> > 
> > 1) Enabling/Disabling the trigger could be easily done from the peripheral even
> > with
> > the resource in the spi engine. I think David already has some code in the series
> > that would make this trivial and so having the property in the spi controller
> > brings
> > no added complexity.
> > 
> > 2) Controlling things like the trigger period/sample_rate. This could be harder
> > to do
> > over SPI (or making it generic enough) so we would still need to have the same
> > property on the peripheral (even if not directly connected to it). I kind of
> > agree
> > with David that having the property both in the peripheral and controller is a
> > bit
> > weird.
> 
> Can you explain what you mean by "same property on the peripheral"? I
> would expect a peripheral to state its trigger period (just like how it
> states the max frequency) and for the trigger period not to appear in
> the controller.
> 

Just have the same 'pwms' property on both the controller and peripheral...

> I think a way that this could be modelled to reduce some software
> complexity is considering that the periodic trigger is a clock, not
> a PWM, provided you are only interested in the period. That'd give you
> an interface that was less concerned about what the provider of the
> periodic trigger is. After all, I doubt the ADC cares how you decide to
> generate the trigger, as long as the periodicity is correct.
> With the examples provided, you'd get something like:
> 

Unfortunately that's not the case. For instance, in the design on the link I gave you
on the last reply we do have an averaging mode where we actually need an offset
(effort for supporting that in PWM is already ongoing) between the offload trigger
and the peripheral conversion signal (so assuming we only care about period will fail
pretty soon :)).

> pwm {
> }
> 
> pclk {
> 	compatible = pwm-clock
> 	pwms = <&pwm 0 x>
> }
> 
> spi {
> 	compatible = spi-engine
> 	clocks = <&clks SPI>, <&pwm>
> 	clock-names = "bus", "offload"
> }
> 
> The pwm-clock driver (clk-pwm.c) doesn't implement .set_rate though, but
> maybe you don't need that or maybe it could be added if needed.
> 
> > And the DMA block is a complete different story. Sharing that data back with the
> > peripheral driver (in this case, the IIO subsystem) would be very interesting at
> > the
> > very least. Note that the DMA block is not really something that is part of the
> > controller nor the offload block. It is an external block that gets the data
> > coming
> > out of the offload engine (or the data reorder block). In IIO, we already have a
> > DMA
> > buffer interface so users of the peripheral can get the data without any
> > intervention
> > of the driver (on the data). We "just" enable buffering and then everything
> > happens
> > on HW and userspace can start requesting data. If we are going to attach the DMA
> > in
> > the controller, I have no idea how we can handle it. Moreover, the offload it's
> > really just a way of replaying the same spi transfer over and over and that
> > happens
> > in HW so I'm not sure how we could "integrate" that with dmaengine.
> > 
> > But maybe I'm overlooking things... And thinking more in how this can be done in
> > SW
> > rather than what makes sense from an HW perspective.
> 
> I don't think you're overlooking things at all, I'm intentionally being
> a bit difficult and ignoring what may be convenient for the adc driver.
> This is being presented as a solution to a generic problem (and I think
> you're right to do that), but it feels as if the one implementation is
> all that's being considered here and I'm trying to ensure that what we
> end up with doesn't make limiting assumptions.

Yeah, I know and I do agree we need something generic enough and not something that
only fits a couple usecases.

> 
> Part of me says "sure, hook the DMAs up to the devices, as that's what
> happens for other IIO devices" but at the same time I recognise that the
> DMA isn't actually hooked up like that and the other IIO devices I see
> like that are all actually on the SoC, rather than connected over SPI.

Yeah, I know... But note (but again, only for ADI designs) that the DMA role is
solely for carrying the peripheral data. It is done like this so everything works in
HW and there's no need for SW to deal with the samples at all. I mean, only the
userspace app touches the samples.

TBH, the DMA is the bit that worries me the most as it may be overly complex to share
buffers (using dma-buf or something else) from the spi controller back to consumers
of it (IIO in this case). And I mean sharing in a way that there's no need to touch
the buffers.

> It might be easy to do it this way right now, but be problematic for a
> future device or if someone wants to chuck away the ADI provided RTL and
> do their own thing for this device. Really it just makes me wonder if
> what's needed to describe more complex data pipelines uses an of_graph,
> just like how video pipelines are handled, rather than the implementation
> of io-backends that don't really seem to model the flow of data.
> 

Yeah, backends is more for devices/soft-cores that extend the functionality of the
device they are connected too. Like having DACs/ADCs hdl cores for connecting to high
speed controllers. Note that in some cases they also manipulate or even create data
but since they fit in IIO, having things like the DMA property in the hdl binding was
fairly straight.

Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
be acceptable. Then we could add support to "import" it in the IIO core. Then it
would be up to the controller to accept or not to share the handle (in some cases the
controller could really want to have the control of the DMA transfers).

Not familiar enough with of_graph so can't argue about it but likely is something
worth looking at.

- Nuno Sá
> >

Conor Dooley May 29, 2024, 8:33 a.m. UTC | #22

On Wed, May 29, 2024 at 10:07:37AM +0200, Nuno Sá wrote:
> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> > On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> > > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> > 
> > > > 
> > > > To remind myself, "Application 2" featured an offload engine designed
> > > > specifically to work with a particular data format that would strip a
> > > > CRC byte and check the validity of the data stream.
> > > > 
> > > 
> > > I think the data manipulation is not really a property of the engine. Typically
> > > data
> > > going out of the offload engine goes into another "data reorder" block that is
> > > pure
> > > HW.
> > > 
> > > > I think you're right something like that is a stretch to say that that
> > > > is a feature of the SPI controller - but I still don't believe that
> > > > modelling it as part of the ADC is correct. I don't fully understand the
> > > > io-backends and how they work yet, but the features you describe there
> > > > seem like something that should/could be modelled as one, with its own
> > > > node and compatible etc. Describing custom RTL stuff ain't always
> > > > strightforward, but the stuff from Analog is versioned and documented
> > > > etc so it shouldn't be quite that hard.
> > > > 
> > > 
> > > Putting this in io-backends is likely a stretch but one thing to add is that the
> > > peripheral is always (I think) kind of the consumer of the resources.
> > 
> > Could you explain you think why making some additional processing done to
> > the data an io-backend is a stretch? Where else can this RTL be
> > represented? hint: it's not part of the ADC, just like how if you have
> > some custom RTL that does video processing that is not part of the
> > camera!
> 
> Maybe we are speaking about two different things... I do agree with the video
> processing example you gave but for this case I'm not sure there#s any data
> manipulation involved. i mean, there is but nothing controlled by SW at this point.
> Or maybe there's already a future usecase that I'm not aware about (maybe the CRC
> stuff David mentioned).

Yes, this was about the CRC or other additional processing - the quoted
text should really make this clear.

David Lechner May 29, 2024, 8:10 p.m. UTC | #23

On 5/26/24 10:45 AM, Conor Dooley wrote:
> On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote:


>> * A is the one we need to figure out. I'm proposing that the PWM consumer
>>   should be whatever kind of composite device node we come up with that
>>   also solves the issue described below about where does the CRC checker
>>   (or whatever) go. I think we are in agreement here at least on the point
>>   that it doesn't belong in the SPI controller node?
> 
> To be clear, you're saying that we agree that the CRC checker doesnt
> belong in the SPI controller node, right?

Yes.

Conor Dooley May 30, 2024, 5:25 p.m. UTC | #24

On Wed, May 29, 2024 at 03:10:54PM -0500, David Lechner wrote:
> On 5/26/24 10:45 AM, Conor Dooley wrote:
> > On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote:
> 
> 
> >> * A is the one we need to figure out. I'm proposing that the PWM consumer
> >>   should be whatever kind of composite device node we come up with that
> >>   also solves the issue described below about where does the CRC checker
> >>   (or whatever) go. I think we are in agreement here at least on the point
> >>   that it doesn't belong in the SPI controller node?
> > 
> > To be clear, you're saying that we agree that the CRC checker doesnt
> > belong in the SPI controller node, right?
> 
> Yes. 

Okay, ye. We're on the same page then about that part.

Conor Dooley May 30, 2024, 6:42 p.m. UTC | #25

On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote:
> On 5/22/24 1:24 PM, Conor Dooley wrote:
> > On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
> >> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <[e-mail]> wrote:

> >>> In fact, looking at the documentation for the "spi engine" some more, I
> >>> am even less convinced that the right place for describing the offload is
> >>> the peripheral as I (finally?) noticed that the registers for the offload
> >>> engine are mapped directly into the "spi engine"'s memory map, rather than
> >>> it being a entirely separate IP in the FPGA fabric.
> >>
> >> True, but we don't use these registers, e.g., to configure the
> >> sampling frequency of a trigger (if it can even be done). That is done
> >> in a completely separate IP block with it's own registers.
> > 
> > Where is the binding for that IP block? I think describing that is
> > pretty key. goto continuation;
> 
> For the real-world case I used to test this series, it is an AXI PWMGEN
> [1] that is providing the trigger event source. It has a typical PWM
> provider binding with #pwm-cells [2].
> 
> Calling this a "trigger" provider to the SPI offload instance just like the
> case above where the ADC is directly connected as the offload trigger makes
> sense to me.
> 
> What I was going for in this patch (slightly revised to use #cells) is that
> this trigger provider, whatever it is, is selected by one of the cells of
> #spi-offload-cells. It doesn't seem like there should be a special case for
> if the trigger provider is a clock or PWM where the SPI controller node
> becomes a consumer of the clock or PWM provider rather than just describing
> the trigger relationship.
> 
> For example, supposing we had an FPGA/HDL that could handle all 3 wiring
> configurations we have discussed so far. A) PWM connected directly to SPI
> offload as trigger, B) PWM connected to CNV of ADC and BSY of ADC connected
> to SPI offload as trigger, C) self clocked ADC with RDY of ADC connected
> to SPI offload as trigger. So the DT would have:
> 
> controller:
> #spi-offload-cells = <2>: /* 1st cell = offload instance
>                            * 2nd cell = trigger provider */
> 
> peripheral (choose one based on actual wiring):
> spi-offloads = <0 0>; /* case A */
> spi-offloads = <0 1>; /* case B */
> spi-offloads = <0 2>; /* case C */
> 
> 
> As to what is the actual consumer of the PWM provider in each of these
> cases...
> 
> * C is easy. There isn't a PWM provider since the ADC is self-clocked.
> * B, as discussed elsewhere is fairly straight forward. The ADC node is
>   the consumer since the PWM is connected directly to the ADC.
> * A is the one we need to figure out. I'm proposing that the PWM consumer
>   should be whatever kind of composite device node we come up with that
>   also solves the issue described below about where does the CRC checker
>   (or whatever) go. I think we are in agreement here at least on the point
>   that it doesn't belong in the SPI controller node?

I think C and B are correct here. The term "composite device" scares me,
but yeah the PWM gets connected to the device that encompasses the
offload engine. In case B and C, the trigger signals are connected to
the offload engine - what property are we using to describe that?
If this were not a "passive" process and the OS was actually involved
in kicking off an action when the ADC asserted the !BSY signal,
interrupts would make sense, but I'm not sure here.

Maybe interrupts is the right thing to do, because that would also make
sense in the case that we used the busy signal without an offload and
plumbed it into a regular interrupt controller?

gpio0: gpio@20120000 {
	compatible = "microchip,mpfs-gpio";
	reg = <0x0 0x20120000 0x0 0x1000>;
	interrupt-parent = <&plic>;
	interrupt-controller;
	#interrupt-cells = <1>;
	clocks = <&clkcfg CLK_GPIO0>;
	gpio-controller;
	#gpio-cells = <2>;
};

adc {
	interrupt-parent = <&gpio0>;
	interrupts = <0 HIGH>;
};

I suppose then the offload needs to be an interrupt provider too, even
if no irqchip is ever actually created for it. I dunno how tf that's
gonna work out on the software side though? If you have spi-offloads you
interpret the interrupts property differently (IOW you ignore it)?

Or were you thinking of not really hooking that up at all, but instead
just saying checking for whatever x is in spi-offloads = <0 x> and
defining 2 to mean "active low" and 1 to mean "active high" etc?

Once again, had to take a break to make food in the middle of responding
here, so I hope I didn't write something incomprehensible.

Thanks,
Conor.

> 
> [1]: http://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html
> [2]: https://lore.kernel.org/linux-pwm/[e-mail]/

Conor Dooley May 30, 2024, 7:18 p.m. UTC | #26

On Wed, May 29, 2024 at 10:07:37AM +0200, Nuno Sá wrote:
> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> > On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> > > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:

> > > Taking the
> > > trigger (PWM) as an example and even when it is directly connected with the
> > > offload
> > > block, the peripheral still needs to know about it. Think of sampling
> > > frequency...
> > > The period of the trigger signal is strictly connected with the sampling
> > > frequency of
> > > the peripheral for example. So I see 2 things:
> > > 
> > > 1) Enabling/Disabling the trigger could be easily done from the peripheral even
> > > with
> > > the resource in the spi engine. I think David already has some code in the series
> > > that would make this trivial and so having the property in the spi controller
> > > brings
> > > no added complexity.
> > > 
> > > 2) Controlling things like the trigger period/sample_rate. This could be harder
> > > to do
> > > over SPI (or making it generic enough) so we would still need to have the same
> > > property on the peripheral (even if not directly connected to it). I kind of
> > > agree
> > > with David that having the property both in the peripheral and controller is a
> > > bit
> > > weird.
> > 
> > Can you explain what you mean by "same property on the peripheral"? I
> > would expect a peripheral to state its trigger period (just like how it
> > states the max frequency) and for the trigger period not to appear in
> > the controller.
> > 
> 
> Just have the same 'pwms' property on both the controller and peripheral...

Yeah, no... Opinion unchanged since my last message.

> > I think a way that this could be modelled to reduce some software
> > complexity is considering that the periodic trigger is a clock, not
> > a PWM, provided you are only interested in the period. That'd give you
> > an interface that was less concerned about what the provider of the
> > periodic trigger is. After all, I doubt the ADC cares how you decide to
> > generate the trigger, as long as the periodicity is correct.
> > With the examples provided, you'd get something like:
> > 
> 
> Unfortunately that's not the case.

Ahh, that sucks. Oh well.

> For instance, in the design on the link I gave you
> on the last reply we do have an averaging mode where we actually need an offset
> (effort for supporting that in PWM is already ongoing) between the offload trigger
> and the peripheral conversion signal (so assuming we only care about period will fail
> pretty soon :)).

> > > And the DMA block is a complete different story. Sharing that data back with the
> > > peripheral driver (in this case, the IIO subsystem) would be very interesting at
> > > the
> > > very least. Note that the DMA block is not really something that is part of the
> > > controller nor the offload block. It is an external block that gets the data
> > > coming
> > > out of the offload engine (or the data reorder block). In IIO, we already have a
> > > DMA
> > > buffer interface so users of the peripheral can get the data without any
> > > intervention
> > > of the driver (on the data). We "just" enable buffering and then everything
> > > happens
> > > on HW and userspace can start requesting data. If we are going to attach the DMA
> > > in
> > > the controller, I have no idea how we can handle it. Moreover, the offload it's
> > > really just a way of replaying the same spi transfer over and over and that
> > > happens
> > > in HW so I'm not sure how we could "integrate" that with dmaengine.
> > > 
> > > But maybe I'm overlooking things... And thinking more in how this can be done in
> > > SW
> > > rather than what makes sense from an HW perspective.
> > 
> > I don't think you're overlooking things at all, I'm intentionally being
> > a bit difficult and ignoring what may be convenient for the adc driver.
> > This is being presented as a solution to a generic problem (and I think
> > you're right to do that), but it feels as if the one implementation is
> > all that's being considered here and I'm trying to ensure that what we
> > end up with doesn't make limiting assumptions.
> 
> Yeah, I know and I do agree we need something generic enough and not something that
> only fits a couple usecases.

If only we had another user... I suppose you lads are the market leader
in these kinds of devices. If I did happen to know if Microchip was
working on anything similar (which I don't, I work on FPGAs not these
kinds of devices) I couldn't even tell you. I suppose I could ask around
and see. Do you know if TI is doing anything along these lines?

> > Part of me says "sure, hook the DMAs up to the devices, as that's what
> > happens for other IIO devices" but at the same time I recognise that the
> > DMA isn't actually hooked up like that and the other IIO devices I see
> > like that are all actually on the SoC, rather than connected over SPI.
> 
> Yeah, I know... But note (but again, only for ADI designs) that the DMA role is
> solely for carrying the peripheral data. It is done like this so everything works in
> HW and there's no need for SW to deal with the samples at all. I mean, only the
> userspace app touches the samples.
> 
> TBH, the DMA is the bit that worries me the most as it may be overly complex to share
> buffers (using dma-buf or something else) from the spi controller back to consumers
> of it (IIO in this case). And I mean sharing in a way that there's no need to touch
> the buffers.

<snip>

> Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
> be acceptable. Then we could add support to "import" it in the IIO core. Then it
> would be up to the controller to accept or not to share the handle (in some cases the
> controller could really want to have the control of the DMA transfers).

Yeah, that is about what I was thinking. I wasn't expecting the spi code
to grow handing for dmabuf or anything like that, just a way for the
offload consumer to say "yo, can you tell me what dma buffer I can
use?". Unless (until?) there's some controller that wants to manage it,
I think that'd be sufficient?

David Lechner May 30, 2024, 7:24 p.m. UTC | #27

On 5/29/24 3:07 AM, Nuno Sá wrote:
> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:


>> It might be easy to do it this way right now, but be problematic for a
>> future device or if someone wants to chuck away the ADI provided RTL and
>> do their own thing for this device. Really it just makes me wonder if
>> what's needed to describe more complex data pipelines uses an of_graph,
>> just like how video pipelines are handled, rather than the implementation
>> of io-backends that don't really seem to model the flow of data.
>>
> 
> Yeah, backends is more for devices/soft-cores that extend the functionality of the
> device they are connected too. Like having DACs/ADCs hdl cores for connecting to high
> speed controllers. Note that in some cases they also manipulate or even create data
> but since they fit in IIO, having things like the DMA property in the hdl binding was
> fairly straight.
> 
> Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
> be acceptable. Then we could add support to "import" it in the IIO core. Then it
> would be up to the controller to accept or not to share the handle (in some cases the
> controller could really want to have the control of the DMA transfers).

I could see this working for some SPI controllers, but for the AXI SPI Engine
+ DMA currently, the DMA has a fixed word size, so can't be used as a generic
DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit
word size, then even if we are reading 16-bit sample data, the DMA is going to
put it in a 32-bit slot. So one could argue that this is still doing some data
manipulation similar to the CRC checker example.

> 
> Not familiar enough with of_graph so can't argue about it but likely is something
> worth looking at.
> 
> - Nuno Sá
>>>

I did try implementing something using graph bindings when I first started
working on this, but it didn't seem to really give us any extra useful
information. It was just describing connections (endpoints) that I thought
we could just implicitly assume. After this discussion though, maybe worth
a second look. I'll have to think about it more.

David Lechner May 30, 2024, 9:28 p.m. UTC | #28

On 5/30/24 2:18 PM, Conor Dooley wrote:

> 
> If only we had another user... I suppose you lads are the market leader
> in these kinds of devices. If I did happen to know if Microchip was
> working on anything similar (which I don't, I work on FPGAs not these
> kinds of devices) I couldn't even tell you. I suppose I could ask around
> and see. Do you know if TI is doing anything along these lines?
> 
I think the most popular use case for the performance improvements made
possible with a SPI offload unrelated to ADCs/DACs is for CAN controllers
(e.g. the discussion from David Jander a few years ago).

I think one of my colleagues was working on one in the last year that we
might still have lying around. But I don't know what we could use as the
SPI controller that would have offload support. 

I suppose we could make something using e.g. the PRU in a BeagleBone, but
that would take lots of engineering resources and we could design it to fit
whatever interface we want, so I'm not sure that really helps much. If we
could find an off-the-shelf SPI controller with offload capabilities that
would be helpful, but I don't know of any.

Nuno Sá May 31, 2024, 7:33 a.m. UTC | #29

On Thu, 2024-05-30 at 14:24 -0500, David Lechner wrote:
> On 5/29/24 3:07 AM, Nuno Sá wrote:
> > On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> 
> 
> > > It might be easy to do it this way right now, but be problematic for a
> > > future device or if someone wants to chuck away the ADI provided RTL and
> > > do their own thing for this device. Really it just makes me wonder if
> > > what's needed to describe more complex data pipelines uses an of_graph,
> > > just like how video pipelines are handled, rather than the implementation
> > > of io-backends that don't really seem to model the flow of data.
> > > 
> > 
> > Yeah, backends is more for devices/soft-cores that extend the functionality of
> > the
> > device they are connected too. Like having DACs/ADCs hdl cores for connecting to
> > high
> > speed controllers. Note that in some cases they also manipulate or even create
> > data
> > but since they fit in IIO, having things like the DMA property in the hdl binding
> > was
> > fairly straight.
> > 
> > Maybe having an offload dedicated API (through spi) to get/share a DMA handle
> > would
> > be acceptable. Then we could add support to "import" it in the IIO core. Then it
> > would be up to the controller to accept or not to share the handle (in some cases
> > the
> > controller could really want to have the control of the DMA transfers).
> 
> I could see this working for some SPI controllers, but for the AXI SPI Engine
> + DMA currently, the DMA has a fixed word size, so can't be used as a generic
> DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit
> word size, then even if we are reading 16-bit sample data, the DMA is going to
> put it in a 32-bit slot. So one could argue that this is still doing some data
> manipulation similar to the CRC checker example.
> 

Note that what I was thinking was something very trivial... Just a way to get
'dma_chan' out of the spi_engine to the consumer so we could import it in the IIO DMA
infrastructure... I assume it may be a sneaky way to just get around the problem
though :).

Another way is to come up with spi like API's to submit DMA request's (passing an
handler or so for completion). I guess we would somehow also need a kind of get()
function that would give consumers some kind of info/interface like spi xfers size
(as in this particular case it's the DMA who defines the src/dst width). We would
likely also need some kind of spi_dma_buffer implementation in IIO (likely to share
some code with the current stuff; at least the userspace interface should definitely
be the same).

Anyways, the above it's just me throwing some random ideas that come to mind :). They
may be stupid but at the very least they could give you some betters ideas :).

> > 
> > Not familiar enough with of_graph so can't argue about it but likely is something
> > worth looking at.
> > 
> > - Nuno Sá
> > > > 
> 
> I did try implementing something using graph bindings when I first started
> working on this, but it didn't seem to really give us any extra useful
> information. It was just describing connections (endpoints) that I thought
> we could just implicitly assume. After this discussion though, maybe worth
> a second look. I'll have to think about it more.

Why not :)?

- Nuno Sá

Nuno Sá May 31, 2024, 7:39 a.m. UTC | #30

On Thu, 2024-05-30 at 20:18 +0100, Conor Dooley wrote:
> On Wed, May 29, 2024 at 10:07:37AM +0200, Nuno Sá wrote:
> > On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> > > On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote:
> > > > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
> 
> > > > Taking the
> > > > trigger (PWM) as an example and even when it is directly connected with the
> > > > offload
> > > > block, the peripheral still needs to know about it. Think of sampling
> > > > frequency...
> > > > The period of the trigger signal is strictly connected with the sampling
> > > > frequency of
> > > > the peripheral for example. So I see 2 things:
> > > > 
> > > > 1) Enabling/Disabling the trigger could be easily done from the peripheral
> > > > even
> > > > with
> > > > the resource in the spi engine. I think David already has some code in the
> > > > series
> > > > that would make this trivial and so having the property in the spi controller
> > > > brings
> > > > no added complexity.
> > > > 
> > > > 2) Controlling things like the trigger period/sample_rate. This could be
> > > > harder
> > > > to do
> > > > over SPI (or making it generic enough) so we would still need to have the
> > > > same
> > > > property on the peripheral (even if not directly connected to it). I kind of
> > > > agree
> > > > with David that having the property both in the peripheral and controller is
> > > > a
> > > > bit
> > > > weird.
> > > 
> > > Can you explain what you mean by "same property on the peripheral"? I
> > > would expect a peripheral to state its trigger period (just like how it
> > > states the max frequency) and for the trigger period not to appear in
> > > the controller.
> > > 
> > 
> > Just have the same 'pwms' property on both the controller and peripheral...
> 
> Yeah, no... Opinion unchanged since my last message.
> 

...

> > 
> 
> If only we had another user... I suppose you lads are the market leader
> in these kinds of devices. If I did happen to know if Microchip was
> working on anything similar (which I don't, I work on FPGAs not these
> kinds of devices) I couldn't even tell you. I suppose I could ask around
> and see. Do you know if TI is doing anything along these lines?
> 

Unfortunately, no idea.

> > > Part of me says "sure, hook the DMAs up to the devices, as that's what
> > > happens for other IIO devices" but at the same time I recognise that the
> > > DMA isn't actually hooked up like that and the other IIO devices I see
> > > like that are all actually on the SoC, rather than connected over SPI.
> > 
> > Yeah, I know... But note (but again, only for ADI designs) that the DMA role is
> > solely for carrying the peripheral data. It is done like this so everything works
> > in
> > HW and there's no need for SW to deal with the samples at all. I mean, only the
> > userspace app touches the samples.
> > 
> > TBH, the DMA is the bit that worries me the most as it may be overly complex to
> > share
> > buffers (using dma-buf or something else) from the spi controller back to
> > consumers
> > of it (IIO in this case). And I mean sharing in a way that there's no need to
> > touch
> > the buffers.
> 
> <snip>
> 
> > Maybe having an offload dedicated API (through spi) to get/share a DMA handle
> > would
> > be acceptable. Then we could add support to "import" it in the IIO core. Then it
> > would be up to the controller to accept or not to share the handle (in some cases
> > the
> > controller could really want to have the control of the DMA transfers).
> 
> Yeah, that is about what I was thinking. I wasn't expecting the spi code
> to grow handing for dmabuf or anything like that, just a way for the
> offload consumer to say "yo, can you tell me what dma buffer I can
> use?". Unless (until?) there's some controller that wants to manage it,
> I think that'd be sufficient?

Yeah, I could see some kind of submit_request() API with some kind of completion
handler for this. But on the IIO side the DMA code is not that straight (even getting
more complex with dma-buf's) so I can't really tell how the whole thing would look
like. But may be something to look at.

- Nuno Sá

Mark Brown May 31, 2024, 12:47 p.m. UTC | #31

On Thu, May 30, 2024 at 04:28:38PM -0500, David Lechner wrote:

> I think one of my colleagues was working on one in the last year that we
> might still have lying around. But I don't know what we could use as the
> SPI controller that would have offload support. 

David was using a Raspberry Pi, their DMA controller can happily write
to the SPI controller registers so functions as an offload engine - I'd
not be surprised if other SPI controllers and DMA controllers could
interact in a similar fashion for the CAN or interrupt controller style
use cases.

Conor Dooley June 4, 2024, 7:33 p.m. UTC | #32

On Thu, May 30, 2024 at 02:24:17PM -0500, David Lechner wrote:
> On 5/29/24 3:07 AM, Nuno Sá wrote:
> > On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> 
> 
> >> It might be easy to do it this way right now, but be problematic for a
> >> future device or if someone wants to chuck away the ADI provided RTL and
> >> do their own thing for this device. Really it just makes me wonder if
> >> what's needed to describe more complex data pipelines uses an of_graph,
> >> just like how video pipelines are handled, rather than the implementation
> >> of io-backends that don't really seem to model the flow of data.
> >>
> > 
> > Yeah, backends is more for devices/soft-cores that extend the functionality of the
> > device they are connected too. Like having DACs/ADCs hdl cores for connecting to high
> > speed controllers. Note that in some cases they also manipulate or even create data
> > but since they fit in IIO, having things like the DMA property in the hdl binding was
> > fairly straight.
> > 
> > Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
> > be acceptable. Then we could add support to "import" it in the IIO core. Then it
> > would be up to the controller to accept or not to share the handle (in some cases the
> > controller could really want to have the control of the DMA transfers).
> 
> I could see this working for some SPI controllers, but for the AXI SPI Engine
> + DMA currently, the DMA has a fixed word size, so can't be used as a generic
> DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit
> word size, then even if we are reading 16-bit sample data, the DMA is going to
> put it in a 32-bit slot. So one could argue that this is still doing some data
> manipulation similar to the CRC checker example.
> 
> > 
> > Not familiar enough with of_graph so can't argue about it but likely is something
> > worth looking at.
> 
> I did try implementing something using graph bindings when I first started
> working on this, but it didn't seem to really give us any extra useful
> information. It was just describing connections (endpoints) that I thought
> we could just implicitly assume. After this discussion though, maybe worth
> a second look. I'll have to think about it more.

Could you elaborate on why you think you can assume the connections? What
happens when you have multiple stages of data processing and/or multiple
ADCs in your system? As I've previously said, I work on FPGA stuff, and
everyone here seems to fawn over having <insert custom DSP IP here> in
their data pipelines. I can't imagine it being any different for ADC data,
and an io-backend property that doesn't describe how the data flows is
gonna become lacklustre I think.

David Lechner June 4, 2024, 7:39 p.m. UTC | #33

On 6/4/24 2:33 PM, Conor Dooley wrote:
> On Thu, May 30, 2024 at 02:24:17PM -0500, David Lechner wrote:
>> On 5/29/24 3:07 AM, Nuno Sá wrote:
>>> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
>>
>>
>>>> It might be easy to do it this way right now, but be problematic for a
>>>> future device or if someone wants to chuck away the ADI provided RTL and
>>>> do their own thing for this device. Really it just makes me wonder if
>>>> what's needed to describe more complex data pipelines uses an of_graph,
>>>> just like how video pipelines are handled, rather than the implementation
>>>> of io-backends that don't really seem to model the flow of data.
>>>>
>>>
>>> Yeah, backends is more for devices/soft-cores that extend the functionality of the
>>> device they are connected too. Like having DACs/ADCs hdl cores for connecting to high
>>> speed controllers. Note that in some cases they also manipulate or even create data
>>> but since they fit in IIO, having things like the DMA property in the hdl binding was
>>> fairly straight.
>>>
>>> Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
>>> be acceptable. Then we could add support to "import" it in the IIO core. Then it
>>> would be up to the controller to accept or not to share the handle (in some cases the
>>> controller could really want to have the control of the DMA transfers).
>>
>> I could see this working for some SPI controllers, but for the AXI SPI Engine
>> + DMA currently, the DMA has a fixed word size, so can't be used as a generic
>> DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit
>> word size, then even if we are reading 16-bit sample data, the DMA is going to
>> put it in a 32-bit slot. So one could argue that this is still doing some data
>> manipulation similar to the CRC checker example.
>>
>>>
>>> Not familiar enough with of_graph so can't argue about it but likely is something
>>> worth looking at.
>>
>> I did try implementing something using graph bindings when I first started
>> working on this, but it didn't seem to really give us any extra useful
>> information. It was just describing connections (endpoints) that I thought
>> we could just implicitly assume. After this discussion though, maybe worth
>> a second look. I'll have to think about it more.
> 
> Could you elaborate on why you think you can assume the connections? What
> happens when you have multiple stages of data processing and/or multiple
> ADCs in your system? As I've previously said, I work on FPGA stuff, and
> everyone here seems to fawn over having <insert custom DSP IP here> in
> their data pipelines. I can't imagine it being any different for ADC data,
> and an io-backend property that doesn't describe how the data flows is
> gonna become lacklustre I think.

I was more ignorant back then. :-)

That is is why I said "thought" instead of "think". I am more enlightened now.

Conor Dooley June 4, 2024, 7:42 p.m. UTC | #34

On Tue, Jun 04, 2024 at 02:39:18PM -0500, David Lechner wrote:
> On 6/4/24 2:33 PM, Conor Dooley wrote:
> > On Thu, May 30, 2024 at 02:24:17PM -0500, David Lechner wrote:
> >> On 5/29/24 3:07 AM, Nuno Sá wrote:
> >>> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
> >>
> >>
> >>>> It might be easy to do it this way right now, but be problematic for a
> >>>> future device or if someone wants to chuck away the ADI provided RTL and
> >>>> do their own thing for this device. Really it just makes me wonder if
> >>>> what's needed to describe more complex data pipelines uses an of_graph,
> >>>> just like how video pipelines are handled, rather than the implementation
> >>>> of io-backends that don't really seem to model the flow of data.
> >>>>
> >>>
> >>> Yeah, backends is more for devices/soft-cores that extend the functionality of the
> >>> device they are connected too. Like having DACs/ADCs hdl cores for connecting to high
> >>> speed controllers. Note that in some cases they also manipulate or even create data
> >>> but since they fit in IIO, having things like the DMA property in the hdl binding was
> >>> fairly straight.
> >>>
> >>> Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
> >>> be acceptable. Then we could add support to "import" it in the IIO core. Then it
> >>> would be up to the controller to accept or not to share the handle (in some cases the
> >>> controller could really want to have the control of the DMA transfers).
> >>
> >> I could see this working for some SPI controllers, but for the AXI SPI Engine
> >> + DMA currently, the DMA has a fixed word size, so can't be used as a generic
> >> DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit
> >> word size, then even if we are reading 16-bit sample data, the DMA is going to
> >> put it in a 32-bit slot. So one could argue that this is still doing some data
> >> manipulation similar to the CRC checker example.
> >>
> >>>
> >>> Not familiar enough with of_graph so can't argue about it but likely is something
> >>> worth looking at.
> >>
> >> I did try implementing something using graph bindings when I first started
> >> working on this, but it didn't seem to really give us any extra useful
> >> information. It was just describing connections (endpoints) that I thought
> >> we could just implicitly assume. After this discussion though, maybe worth
> >> a second look. I'll have to think about it more.
> > 
> > Could you elaborate on why you think you can assume the connections? What
> > happens when you have multiple stages of data processing and/or multiple
> > ADCs in your system? As I've previously said, I work on FPGA stuff, and
> > everyone here seems to fawn over having <insert custom DSP IP here> in
> > their data pipelines. I can't imagine it being any different for ADC data,
> > and an io-backend property that doesn't describe how the data flows is
> > gonna become lacklustre I think.
> 
> I was more ignorant back then. :-)
> 
> That is is why I said "thought" instead of "think". I am more enlightened now.

Heh, I didn't mean it in a bad way. I just wanted to flesh out why you
thought that way.

David Lechner June 4, 2024, 8:04 p.m. UTC | #35

On 6/4/24 2:42 PM, Conor Dooley wrote:
> On Tue, Jun 04, 2024 at 02:39:18PM -0500, David Lechner wrote:
>> On 6/4/24 2:33 PM, Conor Dooley wrote:
>>> On Thu, May 30, 2024 at 02:24:17PM -0500, David Lechner wrote:
>>>> On 5/29/24 3:07 AM, Nuno Sá wrote:
>>>>> On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote:
>>>>
>>>>
>>>>>> It might be easy to do it this way right now, but be problematic for a
>>>>>> future device or if someone wants to chuck away the ADI provided RTL and
>>>>>> do their own thing for this device. Really it just makes me wonder if
>>>>>> what's needed to describe more complex data pipelines uses an of_graph,
>>>>>> just like how video pipelines are handled, rather than the implementation
>>>>>> of io-backends that don't really seem to model the flow of data.
>>>>>>
>>>>>
>>>>> Yeah, backends is more for devices/soft-cores that extend the functionality of the
>>>>> device they are connected too. Like having DACs/ADCs hdl cores for connecting to high
>>>>> speed controllers. Note that in some cases they also manipulate or even create data
>>>>> but since they fit in IIO, having things like the DMA property in the hdl binding was
>>>>> fairly straight.
>>>>>
>>>>> Maybe having an offload dedicated API (through spi) to get/share a DMA handle would
>>>>> be acceptable. Then we could add support to "import" it in the IIO core. Then it
>>>>> would be up to the controller to accept or not to share the handle (in some cases the
>>>>> controller could really want to have the control of the DMA transfers).
>>>>
>>>> I could see this working for some SPI controllers, but for the AXI SPI Engine
>>>> + DMA currently, the DMA has a fixed word size, so can't be used as a generic
>>>> DMA with arbitrary SPI xfers. For example, if the HDL is compiled with a 32-bit
>>>> word size, then even if we are reading 16-bit sample data, the DMA is going to
>>>> put it in a 32-bit slot. So one could argue that this is still doing some data
>>>> manipulation similar to the CRC checker example.
>>>>
>>>>>
>>>>> Not familiar enough with of_graph so can't argue about it but likely is something
>>>>> worth looking at.
>>>>
>>>> I did try implementing something using graph bindings when I first started
>>>> working on this, but it didn't seem to really give us any extra useful
>>>> information. It was just describing connections (endpoints) that I thought
>>>> we could just implicitly assume. After this discussion though, maybe worth
>>>> a second look. I'll have to think about it more.
>>>
>>> Could you elaborate on why you think you can assume the connections? What
>>> happens when you have multiple stages of data processing and/or multiple
>>> ADCs in your system? As I've previously said, I work on FPGA stuff, and
>>> everyone here seems to fawn over having <insert custom DSP IP here> in
>>> their data pipelines. I can't imagine it being any different for ADC data,
>>> and an io-backend property that doesn't describe how the data flows is
>>> gonna become lacklustre I think.
>>
>> I was more ignorant back then. :-)
>>
>> That is is why I said "thought" instead of "think". I am more enlightened now.
> 
> Heh, I didn't mean it in a bad way. I just wanted to flesh out why you
> thought that way.
> 

Back then, we were going on the assumption that no one would want to
make their own custom IP and only use the IP blocks provided by ADI
for ADI chips. So given chip X + SPI offload, we could assume HDL
project Y was being used.

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 15938f81fdce..32991a2d2264 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -113,6 +113,16 @@  properties:
     minItems: 2
     maxItems: 4
 
+  spi-offloads:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Array of controller offload instances that are reserved for use by the
+      peripheral device. The semantic meaning of the values of the array
+      elements is defined by the controller. For example, it could be a simple
+      0-based index of the offload instance, or it could be a bitfield where
+      a few bits represent the assigned hardware trigger, a few bits represent
+      the assigned RX stream, etc.
+
   st,spi-midi-ns:
     description: |
       Only for STM32H7, (Master Inter-Data Idleness) minimum time