Message-ID: <7bbopefg4ordtycyoeqijegzzhypqj7w6l7gjkacjl2hz6olce@npzdsv4j6eto>
Date: Thu, 24 Jul 2025 12:41:56 +0200
From: Jorge Marques <gastmaier@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, 
	Jorge Marques <jorge.marques@...log.com>, Alexandre Belloni <alexandre.belloni@...tlin.com>, 
	Frank Li <Frank.Li@....com>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, linux-i3c@...ts.infradead.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i3c: Add adi-i3c-master


On Mon, Jul 21, 2025 at 09:37:39AM +0200, Krzysztof Kozlowski wrote:
> On Sun, Jul 20, 2025 at 06:27:26PM -0500, Rob Herring wrote:
> > > +description: |
> > > +  FPGA-based I3C controller designed to interface with I3C and I2C peripherals,
> > > +  implementing a subset of the I3C-basic specification. The IP core is tested
> > > +  on arm, microblaze, and arm64 architectures.
> > > +
> > > +  https://analogdevicesinc.github.io/hdl/library/i3c_controller
> > > +
> > > +maintainers:
> > > +  - Jorge Marques <jorge.marques@...log.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: adi,i3c-master-v1
> > 
Hi Krzysztof and Rob,

> > If you want to use version numbers, they need to correlate to something 
> > and you need to document what that is. I don't see anything in the above 
> > link about a version 1. Kind of feels like you just made it up.
I will need a week or two before resubmitting the final version style.
We are in the process of reviewing how we version things in general
and how to comply with internal guidelines also.
For the IP Cores, it was already an internal guideline to use semantic
versioning, and I am making it public here:

* https://github.com/analogdevicesinc/hdl/pull/1831 :

  The IP cores typically should follow `Semantic Versioning <https://semver.org/>`
  ``v<major>.<minor>.<patch>`` format.
  In summary, a fix increases the patch number, a feature the minor number, and a
  breaking change the major number. The first stable release version should be
  higher or equal to v1.0.0.
  
  Device tree compatible take the major number prefixed by ``v``, for example,
  for *axi_my_ip* v1.2.3, the *compatible* is *adi,axi-my-ip-v1* and the
  *dt-binding* filename is *adi,axi-my-ip.yaml* (no major suffix). Per the last
  paragraph, *adi,axi-my-ip-v0* is **never** appropriate. Software drivers parse
  the ``VERSION`` register for feature handling across versions. The patch number
  shouldn't have to be handled by software drivers, if it seems necessary to,
  consider incrementing the minor number instead.
  
  Due to AMD Xilinx old default IP core version, many IP cores bindings start at
  1.00.a. For compatibility, the patch value is kept, but should be treated as
  decimal instead of character.

(I should have had put as a dependency in this patch submission).
The dt-binding major suffix correlates with the major version it
supports all v1.xx.xx releases.
It takes time to get to a final format because versioning may mean
different things to different people/scopes.
The PR that bumps the core to v1 is here and should be merged in the
following week:

  https://github.com/analogdevicesinc/hdl/pull/1724

At the linux level, should I add the following to the
adi,i3c-master-v1.yml?

 +Description: |
 +  FPGA-based I3C controller designed to interface with I3C and I2C peripherals,
 +  implementing a subset of the I3C-basic specification. The IP core is tested
 +  on arm, microblaze, and arm64 architectures. The IP Core versioning follows
 +  semantic versioning and divergent major versions are not compatible
 +  with this binding.

or create a dedicated file for ADI IP Cores with the guideline?

> 
> I asked already at v4 to document the naming/versioning, which was a
> result of one of previous discussions, in the binding description. :/
> 
> > 
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    minItems: 1
> > > +    items:
> > > +      - description: The AXI interconnect clock, drives the register map.
> > > +      - description: The I3C controller clock. AXI clock drives all logic if not provided.
> > 
> > Is that a description of how the h/w works? The controller clock input 
> > can literally be left disconnected? If 1 clock source drives both 
> > inputs, then the binding should reflect that.
>
It cannot be left disconnected if the IP Core was synthesized for 2
clock inputs (1). The dt-binding reflects the topology dictated by the
synthesis parameter.

(1) https://analogdevicesinc.github.io/hdl/library/i3c_controller/i3c_controller_host_interface.html#configuration-parameters
    ASYNC_CLK

How about then:

  +      - description: The AXI interconnect clock, drives the register map.
  +      - description: |
  +          The IP Core may be synthesized with a second clock input
  +          called "i3c" to drive the internal logic asynchronously to
  +          the register map. The absence of this entry reflects the
  +          topology where the "axi" clock input drives all the internal
  +          logic. 

> This was explained in reply, but never made as proper explanation to the binding.
> 
> Jorge,
> When you answer to a review about uncertain pieces like that, usually
> outcome of the discussion must end up also in new patch - either in
> commit msg or better in the binding itself. I also asked about this -
> documenting the outcode - in v4.
> 
> Best regards,
> Krzysztof
> 

Best regards,
Jorge

Powered by blists - more mailing lists