Date: Wed, 10 Jan 2024 18:06:59 -0600
From: David Lechner <dlechner@...libre.com>
To: Rob Herring <robh@...nel.org>
Cc: Mark Brown <broonie@...nel.org>, Jonathan Cameron <jic23@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Michael Hennerich <michael.hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>, 
	Frank Rowand <frowand.list@...il.com>, Thierry Reding <thierry.reding@...il.com>, 
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, 
	Jonathan Corbet <corbet@....net>, linux-spi@...r.kernel.org, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/13] spi: dt-bindings: adi,axi-spi-engine: add offload bindings

On Wed, Jan 10, 2024 at 5:15 PM Rob Herring <robh@...nel.org> wrote:
>
> On Wed, Jan 10, 2024 at 01:49:45PM -0600, David Lechner wrote:
> > The ADI AXI SPI Engine driver supports offloading SPI transfers to
> > hardware. This is essentially a feature that allows recording an
> > arbitrary sequence of SPI transfers and then playing them back with
> > no CPU intervention via a hardware trigger.
> >
> > This adds the bindings for this feature. Each SPI Engine instance
> > can have from 0 to 32 offload instances. Each offload instance has a
> > trigger input and a data stream output. As an example, this could be
> > used with an ADC SPI peripheral. In this case the trigger is connected
> > to a PWM/clock to determine the sampling rate for the ADC and the output
> > stream is connected to a DMA channel to pipe the sample data to memory.
> >
> > SPI peripherals act as consumers of the offload instances. Typically,
> > one SPI peripheral will be connected to one offload instance. But to
> > make the bindings future-proof, the property is an array.
>
> Is there some sort of arbitration between multiple offload engines on
> the same chip select? If not, I don't see how it would work.

There is only one SPI engine driving the SPI controller, so if two
offloads are triggered at the same time, they will be executed
serially.

>
> I think this whole thing could be simplified down to just 3
> SPI controller properties: pwms, dmas, and adi,offload-cs-map. Each
> property is has entries equal the number of offload engines. The last
> one maps an offload engine to a SPI chip-select.

Offloads could be connected to virtually anything, not just pwms and
dmas, so making pwms and dmas controller properties doesn't seem right
to me. What if we have one that uses a gpio trigger or clock trigger?
Or what if we have one where the output goes to a DSP instead of DMA?
This is why I made offload@ nodes with a compatible property - to
describe what is actually connected to each offload instance since it
could be an unlimited range of different things.

>
> >
> > Signed-off-by: David Lechner <dlechner@...libre.com>
> > ---
> >  .../spi/adi,axi-spi-engine-peripheral-props.yaml   | 24 +++++++++++
> >  .../bindings/spi/adi,axi-spi-engine.yaml           | 49 +++++++++++++++++++++-
> >  .../bindings/spi/spi-peripheral-props.yaml         |  1 +
> >  3 files changed, 73 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/adi,axi-spi-engine-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/adi,axi-spi-engine-peripheral-props.yaml
> > new file mode 100644
> > index 000000000000..19b685fc3b39
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/adi,axi-spi-engine-peripheral-props.yaml
> > @@ -0,0 +1,24 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/spi/adi,axi-spi-engine-peripheral-props.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Peripheral properties for Analog Devices AXI SPI Engine Controller
> > +
> > +maintainers:
> > +  - Michael Hennerich <Michael.Hennerich@...log.com>
> > +  - Nuno Sá <nuno.sa@...log.com>
> > +
> > +properties:
> > +  adi,offloads:
> > +    description:
> > +      List of AXI SPI Engine offload instances assigned to this peripheral.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    maxItems: 32
> > +    items:
> > +      items:
> > +        - minimum: 0
> > +          maximum: 31
>
> This defines a matrix. You want:
>
> minItems: 1
> maxItems: 32
> items:
>   maximum: 31
>
> (0 is already the min).

thanks

>
> > +
> > +additionalProperties: true
> > diff --git a/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml b/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml
> > index d48faa42d025..69f3261bab47 100644
> > --- a/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml
> > +++ b/Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml
> > @@ -21,6 +21,23 @@ maintainers:
> >  allOf:
> >    - $ref: /schemas/spi/spi-controller.yaml#
> >
> > +$defs:
> > +  offload:
> > +    description:
> > +      Describes the connections of the trigger input and the data output stream
> > +      of one or more offload instances.
> > +
> > +    properties:
> > +      reg:
> > +        description:
> > +          Index of the offload instance.
> > +        items:
> > +          - minimum: 0
> > +            maximum: 31
> > +
> > +    required:
> > +      - reg
> > +
> >  properties:
> >    compatible:
> >      const: adi,axi-spi-engine-1.00.a
> > @@ -41,6 +58,22 @@ properties:
> >        - const: s_axi_aclk
> >        - const: spi_clk
> >
> > +  offloads:
> > +    type: object
> > +    description: Zero or more offloads supported by the controller.
> > +
> > +    properties:
> > +      "#address-cells":
> > +        const: 1
> > +
> > +      "#size-cells":
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^offload@[0-8a-f]+$":
> > +        type: object
> > +        $ref: '#/$defs/offload'
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -62,5 +95,19 @@ examples:
> >          #address-cells = <1>;
> >          #size-cells = <0>;
> >
> > -        /* SPI devices */
> > +        offloads {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            offload@0 {
> > +                compatible = "adi,example-offload";
>
> No fake examples please. This should give you a warning.

Ack.

FYI, unknown compatibles don't currently give a warning.

$ dt-validate --version
2023.12.dev6+gfb80ec4
$ make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/adi,axi-spi-engine.yaml
ARCH=arm KBUILD_OUTPUT=\$HOME/build-area/ad7944-mainline
make[1]: Entering directory
'/home/david/work/linux/OME/build-area/ad7944-mainline'
  DTEX    Documentation/devicetree/bindings/spi/adi,axi-spi-engine.example.dts
  DTC_CHK Documentation/devicetree/bindings/spi/adi,axi-spi-engine.example.dtb
make[1]: Leaving directory
'/home/david/work/linux/OME/build-area/ad7944-mainline'

>
> > +                reg = <0>;
> > +            };
> > +        };
> > +
> > +        adc@0 {
> > +            compatible = "adi,example-adc";
> > +            reg = <0>;
> > +            adi,offloads = <0>;
> > +        };
> >      };
> > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-propsyaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > index 1c8e71c18234..7beb5a3798a5 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > @@ -132,6 +132,7 @@ properties:
> >
> >  # The controller specific properties go here.
> >  allOf:
> > +  - $ref: adi,axi-spi-engine-peripheral-props.yaml#
> >    - $ref: arm,pl022-peripheral-props.yaml#
> >    - $ref: cdns,qspi-nor-peripheral-props.yaml#
> >    - $ref: samsung,spi-peripheral-props.yaml#
> >
> > --
> > 2.43.0
> >

Powered by blists - more mailing lists