Skip to contentSkip to conversation

MCTP I2C devicetree binding

RFC PATCH Superseded Matt Johnston 2 participants 8 messages Aug 2, 2021 – Aug 13, 2021

Subsystems (2)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

cover MCTP I2C devicetree binding

Hi,

These patches are an RFC for a devicetree binding for MCTP-over-I2C hardware, where MCTP messages are transferred as SMBus block writes. For this, we need to encode a little dt data about the hardware configuration, specifically:

1) which controllers are MCTP endpoints (ie, may host MCTP I2C clients downstream); and
2) the SMBus address of the local MCTP endpoint, which can act as an I2C slave device

In order to represent this configuration, we've added a binding for a node that will exist under a root-level i2c controller. This indicates (1), and provides the address required for (2). For a fictional hardware i2c controller:

    /* for I2C_OWN_SLAVE_ADDRESS */
    #include <dt-bindings/i2c/i2c.h>

    /* root-level i2c controller */
    i2c {
        compatible = "vendor,example-i2c-controller";
        reg = <...>
        #address-cells = <1>;
        #size-cells = <0>;

        mctp@50 {
            compatible = "mctp-i2c";
            reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
        };
    };

This gets a little trickier when i2c muxes are present between the top-level controller and MCTP endpoints, as we need to specify the downstream mux ports that may host MCTP endpoints (which allows an OS implementation to correctly address devices which require a specific mux state to be configured). For this, we introduce the bus-attach property on the root-level node, which indicates subordinate i2c busses that may host MCTP endpoints:

     i2c5: i2cbus5 {
         #address-cells = <1>;
         #size-cells = <0>;
 
         mctp@50 {
             compatible = "mctp-i2c";
             reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
             /* MCTP endpoints behind this controller and the mux port at i2c14 */
             attach-bus = <&i2c5 &i2c14>;
         };
     };
 
     i2cmux0 {
       compatible = "i2c-mux-gpio";
       #address-cells = <1>;
       #size-cells = <0>;
       i2c-parent = <&i2c5>;
       i2c14: i2c@5 {
         reg = <0>;
       };
     };

- where an absent attach-bus property implies that only the top-level controller will host MCTP devices.

An operating system consuming these nodes may want to create a MCTP interface device for each of the mux ports in the topology.

An alternative approach we considered: A pure property-based spec could represent the MCTP capability of an i2c controller through top-level properties instead:

    i2c {
        compatible = "mctp-i2c-controller", "vendor,example-i2c-controller";
        reg = <...>
        #address-cells = <1>;
        #size-cells = <0>;

        /* presence required by the 'mctp-i2c-controller' binding */
        mctp-i2c-address = <0x50>;
    };

(like the above, the i2c controller here will both be available for standard i2c transactions as well as MCTP messaging)

and downstream MCTP-capable busses indicated with a boolean property:

    i2cmux0 {
      compatible = "i2c-mux-gpio";
      #address-cells = <1>;
      #size-cells = <0>;
      i2c-parent = <&i2c5>;

      i2c@5 {
          /* indicates that this subordinate bus hosts MCTP endpoints */
          mctp-i2c-controller;
      };
    };

However, this doesn't quite fit with the existing i2c client bindings (using I2C_OWN_SLAVE_ADDRESS), which uses a distinct node for the local client endpoint. So, we have decided on the node-base approach.

Cheers,
Matt

Matt Johnston (2):
dt-bindings: net: New binding for mctp-i2c
dt-bindings: net: Add mctp-i2c bus-attach property

 .../devicetree/bindings/net/mctp-i2c.yaml     | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mctp-i2c.yaml
Signature · 1 line
2.30.2
#1 lore raw

RFC PATCH 1/2 dt-bindings: net: New binding for mctp-i2c

Used to define an endpoint to communicate with MCTP peripherals attached to an I2C bus.

---
 .../devicetree/bindings/net/mctp-i2c.yaml     | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mctp-i2c.yaml
Documentation/devicetree/bindings/net/mctp-i2c.yaml new file +44 -0
new file mode 100644
index 000000000000..f9378cd845d4
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/mctp-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MCTP I2C transport binding
+
+maintainers:
+  - Matt Johnston <[email protected]>
+
+description:
+  The MCTP I2C binding defines an MCTP endpoint on the I2C bus to
+  communicate with I2C peripherals using MCTP (DMTF specification DSP0237).
+
+  An mctp-i2c device must be attached to a hardware bus adapter which supports
+  slave functionality. The reg address must include I2C_OWN_SLAVE_ADDRESS.
+
+
+properties:
+  compatible:
+    const: mctp-i2c
+
+  reg:
+    maxItems: 1
+
+additionalProperties: true
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/i2c/i2c.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mctp@50 {
+            compatible = "mctp-i2c";
+            reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
+        };
+    };
Signature · 1 line
2.30.2
maintainer
#2 lore raw

Re: [RFC PATCH 1/2] dt-bindings: net: New binding for mctp-i2c

Quoted 20 lines · Matt Johnston

Used to define an endpoint to communicate with MCTP peripherals attached to an I2C bus.

---
 .../devicetree/bindings/net/mctp-i2c.yaml     | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mctp-i2c.yaml
Documentation/devicetree/bindings/net/mctp-i2c.yaml new file +4 -0
new file mode 100644
index 000000000000..f9378cd845d4
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/mctp-i2c.yaml#

Documentation/devicetree/bindings/net/mctp-i2c.yaml line 14

Quoted 10 lines
Documentation/devicetree/bindings/net/mctp-i2c.yaml new file +10 -0
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MCTP I2C transport binding
+
+maintainers:
+  - Matt Johnston <[email protected]>
+
+description:
+  The MCTP I2C binding defines an MCTP endpoint on the I2C bus to
+  communicate with I2C peripherals using MCTP (DMTF specification DSP0237).

The general problem with bindings for a protocol/interface is that those are not complete devices. What if there's something specific about the implementation that has to be handled? For example, if the device has to be powered on, brought out of reset or has some buggy behavior to work around? Or perhaps there's additional functionality. The exception is if the specification covers all of those things.

Having a specific binding doesn't preclude having a generic driver either. With a specific compatible, either a generic or device specific driver could be bound and the kernel could switch between those whenever it wants.

Documentation/devicetree/bindings/net/mctp-i2c.yaml line 17

Quoted 3 lines
Documentation/devicetree/bindings/net/mctp-i2c.yaml new file +3 -0
+
+  An mctp-i2c device must be attached to a hardware bus adapter which supports
+  slave functionality. The reg address must include I2C_OWN_SLAVE_ADDRESS.

This constraint can be a schema.

Documentation/devicetree/bindings/net/mctp-i2c.yaml line 27

Quoted 10 lines
Documentation/devicetree/bindings/net/mctp-i2c.yaml new file +10 -0
+
+
+properties:
+  compatible:
+    const: mctp-i2c
+
+  reg:
+    maxItems: 1
+
+additionalProperties: true

This is only allowed to be 'true' for common, incomplete bindings. What other properties do you expect?

Quoted 20 lines
Documentation/devicetree/bindings/net/mctp-i2c.yaml new file +17 -1
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/i2c/i2c.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mctp@50 {
+            compatible = "mctp-i2c";
+            reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
+        };
+    };
--

2.30.2

#3 lore raw

Re: [RFC PATCH 1/2] dt-bindings: net: New binding for mctp-i2c

Quoted 7 lines · Rob Herring
Quoted 3 lines · Matt Johnston

+  slave functionality. The reg address must include I2C_OWN_SLAVE_ADDRESS.

This constraint can be a schema.

The flag is already described in i2c.txt, is it OK to just make reference to that?

Cheers,
Matt

maintainer
#4 lore raw

Re: [RFC PATCH 1/2] dt-bindings: net: New binding for mctp-i2c

Quoted 5 lines · Matt Johnston
Quoted 3 lines · Rob Herring
Quoted 1 line · Matt Johnston
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

+ slave functionality. The reg address must include I2C_OWN_SLAVE_ADDRESS.

This constraint can be a schema.

The flag is already described in i2c.txt, is it OK to just make reference to that?

I know it is, but you are saying the only addresses valid must have that. Sounds like a constraint to me, so it should be a schema. Why make a user have to debug that they forgot to set that bit?

Rob

#5 lore raw

RFC PATCH 2/2 dt-bindings: net: Add mctp-i2c bus-attach property

In reply to Matt Johnston

Allows attaching multiple child busses in a mux topology to an mctp-i2c instance on the root bus. In general I2C slave mode does not make sense for mux busses, but the MCTP request/response protocol means the the root can switch between child muxes for incoming I2C messages.

---
 .../devicetree/bindings/net/mctp-i2c.yaml     | 42 +++++++++++++++++--
 1 file changed, 38 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/net/mctp-i2c.yaml +38 -4
index f9378cd845d4..45429cbcc6a1 100644
@@ -12,11 +12,10 @@ maintainers:
 description:
   The MCTP I2C binding defines an MCTP endpoint on the I2C bus to
   communicate with I2C peripherals using MCTP (DMTF specification DSP0237).
-
-  An mctp-i2c device must be attached to a hardware bus adapter which supports
+  A single binding node can attach to multiple child busses in a mux topology.
+  An mctp-i2c node's parent must be a hardware bus adapter which supports
   slave functionality. The reg address must include I2C_OWN_SLAVE_ADDRESS.
 
-
 properties:
   compatible:
     const: mctp-i2c
@@ -24,6 +23,17 @@ properties:
   reg:
     maxItems: 1
 
+  bus-attach:
+    $ref: "/schemas/types.yaml#/definitions/phandle-array"
+    description: |
+      List of phandles of I2C busses to attach to. I2C mux busses may only
+      be attached to an mctp-i2c binding on their parent root adapter in the
+      mux topology.
+      If no bus-attach property is specified then only the direct parent
+      I2C bus is attached. Otherwise to include a direct parent bus it
+      must be included in the bus-attach list if needed.
+
+
 additionalProperties: true
 
 required:
@@ -33,12 +43,36 @@ required:
 examples:
   - |
     #include <dt-bindings/i2c/i2c.h>
-    i2c {
+
+    // simple attaching to a root adapter i2c0
+    i2c0: i2cbus0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mctp@50 {
+            compatible = "mctp-i2c";
+            reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
+        };
+    };
+
+    // attaching to a root adapter i2c5 and a child mux bus i2c14
+    i2c5: i2cbus5 {
         #address-cells = <1>;
         #size-cells = <0>;
 
         mctp@50 {
             compatible = "mctp-i2c";
             reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
+            attach-bus = <&i2c5 &i2c14>;
         };
     };
+
+    i2cmux0 {
+      compatible = "i2c-mux-gpio";
+      #address-cells = <1>;
+      #size-cells = <0>;
+      i2c-parent = <&i2c0>;
+      i2c14: i2c@5 {
+        reg = <0>;
+      };
+    };
Signature · 1 line
2.30.2
maintainer
#6 lore raw

Re: [RFC PATCH 2/2] dt-bindings: net: Add mctp-i2c bus-attach property

Quoted 11 lines · Matt Johnston

Allows attaching multiple child busses in a mux topology to an mctp-i2c instance on the root bus. In general I2C slave mode does not make sense for mux busses, but the MCTP request/response protocol means the the root can switch between child muxes for incoming I2C messages.

---
 .../devicetree/bindings/net/mctp-i2c.yaml     | 42 +++++++++++++++++--
 1 file changed, 38 insertions(+), 4 deletions(-)

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/net/mctp-i2c.example.dt.yaml:0:0: /example-0/i2cmux0: failed to match any schema with compatible: ['i2c-mux-gpio'] \ndoc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1512253

This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.

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.

maintainer
#7 lore raw

Re: [RFC PATCH 2/2] dt-bindings: net: Add mctp-i2c bus-attach property

In reply to Matt Johnston
Quoted 5 lines · Matt Johnston

Allows attaching multiple child busses in a mux topology to an mctp-i2c instance on the root bus. In general I2C slave mode does not make sense for mux busses, but the MCTP request/response protocol means the the root can switch between child muxes for incoming I2C messages.

Perhaps a diagram of what I2C buses look like would help because I don't understand this.

Quoted 67 lines
---
 .../devicetree/bindings/net/mctp-i2c.yaml     | 42 +++++++++++++++++--
 1 file changed, 38 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/net/mctp-i2c.yaml +27 -4
index f9378cd845d4..45429cbcc6a1 100644
@@ -12,11 +12,10 @@ maintainers:
 description:
   The MCTP I2C binding defines an MCTP endpoint on the I2C bus to
   communicate with I2C peripherals using MCTP (DMTF specification DSP0237).
-
-  An mctp-i2c device must be attached to a hardware bus adapter which supports
+  A single binding node can attach to multiple child busses in a mux topology.
+  An mctp-i2c node's parent must be a hardware bus adapter which supports
   slave functionality. The reg address must include I2C_OWN_SLAVE_ADDRESS.
 
-
 properties:
   compatible:
     const: mctp-i2c
@@ -24,6 +23,17 @@ properties:
   reg:
     maxItems: 1
 
+  bus-attach:
+    $ref: "/schemas/types.yaml#/definitions/phandle-array"
+    description: |
+      List of phandles of I2C busses to attach to. I2C mux busses may only
+      be attached to an mctp-i2c binding on their parent root adapter in the
+      mux topology.
+      If no bus-attach property is specified then only the direct parent
+      I2C bus is attached. Otherwise to include a direct parent bus it
+      must be included in the bus-attach list if needed.
+
+
 additionalProperties: true
 
 required:
@@ -33,12 +43,36 @@ required:
 examples:
   - |
     #include <dt-bindings/i2c/i2c.h>
-    i2c {
+
+    // simple attaching to a root adapter i2c0
+    i2c0: i2cbus0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mctp@50 {
+            compatible = "mctp-i2c";
+            reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
+        };
+    };
+
+    // attaching to a root adapter i2c5 and a child mux bus i2c14
+    i2c5: i2cbus5 {
         #address-cells = <1>;
         #size-cells = <0>;
 
         mctp@50 {
             compatible = "mctp-i2c";
             reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;

Documentation/devicetree/bindings/net/mctp-i2c.yaml required: · line 75

Quoted 10 lines
Documentation/devicetree/bindings/net/mctp-i2c.yaml +8 -0
+            attach-bus = <&i2c5 &i2c14>;
         };
     };
+
+    i2cmux0 {
+      compatible = "i2c-mux-gpio";
+      #address-cells = <1>;
+      #size-cells = <0>;
+      i2c-parent = <&i2c0>;
+      i2c14: i2c@5 {

This is not how "i2c-mux-gpio" works. First you are missing mux-gpios. Second, how is this a mux with only 1 mux selection as the child nodes are the I2C buses for each mux selection.

Documentation/devicetree/bindings/net/mctp-i2c.yaml required: · line 76

Quoted 1 line
Documentation/devicetree/bindings/net/mctp-i2c.yaml +1 -0
+        reg = <0>;

Either reg should be 5 here or the unit-address should be 0.

Quoted 6 lines
Documentation/devicetree/bindings/net/mctp-i2c.yaml +2 -0
+      };
+    };
Signature · 1 line
2.30.2