Re: [coreboot] New interface for I2C in coreboot

[Werner Zeh]

Hi all.

I am fully aware of how I2C bus works and I never liked this interface. This is because you cannot really probe devices on the bus, there is not really a status information available on the bus and one can confuse a slave in that way that it will block the bus without a way to reset it (at least there is a trick available).
After I have thought for a long time about this issue, I can understand the need of the change that has been done. Nevertheless, I still not want the place where the cut in the interface was done.

With the new interface the driver for I2C bus is truncated because with this interface the driver do not have the knowledge of the whole transfer that must be performed on the bus. Instead, the driver is fed with small pieces of data. In this way, we cannot design a driver that is smarter and can use all the abilities a hardware given I2C controller can provide.

I would like to discuss the following approach:
Construct all the data you need to transmit/receive outside of the I2C driver (i.e. inside the user of I2C driver which can be itself a driver for the slave e.g. an EEPROM, tpm, whatever). We can use a similar structure approach the new interface already has and chain this structures to build a complete transaction. We can add control information (read/write data, start/stop condition...) to every piece of the transaction (let's call it chunk) as well as needed timeout or retry requirements. The list of all this chunks builds up the complete transaction. This transaction will be passed into the I2C driver. 
Now the driver can handle the list as it wants to. We can write simple drivers that consist of GPIO manipulation to emulate the I2C controller. But we also can write smart drivers which can use all the abilities a given hardware controller provide.
In my opinion this approach will allow us to be more flexible without wasting hardware possibilities and with a comparable complexity we currently have with the new interface.

Any thoughts are welcome.

Werner
> Gesendet: Dienstag, 10. Februar 2015 um 20:36 Uhr
> Von: "Julius Werner" 
> An: "Werner Zeh" 
> Cc: [e-mail], "Julius Werner" , "Marc Jones" , [e-mail], "Patrick Georgi" 
> Betreff: Re: New interface for I2C in coreboot
>
> I think the idea behind this change was mostly that the old API was
> too inflexible and some I2C device drivers could not be properly
> written with it. Take a look at line 139 in this file from the first
> patch: http://review.coreboot.org/#/c/7751/3/src/drivers/i2c/tpm/tpm.c@139
> . What this really does is trying to do all of the normal parts of an
> I2C read transaction manually, because TPMs can add so long delays at
> any point that the normal I2C controller drivers would already hit
> their timeout. With the old API this was only possible with a crude
> hack of setting the address length to 0 (so i2c_read() would skip the
> register address write completely). Doing a "raw read" with the new
> API does the same thing much clearer, and it's far more obvious to
> controller driver authors that they need to implement this (otherwise,
> it's easy to just assume alen == 0 is illegal until someone tries to
> use the TPM driver with it, which I think is what hit us on Tegra).
> The new API also makes the rules about when to use a repeated start vs
> a complete stop and start much clearer (which should not make a
> difference for the device, but in practice sometimes does).
>
> I think Werner's original issue is easy to solve: you can either just
> construct struct i2c_seg structures and call i2c_transfer() directly,
> or implement a new i2c_write() wrapper similar to i2c_writeb() (just
> with a variable data length). Maybe even just make i2c_writeb() a
> one-line wrapper macro on top of the new function... all fine by me, I
> think the only reason we didn't make more convenience functions is
> because we didn't need them at the time.
>
> The implementation problems Patrick mentioned are unfortunately true
> for some controllers which attempt to be more clever than they
> should... however, the thing is that we need to somehow implement raw
> I2C reads anyway (to support the alen == 0 case in the old API and
> make things like that TPM driver work). In the end we probably need to
> implement less primitives for the new API ("raw write" and "raw read")
> than for the old one ("full write", "full read", "raw write" and "raw
> read"). If that means we end up not using the
> do-everything-in-one-transaction "feature" of some controllers in
> favor of more controllable low-level accesses, I don't see a problem
> with that.