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
