[RFC PATCH 0/2] MCTP I2C devicetree binding

[RFC PATCH 0/2] MCTP I2C devicetree binding

[Matt Johnston]

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

-- 
2.30.2

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

[Matt Johnston]

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

Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
 .../devicetree/bindings/net/mctp-i2c.yaml     | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mctp-i2c.yaml

diff --git a/Documentation/devicetree/bindings/net/mctp-i2c.yaml b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
new file mode 100644
index 000000000000..f9378cd845d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
@@ -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 <matt@codeconstruct.com.au>
+
+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)>;
+        };
+    };
-- 
2.30.2

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

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

Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
 .../devicetree/bindings/net/mctp-i2c.yaml     | 42 +++++++++++++++++--
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mctp-i2c.yaml b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
index f9378cd845d4..45429cbcc6a1 100644
--- a/Documentation/devicetree/bindings/net/mctp-i2c.yaml
+++ b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
@@ -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>;
+      };
+    };
-- 
2.30.2

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

[Rob Herring]

On Mon, 02 Aug 2021 12:04:58 +0800, Matt Johnston wrote:
> 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.
> 
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> ---
>  .../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.

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

[Rob Herring]

On Sun, Aug 1, 2021 at 10:12 PM Matt Johnston <matt@codeconstruct.com.au> wrote:
>
> Used to define an endpoint to communicate with MCTP peripherals attached
> to an I2C bus.
>
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> ---
>  .../devicetree/bindings/net/mctp-i2c.yaml     | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/mctp-i2c.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/mctp-i2c.yaml b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
> new file mode 100644
> index 000000000000..f9378cd845d4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
> @@ -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 <matt@codeconstruct.com.au>
> +
> +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.

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

> +
> +
> +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?

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

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

[Rob Herring]

On Mon, Aug 02, 2021 at 12:04:58PM +0800, Matt Johnston wrote:
> 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.

> 
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> ---
>  .../devicetree/bindings/net/mctp-i2c.yaml     | 42 +++++++++++++++++--
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/mctp-i2c.yaml b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
> index f9378cd845d4..45429cbcc6a1 100644
> --- a/Documentation/devicetree/bindings/net/mctp-i2c.yaml
> +++ b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
> @@ -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 {

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.

> +        reg = <0>;

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

> +      };
> +    };
> -- 
> 2.30.2
> 
> 

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

[Matt Johnston]

On Mon, 2021-08-02 at 10:45 -0600, Rob Herring wrote:
> On Sun, Aug 1, 2021 at 10:12 PM Matt Johnston <matt@codeconstruct.com.au>
> wrote:
> > 
> > +  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 

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

[Rob Herring]

On Tue, Aug 10, 2021 at 10:39 PM Matt Johnston
<matt@codeconstruct.com.au> wrote:
>
> On Mon, 2021-08-02 at 10:45 -0600, Rob Herring wrote:
> > On Sun, Aug 1, 2021 at 10:12 PM Matt Johnston <matt@codeconstruct.com.au>
> > wrote:
> > >
> > > +  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