Re: [PATCH v2 1/2] dt-bindings: misc: adi,axi-tdd: Add device-tree binding for TDD engine

[Conor Dooley]

On Mon, Oct 02, 2023 at 07:48:42PM +0000, Balas, Eliza wrote:
> > -----Original Message-----
> > From: Conor Dooley <conor@xxxxxxxxxx>
> > Sent: Monday, October 2, 2023 22:21
> > To: Balas, Eliza <Eliza.Balas@xxxxxxxxxx>
> > Cc: Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>;
> > Derek Kiernan <derek.kiernan@xxxxxxx>; Dragan Cvetic <dragan.cvetic@xxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>; Greg
> > Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v2 1/2] dt-bindings: misc: adi,axi-tdd: Add device-tree binding for TDD engine
> > 
> > [External]
> > 
> > On Mon, Oct 02, 2023 at 04:46:26PM +0000, Balas, Eliza wrote:
> > > > -----Original Message-----
> > > > From: Rob Herring <robh@xxxxxxxxxx>
> > > > Sent: Monday, October 2, 2023 19:33
> > > > To: Balas, Eliza <Eliza.Balas@xxxxxxxxxx>
> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Derek Kiernan
> > > > <derek.kiernan@xxxxxxx>; Dragan Cvetic <dragan.cvetic@xxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>; Greg Kroah-Hartman
> > > > <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > > > Subject: Re: [PATCH v2 1/2] dt-bindings: misc: adi,axi-tdd: Add device-tree binding for TDD engine
> > > >
> > > > [External]
> > > >
> > > > On Thu, Sep 28, 2023 at 12:28:03PM +0300, Eliza Balas wrote:
> > > > > Add device tree documentation for the AXI TDD Core.
> > > > > The generic TDD controller is in essence a waveform generator
> > > > > capable of addressing RF applications which require Time Division
> > > > > Duplexing, as well as controlling other modules of general
> > > > > applications through its dedicated 32 channel outputs.
> > > > >
> > > > > The reason of creating the generic TDD controller was to reduce
> > > > > the naming confusion around the existing repurposed TDD core
> > > > > built for AD9361, as well as expanding its number of output
> > > > > channels for systems which require more than six controlling signals.
> > > > >
> > > > > Signed-off-by: Eliza Balas <eliza.balas@xxxxxxxxxx>
> > > > > ---
> > > > >  .../devicetree/bindings/misc/adi,axi-tdd.yaml | 65 +++++++++++++++++++
> > > > >  MAINTAINERS                                   |  7 ++
> > > > >  2 files changed, 72 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml b/Documentation/devicetree/bindings/misc/adi,axi-
> > tdd.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..8938da801b95
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
> > > > > @@ -0,0 +1,65 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +# Copyright 2023 Analog Devices Inc.
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: https://urldefense.com/v3/__http://devicetree.org/schemas/misc/adi,axi-
> > > > tdd.yaml*__;Iw!!A3Ni8CS0y2Y!5Cxus2huppjhkiJZLWTpJEgA0IXdLZx4t0fS9J1yt0xgjp9g3Y1N5PbZ7pAcIkKU4WPbS_TR9yjTKw$
> > > > > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-
> > > >
> > schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!5Cxus2huppjhkiJZLWTpJEgA0IXdLZx4t0fS9J1yt0xgjp9g3Y1N5PbZ7pAcIkKU4WPbS_RK8aQ9xw$
> > > > > +
> > > > > +title: Analog Devices AXI TDD Core
> > > > > +
> > > > > +maintainers:
> > > > > +  - Eliza Balas <eliza.balas@xxxxxxxxxx>
> > > > > +
> > > > > +description: |
> > > > > +  The TDD controller is a waveform generator capable of addressing RF
> > > > > +  applications which require Time Division Duplexing, as well as controlling
> > > > > +  other modules of general applications through its dedicated 32 channel
> > > > > +  outputs. It solves the synchronization issue when transmitting and receiving
> > > > > +  multiple frames of data through multiple buffers.
> > > > > +  The TDD IP core is part of the Analog Devices hdl reference designs and has
> > > > > +  the following features:
> > > > > +    * Up to 32 independent output channels
> > > > > +    * Start/stop time values per channel
> > > > > +    * Enable and polarity bit values per channel
> > > > > +    * 32 bit-max internal reference counter
> > > > > +    * Initial startup delay before waveform generation
> > > > > +    * Configurable frame length and number of frames per burst
> > > > > +    * 3 sources of synchronization: external, internal and software generated
> > > > > +  For more information see the wiki:
> > > > > +  https://wiki.analog.com/resources/fpga/docs/axi_tdd
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - adi,axi-tdd-2.00.a
> > > >
> > > > Where does this version number come from? I looked at the above link and
> > > > see versions such as '2021_R2', '2019_r2', etc. I didn't dig deeper
> > > > whether there's some per IP version.
> > > >
> > > > If you want to use version numbers, please document the versioning
> > > > scheme. For example, see
> > > > Documentation/devicetree/bindings/sifive/sifive-blocks-ip-versioning.txt.
> > > >
> > > > Rob
> > >
> > > The version refers to the IP version. The version of the IP is also
> > > specified in its VERSION register (there is a drop down to expand the
> > > register map on the wiki page) which is verified by the driver during
> > > probe. "2021_R2" refers to the compatible tool version used for
> > > creating the FPGAIP Core.
> > 
> > If you have version registers in these IPs, what benefit does version
> > numbers in the compatible string bring?
> > Rather than using the version numbers to validate what the DT gave you,
> > which not the kernel's job IMO, why not just use the information from
> > the register to determine the version?
> > 
> > Cheers,
> > Conor.
> 
> As the description of this patch says, we want to resolve the naming confusion around the existing repurposed TDD core (https://wiki.analog.com/resources/eval/user-guides/ad-pzsdr2400tdd-eb/reference_hdl#tdd_controller)
> built for AD9361 and this TDD Engine IP core (https://wiki.analog.com/resources/fpga/docs/axi_tdd) which is a similar core, with more output channels and some extra features. The version numbers in the compatible string are used to differentiate between the two IPs.

Firstly, please fix your mail client to wrap text at a sane width :)
Secondly, where is the binding for that TDD ad9361 specific core that
calling this generic one "adi,axi-tdd" would conflict with?
Grepping the bindings directory of the kernel tree for "adi.*tdd" returns
nothing. If there is an ad9361 specific tdd, I would expect it to have a
compatible like "adi,ad9361-tdd".

Attachment: signature.asc
Description: PGP signature