Message-ID: <369a72dd34c0bc457620b88594a975d96aa85a22.camel@gmail.com>
Date:   Sat, 02 Dec 2023 10:37:20 +0100
From:   Nuno Sá <noname.nuno@...il.com>
To:     David Lechner <dlechner@...libre.com>, nuno.sa@...log.com
Cc:     linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-iio@...r.kernel.org,
        Olivier MOYSAN <olivier.moysan@...s.st.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>
Subject: Re: [PATCH 00/12] iio: add new backend framework

On Fri, 2023-12-01 at 21:53 -0600, David Lechner wrote:
> On Thu, Nov 30, 2023 at 5:54 PM David Lechner <dlechner@...libre.com> wrote:
> > 
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@...nel.org> wrote:
> > > 
> > > Hi all,
> > > 
> > > This is a Framework to handle complex IIO aggregate devices.
> > > 
> > > The typical architecture is to have one device as the frontend device which
> > > can be "linked" against one or multiple backend devices. All the IIO and
> > > userspace interface is expected to be registers/managed by the frontend
> > > device which will callback into the backends when needed (to get/set
> > > some configuration that it does not directly control).
> > > 
> > > The basic framework interface is pretty simple:
> > >  - Backends should register themselves with @devm_iio_backend_register()
> > >  - Frontend devices should get backends with @devm_iio_backend_get()
> > > 
> > > (typical provider - consumer stuff)
> > > 
> > 
> > The "typical provider - consumer stuff" seems pretty straight forward
> > for finding and connecting two different devices, but the definition
> > of what is a frontend and what is a backend seems a bit nebulous. It
> > would be nice to seem some example devicetree to be able to get a
> > better picture of how this will be used in practices (links to the the
> > hardware docs for those examples would be nice too).
> > 
> 
> Fulfilling my own request here...
> 
> Since AD9467 is being use as the example first user of the IIO offload framework
> I did a deep dive into how it is actually being used. It looks like this:
> 

This is not an offload framework... I think somehow you're connecting this to the
spi_engine offload and these are two completely different things. Maybe they can
intersect at some point but as of now, I don't see any benefit in doing it. The goal
of this patchseries is to have a simple and generic framework so we can connect IIO
devices (frontends) to a backend device having kind of an IIO aggregate device so to
say. Moreover, we just want to have the ad9467 driver in the same state it was before
to keep things simple. I'm already fixing some things but I don't want extend that
too much as the primary goal is to have the framework in. Cleanups can come
afterwards.

That said, is fine to have this kind of discussion but I honestly think you're over
engineering the whole thing. Maybe you're already too ahead of me :), but where we
stand right now, I don't see any reason for anything so complicated as the below.
Also note this should be simple and generic. As I already said, this is not supposed
to be an ADI only thing and STM also wants to make use of this infrastructure. But
see below some of my comments on why I think it's too much...
 
> ----------------------------------------------------------------------------
> Hardware
> ----------------------------------------------------------------------------
> 
> AD9467
> ------
> 
> High-speed ADC that uses SPI for configuration and LVDS for data capture.
> 
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD9467.pdf
> 
> Pins:
> 
>     Supplies:
>     - AVDD1 (1.8V analog supply)
>     - AVDD2 (3.3V analog supply)
>     - AVDD3 (1.8V analog supply)
>     - SPIVDD (1.8V or 3.3V SPI supply)
>     - DRVDD (1.8V digital output driver supply)
>     - XVREF (optional reference voltage)
> 
>     SPI:
>     - CSB
>     - SDIO
>     - SCLK
> 
>     Analog input:
>     - VIN+/VIN-
> 
>     Clock input:
>     - CLK+/CLK-
> 
>     Digital output:
>     - Dn-/Dn+ (parallel digital output)
>     - DCO+/DCO- (data clock output)
>     - OR+/OR- (out of range output)
> 
> 
> ADI AXI ADC
> -----------
> 
> FPGA IP core for interfacing an ADC device with a high speed serial (JESD204B)
> or source synchronous parallel interface (LVDS/CMOS).
> 
> https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> 
> Interfaces:
> 
>     LVDS:
>     - rx_clk_in_[p|n]   clock input
>     - rx_data_in_[p|n]  parallel data input
>     - rx_frame_in_[p|n] frame input signal (optional/device specific)
>     - rx_or_in_[p|n]    over range input (optional/device specific)
> 
>     Write FIFO:
>     - see link for details, this consists of multiple signals that connect to a
>       "sink module"
> 
>     MMIO:
>     - memory mapped registers (detailed in link)
> 
> 
> "sink module"
> -------------
> 
> FPGA IP core for piping a data stream to DMA.
> 
> https://wiki.analog.com/resources/fpga/docs/util_var_fifo
> 
> Interfaces:
> 
>     Data Input:
>     - data_in           Data to be stored
>     - data_in_valid     Valid for the input data
> 
>     Data Output:
>     - data_out          Data forwarded to the DMA
>     - data_out_valid    Valid for the output data
> 
>     There are additional interfaces but they probably don't concern devicetree
>     since software doesn't interact with them (AFAIK).
> 
> 
> Schematic
> ---------
> 
> +----------------------------+    +---------------+    +-------------------+
> > SOC/FPGA                   |    | ADC           |    | Signal generator  |
> >                            |    |               |    |                   |
> >  +--------------------+    |    |       VIN+/- xxxxxxxx VOUT+/-          |
> >  | SPI                |    |    |               |    |                   |
> >  |               SDIO ------------ SDIO         |    +-------------------+
> >  |               SCLK ------------ SCLK         |
> >  |                 CS ------------ CSB          |    +-------------------+
> >  |                    |    |    |               |    | External clock    |
> >  +--------------------+    |    |               |    |                   |
> >  | AXI ADC            |    |    |      CLK+/- xxxxxxxxx CLKOUT+/-        |
> >  |                    |    |    |               |    |                   |
> >  |  rx_data_in_[p|n] xxxxxxxxxxxxx Dn+/-        |    +-------------------+
> >  |   rx_clk_in_[p|n] xxxxxxxxxxxxx DCO+/DCO-    |
> >  |    rx_or_in_[p|n] xxxxxxxxxxxxx OR+/-        |    +-------------------+
> >  |                    |    |    |               |    | Power supplies    |
> >  |        Write FIFO ===#  |    |               |    |                   |
> >  +--------------------+ H  |    |        AVDD1 --------- 1.8V            |
> >  | "sink module"      | H  |    |        AVDD2 --------- 2.2V            |
> >  |                    | H  |    |        AVDD3 --------- 1.8V            |
> >  |        Data Input ===#  |    |       SPIVDD --------- 3.3V            |
> >  |                    |    |    |        DRVDD --------- 1.8V            |
> >  |       Data Output ===#  |    |        XVREF --------- 1.2V            |
> >  +--------------------+ H  |    |               |    |                   |
> >  | DMA controller     | H  |    +---------------+    +-------------------+
> >  |                    | H  |
> >  |         channel 0 ===#  |
> >  |                    |    |
> >  +--------------------+    |
> >                            |
> +----------------------------+
> 
> ----------------------------------------------------------------------------
> Devicetree
> ----------------------------------------------------------------------------
> 
> Current situation:
> 
> &soc: {
>     spi {
>         ...
> 
>         spi_adc: adc@0 {
>             compatible = "adi,ad9467";
>             reg = <0>;
>             clocks = <&adc_clk>;
>             clock-names = "adc-clk";
>         };
>     };
> 
>     fpga {
>         ...
> 
>         /* IIO device is associated with this node. */
>         axi-adc@...00000 {
>             compatible = "adi,axi-adc-10.0.a";
>             reg = <0x44a00000 0x10000>;
>             /* Not sure dmas belong here since it is a property of the
>              * "sink module" which is separate from AXI ADC module. */
>             dmas = <&rx_dma 0>;
>             dma-names = "rx";
> 
>             adi,adc-dev = <&spi_adc>;
>         };
>     };
> };
> 
> ---
> 
> Proposed IIO backend framework (inferred from v1 patches):
> 
> &soc: {
>     spi {
>         ...
> 
>         /* IIO device is associated with this node. */
>         adc@0 {
>             compatible = "adi,ad9467";
>             reg = <0>;
>             clocks = <&adc_clk>;
>             clock-names = "adc-clk";
>             /* As discussed already, this isn't really the right place for
>              * dmas either. */
>             dmas = <&rx_dma 0>;
>             dma-names = "rx";

Not ideally but if they stay here is also not a big deal/problem...
> 
>             io-backends = <&axi_adc>;
>         };
>     };
> 
>     fpga {
>         ...
> 
>         axi_adc: axi-adc@...00000 {
>             compatible = "adi,axi-adc-10.0.a";
>             reg = <0x44a00000 0x10000>;
>         };
>     };
> };
> 
> ---
> 
> Composite device?
> 
> /* IIO device is associated with this node. */
> adc {
>     compatible = "adi,ad9467";
> 
>     io-backends = <&adc_spi_backend>, <&adc_lvds_backend>;
> 
>     clocks = <&adc_clk>;
>     clock-names = "adc-clk";
> 
>     xvref-supply = <&ref_voltage>;
>     avdd1-supply = <&regulator_1_8V>;
>     avdd2-supply = <&regulator_3_3V>;
>     avdd3-supply = <&regulator_1_8V>;
> };
> 
> &soc: {
>     spi {
>         ...
> 
>         /* This node describes only the SPI aspects of the ADC chip */
>         adc_spi_backend: adc@0 {
>             compatible = "adi,ad9467-spi-io-backend";
>             reg = <0>;
> 
>             spi-3wire;
>             spi-max-frequency = <25000000>;
> 
>             spivdd-supply = <&regulator_1_8V>;
>         };
>     };
> 
>     fpga {
>         ...
> 
>         /* This node is an LVDS bus, analogous to a SPI bus or I2C bus */
>         axi-adc@...00000 {
>             compatible = "adi,axi-adc-10.0.a";
>             reg = <0x44a00000 0x10000>;
> 
>             ...
> 
>             /* This node describes all sink modules that are connected to
>              * the AXI ADC controller through the FPGA fabric. */
>             sink-modules {
>                 ...
> 
>                 /* This node describes the FIFO to DMA sink module used by
>                  * our ADC. */
>                 adc_dma: module@0 {
>                     compatible = "adi,dma-fifo-1.0.a";

Why a different compatible?
> 

>                     reg = <0>;
> 
>                     dmas = <&rx_dma 0>;
>                     dma-names = "rx";
>                 };
>             };
> 
>             /* Then we describe what is connected to each channel of the
>              * controller (reg == channel number). */

I don't think we need to describe that in here. We might get to a point where we
might need some clever properties in the backend device but also note that the
frontend device should also know what to do with the device. Most of the knowledge
should be there (of course we should not have properties in the frontend that don't
belong to that device).

> 
>             /* This node describes only the digital output (LVDS) aspects of
>             * the ADC chip */
>             adc_lvds_backend: adc@0 {
>                 compatible = "adi,ad9467-lvds-io-backend";

This is just adding complexity... why different compatibles? 

>                 reg = <0>;
> 
>                 drvdd-supply = <&regulator_1_8V>;
> 
>                 /* This is a LVDS bus peripheral property that can only be used
>                  * with peripheral nodes that are children of a compatible =
>                  * "adi,axi-adc-10.0.a" node. This works exactly like the
>                  * controller-specific SPI bus peripheral properties, e.g.
>                  * like samsung,spi-peripheral-props.yaml. */
>                 adi,sink-modules = <&adc_dma>;
>             }
>         };
>     };
> };
> 
> 
> ----------------------------------------------------------------------------
> Discussion
> ----------------------------------------------------------------------------
> 
> After reviewing the existing device tree bindings, it appears the current
> adi,ad9467.yaml is incomplete. It lacks the supplies and even though the
> example shows that it is a child of a spi node, it is missing a reference to
> /schemas/spi/spi-peripheral-props.yaml# and spi properties like spi-3wire
> and spi-max-frequency. It also misses a description of what is connected to
> the digital output, but that I think that is the main thing we are trying to
> solve here - if it belongs there or somewhere else.

As stated, bindings can be improved/cleaned afterwards...

> 
> Having read more about the AXI ADC IP block, it seems to me like it should just
> be considered an LVDS bus controller with generic bindings similar to how we
> have SPI and I2C buses.

No... That core can handle LVDS interfaces, yes. But it can also do CMOS and JESD. So
you can see already that having a bus for it is completely over-kill. And those cores
have a 1:1 connection. It's just a serial interface to transfer data at high-speed
rates so I'm not really seeing how we would benefit from having a bus for it. Of
course if the day comes where this gets somehow used outside of converters and IIO
devices, we could think in making it a generic bus. As of now, I'm honestly not
seeing it.

And to add a bit more on the 1:1 stuff, what sometimes happens for highly complex
devices (like RF transceivers) with complex RX/TX data paths where each path as a
serial data interface (LVDS/CMOS/JESD) is that you have an instance of the
AXI_ADC/AXI_DAC cores per data path. So, effectively you end up with one frontend
(the transceiver) with multiple backends which also fits nicely in the current
proposal. Multiple frontends for one backend is something that I'm not aware (at
least ADI wise) but it might be a thing for other usecases. Also doable, but then you
already need to add support for it (like adding counters for enable/disable states
and things like that).

Since I'm talking usecases, one that we have at ADI that is upstreamable is a
cascaded one where the AXI_DAC core can serve as backend and frontend at the same
time. That might happen in cases the DAC core has an interpolation core behind it.
So, some fun ahead of us :) (should also be fairly straight with what we have).

One thing that at some point I might ask our hdl guys is to have an ID register so we
can identify for which frontend the backend was built. This could be useful to pass
come config to the core so we could do some safety checks. A nice thing but also not
a super duper crucial one...

> 
> Following that line of thought, if the compatible = "adi,axi-adc-10.0.a" node
> is a bus node, then logically, the ADC device node should be a child node of
> that LVDS bus node. But since the ADC is also a SPI device it also needs to
> be a child node of the SPI bus node. But is can't be a child of two different
> nodes, so where does it go?
> 

I think the above "kills" this line of thought :).

> This is where the IIO backend framework can come in. We can create a "composite"
> device as seen in the example dts above. This is just a platform device (in
> Linux-speak) and it composes the two "io-backend" devices from the the two
> busses to create a logical iio/adc device.
> 
> To solve the mystery of "where does the dmas property belong?", I have taken
> a page out of the work I am preparing the AXI SPI Engine offload support [1].
> (This hasn't been submitted to a mailing list yet because I'm still working
> on the driver, but the bindings are finished and I ran the idea by Rob on IRC
> a while back who suggested doing it this way, so maybe it works here too?)
> 
> [1]:
> https://github.com/analogdevicesinc/linux/pull/2341/commits/57bb1998371f61f4144689136aba5dd6d16a2a66
> 
> Since the "sink module" is really a separate IP block from the AXI ADC, it gets
> its own node and compatible string. Since these "sink modules" are connected to
> the AXI ADC, they get listed as child nodes, but we group them together under a
> single sink-modules node to separate them from the LVDS peripherals nodes. Then
> they get associated with a peripheral with the adi,sink-modules property (also
> see comment on that property in the example above).
> 

I never really looked at that IP but if it is something sitting behind the axi_adc
might also be a candidate for a cascaded configuration. Anyways, as I said, it's fine
to discuss it but that is all stuff that will only go in once we have users for it.
I'm also not aware if we do have an usecase for it or if it's something we control at
all from linux (might be a pure hw thing).

> My "composite" device example evolved quite a bit as I was writing this but I'm
> pretty happy with where it ended up. I think adding child nodes to the axi-adc
> node answers Nuno's concerns about how to keep a generic axi-adc binding while
> accounting for the fact that it changes slightly depending on what it is
> attached to. And having a separate platform device solves my questions about
> the ambiguity of which should be the front end, the spi node or the axi-adc
> node. It turns out, perhaps the answer is neither.

So, in conclusion, let's make this simple for now. When we have enough users and we
are aware of the real complexity of the whole thing we can see how to proceed. It
might even be you're right from the beginning and we end up in something like this
(but at least for the bus stuff I'm very sceptical). Now, if we add too much
complexity right from the beginning, it might be way harder to change/scale it down
the road.

Anyways, thanks for your feedback and for some bugs issues you already found in the
series!

- Nuno Sá

Powered by blists - more mailing lists