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

Jorge Marques posted 2 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH v3 1/2] dt-bindings: i3c: Add adi-i3c-master
Posted by Jorge Marques 3 months, 3 weeks ago
Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
core that implements the MIPI I3C Basic controller specification.

Signed-off-by: Jorge Marques <jorge.marques@analog.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
@@ -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@analog.com>
+
+properties:
+  compatible:
+    const: adi,i3c-master-1.00.a
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+    items:
+      - description: The AXI interconnect clock.
+      - description: The I3C controller clock.
+
+  clock-names:
+    items:
+      - const: axi
+      - const: i3c
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+
+allOf:
+  - $ref: i3c.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i3c@44a00000 {
+        compatible = "adi,i3c-master";
+        reg = <0x44a00000 0x1000>;
+        interrupts = <0 56 4>;
+        clocks = <&clkc 15>, <&clkc 15>;
+        clock-names = "axi", "i3c";
+        #address-cells = <3>;
+        #size-cells = <0>;
+
+        /* I3C and I2C devices */
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 96b82704950184bd71623ff41fc4df31e4c7fe87..6f56e17dcecf902c6812827c1ec3e067c65e9894 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11243,6 +11243,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml
 F:	drivers/i3c/master/ast2600-i3c-master.c
 
+I3C DRIVER FOR ANALOG DEVICES I3C CONTROLLER IP
+M:	Jorge Marques <jorge.marques@analog.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml
+
 I3C DRIVER FOR CADENCE I3C MASTER IP
 M:	Przemysław Gaj <pgaj@cadence.com>
 S:	Maintained

-- 
2.49.0

Re: [PATCH v3 1/2] dt-bindings: i3c: Add adi-i3c-master
Posted by Krzysztof Kozlowski 3 months, 3 weeks ago
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@analog.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

> @@ -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@analog.com>
> +
> +properties:
> +  compatible:
> +    const: adi,i3c-master-1.00.a
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1

Why?

> +    items:
> +      - description: The AXI interconnect clock.
> +      - description: The I3C controller clock.
> +
> +  clock-names:

Not synced with clocks.

> +    items:
> +      - const: axi
> +      - const: i3c
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +
> +allOf:
> +  - $ref: i3c.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i3c@44a00000 {
> +        compatible = "adi,i3c-master";
> +        reg = <0x44a00000 0x1000>;
> +        interrupts = <0 56 4>;

Use proper defines.

Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: i3c: Add adi-i3c-master
Posted by Jorge Marques 3 months, 3 weeks ago
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@analog.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@analog.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@44a00000 {
> > +        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
Re: [PATCH v3 1/2] dt-bindings: i3c: Add adi-i3c-master
Posted by Krzysztof Kozlowski 3 months, 3 weeks ago
On 18/06/2025 14:15, Jorge Marques wrote:
>>>
>>> Signed-off-by: Jorge Marques <jorge.marques@analog.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.

Filename is not related to whether given ABI works with every device.
Filename helps us to organize bindings and existing convention is that
we want it to follow the compatible.

>>> @@ -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@analog.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.

OK

> 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.

Hm? What does it exactly mean - same clock? You mean one clock is routed
to two pins? That's still two clocks. Or you mean that IP core will
notice grounded clock input and do the routing inside?

> 
> 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@44a00000 {
>>> +        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?

Usually 0 has a meaning as well. Where is this used DTS snippet used (on
which platform)?

Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: i3c: Add adi-i3c-master
Posted by Jorge Marques 3 months, 3 weeks ago
On Wed, Jun 18, 2025 at 05:45:22PM +0200, Krzysztof Kozlowski wrote:
> On 18/06/2025 14:15, Jorge Marques wrote:
> >>>
> >>> Signed-off-by: Jorge Marques <jorge.marques@analog.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.
> 
> Filename is not related to whether given ABI works with every device.
> Filename helps us to organize bindings and existing convention is that
> we want it to follow the compatible.
> 
Hi Krzysztof,

Understood.

> >>> @@ -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@analog.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.
> 
> OK
> 
> > 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.
> 
> Hm? What does it exactly mean - same clock? You mean one clock is routed
> to two pins? That's still two clocks. Or you mean that IP core will
> notice grounded clock input and do the routing inside?
> 

The routing is inside the IP core, and only one clock pin is used. In
fullness, since it is a FPGA-based IP Core, the number of input clock
pins are defined by the parameter [1] ASYNC_CLK that enables the
asynchronous i3c clock input pin. The devicetree then describes how
things are wired, if two clocks provided, it describes that both clock
inputs, axi and i3c, are wired to the IP Core, if only axi, then there
is no clock signal to the i3c input clock pin and axi clock drives the
whole IP.

[1] https://analogdevicesinc.github.io/hdl/library/i3c_controller/i3c_controller_host_interface.html#configuration-parameters

> > 
> > 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@44a00000 {
> >>> +        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?
> 
> Usually 0 has a meaning as well. Where is this used DTS snippet used (on
> which platform)?
> 

The example provided is used on AMD xilinx arm,cortex-a9-gic.
The IP core is tested on AMD Xilinx, Altera FPGAs (arm, microblaze, arm64).
Test/support to RiscV-based Lattice FPGAs should eventually also come.

In this example 0 means Shared Peripheral Interrupt, but has no header
file defining. I guess I could make it generic and set to

  interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;

and let the user figure out for his target platform.
> Best regards,
> Krzysztof

Best regards,
Jorge
Re: [PATCH v3 1/2] dt-bindings: i3c: Add adi-i3c-master
Posted by Krzysztof Kozlowski 3 months, 3 weeks ago
On 18/06/2025 21:55, Jorge Marques wrote:
>> Usually 0 has a meaning as well. Where is this used DTS snippet used (on
>> which platform)?
>>
> 
> The example provided is used on AMD xilinx arm,cortex-a9-gic.
> The IP core is tested on AMD Xilinx, Altera FPGAs (arm, microblaze, arm64).
> Test/support to RiscV-based Lattice FPGAs should eventually also come.
> 
> In this example 0 means Shared Peripheral Interrupt, but has no header

Of course it has... just like 100% of bindings for devices in a SoC.

> file defining. I guess I could make it generic and set to
> 
>   interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
> 

That's fine as well.



Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: i3c: Add adi-i3c-master
Posted by Rob Herring (Arm) 3 months, 3 weeks ago
On Wed, 18 Jun 2025 09:16:43 +0200, 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.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
>  .../devicetree/bindings/i3c/adi,i3c-master.yaml    | 63 ++++++++++++++++++++++
>  MAINTAINERS                                        |  5 ++
>  2 files changed, 68 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/i3c/adi,i3c-master.example.dtb: /example-0/i3c@44a00000: failed to match any schema with compatible: ['adi,i3c-master']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250618-adi-i3c-master-v3-1-e66170a6cb95@analog.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.