[U-Boot] [PATCH] i2c: imx: bus arbitration fix when reading block data

From e643f91ccfa544b7e3153a7721fba66e0e494759 Mon Sep 17 00:00:00 2001 From: Jim Brennan jbrennan@impinj.com Date: Wed, 13 Dec 2017 13:44:02 -0800 Subject: [PATCH] i2c: imx: bus arbitration fix when reading block data
Fixes arbitration failure on imx platform due to incorrect chip address use when reading a block of data. Add support for both reading or writing a block of data or any combination.
Signed-off-by: Jim Brennan jbrennan@impinj.com
---
drivers/i2c/mxc_i2c.c | 65 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 24 deletions(-)
diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index 205274e947..bd2a39ebe6 100644 --- a/drivers/i2c/mxc_i2c.c +++ b/drivers/i2c/mxc_i2c.c @@ -25,6 +25,7 @@ #include <dm.h> #include <dm/pinctrl.h> #include <fdtdec.h> +#include <stdbool.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -275,7 +276,7 @@ static void i2c_imx_stop(struct mxc_i2c_bus *i2c_bus) * write register address */ static int i2c_init_transfer_(struct mxc_i2c_bus *i2c_bus, u8 chip, - u32 addr, int alen) + u32 addr, int alen, bool read) { unsigned int temp; int ret; @@ -317,9 +318,17 @@ static int i2c_init_transfer_(struct mxc_i2c_bus *i2c_bus, u8 chip, temp |= I2CR_MTX | I2CR_TX_NO_AK; writeb(temp, base + (I2CR << reg_shift));
- if (alen >= 0) { - /* write slave address */ - ret = tx_byte(i2c_bus, chip << 1); + /* write slave address */ + u8 slave_address = (chip << 1); + + if (read) + slave_address |= 1; + ret = tx_byte(i2c_bus, slave_address); + if (ret < 0) + return ret; + + while (alen--) { + ret = tx_byte(i2c_bus, (addr >> (alen * 8)) & 0xff); if (ret < 0) return ret;
@@ -413,7 +422,7 @@ exit: #endif
static int i2c_init_transfer(struct mxc_i2c_bus *i2c_bus, u8 chip, - u32 addr, int alen) + u32 addr, int alen, bool read) { int retry; int ret; @@ -424,7 +433,7 @@ static int i2c_init_transfer(struct mxc_i2c_bus *i2c_bus, u8 chip, return -EINVAL;
for (retry = 0; retry < 3; retry++) { - ret = i2c_init_transfer_(i2c_bus, chip, addr, alen); + ret = i2c_init_transfer_(i2c_bus, chip, addr, alen, read); if (ret >= 0) return 0; i2c_imx_stop(i2c_bus); @@ -536,7 +545,7 @@ static int bus_i2c_read(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr, VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT; ulong base = i2c_bus->base;
- ret = i2c_init_transfer(i2c_bus, chip, addr, alen); + ret = i2c_init_transfer(i2c_bus, chip, addr, alen, false); if (ret < 0) return ret;
@@ -566,7 +575,7 @@ static int bus_i2c_write(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr, { int ret = 0;
- ret = i2c_init_transfer(i2c_bus, chip, addr, alen); + ret = i2c_init_transfer(i2c_bus, chip, addr, alen, false); if (ret < 0) return ret;
@@ -817,7 +826,7 @@ static int mxc_i2c_probe_chip(struct udevice *bus, u32 chip_addr, int ret; struct mxc_i2c_bus *i2c_bus = dev_get_priv(bus);
- ret = i2c_init_transfer(i2c_bus, chip_addr, 0, 0); + ret = i2c_init_transfer(i2c_bus, chip_addr, 0, 0, false); if (ret < 0) { debug("%s failed, ret = %d\n", __func__, ret); return ret; @@ -841,35 +850,43 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs) * because here we only want to send out chip address. The register * address is wrapped in msg. */ - ret = i2c_init_transfer(i2c_bus, msg->addr, 0, 0); + + bool read = (msg->flags & I2C_M_RD) ? true : false; + + ret = i2c_init_transfer(i2c_bus, msg->addr, 0, 0, read); if (ret < 0) { debug("i2c_init_transfer error: %d\n", ret); return ret; }
for (; nmsgs > 0; nmsgs--, msg++) { + bool current_is_read = (msg->flags & I2C_M_RD) ? true : false; bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD); debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len); - if (msg->flags & I2C_M_RD) + if (current_is_read) ret = i2c_read_data(i2c_bus, msg->addr, msg->buf, msg->len); else { ret = i2c_write_data(i2c_bus, msg->addr, msg->buf, msg->len); - if (ret) + } + if (ret) + break; + + if (nmsgs > 1 && (current_is_read ^ next_is_read)) { + u8 addr = msg->addr << 1; + + /* Reuse ret */ + ret = readb(base + (I2CR << reg_shift)); + ret |= I2CR_RSTA; + writeb(ret, base + (I2CR << reg_shift)); + + if (next_is_read) + addr |= 1; + + ret = tx_byte(i2c_bus, addr); + if (ret < 0) break; - if (next_is_read) { - /* Reuse ret */ - ret = readb(base + (I2CR << reg_shift)); - ret |= I2CR_RSTA; - writeb(ret, base + (I2CR << reg_shift)); - - ret = tx_byte(i2c_bus, (msg->addr << 1) | 1); - if (ret < 0) { - i2c_imx_stop(i2c_bus); - break; - } - } } }

Hello Jim,
Am 27.12.2017 um 02:05 schrieb Jim Brennan:
From e643f91ccfa544b7e3153a7721fba66e0e494759 Mon Sep 17 00:00:00 2001 From: Jim Brennan jbrennan@impinj.com Date: Wed, 13 Dec 2017 13:44:02 -0800 Subject: [PATCH] i2c: imx: bus arbitration fix when reading block data
Fixes arbitration failure on imx platform due to incorrect chip address use when reading a block of data. Add support for both reading or writing a block of data or any combination.
Signed-off-by: Jim Brennan jbrennan@impinj.com
drivers/i2c/mxc_i2c.c | 65 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 24 deletions(-)
Your patch drops a lot of checkpatch warnings, see:
http://xeidos.ddns.net/tbot/id_590/tbot.txt
and search for "2018-01-16 02:41:58" to see the wget cmd, to get your patch from aptchwork and search for "2018-01-16 02:42:00" to see the checkpatch.pl command.
ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit fatal: bad o ("721fba66e0e494759")' #15:
From e643f91ccfa544b7e3153a7721fba66e0e494759 Mon Sep 17 00:00:00 2001
ERROR: DOS line endings #42: FILE: drivers/i2c/mxc_i2c.c:28: +#include <stdbool.h>^M$ [...]
Please fix this, and resend your patch, thanks!
And some Tested-by would be helpful ...
@Stefano, @Fabio: Any chance to test this change? Thanks!
bye, Heiko
diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index 205274e947..bd2a39ebe6 100644 --- a/drivers/i2c/mxc_i2c.c +++ b/drivers/i2c/mxc_i2c.c @@ -25,6 +25,7 @@ #include <dm.h> #include <dm/pinctrl.h> #include <fdtdec.h> +#include <stdbool.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -275,7 +276,7 @@ static void i2c_imx_stop(struct mxc_i2c_bus *i2c_bus)
- write register address
*/ static int i2c_init_transfer_(struct mxc_i2c_bus *i2c_bus, u8 chip,
u32 addr, int alen)
{ unsigned int temp; int ret;u32 addr, int alen, bool read)
@@ -317,9 +318,17 @@ static int i2c_init_transfer_(struct mxc_i2c_bus *i2c_bus, u8 chip, temp |= I2CR_MTX | I2CR_TX_NO_AK; writeb(temp, base + (I2CR << reg_shift));
- if (alen >= 0) {
/* write slave address */
ret = tx_byte(i2c_bus, chip << 1);
- /* write slave address */
- u8 slave_address = (chip << 1);
- if (read)
slave_address |= 1;
- ret = tx_byte(i2c_bus, slave_address);
- if (ret < 0)
return ret;
- while (alen--) {
if (ret < 0) return ret;ret = tx_byte(i2c_bus, (addr >> (alen * 8)) & 0xff);
@@ -413,7 +422,7 @@ exit: #endif
static int i2c_init_transfer(struct mxc_i2c_bus *i2c_bus, u8 chip,
u32 addr, int alen)
{ int retry; int ret;u32 addr, int alen, bool read)
@@ -424,7 +433,7 @@ static int i2c_init_transfer(struct mxc_i2c_bus *i2c_bus, u8 chip, return -EINVAL;
for (retry = 0; retry < 3; retry++) {
ret = i2c_init_transfer_(i2c_bus, chip, addr, alen);
if (ret >= 0) return 0; i2c_imx_stop(i2c_bus);ret = i2c_init_transfer_(i2c_bus, chip, addr, alen, read);
@@ -536,7 +545,7 @@ static int bus_i2c_read(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr, VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT; ulong base = i2c_bus->base;
- ret = i2c_init_transfer(i2c_bus, chip, addr, alen);
- ret = i2c_init_transfer(i2c_bus, chip, addr, alen, false); if (ret < 0) return ret;
@@ -566,7 +575,7 @@ static int bus_i2c_write(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr, { int ret = 0;
- ret = i2c_init_transfer(i2c_bus, chip, addr, alen);
- ret = i2c_init_transfer(i2c_bus, chip, addr, alen, false); if (ret < 0) return ret;
@@ -817,7 +826,7 @@ static int mxc_i2c_probe_chip(struct udevice *bus, u32 chip_addr, int ret; struct mxc_i2c_bus *i2c_bus = dev_get_priv(bus);
- ret = i2c_init_transfer(i2c_bus, chip_addr, 0, 0);
- ret = i2c_init_transfer(i2c_bus, chip_addr, 0, 0, false); if (ret < 0) { debug("%s failed, ret = %d\n", __func__, ret); return ret;
@@ -841,35 +850,43 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs) * because here we only want to send out chip address. The register * address is wrapped in msg. */
- ret = i2c_init_transfer(i2c_bus, msg->addr, 0, 0);
bool read = (msg->flags & I2C_M_RD) ? true : false;
ret = i2c_init_transfer(i2c_bus, msg->addr, 0, 0, read); if (ret < 0) { debug("i2c_init_transfer error: %d\n", ret); return ret; }
for (; nmsgs > 0; nmsgs--, msg++) {
bool current_is_read = (msg->flags & I2C_M_RD) ? true : false;
bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD); debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
if (msg->flags & I2C_M_RD)
else { ret = i2c_write_data(i2c_bus, msg->addr, msg->buf, msg->len);if (current_is_read) ret = i2c_read_data(i2c_bus, msg->addr, msg->buf, msg->len);
if (ret)
}
if (ret)
break;
if (nmsgs > 1 && (current_is_read ^ next_is_read)) {
u8 addr = msg->addr << 1;
/* Reuse ret */
ret = readb(base + (I2CR << reg_shift));
ret |= I2CR_RSTA;
writeb(ret, base + (I2CR << reg_shift));
if (next_is_read)
addr |= 1;
ret = tx_byte(i2c_bus, addr);
if (ret < 0) break;
if (next_is_read) {
/* Reuse ret */
ret = readb(base + (I2CR << reg_shift));
ret |= I2CR_RSTA;
writeb(ret, base + (I2CR << reg_shift));
ret = tx_byte(i2c_bus, (msg->addr << 1) | 1);
if (ret < 0) {
i2c_imx_stop(i2c_bus);
break;
}
} }}

Hi Heiko,
On Tue, Jan 16, 2018 at 3:29 AM, Heiko Schocher hs@denx.de wrote:
And some Tested-by would be helpful ...
@Stefano, @Fabio: Any chance to test this change? Thanks!
Breno on Cc will do a quick test on this one.
Regards,
Fabio Estevam

Hello Fabio,
Am 16.01.2018 um 12:50 schrieb Fabio Estevam:
Hi Heiko,
On Tue, Jan 16, 2018 at 3:29 AM, Heiko Schocher hs@denx.de wrote:
And some Tested-by would be helpful ...
@Stefano, @Fabio: Any chance to test this change? Thanks!
Breno on Cc will do a quick test on this one.
Thanks!
bye, Heiko

Hi Jim,
2018-01-16 10:47 GMT-02:00 Heiko Schocher hs@denx.de:
Hello Fabio,
Am 16.01.2018 um 12:50 schrieb Fabio Estevam:
Hi Heiko,
On Tue, Jan 16, 2018 at 3:29 AM, Heiko Schocher hs@denx.de wrote:
And some Tested-by would be helpful ...
@Stefano, @Fabio: Any chance to test this change? Thanks!
Breno on Cc will do a quick test on this one.
I tested your patch on a mx6sabresd board and U-Boot is hanging in power_init_board():
U-Boot SPL 2018.01-39204-g3244ca3 (Jan 16 2018 - 12:13:25) Trying to boot from MMC1
U-Boot 2018.01-39204-g3244ca3 (Jan 16 2018 - 12:13:25 -0200)
CPU: Freescale i.MX6QP rev1.0 996 MHz (running at 792 MHz) CPU: Automotive temperature grade (-40C to 125C) at 25C Reset cause: POR Board: MX6-SabreSD I2C: ready DRAM: 1 GiB
With DEBUG mode enabled I'm getting the following output:
U-Boot 2018.01-39204-g3244ca3-dirty (Jan 16 2018 - 12:24:34 -0200)
CPU: Freescale i.MX6QP rev1.0 996 MHz (running at 792 MHz) CPU: Automotive temperature grade (-40C to 125C) at 36C Reset cause: POR Board: MX6-SabreSD I2C: ready DRAM: 1 GiB pmic_alloc: new pmic struct: 0x4f56e1e8 pmic_get: pmic PFUZE100 -> 0x4f56e1e8 i2c_write_data: chip=0x8, len=0x0 write_data:
I also tested on a mx7dsabresd and on this board your patch is working fine. If you need more details please let me know.
Thanks, Breno Lima

Hi Breno,
On Tue, Jan 16, 2018 at 12:36 PM, Breno Matheus Lima brenomatheus@gmail.com wrote:
I tested your patch on a mx6sabresd board and U-Boot is hanging in power_init_board():
Thanks for testing!
Seems like this patch needs a rework then.
Thanks

Hello Breno,
Am 16.01.2018 um 15:36 schrieb Breno Matheus Lima:
Hi Jim,
2018-01-16 10:47 GMT-02:00 Heiko Schocher hs@denx.de:
Hello Fabio,
Am 16.01.2018 um 12:50 schrieb Fabio Estevam:
Hi Heiko,
On Tue, Jan 16, 2018 at 3:29 AM, Heiko Schocher hs@denx.de wrote:
And some Tested-by would be helpful ...
@Stefano, @Fabio: Any chance to test this change? Thanks!
Breno on Cc will do a quick test on this one.
I tested your patch on a mx6sabresd board and U-Boot is hanging in power_init_board():
U-Boot SPL 2018.01-39204-g3244ca3 (Jan 16 2018 - 12:13:25) Trying to boot from MMC1
U-Boot 2018.01-39204-g3244ca3 (Jan 16 2018 - 12:13:25 -0200)
CPU: Freescale i.MX6QP rev1.0 996 MHz (running at 792 MHz) CPU: Automotive temperature grade (-40C to 125C) at 25C Reset cause: POR Board: MX6-SabreSD I2C: ready DRAM: 1 GiB
With DEBUG mode enabled I'm getting the following output:
U-Boot 2018.01-39204-g3244ca3-dirty (Jan 16 2018 - 12:24:34 -0200)
CPU: Freescale i.MX6QP rev1.0 996 MHz (running at 792 MHz) CPU: Automotive temperature grade (-40C to 125C) at 36C Reset cause: POR Board: MX6-SabreSD I2C: ready DRAM: 1 GiB pmic_alloc: new pmic struct: 0x4f56e1e8 pmic_get: pmic PFUZE100 -> 0x4f56e1e8 i2c_write_data: chip=0x8, len=0x0 write_data:
I also tested on a mx7dsabresd and on this board your patch is working fine. If you need more details please let me know.
Many thanks for testing!
bye, Heiko
participants (4)
-
Breno Matheus Lima
-
Fabio Estevam
-
Heiko Schocher
-
Jim Brennan