| Message ID | 1403715449-2177-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Delegated to: | Heiko Schocher |
| Headers | show |
Hi Stephen, > Subject: [U-Boot] [PATCH 1/3] i2c: tegra: use repeated start for reads > > From: Stephen Warren <swarren@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. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> This patch looks good to me. Reviewed-by: Yen Lin <yelin@nvidia.com> Regards, Yen Lin ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
Hi Stephen, On 25 June 2014 10:57, Stephen Warren <swarren@wwwdotorg.org> wrote: > > From: Stephen Warren <swarren@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. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > arch/arm/include/asm/arch-tegra/tegra_i2c.h | 2 ++ > drivers/i2c/tegra_i2c.c | 24 ++++++++++++++++-------- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/include/asm/arch-tegra/tegra_i2c.h b/arch/arm/include/asm/arch-tegra/tegra_i2c.h > index 853e59bb6e65..7ca690700cb4 100644 > --- a/arch/arm/include/asm/arch-tegra/tegra_i2c.h > +++ b/arch/arm/include/asm/arch-tegra/tegra_i2c.h > @@ -124,6 +124,8 @@ struct i2c_ctlr { > /* bit fields definitions for IO Packet Header 3 format */ > #define PKT_HDR3_READ_MODE_SHIFT 19 > #define PKT_HDR3_READ_MODE_MASK (1 << PKT_HDR3_READ_MODE_SHIFT) > +#define PKT_HDR3_REPEAT_START_SHIFT 16 > +#define PKT_HDR3_REPEAT_START_MASK (1 << PKT_HDR3_REPEAT_START_SHIFT) > #define PKT_HDR3_SLAVE_ADDR_SHIFT 0 > #define PKT_HDR3_SLAVE_ADDR_MASK (0x3ff << PKT_HDR3_SLAVE_ADDR_SHIFT) > > diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c > index 594e5ddeb43e..97f0ca44c59a 100644 > --- a/drivers/i2c/tegra_i2c.c > +++ b/drivers/i2c/tegra_i2c.c > @@ -110,7 +110,8 @@ static void i2c_init_controller(struct i2c_bus *i2c_bus) > static void send_packet_headers( > struct i2c_bus *i2c_bus, > struct i2c_trans_info *trans, > - u32 packet_id) > + u32 packet_id, > + bool end_with_repeated_start) > { > u32 data; > > @@ -132,6 +133,8 @@ static void send_packet_headers( > /* Enable Read if it is not a write transaction */ > if (!(trans->flags & I2C_IS_WRITE)) > data |= PKT_HDR3_READ_MODE_MASK; > + if (end_with_repeated_start) > + data |= PKT_HDR3_REPEAT_START_MASK; > > /* Write I2C specific header */ > writel(data, &i2c_bus->control->tx_fifo); > @@ -209,7 +212,8 @@ static int send_recv_packets(struct i2c_bus *i2c_bus, > int_status = readl(&control->int_status); > writel(int_status, &control->int_status); > > - send_packet_headers(i2c_bus, trans, 1); > + send_packet_headers(i2c_bus, trans, 1, > + trans->flags & I2C_USE_REPEATED_START); I'm not sure if it is safe/advisable to pass this value to a bool type. Perhaps the function parameter should be int? My understanding of bool is that it is supposed to be 0 or 1, but I'm happy to be corrected. > > words = DIV_ROUND_UP(trans->num_bytes, 4); > last_bytes = trans->num_bytes & 3; Regards, Simon
On 06/25/2014 08:12 PM, Simon Glass wrote: > Hi Stephen, > > On 25 June 2014 10:57, Stephen Warren <swarren@wwwdotorg.org> wrote: >> >> From: Stephen Warren <swarren@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. >> diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c >> @@ -209,7 +212,8 @@ static int send_recv_packets(struct i2c_bus *i2c_bus, >> int_status = readl(&control->int_status); >> writel(int_status, &control->int_status); >> >> - send_packet_headers(i2c_bus, trans, 1); >> + send_packet_headers(i2c_bus, trans, 1, >> + trans->flags & I2C_USE_REPEATED_START); > > I'm not sure if it is safe/advisable to pass this value to a bool > type. Perhaps the function parameter should be int? My understanding > of bool is that it is supposed to be 0 or 1, but I'm happy to be > corrected. I believe that the "promotion" from int to bool clamps the range to 0 or 1. I've certainly seen compilers warn that this promotion might be a performance issue!. If not, I can always add !! in front.
> From: Stephen Warren <swarren@wwwdotorg.org> > To: u-boot@lists.denx.de, Heiko Schocher <hs@denx.de>, > Cc: Stephen Warren <swarren@nvidia.com>, Tom Warren <twarren@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@lists.denx.de > > From: Stephen Warren <swarren@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. Jocke
On 06/26/2014 02:11 AM, Joakim Tjernlund wrote: >> From: Stephen Warren <swarren@wwwdotorg.org> >> To: u-boot@lists.denx.de, Heiko Schocher <hs@denx.de>, >> Cc: Stephen Warren <swarren@nvidia.com>, Tom Warren <twarren@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@lists.denx.de >> >> From: Stephen Warren <swarren@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. 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:-)
Stephen Warren <swarren@wwwdotorg.org> wrote on 2014/06/26 18:47:55: > > On 06/26/2014 02:11 AM, Joakim Tjernlund wrote: > >> From: Stephen Warren <swarren@wwwdotorg.org> > >> To: u-boot@lists.denx.de, Heiko Schocher <hs@denx.de>, > >> Cc: Stephen Warren <swarren@nvidia.com>, Tom Warren <twarren@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@lists.denx.de > >> > >> From: Stephen Warren <swarren@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. > > 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:-)
On 06/26/2014 01:11 PM, Joakim Tjernlund wrote: > Stephen Warren <swarren@wwwdotorg.org> wrote on 2014/06/26 18:47:55: >> >> On 06/26/2014 02:11 AM, Joakim Tjernlund wrote: >>>> From: Stephen Warren <swarren@wwwdotorg.org> >>>> To: u-boot@lists.denx.de, Heiko Schocher <hs@denx.de>, >>>> Cc: Stephen Warren <swarren@nvidia.com>, Tom Warren > <twarren@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@lists.denx.de >>>> >>>> From: Stephen Warren <swarren@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. 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:-)
On 06/26/2014 01:18 PM, Stephen Warren wrote: > On 06/26/2014 01:11 PM, Joakim Tjernlund wrote: >> Stephen Warren <swarren@wwwdotorg.org> wrote on 2014/06/26 18:47:55: >>> >>> On 06/26/2014 02:11 AM, Joakim Tjernlund wrote: >>>>> From: Stephen Warren <swarren@wwwdotorg.org> >>>>> To: u-boot@lists.denx.de, Heiko Schocher <hs@denx.de>, >>>>> Cc: Stephen Warren <swarren@nvidia.com>, Tom Warren >> <twarren@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@lists.denx.de >>>>> >>>>> From: Stephen Warren <swarren@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).
Stephen Warren <swarren@wwwdotorg.org> wrote on 2014/06/26 21:18:50: > > On 06/26/2014 01:11 PM, Joakim Tjernlund wrote: > > Stephen Warren <swarren@wwwdotorg.org> wrote on 2014/06/26 18:47:55: > >> > >> On 06/26/2014 02:11 AM, Joakim Tjernlund wrote: > >>>> From: Stephen Warren <swarren@wwwdotorg.org> > >>>> To: u-boot@lists.denx.de, Heiko Schocher <hs@denx.de>, > >>>> Cc: Stephen Warren <swarren@nvidia.com>, Tom Warren > > <twarren@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@lists.denx.de > >>>> > >>>> From: Stephen Warren <swarren@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:-) >
Stephen Warren <swarren@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@wwwdotorg.org> wrote on 2014/06/26 18:47:55: > >>> > >>> On 06/26/2014 02:11 AM, Joakim Tjernlund wrote: > >>>>> From: Stephen Warren <swarren@wwwdotorg.org> > >>>>> To: u-boot@lists.denx.de, Heiko Schocher <hs@denx.de>, > >>>>> Cc: Stephen Warren <swarren@nvidia.com>, Tom Warren > >> <twarren@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@lists.denx.de > >>>>> > >>>>> From: Stephen Warren <swarren@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?
On 06/26/2014 02:01 PM, Joakim Tjernlund wrote: ... > 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? STOP (or REPEATED_START) happen automatically in HW once it's finished transferring all the bytes in the TX FIFO, and before the transaction complete interrupt is asserted. I added the delay immediately after the loop that spins waiting for "transaction complete" IRQ status to be set.
On 06/25/2014 10:57 AM, Stephen Warren wrote: > From: Stephen Warren <swarren@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. Heiko, do these patches look good?
Hello Stephen, Am 02.07.2014 20:37, schrieb Stephen Warren: > On 06/25/2014 10:57 AM, Stephen Warren wrote: >> From: Stephen Warren<swarren@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. > > Heiko, do these patches look good? Yes, they look good to me... Hmm.. as you ask, I think you want to have them in v2014.07 ? As it is a bugfix it should go into it, but as it also changes the behaviour of the driver, I am unsure if it can go in so close before the new release ... some Tested-by would be nice ... I applied them to the u-boot-i2c.git tree, and if nobody objects against them, I send tomorrow a pull request to Tom. bye, Heiko
On 07/02/2014 10:34 PM, Heiko Schocher wrote: > Hello Stephen, > > Am 02.07.2014 20:37, schrieb Stephen Warren: >> On 06/25/2014 10:57 AM, Stephen Warren wrote: >>> From: Stephen Warren<swarren@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. >> >> Heiko, do these patches look good? > > Yes, they look good to me... Hmm.. as you ask, I think you want to have > them in v2014.07 ? As it is a bugfix it should go into it, but as it > also changes the behaviour of the driver, I am unsure if it can go in > so close before the new release ... some Tested-by would be nice ... Sorry for the slow response; I was on vacation. I see the patches made it into the release, which should be fine. The I2C functionality is likely used very little, so even if there were problems with the patch, the fallout from it would be minimal. > I applied them to the u-boot-i2c.git tree, and if nobody objects > against them, I send tomorrow a pull request to Tom. Thanks.
diff --git a/arch/arm/include/asm/arch-tegra/tegra_i2c.h b/arch/arm/include/asm/arch-tegra/tegra_i2c.h index 853e59bb6e65..7ca690700cb4 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_i2c.h +++ b/arch/arm/include/asm/arch-tegra/tegra_i2c.h @@ -124,6 +124,8 @@ struct i2c_ctlr { /* bit fields definitions for IO Packet Header 3 format */ #define PKT_HDR3_READ_MODE_SHIFT 19 #define PKT_HDR3_READ_MODE_MASK (1 << PKT_HDR3_READ_MODE_SHIFT) +#define PKT_HDR3_REPEAT_START_SHIFT 16 +#define PKT_HDR3_REPEAT_START_MASK (1 << PKT_HDR3_REPEAT_START_SHIFT) #define PKT_HDR3_SLAVE_ADDR_SHIFT 0 #define PKT_HDR3_SLAVE_ADDR_MASK (0x3ff << PKT_HDR3_SLAVE_ADDR_SHIFT) diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c index 594e5ddeb43e..97f0ca44c59a 100644 --- a/drivers/i2c/tegra_i2c.c +++ b/drivers/i2c/tegra_i2c.c @@ -110,7 +110,8 @@ static void i2c_init_controller(struct i2c_bus *i2c_bus) static void send_packet_headers( struct i2c_bus *i2c_bus, struct i2c_trans_info *trans, - u32 packet_id) + u32 packet_id, + bool end_with_repeated_start) { u32 data; @@ -132,6 +133,8 @@ static void send_packet_headers( /* Enable Read if it is not a write transaction */ if (!(trans->flags & I2C_IS_WRITE)) data |= PKT_HDR3_READ_MODE_MASK; + if (end_with_repeated_start) + data |= PKT_HDR3_REPEAT_START_MASK; /* Write I2C specific header */ writel(data, &i2c_bus->control->tx_fifo); @@ -209,7 +212,8 @@ static int send_recv_packets(struct i2c_bus *i2c_bus, int_status = readl(&control->int_status); writel(int_status, &control->int_status); - send_packet_headers(i2c_bus, trans, 1); + send_packet_headers(i2c_bus, trans, 1, + trans->flags & I2C_USE_REPEATED_START); words = DIV_ROUND_UP(trans->num_bytes, 4); last_bytes = trans->num_bytes & 3; @@ -267,7 +271,7 @@ exit: } static int tegra_i2c_write_data(struct i2c_bus *bus, u32 addr, u8 *data, - u32 len) + u32 len, bool end_with_repeated_start) { int error; struct i2c_trans_info trans_info; @@ -275,6 +279,8 @@ static int tegra_i2c_write_data(struct i2c_bus *bus, u32 addr, u8 *data, trans_info.address = addr; trans_info.buf = data; trans_info.flags = I2C_IS_WRITE; + if (end_with_repeated_start) + trans_info.flags |= I2C_USE_REPEATED_START; trans_info.num_bytes = len; trans_info.is_10bit_address = 0; @@ -463,7 +469,8 @@ static void tegra_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) } /* i2c write version without the register address */ -int i2c_write_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len) +int i2c_write_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len, + bool end_with_repeated_start) { int rc; @@ -475,7 +482,8 @@ int i2c_write_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len) debug("\n"); /* Shift 7-bit address over for lower-level i2c functions */ - rc = tegra_i2c_write_data(bus, chip << 1, buffer, len); + rc = tegra_i2c_write_data(bus, chip << 1, buffer, len, + end_with_repeated_start); if (rc) debug("i2c_write_data(): rc=%d\n", rc); @@ -516,7 +524,7 @@ static int tegra_i2c_probe(struct i2c_adapter *adap, uchar chip) if (!bus) return 1; reg = 0; - rc = i2c_write_data(bus, chip, ®, 1); + rc = i2c_write_data(bus, chip, ®, 1, false); if (rc) { debug("Error probing 0x%x.\n", chip); return 1; @@ -554,7 +562,7 @@ static int tegra_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr, data[alen - i - 1] = (addr + offset) >> (8 * i); } - if (i2c_write_data(bus, chip, data, alen)) { + if (i2c_write_data(bus, chip, data, alen, true)) { debug("i2c_read: error sending (0x%x)\n", addr); return 1; @@ -591,7 +599,7 @@ static int tegra_i2c_write(struct i2c_adapter *adap, uchar chip, uint addr, for (i = 0; i < alen; i++) data[alen - i - 1] = (addr + offset) >> (8 * i); data[alen] = buffer[offset]; - if (i2c_write_data(bus, chip, data, alen + 1)) { + if (i2c_write_data(bus, chip, data, alen + 1, false)) { debug("i2c_write: error sending (0x%x)\n", addr); return 1; }