[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 --- 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, ®, 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; }
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;
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. -----------------------------------------------------------------------------------
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. -----------------------------------------------------------------------------------
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;
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. -----------------------------------------------------------------------------------
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
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
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
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: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
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.
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
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=21f4cbb77299788e2b06c9b0f48c... and
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drive... 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:-)
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 Warrenswarren@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 Warrenswarren@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.
participants (5)
-
Heiko Schocher -
Joakim Tjernlund -
Simon Glass -
Stephen Warren -
Yen Lin