i2c: tegra: use repeated start for reads

PATCH New Stephen Warren 5 participants 20 messages Jun 25, 2014 – Jul 21, 2014

Subsystems (3)

Arm (32-bit) Tegra architecture support I2C

lore

Share & tools

lore

Stephen Warren

author

2014-06-25 16:57 UTC #0 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

From: Stephen Warren <[email protected]>

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 <[email protected]>

---
 arch/arm/include/asm/arch-tegra/tegra_i2c.h |  2 ++
 drivers/i2c/tegra_i2c.c                     | 24 ++++++++++++++++--------
 2 files changed, 18 insertions(+), 8 deletions(-)

arch/arm/include/asm/arch-tegra/tegra_i2c.h +2 -0

index 853e59bb6e65..7ca690700cb4 100644
@@ -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)

drivers/i2c/tegra_i2c.c +16 -8

index 594e5ddeb43e..97f0ca44c59a 100644
@@ -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;
 		}

Signature · 1 line

1.8.1.5

Stephen Warren

author

2014-07-02 18:37 UTC #1 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

Quoted 6 lines · Stephen Warren

From: Stephen Warren <[email protected]>
I2C read transactions are typically implemented as follows:
START(write) address REPEATED_START(read) data... STOP

Quoted 7 lines

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?

Heiko Schocher

2014-07-03 04:34 UTC #2 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

Hello Stephen,

Am 02.07.2014 20:37, schrieb Stephen Warren:

Quoted 9 lines · Stephen Warren

Quoted 8 lines
From: Stephen Warren<[email protected]>
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:

Quoted 7 lines

Quoted 5 lines
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

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Stephen Warren

author

2014-07-21 15:14 UTC #3 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

Quoted 18 lines · Heiko Schocher

Hello Stephen,
Am 02.07.2014 20:37, schrieb Stephen Warren:
Quoted 15 lines · Stephen Warren
Quoted 13 lines
From: Stephen Warren<[email protected]>
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.

Quoted 6 lines

Quoted 1 line
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.

Quoted 2 lines

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.

Stephen Warren

author

2014-06-25 16:57 UTC #4 lore raw

PATCH 2/3 i2c: tegra: write clean data to TX FIFO

In reply to Stephen Warren

From: Stephen Warren <[email protected]>

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 <[email protected]>

---
 drivers/i2c/tegra_i2c.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/i2c/tegra_i2c.c +7 -5

index 97f0ca44c59a..a62f30e66ce1 100644
@@ -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;

Signature · 1 line

1.8.1.5

Yen Lin

2014-06-25 23:37 UTC #5 lore raw

PATCH 2/3 i2c: tegra: write clean data to TX FIFO

Hi Stephen,

Quoted 9 lines

Subject: [U-Boot] [PATCH 2/3] i2c: tegra: write clean data to TX FIFO
From: Stephen Warren <[email protected]>
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.

Quoted 6 lines

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 <[email protected]>

LGME.

Reviewed-by Yen Lin <[email protected]>

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

Yen Lin

2014-06-25 23:42 UTC #6 lore raw

PATCH 2/3 i2c: tegra: write clean data to TX FIFO

In reply to Stephen Warren

Quoted 5 lines

Quoted 3 lines
Signed-off-by Stephen Warren <[email protected]>
LGME.

Oops, should be LGTM.

Quoted 5 lines

Reviewed-by Yen Lin <[email protected]>
Regards,
Yen Lin

Yen Lin

2014-06-25 23:36 UTC #7 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

In reply to Stephen Warren

Hi Stephen,

Quoted 10 lines

Subject: [U-Boot] [PATCH 1/3] i2c: tegra: use repeated start for reads
From: Stephen Warren <[email protected]>
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:

Quoted 7 lines

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 <[email protected]>

This patch looks good to me.

Reviewed-by Yen Lin <[email protected]>

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

Simon Glass

2014-06-26 02:12 UTC #8 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

In reply to Stephen Warren

Hi Stephen,

Quoted 58 lines · Stephen Warren

From: Stephen Warren <[email protected]>

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

Signed-off-by Stephen Warren <[email protected]>

---
 arch/arm/include/asm/arch-tegra/tegra_i2c.h |  2 ++
 drivers/i2c/tegra_i2c.c                     | 24 ++++++++++++++++--------
 2 files changed, 18 insertions(+), 8 deletions(-)

arch/arm/include/asm/arch-tegra/tegra_i2c.h +2 -0

index 853e59bb6e65..7ca690700cb4 100644
@@ -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)

drivers/i2c/tegra_i2c.c +4 -1

index 594e5ddeb43e..97f0ca44c59a 100644
@@ -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);

drivers/i2c/tegra_i2c.c static int send_recv_packets(struct i2c_bus *i2c_bus, · line 216

Quoted 6 lines

drivers/i2c/tegra_i2c.c +2 -1

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

drivers/i2c/tegra_i2c.c static int send_recv_packets(struct i2c_bus *i2c_bus, · line 219

Quoted 3 lines

drivers/i2c/tegra_i2c.c +0 -0

        words = DIV_ROUND_UP(trans->num_bytes, 4);
        last_bytes = trans->num_bytes & 3;

Regards,
Simon

Stephen Warren

author

2014-06-26 04:09 UTC #9 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

Quoted 24 lines · Simon Glass

Hi Stephen,
Quoted 21 lines · Stephen Warren
From: Stephen Warren <[email protected]>
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.
drivers/i2c/tegra_i2c.c +0 -0
@@ -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);
 

Quoted 8 lines

drivers/i2c/tegra_i2c.c static int send_recv_packets(struct i2c_bus *i2c_bus, · line 216
Quoted 3 lines
drivers/i2c/tegra_i2c.c +2 -1
-       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.

Joakim Tjernlund

2014-06-26 08:11 UTC #10 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

In reply to Stephen Warren

Quoted 13 lines

From: Stephen Warren <[email protected]>
To: u-boot at lists.denx.de, Heiko Schocher <[email protected]>,
Cc: Stephen Warren <[email protected]>, Tom Warren <[email protected]>
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 <[email protected]>
I2C read transactions are typically implemented as follows:
START(write) address REPEATED_START(read) data... STOP

Quoted 7 lines

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

Stephen Warren

author

2014-06-26 16:47 UTC #11 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

Quoted 21 lines · Joakim Tjernlund

Quoted 13 lines
From: Stephen Warren <[email protected]>
To: u-boot at lists.denx.de, Heiko Schocher <[email protected]>,
Cc: Stephen Warren <[email protected]>, Tom Warren <[email protected]>
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 <[email protected]>
I2C read transactions are typically implemented as follows:
START(write) address REPEATED_START(read) data... STOP
Quoted 7 lines
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.

Quoted 5 lines

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

Joakim Tjernlund

2014-06-26 19:11 UTC #12 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

Stephen Warren <[email protected]> wrote on 2014/06/26 18:47:55:

Quoted 5 lines · Joakim Tjernlund

Quoted 3 lines
Quoted 3 lines
From: Stephen Warren <[email protected]>
To: u-boot at lists.denx.de, Heiko Schocher <[email protected]>,
Cc: Stephen Warren <[email protected]>, Tom Warren

<[email protected]>

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Date: 2014/06/25 19:05
Subject: [U-Boot] [PATCH 1/3] i2c: tegra: use repeated start for

reads

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Sent by: u-boot-bounces at lists.denx.de
From: Stephen Warren <[email protected]>
I2C read transactions are typically implemented as follows:

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
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,

Quoted 10 lines

Quoted 9 lines
Quoted 2 lines
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.

Quoted 6 lines

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.

Quoted 4 lines

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

author

2014-06-26 19:18 UTC #13 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

Quoted 41 lines · Joakim Tjernlund

Stephen Warren <[email protected]> wrote on 2014/06/26 18:47:55:
Quoted 5 lines · Joakim Tjernlund
Quoted 3 lines
Quoted 3 lines
From: Stephen Warren <[email protected]>
To: u-boot at lists.denx.de, Heiko Schocher <[email protected]>,
Cc: Stephen Warren <[email protected]>, Tom Warren
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Date: 2014/06/25 19:05
Subject: [U-Boot] [PATCH 1/3] i2c: tegra: use repeated start for
reads
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Sent by: u-boot-bounces at lists.denx.de
From: Stephen Warren <[email protected]>
I2C read transactions are typically implemented as follows:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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,
Quoted 10 lines
Quoted 9 lines
Quoted 2 lines
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.
Quoted 6 lines
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.

Quoted 3 lines

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:

Quoted 3 lines

Quoted 3 lines
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

author

2014-06-26 19:24 UTC #14 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

Quoted 42 lines · Stephen Warren

Quoted 41 lines · Joakim Tjernlund
Stephen Warren <[email protected]> wrote on 2014/06/26 18:47:55:
Quoted 5 lines · Joakim Tjernlund
Quoted 3 lines
Quoted 3 lines
From: Stephen Warren <[email protected]>
To: u-boot at lists.denx.de, Heiko Schocher <[email protected]>,
Cc: Stephen Warren <[email protected]>, Tom Warren
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Date: 2014/06/25 19:05
Subject: [U-Boot] [PATCH 1/3] i2c: tegra: use repeated start for
reads
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Sent by: u-boot-bounces at lists.denx.de
From: Stephen Warren <[email protected]>
I2C read transactions are typically implemented as follows:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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,
Quoted 16 lines
Quoted 9 lines
Quoted 2 lines
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.

Quoted 3 lines

Quoted 3 lines
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).

Joakim Tjernlund

2014-06-26 20:01 UTC #15 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

Stephen Warren <[email protected]> wrote on 2014/06/26 21:24:05:

Quoted 20 lines · Stephen Warren

Quoted 19 lines · Joakim Tjernlund
Quoted 18 lines
Stephen Warren <[email protected]> wrote on 2014/06/26 18:47:55:
Quoted 5 lines · Joakim Tjernlund
Quoted 3 lines
Quoted 3 lines
From: Stephen Warren <[email protected]>
To: u-boot at lists.denx.de, Heiko Schocher <[email protected]>,
Cc: Stephen Warren <[email protected]>, Tom Warren
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Date: 2014/06/25 19:05
Subject: [U-Boot] [PATCH 1/3] i2c: tegra: use repeated start for
reads
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Sent by: u-boot-bounces at lists.denx.de
From: Stephen Warren <[email protected]>
I2C read transactions are typically implemented as follows:
START(write) address REPEATED_START(read) data... STOP

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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,
Quoted 1 line
Quoted 1 line
Quoted 1 line
leading to corrupted read data in some cases. Fix the driver to

chain

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
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

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that
the driver initializes STOP but does not wait for the STOP to

complete

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
before issuing a START.
I don't believe that's the case here, since all this patch does is

set a

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
flag to indicate whether the write transaction (to set the

intra-chip

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
register address) generates STOP or REPEATED_START at the end. If

the

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
code or HW wasn't waiting for the STOP to complete, I see no reason

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
would wait for the REPEATED_START to complete either, so I think the subsequent register read transaction would be corrupted in either

case.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
But there is, you have STOP + START vs. ReSTART only and if the code

only

Quoted 4 lines

Quoted 2 lines
Quoted 2 lines
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?

Stephen Warren

author

2014-06-26 22:54 UTC #16 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

On 06/26/2014 02:01 PM, Joakim Tjernlund wrote:
...

Quoted 9 lines

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.

Joakim Tjernlund

2014-06-26 19:40 UTC #17 lore raw

PATCH 1/3 i2c: tegra: use repeated start for reads

In reply to Stephen Warren

Stephen Warren <[email protected]> wrote on 2014/06/26 21:18:50:

Quoted 20 lines · Joakim Tjernlund

Quoted 18 lines
Stephen Warren <[email protected]> wrote on 2014/06/26 18:47:55:
Quoted 5 lines · Joakim Tjernlund
Quoted 3 lines
Quoted 3 lines
From: Stephen Warren <[email protected]>
To: u-boot at lists.denx.de, Heiko Schocher <[email protected]>,
Cc: Stephen Warren <[email protected]>, Tom Warren
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Date: 2014/06/25 19:05
Subject: [U-Boot] [PATCH 1/3] i2c: tegra: use repeated start for
reads
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Sent by: u-boot-bounces at lists.denx.de
From: Stephen Warren <[email protected]>
I2C read transactions are typically implemented as follows:
START(write) address REPEATED_START(read) data... STOP

Quoted 7 lines

Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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,
Quoted 1 line
Quoted 1 line
Quoted 1 line
leading to corrupted read data in some cases. Fix the driver to

chain

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
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

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
that
the driver initializes STOP but does not wait for the STOP to

complete

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 1 line
before issuing a START.
I don't believe that's the case here, since all this patch does is

set a

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
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

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
would wait for the REPEATED_START to complete either, so I think the subsequent register read transaction would be corrupted in either

case.

Quoted 2 lines

Quoted 2 lines
But there is, you have STOP + START vs. ReSTART only and if the code

only

Quoted 6 lines

Quoted 2 lines
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 :)

Quoted 7 lines

Anyway, as I note below, the code is waiting:
Quoted 3 lines
Quoted 3 lines
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

author

2014-06-25 16:57 UTC #18 lore raw

PATCH 3/3 i2c: tegra: dump alen in debug statements

In reply to Stephen Warren

From: Stephen Warren <[email protected]>

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 <[email protected]>

---
 drivers/i2c/tegra_i2c.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/i2c/tegra_i2c.c +4 -4

index a62f30e66ce1..257b72f0f7cd 100644
@@ -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;

Signature · 1 line

1.8.1.5

Yen Lin

2014-06-25 23:40 UTC #19 lore raw

PATCH 3/3 i2c: tegra: dump alen in debug statements

Hi Stephen,

Quoted 9 lines

Subject: [U-Boot] [PATCH 3/3] i2c: tegra: dump alen in debug statements
From: Stephen Warren <[email protected]>
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 <[email protected]>

LGTM.

Reviewed-by Yen Lin <[email protected]>

Regards,
Yen Lin