[U-Boot] [PATCH 1/3] i2c: tegra: use repeated start for reads

[Joakim Tjernlund]

Stephen Warren <swarren at wwwdotorg.org> wrote on 2014/06/26 21:18:50:

> 
> On 06/26/2014 01:11 PM, Joakim Tjernlund wrote:
> > Stephen Warren <swarren at wwwdotorg.org> wrote on 2014/06/26 18:47:55:
> >>
> >> On 06/26/2014 02:11 AM, Joakim Tjernlund wrote:
> >>>> From: Stephen Warren <swarren at wwwdotorg.org>
> >>>> To: u-boot at lists.denx.de, Heiko Schocher <hs at denx.de>, 
> >>>> Cc: Stephen Warren <swarren at nvidia.com>, Tom Warren 
> > <twarren at nvidia.com>
> >>>> Date: 2014/06/25 19:05
> >>>> Subject: [U-Boot] [PATCH 1/3] i2c: tegra: use repeated start for 
> > reads
> >>>> Sent by: u-boot-bounces at lists.denx.de
> >>>>
> >>>> From: Stephen Warren <swarren at nvidia.com>
> >>>>
> >>>> I2C read transactions are typically implemented as follows:
> >>>>
> >>>> START(write) address REPEATED_START(read) data... STOP
> >>>>
> >>>> However, Tegra's I2C driver currently implements reads as follows:
> >>>>
> >>>> START(write) address STOP START(read) data... STOP
> >>>>
> >>>> This sequence confuses at least the AS3722 PMIC on the Jetson TK1 
> > board,
> >>>> leading to corrupted read data in some cases. Fix the driver to 
chain
> >>>> the transactions together using repeated starts to solve this.
> >>>
> >>> While I agree to use Repeated START I just wanted to share this:
> >>> A common reason for STOP START(read) sequence not working sometimes 
is 
> > 
> >>> that
> >>> the driver initializes STOP but does not wait for the STOP to 
complete
> >>> before issuing a START.
> >>
> >> I don't believe that's the case here, since all this patch does is 
set a
> >> flag to indicate whether the write transaction (to set the intra-chip
> >> register address) generates STOP or REPEATED_START at the end. If the
> >> code or HW wasn't waiting for the STOP to complete, I see no reason 
it
> >> would wait for the REPEATED_START to complete either, so I think the
> >> subsequent register read transaction would be corrupted in either 
case.
> > 
> > But there is, you have STOP + START vs. ReSTART only and if the code 
only 
> > flips a flag to change I think there is a chance in this case.
> > You could easily test by adding a udelay(5) after STOP is initiated. 
> 
> If the code doesn't wait for the STOP/REPEATED_START to complete, then
> whatever comes after will trample it, whether it's a START, or the data
> transfer for the next transaction.

You misunderstand, there has to be an extra STOP complete time between the 
STOP
and START. When replacing this with only one ReSTART that STOP complete 
time is gone.
So by changing to ReSTART you fixed this error and got much better 
behaviour too.
I am just saying that there might be other places which lacks STOP 
completion as well
For reference you can look at 
  
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=21f4cbb77299788e2b06c9b0f48cf20a5ab00d4a
and
 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-mpc.c?id=45da790ebe746bb29f7e4adf806c020db6ff7755
That took a while for me to figure out :)

> 
> Anyway, as I note below, the code is waiting:
> 
> >> There's certainly code in tegra_i2c.c:wait_for_transfer_complete() to
> >> poll until each transaction completes before starting the next, and
> >> there's even error handling to detect any problems there:-)
>