linux-kernel - Re: [PATCH v3 1/2] dt-bindings: i3c: Add adi-i3c-master

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ymmn2jgpa4bia2wl4d32ccipybxt4nylz4hspdf2svivk5ao7s@vv7v3soq2e65>
Date: Wed, 18 Jun 2025 14:15:24 +0200
From: Jorge Marques <gastmaier@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	Alexandre Belloni <alexandre.belloni@...tlin.com>, Frank Li <Frank.Li@....com>, Rob Herring <robh@...nel.org>, 
	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 v3 1/2] dt-bindings: i3c: Add adi-i3c-master

On Wed, Jun 18, 2025 at 11:21:22AM +0200, Krzysztof Kozlowski wrote:
> On Wed, Jun 18, 2025 at 09:16:43AM GMT, Jorge Marques wrote:
> > Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> > core that implements the MIPI I3C Basic controller specification.
> 
> Here you put outcome of previous questions - why such compatible was
> chosen, that hardware is this and that.
> 
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@...log.com>
> > ---
> >  .../devicetree/bindings/i3c/adi,i3c-master.yaml    | 63 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  5 ++
> >  2 files changed, 68 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml b/Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..718733bbb450c34c5d4924050cc6f85d8a80fe4b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml
> 
> Filename based on the compatible, so adi,i3c-master-1.00.a.yaml
> 
I agree, but I ended up following the pattern for the other adi,
bindings. I will move for v4. IMO the version suffix has no much use
since IP updates are handled in the driver.
> > @@ -0,0 +1,63 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/i3c/adi,i3c-master.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices I3C Controller
> > +
> > +description: |
> > +  FPGA-based I3C controller designed to interface with I3C and I2C peripherals,
> > +  implementing a subset of the I3C-basic specification.
> > +
> > +  https://analogdevicesinc.github.io/hdl/library/i3c_controller
> > +
> > +maintainers:
> > +  - Jorge Marques <jorge.marques@...log.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: adi,i3c-master-1.00.a
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> 
> Why?
> 
The IP core requires a clock, and the second is optional.
minItems sets the minimum number of required clocks and the maxItems is
inferred from the number of items.

On the IP core itself, one clock is required (axi), and if it is the
only provided, it means that the same clock for the AXI bus is used
also for the rest of the RTL logic.

If a second clock is provided, i3c, it means it drives the RTL logic and is
asynchronous to the axi clock, which then just drives the register map logic.
For i3c specified nominal speeds, the RTL logic should run with a speed of
100MHz. Some FPGAs, such as Altera CycloneV, have a default bus clock speed of
50MHz. Changing the bus speed is possible, but affects timing and it may not be
possible from users to double the bus speed since it will affect timing of all
IP cores using the bus clock.
> > +    items:
> > +      - description: The AXI interconnect clock.
> > +      - description: The I3C controller clock.
I will update the descriptions to:

        - description: The AXI interconnect clock, drives the register map.
        - description: The I3C controller clock. AXI clock drives all logic if not provided.

> > +
> > +  clock-names:
> 
> Not synced with clocks.
> 
I will add `minItems: 1`.
> > +    items:
> > +      - const: axi
> > +      - const: i3c
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +
> > +allOf:
> > +  - $ref: i3c.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    i3c@...00000 {
> > +        compatible = "adi,i3c-master";
> > +        reg = <0x44a00000 0x1000>;
> > +        interrupts = <0 56 4>;
> 
> Use proper defines.
> 
The following can added:

  #include <dt-bindings/interrupt-controller/irq.h>

  interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;

Is there any other to be replaced?

> Best regards,
> Krzysztof
> 

Best regards,
Jorge