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

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

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?

> 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?

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...

But first we need to be clear on the big picture view.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]