MCTP I2C devicetree binding

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 <[email protected]>

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

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.

Quoted 2 lines · Rob Herring

Adding 'generic' is not an improvement nor does it change anything.

I may have misunderstood the feedback then:

Quoted 2 lines

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)

Quoted 1 line

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?

Quoted 9 lines

                                       .-------.
                                       |eeprom |
.----------.   .------.               /'-------'

Quoted 6 lines

adapter  |   | mux  --@0,i2c5------'
i2c1     |-.-|      --@1,i2c6--.--.
..........|  \'------'           \  \  .........
mctp-i2c |   \                   \  \ .mctpB  .
slave    |    \                   \  '.0x30   .
0x50     |     \  .........        \  '.......'

Quoted 5 lines

'----------'      \ .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?

Quoted 2 lines

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

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

Quoted 1 line

Regarding I2C_OWN_SLAVE_ADDRESS validation

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

On Tue, 2021-08-17 at 16:06 -0500, Rob Herring wrote:

Quoted 15 lines · Matt Johnston

Quoted 7 lines

                                       .-------.
                                       |eeprom |
.----------.   .------.               /'-------'
> adapter  |   | mux  --@0,i2c5------'
> i2c1     |-.-|      --@1,i2c6--.--.
> ..........|  \'------'           \  \  .........
> mctp-i2c |   \                   \  \ .mctpB  .

Quoted 7 lines

Quoted 2 lines

slave | \ \ '.0x30 .
0x50 | \ ......... \ '.......'

'----------' \ .mctpA . \ .........

                   '.0x1d   .          '.mctpC  .
                    '.......'          '.0x31   .
                                        '.......'

Quoted 6 lines

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.

Quoted 4 lines

Quoted 2 lines

    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]

Quoted 3 lines

Quoted 1 line

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.

Quoted 9 lines

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.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

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

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

Quoted 9 lines

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.

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.

Hi Rob,

Quoted 10 lines

Quoted 9 lines

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.

Quoted 1 line

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.

Quoted 15 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

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

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

Quoted 9 lines

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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 3 lines

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

On Mon, Aug 23, 2021 at 2:52 AM Jeremy Kerr <[email protected]> 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.

Quoted 5 lines

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