[PATCH v4 1/3] i2c: mux: Add i2c-arbitrator-cros-ec 'mux' driver

[Simon Glass]

HI Wolfram,

On Wed, Apr 3, 2013 at 12:19 PM, Wolfram Sang <wsa at the-dreams.de> wrote:
> Doug,
>
>> Separately from a discussion of the technical merits, I'd say that
>> this patch is needed because the Embedded Controller (EC) on the ARM
>> Chromebook shipped expecting to communicate with this scheme.  While
>
> Uhrm, with all respect, "we already shipped it" is not a convincing
> argument regarding inclusion. Benefit for the kernel is.
>
>> ...but we can also talk technical merits.  One person on our team
>> spent a bit of time thinking about this and decided that traditional
>> i2c arbitration can't actually be used in this case (aside from the
>
> The details would be extremly interesting here. I am very interested in
> this and will ask a few questions further on. This is to get a better
> undestanding for the situation regarding multi-master and what is
> needed, because I expect some demand for it in the near future.
>
>> general experience about multimaster being buggy).  Specifically, our
>
> I'm also interested in these experiences.

Thanks for coming back on this. Please see my comments below.

>
>> The problem here is that the EC is both a master and a slave.  It's my
>> understanding that if the AP tries to talk EC on the bus at the same
>> time that the EC is trying to talk to the battery of PMU that we can
>> get into trouble.
>
> If I understand correctly, the I2C Specs mention this case explicitly:
>
> "If a master also incorporates a slave function and it loses arbitration
> during the addressing stage, it is possible that the winning master is
> trying to address it. The losing master must therefore switch over
> immediately to its slave mode."

This seems pretty complicated - does the EC then need to compare the
address it was sending with the remaining bits that are to be received
in that address?

Anyway yes we did consider this case at the time, and the STM32
datasheet had mention of it. But it adds quite a bit of complexity. I
think the start sequence is intended to make this all work, at least
in theory, but it seemed risky to rely on it.

The practical problem here is that in my experience the I2C
multi-master feature is lightly used in practice, and is quite tricky
to get right. That experience was shared with other engineers in the
team, and no one was willing to take the risk on getting everything
running perfectly with i2c multi-master, or doing it on a tight
timeline. I suspect this same problem has arisen many times before,
and will arise again, so from that point of view this feature is a
useful addition to the kernel.

If you are interested in specific experiences then I might be able to
collect some comments from people here. For myself I have found that
getting a reliable I2C driver at all is a sufficient challenge and
time sink with many embedded chips. My memory is a little vague now,
but I know that in 2-3 projects we had to use bit-bash due to bugs or
insufficient documentation in the chip's dedicated I2C peripheral. One
project which had multi-master used a retry scheme with sequenced
packets which seemed to work well enough (i.e. adding a layer above
the I2C layer to sort out problems). The snow project was more
challenging on the I2C  ront, due to the critical nature of I2C buses
in the device.

For snow we found that the Exynos driver did not support arbitration
loss, the STM32 driver we had did not support it, the STM32F I2C
errata indicated problems with I2C in general which reduced our
confidence, and we were unable to determine whether the slave devices
on the bus (smart battery and pmic) would definitely do the right
thing (both chips had I2C issues which we were tracking). I am not
trying to point fingers here, and certainly anything can be solved
given sufficient persistence...but keeping to common features that are
well-tested and available is prudent. As it was we spent more time on
I2C drivers than I would like, and you can get a feel for that in the
Chromium bug tracker if you wish.

Again, I don't believe this would be an unusual situation in a product
development. We need to consider the risk and cost of an approach,
rather than just whether it is technically possible.

>From the point of view of arbitration, the two critical points in the
I2C transfer are claiming the bus (detecting that the start condition
failed) and detecting loss of arbitration during a transfer. These
points need to be tested and validated on all master devices, which
can add a considerable burden. Silicon and driver bugs may make this
even more onerous and in fact for some non-compliant implementations
it simply might not work, or might not work reliably.

Please don't take this as a negative view of I2C in general, which is
an excellent bus, unmatched for what it was designed in my view.

>
>> Specifically the EC needs to be switched to "master
>> mode" to talk to the battery/PMU and thus has a period of time where
>> it won't be listening for the AP's messages.
>
> When it talks to the battery, the bus is busy and the EC will not be
> addressed by the AP?

That's right - there is only one bus and we want to avoid contention for it.

>
>> The arbitration lines make it very clear that the EC has gained
>> mastery of the bus and is free to switch into "master mode" without
>> worrying that the AP may talk to it.
>
> There is another thing I wonder: The arbitration is not I2C specific.
> You could use this scheme for various busses. So, besides the question
> if this is really needed on top of I2C arbitration, the other question
> is if this should reside inside the I2C kernel realm or should/could be
> generic.

Yes this is an interesting point! So far this feature has been added:

- in a separate 'arbitrator' driver (my original implementation, and
now suggested by you)
- in the Samsung I2C driver (suggested by Olof Johansson, my second
implementation)
- in the I2C core (suggested in code review I think and implemented by
Naveen Chatradhi)
- as an I2C mux (suggested by Grant Likely, implemented by Doug Anderson)

So yes there are clearly a number of places where it could go and we
might be going around in circles. But we should be careful to address
this question based on present need rather than theoretical value. I
believe there is clear value in having an I2C mux which can arbitrate
between masters using an out-of-band method for the reasons explained
above. Other needs and wishes should be justified on their merits when
they arise.

Having a mux, which can be is set up using the device tree, is quite
an attractive option. It allows the feature to be used by any I2C
driver, and does not touch the core I2C code.

The present patch is a nice clean implementation IMO, and I believe it
has value for the kernel.

Regards,
Simon