[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:24:05:
> On 06/26/2014 01:18 PM, Stephen Warren wrote:
> > 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. 
> 
> The delay makes no difference. (I delayed 1ms).

Strange, I had a look at the driver and I have a hard time figuring out 
how/when START/STOP
is generated. However I don't think the current driver's 
wait_for_transfer_complete() waits for
the START/STOP. I guess it waits until all data bytes are finished so STOP 
completion time
isn't accounted for.

Where is STOP initiated and where did you add the delay?