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

On Tue, Aug 17, 2021 at 11:19 PM Matt Johnston
[off-list ref] 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:

quoted

quoted

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

quoted

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.

quoted

quoted

    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]

quoted

quoted

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