[RFC PATCH v2 0/2] MCTP I2C devicetree binding

[RFC PATCH v2 0/2] MCTP I2C devicetree binding

[Matt Johnston]

Hi Rob,

These patches are a v2 RFC for a devicetree binding of MCTP-over-I2C
hardware, where MCTP messages are transferred as SMBus block writes.

Since v1 I've revised the description and commits to hopefully be
clearer, and renamed the binding to indicate that it's generic for any
I2C hardware. That should allow for any later device specific drivers -
please let me know if I'm misunderstanding how it should be changed.

Cheers,
Matt

v2:

- Rename to mctp-i2c-generic rather than mctp-i2c
- Fix mux example to be correct and pass checks
- Set additionalProperties: false
- Commit message of patch 2/2 revised for clarity
- The I2C_OWN_SLAVE_ADDRESS flag is mentioned in reference to
  i2c.txt where it is defined.

Matt Johnston (2):
  dt-bindings: net: New binding for mctp-i2c-generic
  dt-bindings: net: mctp-i2c-generic: support muxes

 .../bindings/net/mctp-i2c-generic.yaml        | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mctp-i2c-generic.yaml

-- 
2.30.2

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

[Matt Johnston]

An I2C bus with attached MCTP peripherals can be configured to be
accessible to the host system, using a specified I2C address.

This mctp-i2c-generic binding can attach to existing I2C bus
that supports slave functionality.

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)>;
        };
    };

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

diff --git a/Documentation/devicetree/bindings/net/mctp-i2c-generic.yaml b/Documentation/devicetree/bindings/net/mctp-i2c-generic.yaml
new file mode 100644
index 000000000000..6092f7e8dc07
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mctp-i2c-generic.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/mctp-i2c-generic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic MCTP I2C transport binding
+
+maintainers:
+  - Matt Johnston <matt@codeconstruct.com.au>
+
+description:
+  The generic MCTP I2C binding defines an MCTP endpoint on an existing I2C
+  adapter. MCTP I2C is specified by DMTF DSP0237.
+
+  An mctp-i2c-generic device must be attached to a hardware bus adapter
+  which supports slave functionality.
+
+properties:
+  compatible:
+    const: mctp-i2c-generic
+
+  reg:
+    maxItems: 1
+    description:
+      7 bit I2C address of the endpoint.
+      I2C_OWN_SLAVE_ADDRESS flag must be set (described in i2c.txt)
+
+additionalProperties: false
+
+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 v2 1/2] dt-bindings: net: New binding for mctp-i2c-generic

[Rob Herring]

On Wed, 11 Aug 2021 11:43:44 +0800, Matt Johnston wrote:
> An I2C bus with attached MCTP peripherals can be configured to be
> accessible to the host system, using a specified I2C address.
> 
> This mctp-i2c-generic binding can attach to existing I2C bus
> that supports slave functionality.
> 
> 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)>;
>         };
>     };
> 
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> ---
>  .../bindings/net/mctp-i2c-generic.yaml        | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/mctp-i2c-generic.yaml
> 

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-generic.example.dt.yaml:0:0: /example-0/i2c/mctp@50: failed to match any schema with compatible: ['mctp-i2c']

doc reference errors (make refcheckdocs):

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

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.

[RFC PATCH v2 2/2] dt-bindings: net: mctp-i2c-generic: support muxes

[Matt Johnston]

Allow a single mctp-i2c-generic controller node to specify multiple
subordinate I2C mux busses that are attached.
The 'bus-attach' property defines which I2C busses should provide an
MCTP endpoint.

This allows a hardware I2C controller at the top of a mux topology to
handle incoming messages (as an I2C slave) for subordinate I2C mux busses.
Otherwise I2C mux busses are not be able to act as an I2C slave which is
required by the MCTP I2C transport.

Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
 .../bindings/net/mctp-i2c-generic.yaml        | 53 +++++++++++++++++--
 1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mctp-i2c-generic.yaml b/Documentation/devicetree/bindings/net/mctp-i2c-generic.yaml
index 6092f7e8dc07..e4d742452078 100644
--- a/Documentation/devicetree/bindings/net/mctp-i2c-generic.yaml
+++ b/Documentation/devicetree/bindings/net/mctp-i2c-generic.yaml
@@ -10,11 +10,14 @@ maintainers:
   - Matt Johnston <matt@codeconstruct.com.au>
 
 description:
-  The generic MCTP I2C binding defines an MCTP endpoint on an existing I2C
-  adapter. MCTP I2C is specified by DMTF DSP0237.
+  An mctp-i2c-generic controller defines an MCTP endpoint on an existing I2C
+  controller. MCTP I2C is specified by DMTF DSP0237.
 
-  An mctp-i2c-generic device must be attached to a hardware bus adapter
-  which supports slave functionality.
+  An mctp-i2c-generic controller must be attached to an I2C controller
+  which supports slave functionality. Subordinate busses in a mux topology
+  can be attached to the same mctp-i2c-generic controller with the bus-attach
+  property. These will be presented to the host system as separate MCTP I2C
+  instances.
 
 properties:
   compatible:
@@ -26,6 +29,16 @@ properties:
       7 bit I2C address of the endpoint.
       I2C_OWN_SLAVE_ADDRESS flag must be set (described in i2c.txt)
 
+  bus-attach:
+    $ref: "/schemas/types.yaml#/definitions/phandle-array"
+    description: |
+      List of phandles of I2C busses to attach to. All busses must
+      be in the same mux topology as the node's parent I2C controller.
+
+      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: false
 
 required:
@@ -35,12 +48,42 @@ 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";
+      #address-cells = <1>;
+      #size-cells = <0>;
+      i2c-parent = <&i2c5>;
+      mux-controls = <&mux>;
+
+      i2c14: i2c@0 {
+        reg = <0>;
+      };
+
+      i2c15: i2c@1 {
+        reg = <1>;
+      };
+    };
-- 
2.30.2

Re: [RFC PATCH v2 0/2] MCTP I2C devicetree binding

[Rob Herring]

On Wed, Aug 11, 2021 at 11:43:43AM +0800, Matt Johnston wrote:
> Hi Rob,
> 
> These patches are a v2 RFC for a devicetree binding of MCTP-over-I2C
> hardware, where MCTP messages are transferred as SMBus block writes.
> 
> Since v1 I've revised the description and commits to hopefully be
> clearer, and renamed the binding to indicate that it's generic for any
> I2C hardware. That should allow for any later device specific drivers -
> please let me know if I'm misunderstanding how it should be changed.

Adding 'generic' is not an improvement nor does it change anything. 
Again, a protocol is not a device. We went thru the same thing with 
HID-over-I2C.

There's still not any diagram to better understand what all this is.

Rob

Re: [RFC PATCH v2 0/2] MCTP I2C devicetree binding

[Matt Johnston]

On Fri, 2021-08-13 at 11:33 -0500, Rob Herring wrote:
> 
> Adding 'generic' is not an improvement nor does it change anything. 

I may have misunderstood the feedback then:

> Again, a protocol is not a device. We went thru the same thing with 
> HID-over-I2C.

Thanks for the pointer to HID-over-I2C, that helps to clarify things.
I'm a still a little unclear on what you mean by "protocol" - is that a 
DT-specific thing? If so, I can't see many references to what's required
for a protocol definition - do you have any pointers I can read up on?

I don't expect for there to be much extra in the way of I2C controller
quirks that we'll need, but I agree that we may need to accommodate
those in future. It looks like the HID example gives us a bit
of a precedent there - is that just through allowing further, more
specific compatible values later? (plus their binding-specified properties)

Essentially at the moment we just need to flag which busses will need
to carry MCTP data, and this should work with any I2C controller. To do
that, this new binding defines which I2C busses in the system will be
accessible by MCTP and which local I2C client address will be used. If
there's a neater way to represent those in the DT we're happy to rework
the binding.

(MCTP I2C uses SMBus Block Write for messages in either direction. This
requires us to include the mux topology in the DT data so the system can
receive response messages. However all we need from the DT binding is to
flag the nodes in the tree that will host endpoints - a driver
implementation can work out the rest)

> There's still not any diagram to better understand what all this is.

I'll add one to 2/2, how's something like this:
                                       .-------.
                                       |eeprom |
.----------.   .------.               /'-------'
| adapter  |   | mux  --@0,i2c5------'
| i2c1     |-.-|      --@1,i2c6--.--.
|..........|  \'------'           \  \  .........
| mctp-i2c |   \                   \  \ .mctpB  .
| slave    |    \                   \  '.0x30   .
| 0x50     |     \  .........        \  '.......'
'----------'      \ .mctpA  .         \ .........
                   '.0x1d   .          '.mctpC  .
                    '.......'          '.0x31   .
                                        '.......'

This shows 3 MCTP peripherals in the system, one toplevel and two
behind a mux. This requires us to define two MCTP controllers: one
on i2c1 - the root controller, and one in i2c6 - an individual
downstream port of the mux.

i2c1: i2cbus1 {
  compatible = "vendor,example-i2c-controller";
  #address-cells = <1>;
  #size-cells = <0>;
  mctp@50 {
    compatible = "mctp-i2c";
    reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
    attach-bus = <&i2c1 &i2c6>;
  };
};

i2cmux0 {
  compatible = "vendor,example-i2c-mux";
  #address-cells = <1>;
  #size-cells = <0>;
  i2c-parent = <&i2c1>;

  i2c5: i2c@0 {
    reg = <0>;
    eeprom@33 {
      compatible = "atmel,24c64";
      reg = <0x33>;
    };
  };

  i2c6: i2c@1 {
    reg = <1>;
  };
};


Regarding I2C_OWN_SLAVE_ADDRESS validation, I can add a i2c-own-
address.yaml schema (name pending) though can't see a way to perform a bit-
set test in json schema validation?

Thanks,
Matt

Re: [RFC PATCH v2 0/2] MCTP I2C devicetree binding

[Rob Herring]

On Mon, Aug 16, 2021 at 03:32:40PM +0800, Matt Johnston wrote:
> On Fri, 2021-08-13 at 11:33 -0500, Rob Herring wrote:
> > 
> > Adding 'generic' is not an improvement nor does it change anything. 
> 
> I may have misunderstood the feedback then:
> 
> > Again, a protocol is not a device. We went thru the same thing with 
> > HID-over-I2C.
> 
> Thanks for the pointer to HID-over-I2C, that helps to clarify things.
> I'm a still a little unclear on what you mean by "protocol" - is that a 
> DT-specific thing? If so, I can't see many references to what's required
> for a protocol definition - do you have any pointers I can read up on?

Protocol is the format of the I2C data to access the device. Or maybe 
it's the register interface definition. Another example is USB OHCI, 
EHCI, XHCI. Those all define a register standard interface, but leave 
out a whole bunch of properties of the h/w blocks. 

I assume MCTP is some sort of spec. Spec's are typically not complete 
in the sense of defining a whole device including power rails, 
reset/enable lines, interrupts, etc. IOW, MCTP is just a subset of all 
that.

> I don't expect for there to be much extra in the way of I2C controller
> quirks that we'll need, but I agree that we may need to accommodate
> those in future. It looks like the HID example gives us a bit
> of a precedent there - is that just through allowing further, more
> specific compatible values later? (plus their binding-specified properties)

There were a lot of discussions on HID and resistance to needing 
specific compatibles. Then later it turns out not all HID-over-I2C 
implementations are exactly the same and we need to know specific 
devices sometimes. That is what I don't want to repeat.

The problem with adding compatibles later is you have to change the DT 
to fix problems vs. just an OS update. Maybe that's fine, maybe not. 
Depends on the system.

> Essentially at the moment we just need to flag which busses will need
> to carry MCTP data, and this should work with any I2C controller. To do
> that, this new binding defines which I2C busses in the system will be
> accessible by MCTP and which local I2C client address will be used. If
> there's a neater way to represent those in the DT we're happy to rework
> the binding.
> 
> (MCTP I2C uses SMBus Block Write for messages in either direction. This
> requires us to include the mux topology in the DT data so the system can
> receive response messages. However all we need from the DT binding is to
> flag the nodes in the tree that will host endpoints - a driver
> implementation can work out the rest)
> 
> > There's still not any diagram to better understand what all this is.
> 
> I'll add one to 2/2, how's something like this:
>                                        .-------.
>                                        |eeprom |
> .----------.   .------.               /'-------'
> | adapter  |   | mux  --@0,i2c5------'
> | i2c1     |-.-|      --@1,i2c6--.--.
> |..........|  \'------'           \  \  .........
> | mctp-i2c |   \                   \  \ .mctpB  .
> | slave    |    \                   \  '.0x30   .
> | 0x50     |     \  .........        \  '.......'
> '----------'      \ .mctpA  .         \ .........
>                    '.0x1d   .          '.mctpC  .
>                     '.......'          '.0x31   .
>                                         '.......'
> 
> This shows 3 MCTP peripherals in the system, one toplevel and two
> behind a mux. This requires us to define two MCTP controllers: one
> on i2c1 - the root controller, and one in i2c6 - an individual
> downstream port of the mux.

Okay, looks pretty normal in terms of i2c bus muxing. That's all well 
defined already.

> i2c1: i2cbus1 {
>   compatible = "vendor,example-i2c-controller";
>   #address-cells = <1>;
>   #size-cells = <0>;
>   mctp@50 {
>     compatible = "mctp-i2c";

I guess 'mctp-i2c' alone here is fine given it's the I2C controller 
slave implementation which is just a protocol. It's the external devices 
where my concern is.

Though, don't you need a different compatible for it and external 
devices?

>     reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
>     attach-bus = <&i2c1 &i2c6>;

Why do you need to say you are attached to yourself?

And you can walk the bus to find other MCTP devices. 

>   };
> };
> 
> i2cmux0 {
>   compatible = "vendor,example-i2c-mux";
>   #address-cells = <1>;
>   #size-cells = <0>;
>   i2c-parent = <&i2c1>;
> 
>   i2c5: i2c@0 {
>     reg = <0>;
>     eeprom@33 {
>       compatible = "atmel,24c64";
>       reg = <0x33>;
>     };
>   };
> 
>   i2c6: i2c@1 {
>     reg = <1>;
>   };
> };
> 
> 
> Regarding I2C_OWN_SLAVE_ADDRESS validation, I can add a i2c-own-
> address.yaml schema (name pending) though can't see a way to perform a bit-
> set test in json schema validation?

You'll have to do a minimum/maximum range with the high bit set and 
addresses 0-7f.

Rob

Re: [RFC PATCH v2 0/2] MCTP I2C devicetree binding

[Matt Johnston]

On Tue, 2021-08-17 at 16:06 -0500, Rob Herring wrote:
On Mon, Aug 16, 2021 at 03:32:40PM +0800, Matt Johnston wrote:
> >                                        .-------.
> >                                        |eeprom |
> > .----------.   .------.               /'-------'
> > > adapter  |   | mux  --@0,i2c5------'
> > > i2c1     |-.-|      --@1,i2c6--.--.
> > > ..........|  \'------'           \  \  .........
> > > mctp-i2c |   \                   \  \ .mctpB  .
> > > slave    |    \                   \  '.0x30   .
> > > 0x50     |     \  .........        \  '.......'
> > '----------'      \ .mctpA  .         \ .........
> >                    '.0x1d   .          '.mctpC  .
> >                     '.......'          '.0x31   .
> >                                         '.......'
> > 
> 
> I guess 'mctp-i2c' alone here is fine given it's the I2C controller 
> slave implementation which is just a protocol. It's the external devices 
> where my concern is.
> 
> Though, don't you need a different compatible for it and external 
> devices?

We are only defining a binding for the system's own MCTP "controller"
here, not the external devices on the other side of the I2C link. Those are
probed outside of DT, for example hotplug NVMe disks can expose MCTP-over-
I2C.

This ends up describing something like a network interface, which
happens to use I2C as a transport in this case. (There are other
transports like MCTP-over-serial, but those don't require DT topology
data). For other network-type DT bindings (say, ethernet@), we don't
describe remote network endpoints either, so we're proposing the same
pattern for MCTP.

> >     reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
> >     attach-bus = <&i2c1 &i2c6>;
> 
> Why do you need to say you are attached to yourself?

This indicates that the top-level MCTP controller needs to talk to MCTP
endpoints, eg mctpA on the directly attached bus i2c1. In some topologies
there will be no directly-attached endpoints, in which case we would omit
i2c1 from the list. We need to specify the attach-bus property since we
don't have a list of external device endpoints to walk.

[In the case of not directly attached, we still need to plant the mctp-i2c@
node at the root controller level, as the claimed client address (in reg)
is global across the entire bus. The attach-bus list gives us the set of
interfaces that are necessary for the OS to control]


> > Regarding I2C_OWN_SLAVE_ADDRESS validation 
> You'll have to do a minimum/maximum range with the high bit set and 
> addresses 0-7f.

OK, that will work fine for our binding. The general I2C slave client case
would also have to allow for I2C_TEN_BIT_ADDRESS (1<<31), but MCTP-over-I2C
only accepts 7 bit.

Thanks,
Matt

Re: [RFC PATCH v2 0/2] MCTP I2C devicetree binding

[Rob Herring]

On Tue, Aug 17, 2021 at 11:19 PM Matt Johnston
<matt@codeconstruct.com.au> wrote:
>
> On Tue, 2021-08-17 at 16:06 -0500, Rob Herring wrote:
> On Mon, Aug 16, 2021 at 03:32:40PM +0800, Matt Johnston wrote:
> > >                                        .-------.
> > >                                        |eeprom |
> > > .----------.   .------.               /'-------'
> > > > adapter  |   | mux  --@0,i2c5------'
> > > > i2c1     |-.-|      --@1,i2c6--.--.
> > > > ..........|  \'------'           \  \  .........
> > > > mctp-i2c |   \                   \  \ .mctpB  .
> > > > slave    |    \                   \  '.0x30   .
> > > > 0x50     |     \  .........        \  '.......'
> > > '----------'      \ .mctpA  .         \ .........
> > >                    '.0x1d   .          '.mctpC  .
> > >                     '.......'          '.0x31   .
> > >                                         '.......'
> > >
> >
> > I guess 'mctp-i2c' alone here is fine given it's the I2C controller
> > slave implementation which is just a protocol. It's the external devices
> > where my concern is.
> >
> > Though, don't you need a different compatible for it and external
> > devices?
>
> We are only defining a binding for the system's own MCTP "controller"
> here, not the external devices on the other side of the I2C link. Those are
> probed outside of DT, for example hotplug NVMe disks can expose MCTP-over-
> I2C.
>
> This ends up describing something like a network interface, which
> happens to use I2C as a transport in this case. (There are other
> transports like MCTP-over-serial, but those don't require DT topology
> data). For other network-type DT bindings (say, ethernet@), we don't
> describe remote network endpoints either, so we're proposing the same
> pattern for MCTP.

When a switch becomes integrated in, we do.

> > >     reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
> > >     attach-bus = <&i2c1 &i2c6>;
> >
> > Why do you need to say you are attached to yourself?
>
> This indicates that the top-level MCTP controller needs to talk to MCTP
> endpoints, eg mctpA on the directly attached bus i2c1. In some topologies
> there will be no directly-attached endpoints, in which case we would omit
> i2c1 from the list. We need to specify the attach-bus property since we
> don't have a list of external device endpoints to walk.

Okay, so it's a 'what I2C buses should be scanned for MCTP devices'.
Why can't that just be all the buses under i2c1 in this example?
Limiting it seems like an optimization only. You don't know the
endpoint addresses, so you are scanning the whole bus, right?

In any case, 'attach-bus' sounds very generic and I'm not sure this
is. I'd like to hear from others familiar with I2C on this aspect at
least.

> [In the case of not directly attached, we still need to plant the mctp-i2c@
> node at the root controller level, as the claimed client address (in reg)
> is global across the entire bus. The attach-bus list gives us the set of
> interfaces that are necessary for the OS to control]
>
> > > Regarding I2C_OWN_SLAVE_ADDRESS validation
> > You'll have to do a minimum/maximum range with the high bit set and
> > addresses 0-7f.
>
> OK, that will work fine for our binding. The general I2C slave client case
> would also have to allow for I2C_TEN_BIT_ADDRESS (1<<31), but MCTP-over-I2C
> only accepts 7 bit.

TBC, requiring I2C_OWN_SLAVE_ADDRESS being set is specific to
MCTP-over-I2C and also serves to document the binding is only for the
host side.

Rob

Re: [RFC PATCH v2 0/2] MCTP I2C devicetree binding

[Jeremy Kerr]

Hi Rob,

> > This ends up describing something like a network interface, which
> > happens to use I2C as a transport in this case. (There are other
> > transports like MCTP-over-serial, but those don't require DT
> > topology
> > data). For other network-type DT bindings (say, ethernet@), we
> > don't
> > describe remote network endpoints either, so we're proposing the
> > same
> > pattern for MCTP.
> 
> When a switch becomes integrated in, we do.

OK, we'll allow for cases like that, where we do need a representation
of a "remote" endpoint. However, we don't *currently* have a scenario
where that is necessary.

> > > >     reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
> > > >     attach-bus = <&i2c1 &i2c6>;
> > > 
> > > Why do you need to say you are attached to yourself?
> > 
> > This indicates that the top-level MCTP controller needs to talk to
> > MCTP
> > endpoints, eg mctpA on the directly attached bus i2c1. In some
> > topologies
> > there will be no directly-attached endpoints, in which case we
> > would omit
> > i2c1 from the list. We need to specify the attach-bus property
> > since we
> > don't have a list of external device endpoints to walk.
> 
> Okay, so it's a 'what I2C buses should be scanned for MCTP devices'.

Not quite "scanned", more "marked as MCTP-capable". The indication that
an i2c bus is a MCTP controller doesn't initiate any scanning, but
rather provides a facility for software further up the stack to perform
any scanning / monitoring for hotplug devices / setting up fixed remote
endpoints - whatever is suitable for the system.

> Why can't that just be all the buses under i2c1 in this example?
> Limiting it seems like an optimization only.

It's not so much an optimisation, rather a way to avoid overly complex
network topologies. We may have on the order of 100 i2c busses
(including both root busses and mux subordinates) on some platforms.
Since physical addressing requires knowing both the SMBus address plus
the MUX state, any software/user that deals with physical addreses
will need to know about those ~100 busses.

If I can use the Linux implementation as an example: flagging an i2c
controller as MCTP-capable will create a MCTP netdev, which allows
communicating with specific physaddrs on that segment of the bus. I'd
like to avoid creating ~100 netdevs, all visible to userspace, when only
a small subset of those can carry actual MCTP data.

If we can limit the possible MCTP controllers to just the i2c busses
that host MCTP hardware downstream, that makes things much easier for
any OS implementation to deal with. While we can do the i2c/MCTP mapping
at a higher level (ie. userspace), representing this in the DT does
keep the local-hardware-specific data all in the one place.

> In any case, 'attach-bus' sounds very generic and I'm not sure this
> is. I'd like to hear from others familiar with I2C on this aspect at
> least.

We're certainly open to other structures for flagging busses as
MCTP-capable; we can use a more representative name for this phandle
list, or switch to boolean properties on the subordinate nodes
themselves (something like the gpio-controller boolean props, perhaps?
though that seems harder to confine to a schema for mctp-i2c...)

Cheers,


Jeremy

Re: [RFC PATCH v2 0/2] MCTP I2C devicetree binding

[Rob Herring]

On Mon, Aug 23, 2021 at 2:52 AM Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Hi Rob,
>
> > > This ends up describing something like a network interface, which
> > > happens to use I2C as a transport in this case. (There are other
> > > transports like MCTP-over-serial, but those don't require DT
> > > topology
> > > data). For other network-type DT bindings (say, ethernet@), we
> > > don't
> > > describe remote network endpoints either, so we're proposing the
> > > same
> > > pattern for MCTP.
> >
> > When a switch becomes integrated in, we do.
>
> OK, we'll allow for cases like that, where we do need a representation
> of a "remote" endpoint. However, we don't *currently* have a scenario
> where that is necessary.

The issue here tends to be we design things based on not having nodes
in DT and then eventually evolve to the point where we should have had
a separate node. Connectors or per slot PCI properties are some
examples. Just something to keep in mind.

> > > > >     reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
> > > > >     attach-bus = <&i2c1 &i2c6>;
> > > >
> > > > Why do you need to say you are attached to yourself?
> > >
> > > This indicates that the top-level MCTP controller needs to talk to
> > > MCTP
> > > endpoints, eg mctpA on the directly attached bus i2c1. In some
> > > topologies
> > > there will be no directly-attached endpoints, in which case we
> > > would omit
> > > i2c1 from the list. We need to specify the attach-bus property
> > > since we
> > > don't have a list of external device endpoints to walk.
> >
> > Okay, so it's a 'what I2C buses should be scanned for MCTP devices'.
>
> Not quite "scanned", more "marked as MCTP-capable". The indication that
> an i2c bus is a MCTP controller doesn't initiate any scanning, but
> rather provides a facility for software further up the stack to perform
> any scanning / monitoring for hotplug devices / setting up fixed remote
> endpoints - whatever is suitable for the system.
>
> > Why can't that just be all the buses under i2c1 in this example?
> > Limiting it seems like an optimization only.
>
> It's not so much an optimisation, rather a way to avoid overly complex
> network topologies. We may have on the order of 100 i2c busses
> (including both root busses and mux subordinates) on some platforms.
> Since physical addressing requires knowing both the SMBus address plus
> the MUX state, any software/user that deals with physical addreses
> will need to know about those ~100 busses.

Any system with muxes has them described in DT as I'm not aware of any
way muxes are discoverable.

> If I can use the Linux implementation as an example: flagging an i2c
> controller as MCTP-capable will create a MCTP netdev, which allows
> communicating with specific physaddrs on that segment of the bus. I'd
> like to avoid creating ~100 netdevs, all visible to userspace, when only
> a small subset of those can carry actual MCTP data.
>
> If we can limit the possible MCTP controllers to just the i2c busses
> that host MCTP hardware downstream, that makes things much easier for
> any OS implementation to deal with. While we can do the i2c/MCTP mapping
> at a higher level (ie. userspace), representing this in the DT does
> keep the local-hardware-specific data all in the one place.
>
> > In any case, 'attach-bus' sounds very generic and I'm not sure this
> > is. I'd like to hear from others familiar with I2C on this aspect at
> > least.
>
> We're certainly open to other structures for flagging busses as
> MCTP-capable; we can use a more representative name for this phandle
> list, or switch to boolean properties on the subordinate nodes
> themselves (something like the gpio-controller boolean props, perhaps?
> though that seems harder to confine to a schema for mctp-i2c...)

Either option is fine with me. A per bus property scales better (you
can add buses without changing the root MCTP node). We already have
per bus properties such as 'smbus' and 'multi-master'.

Rob

Re: [RFC PATCH v2 0/2] MCTP I2C devicetree binding

[Matt Johnston]

On Mon, 2021-08-23 at 11:16 -0500, Rob Herring wrote:
> On Mon, Aug 23, 2021 at 2:52 AM Jeremy Kerr <jk@codeconstruct.com.au>
> wrote:

> The issue here tends to be we design things based on not having nodes
> in DT and then eventually evolve to the point where we should have had
> a separate node. Connectors or per slot PCI properties are some
> examples. Just something to keep in mind.

When these are needed we can define a DT binding for endpoint devices, as a
I2C client node. I've given the example 'mctp-device' below as a possible
future binding.

> > We're certainly open to other structures for flagging busses as
> > MCTP-capable; we can use a more representative name for this phandle
> > list, or switch to boolean properties on the subordinate nodes
> > themselves (something like the gpio-controller boolean props, perhaps?
> > though that seems harder to confine to a schema for mctp-i2c...)
> 
> Either option is fine with me. A per bus property scales better (you
> can add buses without changing the root MCTP node). We already have
> per bus properties such as 'smbus' and 'multi-master'.

How does this look, adding a property to the generic I2C bus? We'll define
a 'mctp-controller' property that can be set on any I2C bus to flag it as
having MCTP endpoints. A 'mctp-i2c-controller' binding client node is
placed on the root bus to define the local I2C address, that needs to be
present if any child mux busses have the mctp-controller property. So
busses i2c1 and i2c6 will handle MCTP devices. We could omit mctp-
controller from i2c1, in that case only i2c6 would have MCTP devices,
though we still need the 'mctp-i2c-controller' at the root.

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

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

i2cmux0 {
  compatible = "vendor,example-i2c-mux";
  #address-cells = <1>;
  #size-cells = <0>;
  i2c-parent = <&i2c1>;

  i2c5: i2c@0 {
    reg = <0>;
    eeprom@33 {
      compatible = "atmel,24c64";
      reg = <0x33>;
    };
  };

  i2c6: i2c@1 {
    reg = <1>;
    mctp-controller;

    // A endpoint device can optionally be described in DT.
    // (as an example, not defining it in this patch series)
    nvme@0x20 {
      compatible = "mctp-device";
      reg = <0x20>;
      // only accepts a fixed MCTP address, not using MCTP control protocol
      mctp-fixed-address = 180;
    };
  };
};


Cheers,
Matt