On Wed, Mar 23, 2016 at 09:32:15PM +0100, Wolfram Sang wrote: > On Fri, Mar 18, 2016 at 09:46:30AM +0100, Jan Glauber wrote: > > From: David Daney <david.daney@xxxxxxxxxx> > > > > Use High Level Controller when possible. > > Can you give me a one line description what this Controller is? I'd > assume it can do simple write-then-read messages with less setup? Of course, I'll add this to the patch description too. The HLC can read/write up to 8 bytes and is completely optional. The most important difference of the HLC is that it only requires one interrupt for a transfer (up to 8 bytes) where the low-level read/write requires 2 interrupts plus one interrupt per transferred byte. Since the interrupts are costly using the HLC improves the performance. Also, the HLC provides improved error handling. > > i2c-octeon was reacting badly to bus contention: when in > > direct-access mode (for transfers > 8 bytes, which cannot use the > > high-level controller) some !ACK or arbitration-loss states were > > not causing the current transfer to be aborted, and the bus released. > > So, what does this patch do? Enable HLC for transfers < 8 byte? And for > all other transfers we still suffer from the same problem? I think the patch description was misleading, which is my fault because I merged several incremental patches into one. The HLC is used when possible (up to 8 bytes). For bigger transfers the handling is improved and special treatment is done for the first and last part of a transfer. > Such information should be here, too. It helps reviewing when I already > have the big picture. > > > There's one place in i2c protocol that !ACK is an acceptable > > response: in the final byte of a read cycle. In this case the > > destination is not saying that the transfer failed, just that it > > doesn't want more data. > > Ehrm, no? For reads, the MASTER is saying it doesn't need any more data. > And an I2C eeprom can legally NACK a write, e.g. when it is still > processing the previous write. Also, NACK is a valid response after the > address phase, meaning there is no device listening. > > Does the implementation cover the above cases? > > > This enables correct behavior of ACK on final byte of non-final read > > msgs too. > > The patch is huge and very hard to review. Maybe it needs to be split > up. Brainstorming example: a) move functions like octeon_i2c_set_clock() > upwards, b) change them if needed, c) implement HLC functions, d) add > switching logic to use HLC or non-HLC functions... I was reluctant to split the patch because of the high risk of breaking the bi-sectability, but your proposal makes sense. I've seperated the error handling changes from the HLC feature now (plus seperate patches for the moved functions). Thanks, Jan > But first we need to be clear on the big picture view. > > Thanks, > > Wolfram > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
