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

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

[Stephen Warren]

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);
 
 	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, &reg, 1);
+	rc = i2c_write_data(bus, chip, &reg, 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;
 		}
-- 
1.8.1.5

[U-Boot] [PATCH 2/3] i2c: tegra: write clean data to TX FIFO

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The Tegra I2C controller's TX FIFO contains 32-bit words. If the final
FIFO entry of a transaction contains fewer than 4 bytes, the driver
currently fills the unused FIFO bytes with uninitialized data. This can
be confusing when reading back the FIFO content for debugging purposes.

Solve this by explicitly initializing the variable containing FIFO data
before filling it (partially) with data. With this change,
send_recv_packets()'s loop's if (is_write) code mirrors the else (i.e.
read) branch.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/i2c/tegra_i2c.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c
index 97f0ca44c59a..a62f30e66ce1 100644
--- a/drivers/i2c/tegra_i2c.c
+++ b/drivers/i2c/tegra_i2c.c
@@ -224,14 +224,16 @@ static int send_recv_packets(struct i2c_bus *i2c_bus,
 
 		if (is_write) {
 			/* deal with word alignment */
-			if ((unsigned)dptr & 3) {
+			if ((words == 1) && last_bytes) {
+				local = 0;
+				memcpy(&local, dptr, last_bytes);
+			} else if ((unsigned)dptr & 3) {
 				memcpy(&local, dptr, sizeof(u32));
-				writel(local, &control->tx_fifo);
-				debug("pkt data sent (0x%x)\n", local);
 			} else {
-				writel(*wptr, &control->tx_fifo);
-				debug("pkt data sent (0x%x)\n", *wptr);
+				local = *wptr;
 			}
+			writel(local, &control->tx_fifo);
+			debug("pkt data sent (0x%x)\n", local);
 			if (!wait_for_tx_fifo_empty(control)) {
 				error = -1;
 				goto exit;
-- 
1.8.1.5

[U-Boot] [PATCH 2/3] i2c: tegra: write clean data to TX FIFO

[Yen Lin]

Hi Stephen,

> Subject: [U-Boot] [PATCH 2/3] i2c: tegra: write clean data to TX FIFO
> 
> From: Stephen Warren <swarren@nvidia.com>
> 
> The Tegra I2C controller's TX FIFO contains 32-bit words. If the final FIFO
> entry of a transaction contains fewer than 4 bytes, the driver currently fills
> the unused FIFO bytes with uninitialized data. This can be confusing when
> reading back the FIFO content for debugging purposes.
> 
> Solve this by explicitly initializing the variable containing FIFO data before
> filling it (partially) with data. With this change, send_recv_packets()'s loop's if
> (is_write) code mirrors the else (i.e.
> read) branch.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

LGME.

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.
-----------------------------------------------------------------------------------

[U-Boot] [PATCH 2/3] i2c: tegra: write clean data to TX FIFO

[Yen Lin]

> >
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> 
> LGME.

Oops, should be LGTM.

> 
> 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.
-----------------------------------------------------------------------------------

[U-Boot] [PATCH 3/3] i2c: tegra: dump alen in debug statements

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Since tegra_i2c_{read,write}'s debug() call dumps the chip address, dump
the address length (alen) too, so the address value can be correctly
interpreted.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/i2c/tegra_i2c.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c
index a62f30e66ce1..257b72f0f7cd 100644
--- a/drivers/i2c/tegra_i2c.c
+++ b/drivers/i2c/tegra_i2c.c
@@ -548,8 +548,8 @@ static int tegra_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr,
 	uint offset;
 	int i;
 
-	debug("i2c_read: chip=0x%x, addr=0x%x, len=0x%x\n",
-				chip, addr, len);
+	debug("i2c_read: chip=0x%x, addr=0x%x, alen=0x%x len=0x%x\n",
+	      chip, addr, alen, len);
 	bus = tegra_i2c_get_bus(adap);
 	if (!bus)
 		return 1;
@@ -587,8 +587,8 @@ static int tegra_i2c_write(struct i2c_adapter *adap, uchar chip, uint addr,
 	uint offset;
 	int i;
 
-	debug("i2c_write: chip=0x%x, addr=0x%x, len=0x%x\n",
-				chip, addr, len);
+	debug("i2c_write: chip=0x%x, addr=0x%x, alen=0x%x len=0x%x\n",
+	      chip, addr, alen, len);
 	bus = tegra_i2c_get_bus(adap);
 	if (!bus)
 		return 1;
-- 
1.8.1.5

[U-Boot] [PATCH 3/3] i2c: tegra: dump alen in debug statements

[Yen Lin]

Hi Stephen,

> Subject: [U-Boot] [PATCH 3/3] i2c: tegra: dump alen in debug statements
> 
> From: Stephen Warren <swarren@nvidia.com>
> 
> Since tegra_i2c_{read,write}'s debug() call dumps the chip address, dump
> the address length (alen) too, so the address value can be correctly
> interpreted.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

LGTM.

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.
-----------------------------------------------------------------------------------

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

[Yen Lin]

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.
-----------------------------------------------------------------------------------

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

[Simon Glass]

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

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

[Stephen Warren]

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.

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

[Joakim Tjernlund]

> From: Stephen Warren <swarren@wwwdotorg.org>
> To: u-boot at 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 at 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 

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

[Stephen Warren]

On 06/26/2014 02:11 AM, Joakim Tjernlund wrote:
>> From: Stephen Warren <swarren@wwwdotorg.org>
>> To: u-boot at 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 at 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:-)

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

[Joakim Tjernlund]

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 at 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 at 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:-)

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

[Stephen Warren]

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 at 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 at 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:-)

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

[Stephen Warren]

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 at 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 at 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).

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

[Joakim Tjernlund]

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 at 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 at 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?

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

[Stephen Warren]

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.

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

[Joakim Tjernlund]

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 at 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 at 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:-)
> 

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

[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?

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

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

[Stephen Warren]

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.