[U-Boot] [PATCH 0/5]: Correct i2c support on am33xx
Hey all,
The following series corrects i2c support for the am33xx family of devices (including beaglebone) and has been tested on a beagleboard xM and pandaboard. I would like to take these changes in via the u-boot-ti tree as it's mostly changes to arch and board files but I've cc'd Heiko on the i2c related parts. The biggest change here is to revert a previous change to the omap24xx_i2c.c driver for the i2c probe function. In short, the change made before violated the TRM constraints but did no harm on the IP block found in omap3/related boards. Moving forward to the IP block found on omap4 and am33xx (and others) it still voilates the TRM and now leaves the bus in an unboundedly defined state like the TRM states. We must revert this change for both correctness and functionality. The rest of the series is minor corrections to structs/defines and adding CONFIG_AM33XX to the block of !CONFIG_OMAP2430. I've thought of, but think it should be separate to change omap24xx_i2c.c to test for CONFIG_OMAP2430 rather than every other present and future user in a few areas.
This reverts commit 0e57968a215d1b9d271f3fa5bebeddeaea0c8075.
The short version of the original commit is that some i2c devices cannot be probed via read as they NAK the first cycle, so try and probe via a write that we abort before it writes to the device. This however is not allowed by the TRM for any of these parts. The section on I2C_CON (table 17-35 I2C_CON for am/dm37x for example) says you must not change the register while STT has been set. On these parts, the unpredictable behavior that the chip exhibits is not problematic. On OMAP4 however it results in the chip being in a bad state: Panda # i2c probe Valid chip addresses: 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F 10 11 12 13 14 15 16 17 18 19 1A 1B 1C 1D 1E 1F 20 21 22 23 24 25 26 27 28 29 2A 2B 2C 2D 2E 2F 30 31 32 33 34 35 36 37 38 39 3A 3B 3C 3D 3E 3F 40 41 42 43 44 45 46 47 48 49 4A 4B 4C 4D 4E 4F 50 51 52 53 54 55 56 57 58 59 5A 5B 5C 5D 5E 5F 60 61 62 63 64 65 66 67 68 69 6A 6B 6C 6D 6E 6F 70 71 72 73 74 75 76 77 78 79 7A 7B 7C 7D 7E 7F Panda # i2c md 50 0 timed out in wait_for_pin: I2C_STAT=0 I2C read: I/O error Error reading the chip.
We must revert the original behavior to bring probe back into line with the TRM.
Cc: Nick Thompson nick.thompson@ge.com Cc: Heiko Schocher hs@denx.de Signed-off-by: Tom Rini trini@ti.com --- drivers/i2c/omap24xx_i2c.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index a7ffd95..3a50417 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -255,23 +255,43 @@ int i2c_probe(uchar chip) /* wait until bus not busy */ wait_for_bb();
- /* try to write one byte */ + /* try to read one byte */ writew(1, &i2c_base->cnt); /* set slave address */ writew(chip, &i2c_base->sa); /* stop bit needed here */ - writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX | - I2C_CON_STP, &i2c_base->con); - - status = wait_for_pin(); + writew (I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP, &i2c_base->con);
- /* check for ACK (!NAK) */ - if (!(status & I2C_STAT_NACK)) - res = 0; - - /* abort transfer (force idle state) */ - writew(0, &i2c_base->con); + while (1) { + status = wait_for_pin(); + if (status == 0 || status & I2C_STAT_AL) { + res = 1; + goto probe_exit; + } + if (status & I2C_STAT_NACK) { + res = 1; + writew(0xff, &i2c_base->stat); + writew (readw (&i2c_base->con) | I2C_CON_STP, &i2c_base->con); + wait_for_bb (); + break; + } + if (status & I2C_STAT_ARDY) { + writew(I2C_STAT_ARDY, &i2c_base->stat); + break; + } + if (status & I2C_STAT_RRDY) { + res = 0; +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ + defined(CONFIG_OMAP44XX) + readb(&i2c_base->data); +#else + readw(&i2c_base->data); +#endif + writew(I2C_STAT_RRDY, &i2c_base->stat); + } + }
+probe_exit: flush_fifo(); /* don't allow any more data in... we don't want it. */ writew(0, &i2c_base->cnt);
Hello Tom,
Tom Rini wrote:
This reverts commit 0e57968a215d1b9d271f3fa5bebeddeaea0c8075.
The short version of the original commit is that some i2c devices cannot be probed via read as they NAK the first cycle, so try and probe via a write that we abort before it writes to the device. This however is not allowed by the TRM for any of these parts. The section on I2C_CON (table 17-35 I2C_CON for am/dm37x for example) says you must not change the register while STT has been set. On these parts, the unpredictable behavior that the chip exhibits is not problematic. On OMAP4 however it results in the chip being in a bad state: Panda # i2c probe Valid chip addresses: 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F 10 11 12 13 14 15 16 17 18 19 1A 1B 1C 1D 1E 1F 20 21 22 23 24 25 26 27 28 29 2A 2B 2C 2D 2E 2F 30 31 32 33 34 35 36 37 38 39 3A 3B 3C 3D 3E 3F 40 41 42 43 44 45 46 47 48 49 4A 4B 4C 4D 4E 4F 50 51 52 53 54 55 56 57 58 59 5A 5B 5C 5D 5E 5F 60 61 62 63 64 65 66 67 68 69 6A 6B 6C 6D 6E 6F 70 71 72 73 74 75 76 77 78 79 7A 7B 7C 7D 7E 7F Panda # i2c md 50 0 timed out in wait_for_pin: I2C_STAT=0 I2C read: I/O error Error reading the chip.
We must revert the original behavior to bring probe back into line with the TRM.
Cc: Nick Thompson nick.thompson@ge.com Cc: Heiko Schocher hs@denx.de Signed-off-by: Tom Rini trini@ti.com
drivers/i2c/omap24xx_i2c.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)
Acked-by: Heiko Schocher hs@denx.de
Thanks!
bye, Heiko
Signed-off-by: Tom Rini trini@ti.com --- arch/arm/include/asm/arch-am33xx/i2c.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/include/asm/arch-am33xx/i2c.h b/arch/arm/include/asm/arch-am33xx/i2c.h index 366e2bb..dcb294c 100644 --- a/arch/arm/include/asm/arch-am33xx/i2c.h +++ b/arch/arm/include/asm/arch-am33xx/i2c.h @@ -34,9 +34,9 @@ struct i2c { unsigned short revnb_lo; /* 0x00 */ unsigned short res1; unsigned short revnb_hi; /* 0x04 */ - unsigned short res2[13]; - unsigned short sysc; /* 0x20 */ - unsigned short res3; + unsigned short res2[5]; + unsigned short sysc; /* 0x10 */ + unsigned short res3[9]; unsigned short irqstatus_raw; /* 0x24 */ unsigned short res4; unsigned short stat; /* 0x28 */
Hello Tom,
Tom Rini wrote:
Signed-off-by: Tom Rini trini@ti.com
arch/arm/include/asm/arch-am33xx/i2c.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Acked-by: Heiko Schocher hs@denx.de
Thanks!
bye, Heiko
Signed-off-by: Tom Rini trini@ti.com --- arch/arm/include/asm/arch-am33xx/cpu.h | 40 ++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/arch/arm/include/asm/arch-am33xx/cpu.h b/arch/arm/include/asm/arch-am33xx/cpu.h index 8760cc0..5a6534e 100644 --- a/arch/arm/include/asm/arch-am33xx/cpu.h +++ b/arch/arm/include/asm/arch-am33xx/cpu.h @@ -62,7 +62,7 @@ struct cm_wkuppll { unsigned int wkclkstctrl; /* offset 0x00 */ unsigned int wkctrlclkctrl; /* offset 0x04 */ - unsigned int resv1[1]; + unsigned int wkgpio0clkctrl; /* offset 0x08 */ unsigned int wkl4wkclkctrl; /* offset 0x0c */ unsigned int resv2[4]; unsigned int idlestdpllmpu; /* offset 0x20 */ @@ -111,34 +111,54 @@ struct cm_perpll { unsigned int l3clkstctrl; /* offset 0x0c */ unsigned int resv1; unsigned int cpgmac0clkctrl; /* offset 0x14 */ - unsigned int resv2[4]; + unsigned int lcdclkctrl; /* offset 0x18 */ + unsigned int usb0clkctrl; /* offset 0x1C */ + unsigned int resv2; + unsigned int tptc0clkctrl; /* offset 0x24 */ unsigned int emifclkctrl; /* offset 0x28 */ unsigned int ocmcramclkctrl; /* offset 0x2c */ unsigned int gpmcclkctrl; /* offset 0x30 */ - unsigned int resv3[2]; + unsigned int mcasp0clkctrl; /* offset 0x34 */ + unsigned int uart5clkctrl; /* offset 0x38 */ unsigned int mmc0clkctrl; /* offset 0x3C */ unsigned int elmclkctrl; /* offset 0x40 */ unsigned int i2c2clkctrl; /* offset 0x44 */ unsigned int i2c1clkctrl; /* offset 0x48 */ unsigned int spi0clkctrl; /* offset 0x4C */ unsigned int spi1clkctrl; /* offset 0x50 */ - unsigned int resv4[3]; + unsigned int resv3[3]; unsigned int l4lsclkctrl; /* offset 0x60 */ unsigned int l4fwclkctrl; /* offset 0x64 */ - unsigned int resv5[6]; + unsigned int mcasp1clkctrl; /* offset 0x68 */ + unsigned int uart1clkctrl; /* offset 0x6C */ + unsigned int uart2clkctrl; /* offset 0x70 */ + unsigned int uart3clkctrl; /* offset 0x74 */ + unsigned int uart4clkctrl; /* offset 0x78 */ + unsigned int timer7clkctrl; /* offset 0x7C */ unsigned int timer2clkctrl; /* offset 0x80 */ - unsigned int resv6[11]; + unsigned int timer3clkctrl; /* offset 0x84 */ + unsigned int timer4clkctrl; /* offset 0x88 */ + unsigned int resv4[8]; + unsigned int gpio1clkctrl; /* offset 0xAC */ unsigned int gpio2clkctrl; /* offset 0xB0 */ - unsigned int resv7[7]; + unsigned int gpio3clkctrl; /* offset 0xB4 */ + unsigned int resv5; + unsigned int tpccclkctrl; /* offset 0xBC */ + unsigned int dcan0clkctrl; /* offset 0xC0 */ + unsigned int dcan1clkctrl; /* offset 0xC4 */ + unsigned int resv6[2]; unsigned int emiffwclkctrl; /* offset 0xD0 */ - unsigned int resv8[2]; + unsigned int resv7[2]; unsigned int l3instrclkctrl; /* offset 0xDC */ unsigned int l3clkctrl; /* Offset 0xE0 */ - unsigned int resv9[14]; + unsigned int resv8[4]; + unsigned int mmc1clkctrl; /* offset 0xF4 */ + unsigned int mmc2clkctrl; /* offset 0xF8 */ + unsigned int resv9[8]; unsigned int l4hsclkstctrl; /* offset 0x11C */ unsigned int l4hsclkctrl; /* offset 0x120 */ unsigned int resv10[8]; - unsigned int cpswclkctrl; /* offset 0x144 */ + unsigned int cpswclkstctrl; /* offset 0x144 */ };
/* Encapsulating Display pll registers */
Hello Tom,
Tom Rini wrote:
Signed-off-by: Tom Rini trini@ti.com
arch/arm/include/asm/arch-am33xx/cpu.h | 40 ++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 10 deletions(-)
Acked-by: Heiko Schocher hs@denx.de
Thanks!
bye, Heiko
Signed-off-by: Tom Rini trini@ti.com --- arch/arm/include/asm/arch-am33xx/i2c.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/arch-am33xx/i2c.h b/arch/arm/include/asm/arch-am33xx/i2c.h index dcb294c..32b2258 100644 --- a/arch/arm/include/asm/arch-am33xx/i2c.h +++ b/arch/arm/include/asm/arch-am33xx/i2c.h @@ -76,6 +76,6 @@ struct i2c { };
#define I2C_IP_CLK 48000000 -#define I2C_INTERNAL_SAMLPING_CLK 12000000 +#define I2C_INTERNAL_SAMPLING_CLK 12000000
#endif /* _I2C_H_ */
Hello Tom,
Tom Rini wrote:
Signed-off-by: Tom Rini trini@ti.com
arch/arm/include/asm/arch-am33xx/i2c.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Heiko Schocher hs@denx.de
Thanks!
bye, Heiko
The same places that check for CONFIG_OMAP44XX need to check for CONFIG_AM33XX as we share the same i2c block.
Cc: Heiko Schocher hs@denx.de Signed-off-by: Tom Rini trini@ti.com --- drivers/i2c/omap24xx_i2c.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index 3a50417..81193b0 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -202,7 +202,7 @@ static int i2c_read_byte(u8 devaddr, u8 regoffset, u8 *value) } if (status & I2C_STAT_RRDY) { #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ - defined(CONFIG_OMAP44XX) + defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) *value = readb(&i2c_base->data); #else *value = readw(&i2c_base->data); @@ -232,7 +232,7 @@ static void flush_fifo(void) stat = readw(&i2c_base->stat); if (stat == I2C_STAT_RRDY) { #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ - defined(CONFIG_OMAP44XX) + defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) readb(&i2c_base->data); #else readw(&i2c_base->data); @@ -282,7 +282,7 @@ int i2c_probe(uchar chip) if (status & I2C_STAT_RRDY) { res = 0; #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ - defined(CONFIG_OMAP44XX) + defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) readb(&i2c_base->data); #else readw(&i2c_base->data);
Hello Tom,
Tom Rini wrote:
The same places that check for CONFIG_OMAP44XX need to check for CONFIG_AM33XX as we share the same i2c block.
Cc: Heiko Schocher hs@denx.de Signed-off-by: Tom Rini trini@ti.com
drivers/i2c/omap24xx_i2c.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Acked-by: Heiko Schocher hs@denx.de
Thanks!
bye, Heiko
Hello Tom,
Tom Rini wrote:
The following series corrects i2c support for the am33xx family of devices (including beaglebone) and has been tested on a beagleboard xM and pandaboard. I would like to take these changes in via the u-boot-ti tree as it's mostly changes to arch and board files but I've cc'd Heiko on the i2c related parts. The biggest change here is to revert a previous change to the omap24xx_i2c.c driver for the i2c probe function. In short, the change made before violated the TRM constraints but did no harm on the IP block found in omap3/related boards. Moving forward to the IP block found on omap4 and am33xx (and others) it still voilates the TRM and now leaves the bus in an unboundedly defined state like the TRM states. We must revert this change for both correctness and functionality. The rest of the series is minor corrections to structs/defines and adding CONFIG_AM33XX to the block of !CONFIG_OMAP2430. I've thought of, but think it should be separate to change omap24xx_i2c.c to test for CONFIG_OMAP2430 rather than every other present and future user in a few areas.
Ok, just saw, that you delegate this patches in patchwork to you. I am fine with that, but want to ack this patches, so please wait until I sent my Ack ... thanks!
bye, Heiko
participants (2)
-
Heiko Schocher -
Tom Rini