OK, forget my last Mail...I was to blind to see the truth :-)
We already have my approach with the new interface.
We maybe can expand it to have informations like time-out or retry count for a given
Gesendet: Donnerstag, 19. Februar 2015 um 07:41 Uhr
Von: "Werner Zeh" <werner.zeh(a)gmx.net>
An: "Julius Werner" <jwerner(a)chromium.org>rg>, gaumless(a)gmail.com,
Betreff: Re: [coreboot] New interface for I2C in coreboot
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
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
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.
Gesendet: Dienstag, 10. Februar 2015 um 20:36
Von: "Julius Werner" <jwerner(a)chromium.org>
An: "Werner Zeh" <werner.zeh(a)gmx.net>
Cc: coreboot(a)coreboot.org, "Julius Werner" <jwerner(a)chromium.org>rg>,
"Marc Jones" <marc.jones(a)se-eng.com>om>, martin(a)se-eng.com, "Patrick
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
. 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
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
coreboot mailing list: coreboot(a)coreboot.org