On Fri, Jul 11, 2025 at 05:28:57PM +0100, Conor Dooley wrote:

On Fri, Jul 11, 2025 at 10:14:45AM +0200, Jorge Marques wrote:

quoted

On Wed, Jul 02, 2025 at 12:38:41PM +0100, Conor Dooley wrote:

quoted

On Wed, Jul 02, 2025 at 12:58:00PM +0200, Jorge Marques wrote:

quoted

On Fri, Jun 27, 2025 at 06:02:26PM +0200, Jorge Marques wrote:

quoted

On Fri, Jun 27, 2025 at 04:49:19PM +0200, Krzysztof Kozlowski wrote:

quoted

On 27/06/2025 16:38, Jorge Marques wrote:

quoted

On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:

quoted

On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:

quoted

Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
core that implements the MIPI I3C Basic controller specification.

How did you resolve my last comment? I don't see any explanation -
neither here nor in the binding description. Binding description is
actually better place, I think now.

Best regards,
Krzysztof

Hi Krzysztof,

I forgot to condense out discussion on v4.
What about this binding description:

  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.
    It takes one or two clocks, axi and i3c. If only axi is provided,
    then there is no clock signal to the i3c input clock pin and axi

This is obvious from the schema, drop.

Ack.

quoted

quoted

    clock drives the whole IP. The compatible is suffixed by 1.00.a
    foreseeing future controllers by Analog Devices Inc. and breaking
    changes.

I don't understand that. How are you breaking any changes? And how
1.00.a predicts future? I don't think this reflects previous discussion.
Why you were asked to go with v1.00.a?

The -1.00.a suffix came from this discussion:
  
  https://lore.kernel.org/linux-i3c/ildi2pup2zkyv4stuknkrjysex3yzsbrrsrwbgcc4xgvdhwrdd@7qh4y6mutgy2/ (local)

Other adi bindings use this suffix. I personally wouldn't add any suffix
unless told otherwise, as I expressed on the thread. Should I drop it?
or suffix it with something else?

quoted

I went after the reason of the historically -1.00.a suffix and
discovered that they came into existence due to AMD Xilinx auto
generation devicetree tool SDTGen
https://github.com/Xilinx/system-device-tree-xlnx
that would automatically suffix with the IP version, defaulting to 1.00.a,
and for a seamless experience, the were copied over to the dt-bindings.

The adi,axi-pwmgen dt-binding went more creative and suffixed with
-2.00.a, while never enforcing Vivado to yield the devicetree with such
value (Major version is asserted in the driver through reg access, the
current core version is v2.1.1)

Testing on my side (AMD Xilinx Vivado 2024.2), it seems Vivado now
defaults to 1.0, so the previous bindings from the other IPs are not
accurate anymore, either, (axi-pwmgen auto gens
`compatible = "xlnx,axi-pwm-gen-1.0";` (`xlnx` instead of `adi`, also)).

For fun, the current Vivado version thinks the devicetree node for the
i3c master should be as follows:

  i3c_host_interface: i3c_controller_host_interface@44a00000 {
          compatible = "xlnx,i3c-controller-host-interface-1.0";
          reg = <0x44a00000 0x10000>;
          clocks = <&clkc 15>;
          clock-names = "s_axi_aclk";
  };

Let me know if we can drop the suffix, or replace with something else.
The current register defined core version is v0.1.0.

Please, as I requested in the original thread, add versioning
information. I'm clearly not asking you to make up some garbage version
number, or to add "1.00a" if that's not an accurate version (and never
did AFAICT). If 0.1.0 is the version of the IP core, I'd like you to use
that.

If other ADI IP cores are using crap version numbers that came from
some Xilinx tooling that don't represent the actual versions of the IPs,
that's probably something you should mention to your colleagues that
maintain those bindings.

My proposal (1) for the Analog Devices Inc. open source IP Cores is to
migrate to semantic versioning.
(1) https://github.com/analogdevicesinc/hdl/pull/1831

The dt-binding must end with the major value, with the first supported
version starting at 1. Therefore, the binding will be:

* adi,i3c-master-1

At driver level, the debug version value is `v%u.%u.%u`, and the driver
shall check the minor version value for handling feature changes. While
the major bump is restricted for breaking changes, which, in some cases,
also generates a new dt-binding.

For backwards compatibility, existing ADI IP Cores that prints `v%u.%u.%c`
should print `v%u.%u.%u` instead, but keep their current value (e.g.,
`a`->`97`), until the minor is bumped, when the patch version resets to 0.

Rework across register accessed IPs is being handled here:
https://github.com/analogdevicesinc/hdl/pull/1829
There are more than 50 IPs with register access, so I need to be cautious.

What is your take on this? Does it resolve the bindings concern?

I think it is a good idea to establish version numbers that actually
have a meaning, rather than using something garbage. I think you should
probably put the "v" into the number in the compatible - 1.00 or 1.00.a
is fairly clearly a version number but "-1" is less clear.

Hi Conor,

The format that is currently winning my internal discussion is
`adi,<ip-name>-v<major>`.
And the next patch will be submitted as `adi,i3c-master-v1`.

Also "the binding must end with the major value" is a bit of an odd
requirement I think. The sort of situations that may cause different
compatibles to be introduced are not likely to be restricted to majors.

sifive-blocks-ip-versioning.txt uses
"sifive,<ip-name><integer-version-number>", While the xilinx.txt uses
the last version that uses the same interface. Both implicitly hide away
features (minor) and fixes (patches). What I am enforcing is that, if
the interface changes, the major field must be bumped. In most cases,
everything else is minor matching at the driver level, and if exceptions
occur, add to the commit message the justification and that's it.

That may be something like properties only valid for some versions, or
at worst a bug that requires checking the compatible (even if these
are much rarer when there's a version register, they're still possible).

IDM what number you opt for in the end either, it could well just be the
lowest number where the current register offset/format for the version
number register exists.

It's a bit hypocritical of me to ask this, since I have not yet written
one for the Microchip stuff - but I think you should document whatever
numbering you end up on, like in xilinx.txt and
sifive-blocks-ip-versioning.txt, so that we can point people at it.

Best regards,
Jorge

quoted

Adding David to the thread, since the axi-spi-engine already moved to
`v%u.%u.%u` format.

Best regards,
Jorge