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 segment. Werner > Gesendet: Donnerstag, 19. Februar 2015 um 07:41 Uhr > Von: "Werner Zeh" <[email protected]> > An: "Julius Werner" <[email protected]>, [email protected], > [email protected] > Cc: [email protected] > Betreff: Re: [coreboot] New interface for I2C in coreboot > > 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" <[email protected]> > > An: "Werner Zeh" <[email protected]> > > Cc: [email protected], "Julius Werner" <[email protected]>, "Marc > > Jones" <[email protected]>, [email protected], "Patrick Georgi" > > <[email protected]> > > 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. > > > > -- > coreboot mailing list: [email protected] > http://www.coreboot.org/mailman/listinfo/coreboot > -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
