Message-ID: <f9a2b74f371fb4b02486ad7426c57b75739438f3.camel@gmail.com>
Date: Wed, 02 Oct 2024 11:00:06 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Angelo Dureghello
 <adureghello@...libre.com>, Krzysztof Kozlowski <krzk@...nel.org>, 
 Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sa <nuno.sa@...log.com>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Olivier Moysan <olivier.moysan@...s.st.com>,
 linux-iio@...r.kernel.org,  linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, dlechner@...libre.com
Subject: Re: [PATCH v3 04/10] dt-bindings: iio: dac: ad3552r: add io-backend
 support

On Tue, 2024-10-01 at 19:29 +0100, Jonathan Cameron wrote:
> On Tue, 01 Oct 2024 10:23:45 +0200
> Nuno Sá <noname.nuno@...il.com> wrote:
> 
> > On Mon, 2024-09-30 at 16:09 +0100, Jonathan Cameron wrote:
> > > On Mon, 30 Sep 2024 15:22:01 +0200
> > > Angelo Dureghello <adureghello@...libre.com> wrote:
> > >   
> > > > On 30.09.2024 09:20, Nuno Sá wrote:  
> > > > > On Sun, 2024-09-29 at 11:59 +0100, Jonathan Cameron wrote:    
> > > > > > On Sat, 28 Sep 2024 14:20:29 +0200
> > > > > > Krzysztof Kozlowski <krzk@...nel.org> wrote:
> > > > > >     
> > > > > > > On 25/09/2024 13:55, Nuno Sá wrote:    
> > > > > > > > On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:      
> > > > > > > > > On 24/09/2024 14:27, Nuno Sá wrote:      
> > > > > > > > > > On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:    
> > > > > > > > > > > On 23/09/2024 17:50, Angelo Dureghello wrote:      
> > > > > > > > > > > > Hi Krzysztof,
> > > > > > > > > > > > 
> > > > > > > > > > > > On 22/09/24 23:02, Krzysztof Kozlowski wrote:      
> > > > > > > > > > > > > On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo
> > > > > > > > > > > > > Dureghello
> > > > > > > > > > > > > wrote:      
> > > > > > > > > > > > > > From: Angelo Dureghello <adureghello@...libre.com>
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > There is a version AXI DAC IP block (for FPGAs) that
> > > > > > > > > > > > > > provides
> > > > > > > > > > > > > > a physical bus for AD3552R and similar chips, and acts
> > > > > > > > > > > > > > as
> > > > > > > > > > > > > > an SPI controller.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > For this case, the binding is modified to include some
> > > > > > > > > > > > > > additional properties.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Signed-off-by: Angelo Dureghello
> > > > > > > > > > > > > > <adureghello@...libre.com>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   |
> > > > > > > > > > > > > > 42
> > > > > > > > > > > > > > ++++++++++++++++++++++
> > > > > > > > > > > > > >   1 file changed, 42 insertions(+)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > diff --git
> > > > > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > > yaml
> > > > > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > > yaml
> > > > > > > > > > > > > > index 41fe00034742..aca4a41c2633 100644
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > > yaml
> > > > > > > > > > > > > > +++
> > > > > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > > yaml
> > > > > > > > > > > > > > @@ -60,6 +60,18 @@ properties:
> > > > > > > > > > > > > >       $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > > > > > > >       enum: [0, 1, 2, 3]
> > > > > > > > > > > > > >   
> > > > > > > > > > > > > > +  io-backends:
> > > > > > > > > > > > > > +    description: The iio backend reference.
> > > > > > > > > > > > > > +      An example backend can be found at
> > > > > > > > > > > > > > +       
> > > > > > > > > > > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > > > > > > > > > > > +    maxItems: 1
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  adi,synchronous-mode:
> > > > > > > > > > > > > > +    description: Enable waiting for external
> > > > > > > > > > > > > > synchronization
> > > > > > > > > > > > > > signal.
> > > > > > > > > > > > > > +      Some AXI IP configuration can implement a dual-IP
> > > > > > > > > > > > > > layout,
> > > > > > > > > > > > > > with
> > > > > > > > > > > > > > internal
> > > > > > > > > > > > > > +      wirings for streaming synchronization.
> > > > > > > > > > > > > > +    type: boolean
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > >     '#address-cells':
> > > > > > > > > > > > > >       const: 1
> > > > > > > > > > > > > >   
> > > > > > > > > > > > > > @@ -128,6 +140,7 @@ patternProperties:
> > > > > > > > > > > > > >             - custom-output-range-config
> > > > > > > > > > > > > >   
> > > > > > > > > > > > > >   allOf:
> > > > > > > > > > > > > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > > > > > > > > > > > >     - if:
> > > > > > > > > > > > > >         properties:
> > > > > > > > > > > > > >           compatible:
> > > > > > > > > > > > > > @@ -238,4 +251,33 @@ examples:
> > > > > > > > > > > > > >               };
> > > > > > > > > > > > > >           };
> > > > > > > > > > > > > >       };
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  - |
> > > > > > > > > > > > > > +    axi_dac: spi@...70000 {
> > > > > > > > > > > > > > +        compatible = "adi,axi-ad3552r";      
> > > > > > > > > > > > > That is either redundant or entire example should go to
> > > > > > > > > > > > > the
> > > > > > > > > > > > > parent
> > > > > > > > > > > > > node,
> > > > > > > > > > > > > if this device is fixed child of complex device (IOW,
> > > > > > > > > > > > > adi,ad3552r
> > > > > > > > > > > > > cannot
> > > > > > > > > > > > > be used outside of adi,axi-ad3552r).      
> > > > > > > > > > > > 
> > > > > > > > > > > > ad3552r can still be used by a generic "classic" spi
> > > > > > > > > > > > controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> > > > > > > > > > > > controller only (axi-ad3552r) can reach 33MUPS.      
> > > > > > > > > > > 
> > > > > > > > > > > OK, then this is just redundant. Drop the node. Parent example
> > > > > > > > > > > should
> > > > > > > > > > > contain the children, though.      
> > > > > > > > > > > >      
> > > > > > > > > > > > >      
> > > > > > > > > > > > > > +        reg = <0x44a70000 0x1000>;
> > > > > > > > > > > > > > +        dmas = <&dac_tx_dma 0>;
> > > > > > > > > > > > > > +        dma-names = "tx";
> > > > > > > > > > > > > > +        #io-backend-cells = <0>;
> > > > > > > > > > > > > > +        clocks = <&ref_clk>;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +        #address-cells = <1>;
> > > > > > > > > > > > > > +        #size-cells = <0>;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +        dac@0 {
> > > > > > > > > > > > > > +            compatible = "adi,ad3552r";
> > > > > > > > > > > > > > +            reg = <0>;
> > > > > > > > > > > > > > +            reset-gpios = <&gpio0 92 0>;      
> > > > > > > > > > > > > Use standard defines for GPIO flags.      
> > > > > > > > > > > > 
> > > > > > > > > > > > fixed, thanks
> > > > > > > > > > > >      
> > > > > > > > > > > > > > +            io-backends = <&axi_dac>;      
> > > > > > > > > > > > > Why do you need to point to the parent? How much coupled
> > > > > > > > > > > > > are
> > > > > > > > > > > > > these
> > > > > > > > > > > > > devices? Child pointing to parent is not usually expected,
> > > > > > > > > > > > > because
> > > > > > > > > > > > > that's obvious.      
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > "io-backends" is actually the way to refer to the backend
> > > > > > > > > > > > module,
> > > > > > > > > > > > (used already for i.e. ad9739a),
> > > > > > > > > > > > it is needed because the backend is not only acting as spi-
> > > > > > > > > > > > controller,
> > > > > > > > > > > > but is also providing some APIs for synchronization and bus
> > > > > > > > > > > > setup
> > > > > > > > > > > > support.      
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > But if backend is the parent, then this is redundant. You can
> > > > > > > > > > > take
> > > > > > > > > > > it
> > > > > > > > > > > from the child-parent relationship. Is this pointing to other
> > > > > > > > > > > devices
> > > > > > > > > > > (non-parent) in other ad3552r configurations?
> > > > > > > > > > >      
> > > > > > > > > > 
> > > > > > > > > > The backend is a provider-consumer type of API. On the consumer
> > > > > > > > > > side
> > > > > > > > > > (which
> > > > > > > > > > is the
> > > > > > > > > > driver the child node will probe on), we need to call
> > > > > > > > > > devm_iio_backend_get()
> > > > > > > > > > to get
> > > > > > > > > > the backend object (which obviously is the parent). For that,
> > > > > > > > > > 'io-
> > > > > > > > > > backends'
> > > > > > > > > > is being      
> > > > > > > > > 
> > > > > > > > > You described the driver, so how does it matter? Driver can call
> > > > > > > > > get_backend_from_parent(), right? Or
> > > > > > > > > get_backend_from_fwnode(parent)?      
> > > > > > > > 
> > > > > > > > Well yes, just stating what the framework (also in terms of
> > > > > > > > bindings) is
> > > > > > > > expecting. Of course that on the driver side we can paper around it
> > > > > > > > the
> > > > > > > > way we
> > > > > > > > want. But my main point was that we can only paper around it if we
> > > > > > > > use
> > > > > > > > code that
> > > > > > > > is meant not to be used.
> > > > > > > > 
> > > > > > > > And, FWIW, I was (trying) replying to your comment
> > > > > > > > 
> > > > > > > > "You can take it from the child-parent relationship"
> > > > > > > > 
> > > > > > > > Again, we can only do that by introducing new code or use code
> > > > > > > > that's not
> > > > > > > > meant
> > > > > > > > to be used. The way we're supposed to reference backends is by
> > > > > > > > explicitly
> > > > > > > > using
> > > > > > > > the proper FW property.
> > > > > > > > 
> > > > > > > > Put it in another way and a completely hypothetical case. If we have
> > > > > > > > a spi
> > > > > > > > controller which happens to export some clock and one of it's
> > > > > > > > peripherals
> > > > > > > > ends
> > > > > > > > up using that clock, wouldn't we still use 'clocks' to reference
> > > > > > > > that
> > > > > > > > clock?      
> > > > > > > 
> > > > > > > I asked how coupled are these devices. Never got the answer and you
> > > > > > > are
> > > > > > > reflecting with question. Depends. Please do not create hypothetical,
> > > > > > > generic scenarios and then apply them to your one particular opposite
> > > > > > > case.    
> > > > > > 
> > > > > > I'll throw a possible clarifying question in here.  Could we use this
> > > > > > device with a multimaster SPI setup such that the control is on a
> > > > > > conventional
> > > > > > SPI controller (maybe a qspi capable one), and the data plane only goes
> > > > > > through
> > > > > > a specific purpose backend?  If so, then they are not tightly coupled
> > > > > > and
> > > > > > the reference makes sense.  Putting it another way, the difference
> > > > > > between
> > > > > > this case and all the prior iio-backend bindings is the control and
> > > > > > dataplanes
> > > > > > use the same pins.  Does that have to be the case at the host end?  If
> > > > > > it
> > > > > > does,
> > > > > > then the reference isn't strictly needed and this becomes a bit like
> > > > > > registering a single device on an spi bus or an i2c bus depending on who
> > > > > > does the registering (which is down to the parent in DT).
> > > > > >     
> > > > > 
> > > > > So, we currently have two drivers (with a new one being added in this
> > > > > series)
> > > > > for the same device:
> > > > > 
> > > > > 1) A SPI one tied to a typical spi controller. This is the "low speed"
> > > > > implementation and does not use backends;
> > > > > 2) The new platform device that is connected like this to the backend.
> > > > > 
> > > > > So yes, my understanding (but Angelo should know better :)) is that they
> > > > > are
> > > > > tightly coupled. Putting it in another way, the new platform device is
> > > > > very much
> > > > > specific to this parent (and yeah, this is a very special usecase where
> > > > > control
> > > > > and data planes are controlled by the IIO backend) and should not exist
> > > > > with it.    
> > > > 
> > > > ad3552r device can be coupled to the axi-ad3552r controller or to a generic 
> > > > spi controler.
> > > > 
> > > > We have actually 2 drivers, SPI and platform (for AXI controller, in this
> > > > patch).
> > > > 
> > > > Scenario 1 (SPI):
> > > > ad3522r can hypotetically work with whatever simple spi controller that can
> > > > read/write registers in raw mode. On simple SPI (CS, SCLK, MOSI), due to
> > > > ad3552r
> > > > chip limitation of 66Mhz clock, the maximum 33MUPS (16 bit samples) cannot
> > > > be
> > > > reached. Some QSPI DDR controller seems to be around, in that case, ad3552r
> > > > may work extending the SPI driver. 
> > > > 
> > > > Scenario 2 (AXI):
> > > > From an hardware-only point ov view axi-ad3552r IP acts as QSPI+DDR
> > > > controller
> > > > plus some additional features for stream synchronization.
> > > > From a sowftware point of view, really different from a spi controller
> > > > driver.
> > > > It's just a backend with APIes that can be called from the child driver.  
> > > 
> > > Potential? scenario 3 is the one that interested me.
> > > 
> > > ad3552 double wired to a normal SPI controller (so like option 1) and
> > > to a an offload engine (so like option 2).  With a few pull up resistors
> > > (cs and clk?) and some care it should electrically work I think.
> > > In that case we'd need the io-backend reference because the parent
> > > would be the option 1 like SPI bus and the io-backend would not be
> > > the parent.
> > > 
> > > _______________________
> > > Host       SPI MOSI    |-------------------\
> > > hard       SPI MISO 0-3|----------------\  |
> > > QSPI       SPI CLK     |--------------\  | |
> > >            SPI CS      |----------\    | | |
> > >                        |           |   | | |
> > > FPGA                   |           |   | | |   |
> > > Soft       SPI MOSI    |-----------|---|-|-x---|
> > > QSPI       SPI MISO 0-3|-----------|---|-x-----|  DAC
> > > Offload    SPI CLK     |-----------|---x-------|
> > > with DDR   SPI CS      |-----------x-----------|
> > > _______________________|
> > > 
> > > Makes all sorts of assumptions about the SPI controllers being designed
> > > for multi controllers on the same SPI buses but I'm not aware of a reason
> > > you can't do that.
> > > 
> > > As the only control message that would need to go over the offload engine
> > > would be the exit DDR (I think) that might be hard coded into a slightly
> > > simpler soft IP along with the bulk data transfer stuff.
> > >   
> > 
> > Not even sure if DDR would be a problem. Right now, I __think__ we need to
> > enable DDR both the peripheral and on the backend. On the peripheral we could
> > use the control link on the non offload controller. On the offload controller,
> > we would set it through IIO backend and that would be a backend config and not
> > go over the bus.
> 
> It's the path back to SDR that I wasn't sure about as it might need a
> DDR register write?

Ah yeah, I see what you mean now. Yeah, that one access would likely have to go
through the offload capable controller.

- Nuno Sá

Powered by blists - more mailing lists