Stephen Warren <swar...@wwwdotorg.org> wrote on 2014/06/26 21:18:50: > > On 06/26/2014 01:11 PM, Joakim Tjernlund wrote: > > Stephen Warren <swar...@wwwdotorg.org> wrote on 2014/06/26 18:47:55: > >> > >> On 06/26/2014 02:11 AM, Joakim Tjernlund wrote: > >>>> From: Stephen Warren <swar...@wwwdotorg.org> > >>>> To: u-boot@lists.denx.de, Heiko Schocher <h...@denx.de>, > >>>> Cc: Stephen Warren <swar...@nvidia.com>, Tom Warren > > <twar...@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-boun...@lists.denx.de > >>>> > >>>> From: Stephen Warren <swar...@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:-) > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
