Re: [PATCH v4 05/14] i2c-octeon: Enable high-level controller and improve on bus contention

[Jan Glauber <jan.glauber@caviumnetworks.com>]

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@cavium.com>
> > 
> > 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
>