Date: Thu, 31 Mar 2016 12:24:33 +0200
From: Jan Glauber <jan.glauber@caviumnetworks.com>
To: Wolfram Sang <wsa@the-dreams.de>
CC: <linux-kernel@vger.kernel.org>, <linux-i2c@vger.kernel.org>,
        David Daney <ddaney@caviumnetworks.com>,
        David Daney <david.daney@cavium.com>
Subject: Re: [PATCH v4 05/14] i2c-octeon: Enable high-level controller and
 improve on bus contention
Message-ID: <20160331102433.GB31112@hardcore>
References: <cover.1458289385.git.jglauber@cavium.com>
 <27b7d9015f8165da0371e4d26c9acc72772ae3a0.1458289385.git.jglauber@cavium.com>
 <20160323203215.GF19849@katana>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20160323203215.GF19849@katana>
User-Agent: Mutt/1.5.21 (2010-09-15)
SpamDiagnosticOutput: 1:23
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Mar 2016 10:24:46.8926 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN2PR0701MB765
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2877
Lines: 71

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
>